diff mbox series

[4/4] reftable/stack: register compacted tables as tempfiles

Message ID b952d54a05e1c0cf47371f78e3901cfb2119e246.1709549619.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable/stack: register temporary files | expand

Commit Message

Patrick Steinhardt March 4, 2024, 11:10 a.m. UTC 
We do not register tables resulting from stack compaction with the
tempfile API. Those tables will thus not be deleted in case Git gets
killed.

Refactor the code to register compacted tables as tempfiles.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 53 +++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

Comments

Toon Claes March 7, 2024, 12:38 p.m. UTC | #1 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 977336b7d5..40129da16c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
>  
>  static int stack_compact_locked(struct reftable_stack *st,
>  				size_t first, size_t last,
> -				struct strbuf *temp_tab,
> -				struct reftable_log_expiry_config *config)
> +				struct reftable_log_expiry_config *config,
> +				struct tempfile **temp_table_out)
>  {
>  	struct strbuf next_name = STRBUF_INIT;
> -	int tab_fd = -1;
> +	struct strbuf table_path = STRBUF_INIT;
>  	struct reftable_writer *wr = NULL;
> +	struct tempfile *temp_table;
> +	int temp_table_fd;

Just one small nit, if you don't mind? In PATCH 2/4 you use
`struct tempfile *tab_file` and `int tab_fd`. I would like to see
consistency and use similar names. Personally I don't like table being
shortened to "tab", and I think you feel the same as you've renamed the
parameter from this function.
Patrick Steinhardt March 7, 2024, 12:58 p.m. UTC | #2 
On Thu, Mar 07, 2024 at 01:38:56PM +0100, Toon claes wrote:
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 977336b7d5..40129da16c 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
> >  
> >  static int stack_compact_locked(struct reftable_stack *st,
> >  				size_t first, size_t last,
> > -				struct strbuf *temp_tab,
> > -				struct reftable_log_expiry_config *config)
> > +				struct reftable_log_expiry_config *config,
> > +				struct tempfile **temp_table_out)
> >  {
> >  	struct strbuf next_name = STRBUF_INIT;
> > -	int tab_fd = -1;
> > +	struct strbuf table_path = STRBUF_INIT;
> >  	struct reftable_writer *wr = NULL;
> > +	struct tempfile *temp_table;
> > +	int temp_table_fd;
> 
> Just one small nit, if you don't mind? In PATCH 2/4 you use
> `struct tempfile *tab_file` and `int tab_fd`. I would like to see
> consistency and use similar names. Personally I don't like table being
> shortened to "tab", and I think you feel the same as you've renamed the
> parameter from this function.

Yeah, I'm always a proponent of descriptive, unabbreviated names and
would thus rather want to use "temp_table" or something like that. But
in 2/4 I decided to stick with "tab_file" because there is a bunch of
already existing variables in that function which are called similar --
"temp_tab_file_name", "tab_file_name" and "tab_fd". But renaming all of
them to ensure function-level consistency would create a lot of churn.

I'll thus instead rename variables over here to their abbreviated
versions.

Patrick
diff mbox series

Patch

diff --git a/reftable/stack.c b/reftable/stack.c
index 977336b7d5..40129da16c 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -827,51 +827,57 @@  uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
 
 static int stack_compact_locked(struct reftable_stack *st,
 				size_t first, size_t last,
-				struct strbuf *temp_tab,
-				struct reftable_log_expiry_config *config)
+				struct reftable_log_expiry_config *config,
+				struct tempfile **temp_table_out)
 {
 	struct strbuf next_name = STRBUF_INIT;
-	int tab_fd = -1;
+	struct strbuf table_path = STRBUF_INIT;
 	struct reftable_writer *wr = NULL;
+	struct tempfile *temp_table;
+	int temp_table_fd;
 	int err = 0;
 
 	format_name(&next_name,
 		    reftable_reader_min_update_index(st->readers[first]),
 		    reftable_reader_max_update_index(st->readers[last]));
+	stack_filename(&table_path, st, next_name.buf);
+	strbuf_addstr(&table_path, ".temp.XXXXXX");
 
-	stack_filename(temp_tab, st, next_name.buf);
-	strbuf_addstr(temp_tab, ".temp.XXXXXX");
+	temp_table = mks_tempfile(table_path.buf);
+	if (!temp_table) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+	temp_table_fd = get_tempfile_fd(temp_table);
 
-	tab_fd = mkstemp(temp_tab->buf);
 	if (st->config.default_permissions &&
-	    chmod(temp_tab->buf, st->config.default_permissions) < 0) {
+	    chmod(get_tempfile_path(temp_table), st->config.default_permissions) < 0) {
 		err = REFTABLE_IO_ERROR;
 		goto done;
 	}
 
-	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
-
+	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,
+				 &temp_table_fd, &st->config);
 	err = stack_write_compact(st, wr, first, last, config);
 	if (err < 0)
 		goto done;
