[PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[Taylor Blau]

This pair of patches addresses two issues (in the commit-graph and
update-server-info areas, respectively), where temporary files are
created outside of the tempfile.h API and thus survive abnormal process
death.

The commit-graph one is more prevalent, and has been the cause of some
minor headaches (for e.g. automated tooling to detect repository cruft
at GitHub complaining about unknown tmp_graph_XXXXXX files left around).

The fixes in both instances are relatively straightforward conversions
to use the tempfile.h API.

Looking at the remaining uses of mkstemp(), the remaining class of
callers that don't use the tempfile.h API are for creating temporary
.idx, .rev files, and similar. My personal feeling is that we should
apply similar treatment there, since these files are generated based on
.pack data, and thus keeping around temporary copies is unnecessary when
they can be regenerated.

But I'd rather keep that part out of this series and clean up a couple
of more straightforward spots where possible.

Thanks in advance for your review!

Taylor Blau (2):
  commit-graph.c: remove temporary graph layers on exit
  server-info.c: remove temporary info files on exit

 commit-graph.c                | 19 ++++++++-----------
 server-info.c                 | 26 ++++++++++----------------
 t/t5324-split-commit-graph.sh | 26 +++++++++++++++++++++++++-
 3 files changed, 43 insertions(+), 28 deletions(-)


base-commit: 7b0defb3915eaa0bd118f0996e8c00b4eb2dc1ca
-- 
2.45.2.411.g2d5a0536af1

[PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Taylor Blau]

Since the introduction of split commit graph layers in 92b1ea66b9a
(Merge branch 'ds/commit-graph-incremental', 2019-07-19), the function
write_commit_graph_file() has done the following when writing an
incremental commit-graph layer:

  - used a lock_file to control access to the commit-graph-chain file

  - used an auxiliary file (whose descriptor was stored in 'fd') to
    write the new commit-graph layer itself

Using a lock_file to control access to the commit-graph-chain is
sensible, since only one writer may modify it at a time. Likewise, when
the commit-graph machinery is writing out a single layer, the lock_file
structure is used to modify the commit-graph itself. This is also
sensible, since the non-incremental commit-graph may also have at most
one writer.

However, using an auxiliary temporary file without using the tempfile.h
API means that writes that fail after the temporary graph layer has been
created will leave around a file in

    $GIT_DIR/objects/info/commit-graphs/tmp_graph_XXXXXX

The commit-graph-chain file and non-incremental commit-graph do not
suffer from this problem as the lockfile.h API uses the tempfile.h API
transparently, so processes that died before moving those finals into
their final location cleaned up after themselves.

Ensure that the temporary file used to write incremental commit-graphs
is also managed with the tempfile.h API, to ensure that we do not ever
leave tmp_graph_XXXXXX files laying around.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c                | 19 ++++++++-----------
 t/t5324-split-commit-graph.sh | 26 +++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index e5dd3553dfe..92c71637142 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2002,8 +2002,8 @@ static int write_graph_chunk_base(struct hashfile *f,
 static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 {
 	uint32_t i;
-	int fd;
 	struct hashfile *f;
+	struct tempfile *graph_layer; /* when ctx->split is non-zero */
 	struct lock_file lk = LOCK_INIT;
 	const unsigned hashsz = the_hash_algo->rawsz;
 	struct strbuf progress_title = STRBUF_INIT;
@@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 					       LOCK_DIE_ON_ERROR, 0444);
 		free(lock_name);
 
-		fd = git_mkstemp_mode(ctx->graph_name, 0444);
-		if (fd < 0) {
+		graph_layer = mks_tempfile_m(ctx->graph_name, 0444);
+		if (!graph_layer) {
 			error(_("unable to create temporary graph layer"));
 			return -1;
 		}
 
-		if (adjust_shared_perm(ctx->graph_name)) {
+		if (adjust_shared_perm(get_tempfile_path(graph_layer))) {
 			error(_("unable to adjust shared permissions for '%s'"),
-			      ctx->graph_name);
+			      get_tempfile_path(graph_layer));
 			return -1;
 		}
 
-		f = hashfd(fd, ctx->graph_name);
+		f = hashfd(get_tempfile_fd(graph_layer), get_tempfile_path(graph_layer));
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
 					       LOCK_DIE_ON_ERROR, 0444);
-		fd = get_lock_file_fd(&lk);
-		f = hashfd(fd, get_lock_file_path(&lk));
+		f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	}
 
 	cf = init_chunkfile(f);
@@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		char *final_graph_name;
 		int result;
 
-		close(fd);
-
 		if (!chainf) {
 			error(_("unable to open commit-graph chain file"));
 			return -1;
@@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
 		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
 
-		result = rename(ctx->graph_name, final_graph_name);
+		result = rename_tempfile(&graph_layer, final_graph_name);
 
 		for (i = 0; i < ctx->num_commit_graphs_after; i++)
 			fprintf(get_lock_file_fp(&lk), "%s\n", ctx->commit_graph_hash_after[i]);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 281266f7883..77e91547ea3 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -13,7 +13,8 @@ test_expect_success 'setup repo' '
 	git init &&
 	git config core.commitGraph true &&
 	git config gc.writeCommitGraph false &&
-	infodir=".git/objects/info" &&
+	objdir=".git/objects" &&
+	infodir="$objdir/info" &&
 	graphdir="$infodir/commit-graphs" &&
 	test_oid_cache <<-EOM
 	shallow sha1:2132
@@ -718,4 +719,27 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl
 	)
 '
 
+test_expect_success 'temporary graph layer is discarded upon failure' '
+	git init layer-discard &&
+	(
+		cd layer-discard &&
+
+		test_commit A &&
+		test_commit B &&
+
+		# Intentionally remove commit "A" from the object store
+		# so that the commit-graph machinery fails to parse the
+		# parents of "B".
+		#
+		# This takes place after the commit-graph machinery has
+		# initialized a new temporary file to store the contents
+		# of the new graph layer, so will allow us to ensure
+		# that the temporary file is discarded upon failure.
+		rm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) &&
+
+		test_must_fail git commit-graph write --reachable --split &&
+		test_dir_is_empty $graphdir
+	)
+'
+
 test_done
-- 
2.45.2.411.g2d5a0536af1

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  		char *final_graph_name;
>  		int result;
>  
> -		close(fd);
> -
>  		if (!chainf) {
>  			error(_("unable to open commit-graph chain file"));
>  			return -1;
> @@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
>  		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
>  
> -		result = rename(ctx->graph_name, final_graph_name);
> +		result = rename_tempfile(&graph_layer, final_graph_name);

Before this rename, after the close(fd) we saw in the previous hunk,
there is one early error return when we fail to rename the base
graph file.  Do we need to do anything there, or an unfinished
tempfile getting removed at the process termination is sufficient
for cleaning up the mess?

Other than that, this looked quite straight-forward.

Thanks.

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Taylor Blau]

On Fri, Jun 07, 2024 at 09:33:56AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
> >  		char *final_graph_name;
> >  		int result;
> >
> > -		close(fd);
> > -
> >  		if (!chainf) {
> >  			error(_("unable to open commit-graph chain file"));
> >  			return -1;
> > @@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
> >  		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
> >  		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
> >
> > -		result = rename(ctx->graph_name, final_graph_name);
> > +		result = rename_tempfile(&graph_layer, final_graph_name);
>
> Before this rename, after the close(fd) we saw in the previous hunk,
> there is one early error return when we fail to rename the base
> graph file.  Do we need to do anything there, or an unfinished
> tempfile getting removed at the process termination is sufficient
> for cleaning up the mess?

We could explicitly clean it up, but we'll do so implicitly upon exit,
so I think it's fine to leave it as-is.

Thanks,
Taylor

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> > -		result = rename(ctx->graph_name, final_graph_name);
>> > +		result = rename_tempfile(&graph_layer, final_graph_name);
>>
>> Before this rename, after the close(fd) we saw in the previous hunk,
>> there is one early error return when we fail to rename the base
>> graph file.  Do we need to do anything there, or an unfinished
>> tempfile getting removed at the process termination is sufficient
>> for cleaning up the mess?
>
> We could explicitly clean it up, but we'll do so implicitly upon exit,
> so I think it's fine to leave it as-is.

I am not worried about cleaning it up.  Upon exit, the underlying
file descriptor will be closed, but this new code never does
fclose() on the FILE* that has a buffer around the underlying file
descriptor.  How are we guaranteeing that we are not losing anything
buffered but not flushed yet?

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Jeff King]