+
 	err = reftable_writer_close(wr);
 	if (err < 0)
 		goto done;
 
-	err = close(tab_fd);
-	tab_fd = 0;
+	err = close_tempfile_gently(temp_table);
+	if (err < 0)
+		goto done;
+
+	*temp_table_out = temp_table;
+	temp_table = NULL;
 
 done:
+	delete_tempfile(&temp_table);
 	reftable_writer_free(wr);
-	if (tab_fd > 0) {
-		close(tab_fd);
-		tab_fd = 0;
-	}
-	if (err != 0 && temp_tab->len > 0) {
-		unlink(temp_tab->buf);
-		strbuf_release(temp_tab);
-	}
 	strbuf_release(&next_name);
+	strbuf_release(&table_path);
 	return err;
 }
 
@@ -979,12 +985,12 @@  static int stack_compact_range(struct reftable_stack *st,
 			       struct reftable_log_expiry_config *expiry)
 {
 	struct strbuf tables_list_buf = STRBUF_INIT;
-	struct strbuf new_table_temp_path = STRBUF_INIT;
 	struct strbuf new_table_name = STRBUF_INIT;
 	struct strbuf new_table_path = STRBUF_INIT;
 	struct strbuf table_name = STRBUF_INIT;
 	struct lock_file tables_list_lock = LOCK_INIT;
 	struct lock_file *table_locks = NULL;
+	struct tempfile *new_table = NULL;
 	int is_empty_table = 0, err = 0;
 	size_t i;
 
@@ -1059,7 +1065,7 @@  static int stack_compact_range(struct reftable_stack *st,
 	 * these tables may end up with an empty new table in case tombstones
 	 * end up cancelling out all refs in that range.
 	 */
-	err = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);
+	err = stack_compact_locked(st, first, last, expiry, &new_table);
 	if (err < 0) {
 		if (err != REFTABLE_EMPTY_TABLE_ERROR)
 			goto done;
@@ -1099,7 +1105,7 @@  static int stack_compact_range(struct reftable_stack *st,
 		strbuf_addstr(&new_table_name, ".ref");
 		stack_filename(&new_table_path, st, new_table_name.buf);
 
-		err = rename(new_table_temp_path.buf, new_table_path.buf);
+		err = rename_tempfile(&new_table, new_table_path.buf);
 		if (err < 0) {
 			err = REFTABLE_IO_ERROR;
 			goto done;
@@ -1166,9 +1172,10 @@  static int stack_compact_range(struct reftable_stack *st,
 		rollback_lock_file(&table_locks[i - first]);
 	reftable_free(table_locks);
 
+	delete_tempfile(&new_table);
 	strbuf_release(&new_table_name);
 	strbuf_release(&new_table_path);
-	strbuf_release(&new_table_temp_path);
+
 	strbuf_release(&tables_list_buf);
 	strbuf_release(&table_name);
 	return err;