On Fri, Jun 07, 2024 at 02:47:58PM -0700, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >> > -		result = rename(ctx->graph_name, final_graph_name);
> >> > +		result = rename_tempfile(&graph_layer, final_graph_name);
> >>
> >> Before this rename, after the close(fd) we saw in the previous hunk,
> >> there is one early error return when we fail to rename the base
> >> graph file.  Do we need to do anything there, or an unfinished
> >> tempfile getting removed at the process termination is sufficient
> >> for cleaning up the mess?
> >
> > We could explicitly clean it up, but we'll do so implicitly upon exit,
> > so I think it's fine to leave it as-is.
> 
> I am not worried about cleaning it up.  Upon exit, the underlying
> file descriptor will be closed, but this new code never does
> fclose() on the FILE* that has a buffer around the underlying file
> descriptor.  How are we guaranteeing that we are not losing anything
> buffered but not flushed yet?

I'm not sure I understand. There is no "FILE *" at all in this patch (we
use the descriptor directly via the hashfd interface). But even if there
were, if we leave without renaming the temp file into place, then that
tempfile will be deleted. So it wouldn't matter if we flushed to it or
not, because all of those byte are destined to be thrown away.

-Peff

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> Before this rename, after the close(fd) we saw in the previous hunk,
>> there is one early error return when we fail to rename the base
>> graph file.  Do we need to do anything there, or an unfinished
>> tempfile getting removed at the process termination is sufficient
>> for cleaning up the mess?
>
> We could explicitly clean it up, but we'll do so implicitly upon exit,
> so I think it's fine to leave it as-is.

OK.  That sounds good.

Re: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit

[Jeff King]

On Thu, Jun 06, 2024 at 06:19:25PM -0400, Taylor Blau wrote:

> @@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>  					       LOCK_DIE_ON_ERROR, 0444);
>  		free(lock_name);
>  
> -		fd = git_mkstemp_mode(ctx->graph_name, 0444);
> -		if (fd < 0) {
> +		graph_layer = mks_tempfile_m(ctx->graph_name, 0444);
> +		if (!graph_layer) {
>  			error(_("unable to create temporary graph layer"));
>  			return -1;
>  		}
>  
> -		if (adjust_shared_perm(ctx->graph_name)) {
> +		if (adjust_shared_perm(get_tempfile_path(graph_layer))) {
>  			error(_("unable to adjust shared permissions for '%s'"),
> -			      ctx->graph_name);
> +			      get_tempfile_path(graph_layer));
>  			return -1;
>  		}

Most errors will cause us to die(), but this "return" and the one that
Junio noted later (when renaming the base graph file fails) mean we'll
return with the tempfile still active. We'll clean it up when the
program exits, but if there were a long-running program that
called write_commit_graph_file(), that tempfile might hang around longer
than necessary.

But that is the same strategy that the existing code uses for the lock
we use for the chain filename! And it is much worse there, because now
it is not just a tempfile hanging around, but we're blocking anybody
else from taking the lock.

So I think it would be OK to punt on this for now. Your patch is not
making the situation worse, and it's all a problem for a hypothetical
libification of this function.

> +test_expect_success 'temporary graph layer is discarded upon failure' '
> +	git init layer-discard &&
> +	(
> +		cd layer-discard &&
> +
> +		test_commit A &&
> +		test_commit B &&
> +
> +		# Intentionally remove commit "A" from the object store
> +		# so that the commit-graph machinery fails to parse the
> +		# parents of "B".
> +		#
> +		# This takes place after the commit-graph machinery has
> +		# initialized a new temporary file to store the contents
> +		# of the new graph layer, so will allow us to ensure
> +		# that the temporary file is discarded upon failure.
> +		rm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) &&
> +
> +		test_must_fail git commit-graph write --reachable --split &&
> +		test_dir_is_empty $graphdir
> +	)
> +'

I'm glad you were able to come up with a case that fails cleanly and
non-racily. The exit code of rev-parse will be lost. I doubt that it
matters in practice, but I wouldn't be surprised if our shell linting
eventually learns to complain about this spot.

Looks like there are a few similar ones in the test suite already, from
t5329. I'd be content to leave it for now and deal with it later if
somebody really wants to go on a crusade against lost exit codes.

-Peff

[PATCH 2/2] server-info.c: remove temporary info files on exit

[Taylor Blau]

The update_info_file() function within server-info.c is responsible for
moving the info/refs and info/packs files around when updating server
info.

These updates are staged into a temporary file and then moved into place
atomically to avoid race conditions when reading those files. However,
the temporary file used to stage these changes is managed outside of the
tempfile.h API, and thus survives process death.

Manage these files instead with the tempfile.h API so that they are
automatically cleaned up upon abnormal process death.

Unfortunately, and unlike in the previous step, there isn't a
straightforward way to inject a failure into the update-server-info step
that causes us to die() rather than take the cleanup path in label
'out', hence the lack of a test here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 server-info.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/server-info.c b/server-info.c
index 6feaa457c5c..37d1085982e 100644
--- a/server-info.c
+++ b/server-info.c
@@ -13,6 +13,7 @@
 #include "object-store-ll.h"
 #include "server-info.h"
 #include "strbuf.h"
+#include "tempfile.h"
 
 struct update_info_ctx {
 	FILE *cur_fp;
@@ -75,9 +76,8 @@ static int update_info_file(char *path,
 			int force)
 {
 	char *tmp = mkpathdup("%s_XXXXXX", path);
+	struct tempfile *f = NULL;
 	int ret = -1;
-	int fd = -1;
-	FILE *to_close;
 	struct update_info_ctx uic = {
 		.cur_fp = NULL,
 		.old_fp = NULL,
@@ -86,13 +86,12 @@ static int update_info_file(char *path,
 	};
 
 	safe_create_leading_directories(path);
-	fd = git_mkstemp_mode(tmp, 0666);
-	if (fd < 0)
+	f = mks_tempfile_m(tmp, 0666);
+	if (!f)
 		goto out;
-	to_close = uic.cur_fp = fdopen(fd, "w");
+	uic.cur_fp = fdopen_tempfile(f, "w");
 	if (!uic.cur_fp)
 		goto out;
-	fd = -1;
 
 	/* no problem on ENOENT and old_fp == NULL, it's stale, now */
 	if (!force)
@@ -121,27 +120,22 @@ static int update_info_file(char *path,
 	}
 
 	uic.cur_fp = NULL;
-	if (fclose(to_close))
-		goto out;
 
 	if (uic_is_stale(&uic)) {
-		if (adjust_shared_perm(tmp) < 0)
+		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
 			goto out;
-		if (rename(tmp, path) < 0)
+		if (rename_tempfile(&f, path) < 0)
 			goto out;
 	} else {
-		unlink(tmp);
+		delete_tempfile(&f);
 	}
 	ret = 0;
 
 out:
 	if (ret) {
 		error_errno("unable to update %s", path);
-		if (uic.cur_fp)
-			fclose(uic.cur_fp);
-		else if (fd >= 0)
-			close(fd);
-		unlink(tmp);
+		if (f)
+			delete_tempfile(&f);
 	}
 	free(tmp);
 	if (uic.old_fp)
-- 
2.45.2.411.g2d5a0536af1

Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -121,27 +120,22 @@ static int update_info_file(char *path,
>  	}
>  
>  	uic.cur_fp = NULL;
> -	if (fclose(to_close))
> -		goto out;

We should fflush() of cur_fp before nuking it, at least, no?

In the original code, to_close was a mere copy of uic.cur_fp and we
made sure that anything buffered at the stdio layer are flushed to
the underlying file desciptor (fd that we obtained from
git_mkstemp_mode() in the original code) with this fclose() call.

We no longer do so.  We later call rename_tempfile() to close the
underlying file descriptor and move the temporary file to its final
place, but I do not see what guarantee we have that we do not lose
what we had buffered in the stdio with the updated code.

>  	if (uic_is_stale(&uic)) {
> -		if (adjust_shared_perm(tmp) < 0)
> +		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
>  			goto out;
> -		if (rename(tmp, path) < 0)
> +		if (rename_tempfile(&f, path) < 0)
>  			goto out;
>  	} else {
> -		unlink(tmp);
> +		delete_tempfile(&f);
>  	}
>  	ret = 0;
>  
>  out:
>  	if (ret) {
>  		error_errno("unable to update %s", path);
> -		if (uic.cur_fp)
> -			fclose(uic.cur_fp);
> -		else if (fd >= 0)
> -			close(fd);
> -		unlink(tmp);
> +		if (f)
> +			delete_tempfile(&f);
>  	}
>  	free(tmp);
>  	if (uic.old_fp)

Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

[Taylor Blau]

On Fri, Jun 07, 2024 at 09:02:14AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -121,27 +120,22 @@ static int update_info_file(char *path,
> >  	}
> >
> >  	uic.cur_fp = NULL;
> > -	if (fclose(to_close))
> > -		goto out;
>
> We should fflush() of cur_fp before nuking it, at least, no?
>
> In the original code, to_close was a mere copy of uic.cur_fp and we
> made sure that anything buffered at the stdio layer are flushed to
> the underlying file desciptor (fd that we obtained from
> git_mkstemp_mode() in the original code) with this fclose() call.
>
> We no longer do so.  We later call rename_tempfile() to close the
> underlying file descriptor and move the temporary file to its final
> place, but I do not see what guarantee we have that we do not lose
> what we had buffered in the stdio with the updated code.

rename_tempfile() first calls close_tempfile_gently() before calling
rename(). close_tempfile_gently() calls fclose() on temp->fp before
returns, so we get our fflush() call implicitly there.

IOW, the tempfile.h API is happy for us to call rename_tempfile()
without having explicitly closed or flushed the underlying file pointer.

Thanks,
Taylor

Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> We no longer do so.  We later call rename_tempfile() to close the
>> underlying file descriptor and move the temporary file to its final
>> place, but I do not see what guarantee we have that we do not lose
>> what we had buffered in the stdio with the updated code.
>
> rename_tempfile() first calls close_tempfile_gently() before calling
> rename(). close_tempfile_gently() calls fclose() on temp->fp before
> returns, so we get our fflush() call implicitly there.

OK.  I didn't see that fclose().  Then we are safe.

Thanks.

Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

[Jeff King]

On Thu, Jun 06, 2024 at 06:19:31PM -0400, Taylor Blau wrote:

> The update_info_file() function within server-info.c is responsible for
> moving the info/refs and info/packs files around when updating server
> info.
> 
> These updates are staged into a temporary file and then moved into place
> atomically to avoid race conditions when reading those files. However,
> the temporary file used to stage these changes is managed outside of the
> tempfile.h API, and thus survives process death.
> 
> Manage these files instead with the tempfile.h API so that they are
> automatically cleaned up upon abnormal process death.

Makes sense. I was going to suggest that these could even be lockfiles,
but it is intentional to let two simultaneous processes race (with an
atomic last-one-wins result). See d38379ece9 (make update-server-info
more robust, 2014-09-13).

> Unfortunately, and unlike in the previous step, there isn't a
> straightforward way to inject a failure into the update-server-info step
> that causes us to die() rather than take the cleanup path in label
> 'out', hence the lack of a test here.

That sounds like a challenge. ;)

  $ echo garbage >.git/packed-refs
  $ git update-server-info
  fatal: unexpected line in .git/packed-refs: garbage
  $ ls .git/info/
  exclude  refs  refs_QYvQGb

I don't know if it's worth adding such a test. It seems rather brittle
to assume that we'd die() here (let alone that we are using the files
backend at all).

> @@ -86,13 +86,12 @@ static int update_info_file(char *path,
>  	};
>  
>  	safe_create_leading_directories(path);
> -	fd = git_mkstemp_mode(tmp, 0666);
> -	if (fd < 0)
> +	f = mks_tempfile_m(tmp, 0666);
> +	if (!f)
>  		goto out;
> -	to_close = uic.cur_fp = fdopen(fd, "w");
> +	uic.cur_fp = fdopen_tempfile(f, "w");

OK, good, fdopen_tempfile() means that the FILE handle is owned by the
tempfile, too.

> @@ -121,27 +120,22 @@ static int update_info_file(char *path,
>  	}
>  
>  	uic.cur_fp = NULL;
> -	if (fclose(to_close))
> -		goto out;

And we don't need to fclose() anymore since the tempfile code handles
that for us. Nice.

>  	if (uic_is_stale(&uic)) {
> -		if (adjust_shared_perm(tmp) < 0)
> +		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
>  			goto out;
> -		if (rename(tmp, path) < 0)
> +		if (rename_tempfile(&f, path) < 0)
>  			goto out;
>  	} else {
> -		unlink(tmp);
> +		delete_tempfile(&f);
>  	}
>  	ret = 0;

OK, so we always rename or delete here, unless we jumped to the error
path...

>  out:
>  	if (ret) {
>  		error_errno("unable to update %s", path);
> -		if (uic.cur_fp)
> -			fclose(uic.cur_fp);
> -		else if (fd >= 0)
> -			close(fd);
> -		unlink(tmp);
> +		if (f)
> +			delete_tempfile(&f);
>  	}

And here we do an explicit delete, which is good for a lib-ified world
where the process doesn't just exit immediately.

I think you could actually call delete_tempfile() unconditionally, even
outside the "if (ret)" block. It is a noop for a NULL tempfile (so OK
even if we jump to "out" before opening it). And a renamed tempfile goes
back to NULL as well.

I.e., one of the advantages to using the tempfile interface is that it's
always in a consistent state, and you just use delete() on exit, like we
do strbuf_release().

That said, it's a pretty minor style question, and I don't think is
worth a re-roll.

-Peff

Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> This pair of patches addresses two issues (in the commit-graph and
> update-server-info areas, respectively), where temporary files are
> created outside of the tempfile.h API and thus survive abnormal process
> death.
>
> The commit-graph one is more prevalent, and has been the cause of some
> minor headaches (for e.g. automated tooling to detect repository cruft
> at GitHub complaining about unknown tmp_graph_XXXXXX files left around).

;-)  

I'd be very surprised if a stale "update-server-info" product causes
any harm, but we unfortunately cannot remove it until we fully
remove the suport for HTTP walkers.

> The fixes in both instances are relatively straightforward conversions
> to use the tempfile.h API.

> Looking at the remaining uses of mkstemp(), the remaining class of
> callers that don't use the tempfile.h API are for creating temporary
> .idx, .rev files, and similar. My personal feeling is that we should
> apply similar treatment there, since these files are generated based on
> .pack data, and thus keeping around temporary copies is unnecessary when
> they can be regenerated.

Absolutely.

I wouldn't be surprised by .idx as it and .pack are so old, dating
back to 2005, but anything newer like .rev I am mildly surprised
that we haven't done this conversion.

Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[Jeff King]

On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:

> Looking at the remaining uses of mkstemp(), the remaining class of
> callers that don't use the tempfile.h API are for creating temporary
> .idx, .rev files, and similar. My personal feeling is that we should
> apply similar treatment there, since these files are generated based on
> .pack data, and thus keeping around temporary copies is unnecessary when
> they can be regenerated.

And actual loose object and pack files themselves, I think. I think it
was a deliberate choice long ago to keep those files around, since in
the worst case they could be used to recover actual repo content (e.g.,
a failed fetch will leave its tmp_pack_* file around, and you could
probably extract _some_ objects from it).

And the philosophy is that we'd leave them sitting around until gc ran
and found tmp_* in objects/, check their mtimes, and remove them then.

In practice, I don't think I have really seen anybody recover anything
from a temporary file. You're better off looking at whatever was feeding
the temporary file (if it was "git add", then look at the filesystem,
and if it was index-pack, look at the fetch/push source, etc).

And leaving them around is a minor nuisance, since degenerate cases
(like a script retrying a failed operation over and over) can let them
pile up.

And indeed, for push we don't keep the contents around anymore. Even
though we don't clean up the .pack files, we stick them in a quarantine
directory which does get cleaned up. This solved what used to be an
operational headache for GitHub, and I don't think anybody has ever
complained about not having their partial failed packfile available.

So I'd argue that we should just treat object/pack tempfiles like the
rest, and delete them if they don't make it all the way to the rename
step. If we really want to support debugging, we could perhaps provide
a run-time knob to leave them in place (and maybe even have it apply to
_all_ tempfiles).

But that is all way beyond your series, and I don't think there is any
urgent need to tackle it. IIRC, GitHub's fork does sprinkle some
register_tempfile() calls around the odb code, but I don't have access
to that code anymore.

-Peff

Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[Elijah Newren]

On Sat, Jun 8, 2024 at 3:48 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:
>
> > Looking at the remaining uses of mkstemp(), the remaining class of
> > callers that don't use the tempfile.h API are for creating temporary
> > .idx, .rev files, and similar. My personal feeling is that we should
> > apply similar treatment there, since these files are generated based on
> > .pack data, and thus keeping around temporary copies is unnecessary when
> > they can be regenerated.
>
> And actual loose object and pack files themselves, I think.
[...]
> So I'd argue that we should just treat object/pack tempfiles like the
> rest, and delete them if they don't make it all the way to the rename
> step. If we really want to support debugging, we could perhaps provide
> a run-time knob to leave them in place (and maybe even have it apply to
> _all_ tempfiles).
>
> But that is all way beyond your series, and I don't think there is any
> urgent need to tackle it.

Regardless, it provides more context around the exact questions I had
while reading the series.  Everything in the series looked fine to me,
but I wondered about packs and loose objects and why those are
different.  Anyway, I like your suggestions as a long term goal.
(Perhaps handling packs and loose objects with tempfiles could serve
as good microprojects?)

Re: [PATCH 0/2] commit-graph/server-info: use tempfile.h in more places

[Taylor Blau]

On Sat, Jun 08, 2024 at 06:48:21AM -0400, Jeff King wrote:
> On Thu, Jun 06, 2024 at 06:19:21PM -0400, Taylor Blau wrote:
>
> > Looking at the remaining uses of mkstemp(), the remaining class of
> > callers that don't use the tempfile.h API are for creating temporary
> > .idx, .rev files, and similar. My personal feeling is that we should
> > apply similar treatment there, since these files are generated based on
> > .pack data, and thus keeping around temporary copies is unnecessary when
> > they can be regenerated.
>
> And actual loose object and pack files themselves, I think. I think it
> was a deliberate choice long ago to keep those files around, since in
> the worst case they could be used to recover actual repo content (e.g.,
> a failed fetch will leave its tmp_pack_* file around, and you could
> probably extract _some_ objects from it).

Heh, yes. Those were intentionally omitted from this list ;-).

I agree that having the content stick around in failed packfile and
loose object writes is useful as a last-resort recovery mechanism. I
suspect that it is often difficult or impossible to recover the contents
of an object/pack from a broken write, but I think it's better than the
alternative of just throwing it away up front.

> And the philosophy is that we'd leave them sitting around until gc ran
> and found tmp_* in objects/, check their mtimes, and remove them then.
>
> In practice, I don't think I have really seen anybody recover anything
> from a temporary file. You're better off looking at whatever was feeding
> the temporary file (if it was "git add", then look at the filesystem,
> and if it was index-pack, look at the fetch/push source, etc).

Yup.

> So I'd argue that we should just treat object/pack tempfiles like the
> rest, and delete them if they don't make it all the way to the rename
> step. If we really want to support debugging, we could perhaps provide
> a run-time knob to leave them in place (and maybe even have it apply to
> _all_ tempfiles).

I dunno... I don't disagree with what you're saying, but it does seem a
little scary to delete files containing data that we might have been
able to recover.

I think the current series ends at a reasonable stopping point... I'd
honestly have to think more about whether I agree with what you're
saying here or not ;-).

Thanks,
Taylor