[PATCH 0/4] repack tempfile-cleanup signal deadlock

[PATCH 0/4] repack tempfile-cleanup signal deadlock

[Jeff King]

Here's a series which should fix the deadlock Jan reported in:

  https://lore.kernel.org/git/00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz/

I split it into a new thread since the problem here is distinct from the
one fixed in that thread (but they're indeed the same class of problem).
The fix is along the same lines as what I showed there, switching to
using the tempfile API. But this has the cleanups I noted there, plus
nice things like commit messages and tests.

As I noted there, pack-objects is still happy to leave its internal
tempfiles sitting around. I didn't tackle that in this series, since it
really is orthogonal, at least in terms of implementation.

I've cc'd Taylor for review. The main problem here goes all the way back
to a1bbc6c017 (repack: rewrite the shell script in C, 2013-09-15), but
the solution is enabled by your more recent populate_pack_ext(), etc.

  [1/4]: repack: convert "names" util bitfield to array
  [2/4]: repack: populate extension bits incrementally
  [3/4]: repack: use tempfiles for signal cleanup
  [4/4]: repack: drop remove_temporary_files()

 builtin/repack.c  | 78 ++++++++++++++---------------------------------
 t/t7700-repack.sh | 16 ++++++++++
 2 files changed, 39 insertions(+), 55 deletions(-)

-Peff

[PATCH 1/4] repack: convert "names" util bitfield to array

[Jeff King]

We keep a string_list "names" containing the hashes of packs generated
on our behalf by pack-objects. The util field of each item is treated as
a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are
present for each name.

Let's switch this to allocating a real array. That will give us room in
a future patch to store more data than just a single bit per extension.
And it makes the code a little easier to read, as we avoid casting back
and forth between uintptr_t and a void pointer.

Since the only thing we're storing is an array, we could just allocate
it directly. But instead I've put it into a named struct here. That
further increases readability around the casts, and in particular helps
differentiate us from other string_lists in the same file which use
their util field differently. E.g., the existing_*_packs lists still do
bit-twiddling, but their bits have different meaning than the ones in
"names". This makes it hard to grep around the code to see how the util
fields are used; now you can look for "generated_pack_data".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index a5bacc7797..8e71230bf7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -247,11 +247,15 @@ static struct {
 	{".idx"},
 };
 
-static unsigned populate_pack_exts(char *name)
+struct generated_pack_data {
+	char exts[ARRAY_SIZE(exts)];
+};
+
+static struct generated_pack_data *populate_pack_exts(const char *name)
 {
 	struct stat statbuf;
 	struct strbuf path = STRBUF_INIT;
-	unsigned ret = 0;
+	struct generated_pack_data *data = xcalloc(1, sizeof(*data));
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(exts); i++) {
@@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)
 		if (stat(path.buf, &statbuf))
 			continue;
 
-		ret |= (1 << i);
+		data->exts[i] = 1;
 	}
 
 	strbuf_release(&path);
-	return ret;
+	return data;
 }
 
 static void repack_promisor_objects(const struct pack_objects_args *args,
@@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 					  line.buf);
 		write_promisor_file(promisor_name, NULL, 0);
 
-		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
+		item->util = populate_pack_exts(item->string);
 
 		free(promisor_name);
 	}
@@ -994,7 +998,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_sort(&names);
 
 	for_each_string_list_item(item, &names) {
-		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
+		item->util = populate_pack_exts(item->string);
 	}
 
 	close_object_store(the_repository->objects);
@@ -1003,6 +1007,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	 * Ok we have prepared all new packfiles.
 	 */
 	for_each_string_list_item(item, &names) {
+		struct generated_pack_data *data = item->util;
+
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
 			char *fname, *fname_old;
 
@@ -1011,7 +1017,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);
 
-			if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {
+			if (data->exts[ext]) {
 				struct stat statbuffer;
 				if (!stat(fname_old, &statbuffer)) {
 					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
@@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_midx_file(get_object_directory(), NULL, NULL, flags);
 	}
 
-	string_list_clear(&names, 0);
+	string_list_clear(&names, 1);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
-- 
2.38.1.496.ga614b0e9bd

[PATCH 2/4] repack: populate extension bits incrementally

[Jeff King]

After generating the main pack and then any additional cruft packs, we
iterate over the "names" list (which contains hashes of packs generated
by pack-objects), and call populate_pack_exts() for each.

There are two small problems with this:

  - repack_promisor_objects() may have added entries to "names", and
    already called populate_pack_exts() for them. This is mostly just
    wasteful, as we'll stat() the filename with each possible extension,
    get the same result, and just overwrite our bits. But it makes the
    code flow confusing, and it will become a problem if we try to make
    populate_pack_exts() do more things.

  - it would be nice to record the generated filenames as soon as
    possible. We don't currently use them for cleaning up from a failed
    operation, but a future patch will do so.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 8e71230bf7..b5bd9e5fed 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -714,10 +714,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 
 	out = xfdopen(cmd.out, "r");
 	while (strbuf_getline_lf(&line, out) != EOF) {
+		struct string_list_item *item;
+
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only "
 			      "from pack-objects."));
-		string_list_append(names, line.buf);
+
+		item = string_list_append(names, line.buf);
+		item->util = populate_pack_exts(line.buf);
 	}
 	fclose(out);
 
@@ -956,9 +960,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	out = xfdopen(cmd.out, "r");
 	while (strbuf_getline_lf(&line, out) != EOF) {
+		struct string_list_item *item;
+
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only from pack-objects."));
-		string_list_append(&names, line.buf);
+		item = string_list_append(&names, line.buf);
+		item->util = populate_pack_exts(item->string);
 	}
 	fclose(out);
 	ret = finish_command(&cmd);
@@ -997,10 +1004,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	string_list_sort(&names);
 
-	for_each_string_list_item(item, &names) {
-		item->util = populate_pack_exts(item->string);
-	}
-
 	close_object_store(the_repository->objects);
 
 	/*
-- 
2.38.1.496.ga614b0e9bd

[PATCH 3/4] repack: use tempfiles for signal cleanup

[Jeff King]

When git-repack exits due to a signal, it tries to clean up by calling
its remove_temporary_files() function, which walks through the packs dir
looking for ".tmp-$$-pack-*" files to delete (where "$$" is the pid of
the current process).

The biggest problem here is that remove_temporary_files() is not safe to
call in a signal handler. It uses opendir(), which isn't on the POSIX
async-signal-safe list. The details will be platform-specific, but a
likely issue is that it needs to allocate memory; if we receive a signal
while inside malloc(), etc, we'll conflict on the allocator lock and
deadlock with ourselves.

We can fix this by just cleaning up the files directly, without walking
the directory. We already know the complete list of .tmp-* files that
were generated, because we recorded them via populate_pack_exts(). When
we find files there, we can use register_tempfile() to record the
filenames. If we receive a signal, then the tempfile API will clean them
up for us, and it's async-safe and pretty battle-tested.

Note that this is slightly racier than the existing scheme. We don't
record the filenames until pack-objects tells us the hash over stdout.
So during the period between it generating the file and reporting the
hash, we'd fail to clean up. However, that period is very small. During
most of the pack generation process pack-objects is using its own
internal tempfiles. It's only at the very end that it moves them into
the names git-repack expects, and then it immediately reports the name
to us. Given that cleanup like this is best effort (after all, we may
get SIGKILL), this level of race is acceptable.

When we register the tempfiles, we'll record them locally and use the
result to call rename_tempfile(), rather than renaming by hand.  This
isn't strictly necessary, as once we've renamed the files they're gone,
and the tempfile API's cleanup unlink() would simply become a pointless
noop. But managing the lifetimes of the tempfile objects is the cleanest
thing to do, and the tempfile pointers naturally fill the same role as
the old booleans.

This patch also fixes another small problem. We only hook signals, and
don't set up an atexit handler. So if we see an error that causes us to
die(), we'll leave the .tmp-* files in place. But since the tempfile API
handles this for us, this is now fixed for free. The new test covers
this by stimulating a failure of pack-objects when generating a cruft
pack. Before this patch, the .tmp-* file for the main pack would have
been left, but now we correctly clean it up.

Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 17 ++++-------------
 t/t7700-repack.sh |  8 ++++++++
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index b5bd9e5fed..15b6f24626 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -122,13 +122,6 @@ static void remove_temporary_files(void)
 	strbuf_release(&buf);
 }
 
-static void remove_pack_on_signal(int signo)
-{
-	remove_temporary_files();
-	sigchain_pop(signo);
-	raise(signo);
-}
-
 /*
  * Adds all packs hex strings to either fname_nonkept_list or
  * fname_kept_list based on whether each pack has a corresponding
@@ -248,7 +241,7 @@ static struct {
 };
 
 struct generated_pack_data {
-	char exts[ARRAY_SIZE(exts)];
+	struct tempfile *tempfiles[ARRAY_SIZE(exts)];
 };
 
 static struct generated_pack_data *populate_pack_exts(const char *name)
@@ -265,7 +258,7 @@ static struct generated_pack_data *populate_pack_exts(const char *name)
 		if (stat(path.buf, &statbuf))
 			continue;
 
-		data->exts[i] = 1;
+		data->tempfiles[i] = register_tempfile(path.buf);
 	}
 
 	strbuf_release(&path);
@@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		split_pack_geometry(geometry, geometric_factor);
 	}
 
-	sigchain_push_common(remove_pack_on_signal);
-
 	prepare_pack_objects(&cmd, &po_args);
 
 	show_progress = !po_args.quiet && isatty(2);
@@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);
 
-			if (data->exts[ext]) {
+			if (data->tempfiles[ext]) {
 				struct stat statbuffer;
 				if (!stat(fname_old, &statbuffer)) {
 					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
 					chmod(fname_old, statbuffer.st_mode);
 				}
 
-				if (rename(fname_old, fname))
+				if (rename_tempfile(&data->tempfiles[ext], fname))
 					die_errno(_("renaming '%s' failed"), fname_old);
 			} else if (!exts[ext].optional)
 				die(_("missing required file: %s"), fname_old);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index ca45c4cd2c..592016f64a 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -432,6 +432,14 @@ test_expect_success TTY '--quiet disables progress' '
 	test_must_be_empty stderr
 '
 
+test_expect_success 'clean up .tmp-* packs on error' '
+	test_must_fail git \
+		-c repack.cruftwindow=bogus \
+		repack -ad --cruft &&
+	find $objdir/pack -name '.tmp-*' >tmpfiles &&
+	test_must_be_empty tmpfiles
+'
+
 test_expect_success 'setup for update-server-info' '
 	git init update-server-info &&
 	test_commit -C update-server-info message
-- 
2.38.1.496.ga614b0e9bd

[PATCH 4/4] repack: drop remove_temporary_files()

[Jeff King]

After we've successfully finished the repack, we call
remove_temporary_files(), which looks for and removes any files matching
".tmp-$$-pack-*", where $$ is the pid of the current process. But this
is pointless. If we make it this far in the process, we've already
renamed these tempfiles into place, and there is nothing left to delete.

Nor is there a point in trying to call it to clean up when we _aren't_
successful. It's not safe for using in a signal handler, and the
previous commit already handed that job over to the tempfile API.

It might seem like it would be useful to clean up stray .tmp files left
by other invocations of git-repack. But it won't clean those files; it
only matches ones with its pid, and leaves the rest. Fortunately, those
are cleaned up naturally by successive calls to git-repack; we'll
consider .tmp-*.pack the same as normal packfiles, so "repack -ad", etc,
will roll up their contents and eventually delete them.

The one case that could matter is if pack-objects generates an extension
we don't know about, like ".tmp-pack-$$-$hash.some-new-ext". The current
code will quietly delete such a file, while after this patch we'd leave
it in place. In practice this doesn't happen, and would be indicative of
a bug. Leaving the file as cruft is arguably a better behavior, as it
means somebody is more likely to eventually notice and fix the bug.  If
we really wanted to be paranoid, we could scan for and warn about such
files, but that seems like overkill.

There's nothing to test with regard to the removal of this function. It
was doing nothing, so the behavior should be the same.  However, we can
verify (and protect) our assumption that "repack -ad" will eventually
remove stray files by adding a test for that.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 32 --------------------------------
 t/t7700-repack.sh |  8 ++++++++
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 15b6f24626..68294c7bc7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -91,37 +91,6 @@ static int repack_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-/*
- * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
- */
-static void remove_temporary_files(void)
-{
-	struct strbuf buf = STRBUF_INIT;
-	size_t dirlen, prefixlen;
-	DIR *dir;
-	struct dirent *e;
-
-	dir = opendir(packdir);
-	if (!dir)
-		return;
-
-	/* Point at the slash at the end of ".../objects/pack/" */
-	dirlen = strlen(packdir) + 1;
-	strbuf_addstr(&buf, packtmp);
-	/* Hold the length of  ".tmp-%d-pack-" */
-	prefixlen = buf.len - dirlen;
-
-	while ((e = readdir(dir))) {
-		if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
-			continue;
-		strbuf_setlen(&buf, dirlen);
-		strbuf_addstr(&buf, e->d_name);
-		unlink(buf.buf);
-	}
-	closedir(dir);
-	strbuf_release(&buf);
-}
-
 /*
  * Adds all packs hex strings to either fname_nonkept_list or
  * fname_kept_list based on whether each pack has a corresponding
@@ -1106,7 +1075,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	if (run_update_server_info)
 		update_server_info(0);
-	remove_temporary_files();
 
 	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {
 		unsigned flags = 0;
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 592016f64a..edcda849b9 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -440,6 +440,14 @@ test_expect_success 'clean up .tmp-* packs on error' '
 	test_must_be_empty tmpfiles
 '
 
+test_expect_success 'repack -ad cleans up old .tmp-* packs' '
+	git rev-parse HEAD >input &&
+	git pack-objects $objdir/pack/.tmp-1234 <input &&
+	git repack -ad &&
+	find $objdir/pack -name '.tmp-*' >tmpfiles &&
+	test_must_be_empty tmpfiles
+'
+
 test_expect_success 'setup for update-server-info' '
 	git init update-server-info &&
 	test_commit -C update-server-info message
-- 
2.38.1.496.ga614b0e9bd

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We keep a string_list "names" containing the hashes of packs generated
> on our behalf by pack-objects. The util field of each item is treated as
> a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are
> present for each name.
>
> Let's switch this to allocating a real array. That will give us room in
> a future patch to store more data than just a single bit per extension.
> And it makes the code a little easier to read, as we avoid casting back
> and forth between uintptr_t and a void pointer.
>
> Since the only thing we're storing is an array, we could just allocate
> it directly. But instead I've put it into a named struct here. That
> further increases readability around the casts, and in particular helps
> differentiate us from other string_lists in the same file which use
> their util field differently. E.g., the existing_*_packs lists still do
> bit-twiddling, but their bits have different meaning than the ones in
> "names". This makes it hard to grep around the code to see how the util
> fields are used; now you can look for "generated_pack_data".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/repack.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index a5bacc7797..8e71230bf7 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -247,11 +247,15 @@ static struct {
>  	{".idx"},
>  };
>  
> -static unsigned populate_pack_exts(char *name)
> +struct generated_pack_data {
> +	char exts[ARRAY_SIZE(exts)];
> +};

OK, so instead of a single "unsigned" word holding six bits (the
number of elements in the exts[] array), we have one byte per the
file extension.  Since ...

> +static struct generated_pack_data *populate_pack_exts(const char *name)
>  {
>  	struct stat statbuf;
>  	struct strbuf path = STRBUF_INIT;
> -	unsigned ret = 0;
> +	struct generated_pack_data *data = xcalloc(1, sizeof(*data));

... this is allocated a real piece of storage and the function
returns a pointer to it, ...

> -		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
> +		item->util = populate_pack_exts(item->string);

... we no longer need to cast but the value can go straight to the
.util member, and ...

> -	string_list_clear(&names, 0);
> +	string_list_clear(&names, 1);

... we now need to free these structs pointed at by the .util
member.

All make sense.

Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch also fixes another small problem. We only hook signals, and
> don't set up an atexit handler. So if we see an error that causes us to
> die(), we'll leave the .tmp-* files in place. But since the tempfile API
> handles this for us, this is now fixed for free. The new test covers
> this by stimulating a failure of pack-objects when generating a cruft
> pack. Before this patch, the .tmp-* file for the main pack would have
> been left, but now we correctly clean it up.
>
> Reported-by: Jan Pokorný <poki@fnusa.cz>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/repack.c  | 17 ++++-------------
>  t/t7700-repack.sh |  8 ++++++++
>  2 files changed, 12 insertions(+), 13 deletions(-)

Nice.  

>  struct generated_pack_data {
> -	char exts[ARRAY_SIZE(exts)];
> +	struct tempfile *tempfiles[ARRAY_SIZE(exts)];
> ...
> -		data->exts[i] = 1;
> +		data->tempfiles[i] = register_tempfile(path.buf);

OK.

>  	}
>  
>  	strbuf_release(&path);
> @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		split_pack_geometry(geometry, geometric_factor);
>  	}
>  
> -	sigchain_push_common(remove_pack_on_signal);
> -
>  	prepare_pack_objects(&cmd, &po_args);
>  
>  	show_progress = !po_args.quiet && isatty(2);
> @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  			fname_old = mkpathdup("%s-%s%s",
>  					packtmp, item->string, exts[ext].name);
>  
> -			if (data->exts[ext]) {
> +			if (data->tempfiles[ext]) {
>  				struct stat statbuffer;
>  				if (!stat(fname_old, &statbuffer)) {
>  					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  					chmod(fname_old, statbuffer.st_mode);
>  				}
>  
> -				if (rename(fname_old, fname))
> +				if (rename_tempfile(&data->tempfiles[ext], fname))
>  					die_errno(_("renaming '%s' failed"), fname_old);

It now got a bit confusing that we have 'fname', 'fname_old', and
the tempfile.  The path.buf used as the argument to register_tempfile()
matches what is used to compute fname_old above.  I wonder if tempfile
API does not give us that name so that we can stop using fname_old here?

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Taylor Blau]

On Fri, Oct 21, 2022 at 05:42:48PM -0400, Jeff King wrote:
> We keep a string_list "names" containing the hashes of packs generated
> on our behalf by pack-objects. The util field of each item is treated as
> a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are
> present for each name.
>
> Let's switch this to allocating a real array. That will give us room in
> a future patch to store more data than just a single bit per extension.
> And it makes the code a little easier to read, as we avoid casting back
> and forth between uintptr_t and a void pointer.

Thanks, this is a nice summary that matches my own recollection from
working in this area.

> diff --git a/builtin/repack.c b/builtin/repack.c
> index a5bacc7797..8e71230bf7 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -247,11 +247,15 @@ static struct {
>  	{".idx"},
>  };
>
> -static unsigned populate_pack_exts(char *name)
> +struct generated_pack_data {
> +	char exts[ARRAY_SIZE(exts)];
> +};

Makes sense, this replaces the individual bits of the `void *` we were
treating as an array of "which extensions associated with this pack were
generated by pack-objects"?

> +
> +static struct generated_pack_data *populate_pack_exts(const char *name)
>  {
>  	struct stat statbuf;
>  	struct strbuf path = STRBUF_INIT;
> -	unsigned ret = 0;
> +	struct generated_pack_data *data = xcalloc(1, sizeof(*data));

I'm nitpicking, but we could replace this with;

  struct generated_pack_data *data;

  CALLOC_ARRAY(data, 1);

so that we don't have to rely on calling sizeof(*data). But
sizeof(*data) will always give us the right answer anyway, even if the
name of data's type changed, so what you have is fine, too.

> @@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)
>  		if (stat(path.buf, &statbuf))
>  			continue;
>
> -		ret |= (1 << i);
> +		data->exts[i] = 1;

Great. I'm happy to see this bit-twiddling go away, but..

> @@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
>  					  line.buf);
>  		write_promisor_file(promisor_name, NULL, 0);
>
> -		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
> +		item->util = populate_pack_exts(item->string);

I'm even happier to see this go away ;-).

> @@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		write_midx_file(get_object_directory(), NULL, NULL, flags);
>  	}
>
> -	string_list_clear(&names, 0);
> +	string_list_clear(&names, 1);

Good. Now that we're actually allocating memory in the `->util` field,
we care about freeing it, too. Thanks for remembering this.

Thanks,
Taylor

Re: [PATCH 2/4] repack: populate extension bits incrementally

[Taylor Blau]

On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:
> There are two small problems with this:
>
>   - repack_promisor_objects() may have added entries to "names", and
>     already called populate_pack_exts() for them. This is mostly just
>     wasteful, as we'll stat() the filename with each possible extension,
>     get the same result, and just overwrite our bits. But it makes the
>     code flow confusing, and it will become a problem if we try to make
>     populate_pack_exts() do more things.

Hmm. I agree with you that repack_promisor_objects() calling
populate_pack_exts() itself is at best weird, and at worst wasteful.
>
>   - it would be nice to record the generated filenames as soon as
>     possible. We don't currently use them for cleaning up from a failed
>     operation, but a future patch will do so.

That said, I think that it's worth having a single spot within
cmd_repack() that is responsible for populating the generated pack
extensions, since it protects against a caller forgetting to do so (and
then tricking repack into thinking that we didn't generate *any* of the
corresponding extensions).

But I'm sure future patch you're referring to cares about knowing
these as soon as possible, since that's the point of this series ;-).

I think a reasonable middle ground here is to do something like the
following on top of this patch:

--- >8 ---
diff --git a/builtin/repack.c b/builtin/repack.c
index b5bd9e5fed..16a941f48b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1002,6 +1002,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			return ret;
 	}

+	for_each_string_list_item(item, &names) {
+		if (!item->util)
+			BUG("missing generated_pack_data for pack %s",
+			    item->string);
+	}
+
 	string_list_sort(&names);

 	close_object_store(the_repository->objects);
--- 8< ---

which still lets you eagerly keep track of the generated pack extensions
while also protecting against forgetful callers. Obviously we're relying
on a runtime check which is going to be somewhat weaker. But I think

The patch itself looks good.

Thanks,
Taylor

Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

[Jeff King]

On Fri, Oct 21, 2022 at 03:30:29PM -0700, Junio C Hamano wrote:

> > @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  		split_pack_geometry(geometry, geometric_factor);
> >  	}
> >  
> > -	sigchain_push_common(remove_pack_on_signal);
> > -
> >  	prepare_pack_objects(&cmd, &po_args);
> >  
> >  	show_progress = !po_args.quiet && isatty(2);
> > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  			fname_old = mkpathdup("%s-%s%s",
> >  					packtmp, item->string, exts[ext].name);
> >  
> > -			if (data->exts[ext]) {
> > +			if (data->tempfiles[ext]) {
> >  				struct stat statbuffer;
> >  				if (!stat(fname_old, &statbuffer)) {
> >  					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> >  					chmod(fname_old, statbuffer.st_mode);
> >  				}
> >  
> > -				if (rename(fname_old, fname))
> > +				if (rename_tempfile(&data->tempfiles[ext], fname))
> >  					die_errno(_("renaming '%s' failed"), fname_old);
> 
> It now got a bit confusing that we have 'fname', 'fname_old', and
> the tempfile.  The path.buf used as the argument to register_tempfile()
> matches what is used to compute fname_old above.  I wonder if tempfile
> API does not give us that name so that we can stop using fname_old here?

It does, and we probably should use get_tempfile_path() in the error
message here. But sadly we can't get rid of fname_old entirely, as it's
used below this for the second block in the if-else chain:

  if (data->tempfiles[ext]) {
     ...do the rename ...
  } else if (!exts[ext].optional)
	  die(_("missing required file: %s"), fname_old);
  else if (unlink(fname) < 0 && errno != ENOENT)
	  die_errno(_("could not unlink: %s"), fname);

OTOH, it would probably be equally readable (or perhaps even better) for
that second block to say:

  die("pack-objects did not generate a '%s' file for pack %s",
      exts[ext].name, item->string);

And then we could drop fname_old entirely. Which is nice, because it
gets rid of the implicit assumption that the tempfile matches what is in
fname_old (which is always true, but since they are generated by
individual lines far apart from each other, it's possible for that to
change).

-Peff

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Jeff King]

On Fri, Oct 21, 2022 at 07:10:47PM -0400, Taylor Blau wrote:

> > +	struct generated_pack_data *data = xcalloc(1, sizeof(*data));
> 
> I'm nitpicking, but we could replace this with;
> 
>   struct generated_pack_data *data;
> 
>   CALLOC_ARRAY(data, 1);
> 
> so that we don't have to rely on calling sizeof(*data). But
> sizeof(*data) will always give us the right answer anyway, even if the
> name of data's type changed, so what you have is fine, too.

Yeah, I actually considered writing it that way, but it felt a bit silly
to use _ARRAY for something which is clearly meant to be a single item.
Grepping for "CALLOC_ARRAY([^)]*, 1)" does seem to turn up quite a few
hits, though, so maybe it is just me.

We could also introduce:

  #define CALLOC(x) CALLOC_ARRAY((x), 1)

but even if we think that is a good idea, it should not be in this
series.

It looks I may re-roll for the fname_old stuff in the other part of the
thread, so I'll take your suggestion.

-Peff

Re: [PATCH 2/4] repack: populate extension bits incrementally

[Jeff King]

On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:

> On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:
> > There are two small problems with this:
> >
> >   - repack_promisor_objects() may have added entries to "names", and
> >     already called populate_pack_exts() for them. This is mostly just
> >     wasteful, as we'll stat() the filename with each possible extension,
> >     get the same result, and just overwrite our bits. But it makes the
> >     code flow confusing, and it will become a problem if we try to make
> >     populate_pack_exts() do more things.
> 
> Hmm. I agree with you that repack_promisor_objects() calling
> populate_pack_exts() itself is at best weird, and at worst wasteful.

I don't think it's weird, really. It is setting up the entries in the
string-list completely when we add them, rather than annotating later.
If there were some performance gain from doing them all at once, I could
see it, but otherwise I like that it means the entries are always in a
consistent state.

> But I'm sure future patch you're referring to cares about knowing
> these as soon as possible, since that's the point of this series ;-).

Yes. :)

> I think a reasonable middle ground here is to do something like the
> following on top of this patch:
> 
> --- >8 ---
> diff --git a/builtin/repack.c b/builtin/repack.c
> index b5bd9e5fed..16a941f48b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -1002,6 +1002,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  			return ret;
>  	}
> 
> +	for_each_string_list_item(item, &names) {
> +		if (!item->util)
> +			BUG("missing generated_pack_data for pack %s",
> +			    item->string);
> +	}
> +
>  	string_list_sort(&names);
> 
>  	close_object_store(the_repository->objects);
> --- 8< ---
> 
> which still lets you eagerly keep track of the generated pack extensions
> while also protecting against forgetful callers. Obviously we're relying
> on a runtime check which is going to be somewhat weaker. But I think

I don't think we need that. The renaming loop a few lines below will
happily segfault if anybody forgot to populate it. With a less nice
message, obviously, but if the point is to notice a bug, it will get the
job done.

-Peff

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I actually considered writing it that way, but it felt a bit silly
> to use _ARRAY for something which is clearly meant to be a single item.

You are not alone.  I updated Réne's coccinelle rules to refrain
from converting xcalloc() to CALLOC_ARRAY() for a single element
case exactly because it feels silly to use array function for a
singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).

Re: [PATCH 2/4] repack: populate extension bits incrementally

[Taylor Blau]

On Fri, Oct 21, 2022 at 07:34:12PM -0400, Jeff King wrote:
> > which still lets you eagerly keep track of the generated pack extensions
> > while also protecting against forgetful callers. Obviously we're relying
> > on a runtime check which is going to be somewhat weaker. But I think
>
> I don't think we need that. The renaming loop a few lines below will
> happily segfault if anybody forgot to populate it. With a less nice
> message, obviously, but if the point is to notice a bug, it will get the
> job done.

That's reasonable, I think. I'm happy to abandon that suggestion :-).

Thanks,
Taylor

Re: [PATCH 2/4] repack: populate extension bits incrementally

[Jeff King]

On Fri, Oct 21, 2022 at 07:34:13PM -0400, Jeff King wrote:

> On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:
> 
> > On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:
> > > There are two small problems with this:
> > >
> > >   - repack_promisor_objects() may have added entries to "names", and
> > >     already called populate_pack_exts() for them. This is mostly just
> > >     wasteful, as we'll stat() the filename with each possible extension,
> > >     get the same result, and just overwrite our bits. But it makes the
> > >     code flow confusing, and it will become a problem if we try to make
> > >     populate_pack_exts() do more things.
> > 
> > Hmm. I agree with you that repack_promisor_objects() calling
> > populate_pack_exts() itself is at best weird, and at worst wasteful.
> 
> I don't think it's weird, really. It is setting up the entries in the
> string-list completely when we add them, rather than annotating later.
> If there were some performance gain from doing them all at once, I could
> see it, but otherwise I like that it means the entries are always in a
> consistent state.

I think my original didn't explain my thinking very well. And its "two
small problems" is really a bit of a lie. It is really one small
problem, and a tweak I want in order to make a future patch work. :)

So here's what I wrote instead:

-- >8 --
There's one small problem with this. In repack_promisor_objects(), we
may add entries to "names" and call populate_pack_exts() for them.
Calling it again is mostly just wasteful, as we'll stat() the filename
with each possible extension, get the same result, and just overwrite
our bits.

So we could drop the call there, and leave the final loop to populate
all of the bits. But instead, this patch does the reverse: drops the
final loop, and teaches the other two sites to populate the bits as they
add entries.

This makes the code easier to reason about, as you never have to worry
about when the util field is valid; it is always valid for each entry.

It also serves my ulterior purpose: recording the generated filenames as
soon as possible will make it easier for a future patch to use them for
cleaning up from a failed operation.
-- >8 --

-Peff

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Jeff King]

On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, I actually considered writing it that way, but it felt a bit silly
> > to use _ARRAY for something which is clearly meant to be a single item.
> 
> You are not alone.  I updated Réne's coccinelle rules to refrain
> from converting xcalloc() to CALLOC_ARRAY() for a single element
> case exactly because it feels silly to use array function for a
> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).

OK. Then I'll leave it as-is, then. :)

-Peff

Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

[Taylor Blau]

On Fri, Oct 21, 2022 at 07:24:46PM -0400, Jeff King wrote:
> On Fri, Oct 21, 2022 at 03:30:29PM -0700, Junio C Hamano wrote:
>
> > > @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > >  		split_pack_geometry(geometry, geometric_factor);
> > >  	}
> > >
> > > -	sigchain_push_common(remove_pack_on_signal);
> > > -
> > >  	prepare_pack_objects(&cmd, &po_args);
> > >
> > >  	show_progress = !po_args.quiet && isatty(2);
> > > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > >  			fname_old = mkpathdup("%s-%s%s",
> > >  					packtmp, item->string, exts[ext].name);
> > >
> > > -			if (data->exts[ext]) {
> > > +			if (data->tempfiles[ext]) {
> > >  				struct stat statbuffer;
> > >  				if (!stat(fname_old, &statbuffer)) {
> > >  					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> > >  					chmod(fname_old, statbuffer.st_mode);
> > >  				}
> > >
> > > -				if (rename(fname_old, fname))
> > > +				if (rename_tempfile(&data->tempfiles[ext], fname))
> > >  					die_errno(_("renaming '%s' failed"), fname_old);
> >
> > It now got a bit confusing that we have 'fname', 'fname_old', and
> > the tempfile.  The path.buf used as the argument to register_tempfile()
> > matches what is used to compute fname_old above.  I wonder if tempfile
> > API does not give us that name so that we can stop using fname_old here?
>
> It does, and we probably should use get_tempfile_path() in the error
> message here. But sadly we can't get rid of fname_old entirely, as it's
> used below this for the second block in the if-else chain:
>
>   if (data->tempfiles[ext]) {
>      ...do the rename ...
>   } else if (!exts[ext].optional)
> 	  die(_("missing required file: %s"), fname_old);
>   else if (unlink(fname) < 0 && errno != ENOENT)
> 	  die_errno(_("could not unlink: %s"), fname);
>
> OTOH, it would probably be equally readable (or perhaps even better) for
> that second block to say:
>
>   die("pack-objects did not generate a '%s' file for pack %s",
>       exts[ext].name, item->string);
>
> And then we could drop fname_old entirely. Which is nice, because it
> gets rid of the implicit assumption that the tempfile matches what is in
> fname_old (which is always true, but since they are generated by
> individual lines far apart from each other, it's possible for that to
> change).

TBH, I've always found fname_old to be a confusing name. It's not really
"old", in fact we just had pack-objects write that file ;-). It really
does pertain to the tempfile, and I think using get_tempfile_path() when
we have a tempfile to rename is sensible.

I think that your proposed error message is good, too, and doubly so
since it lets us get rid of fname_old entirely. Yay :-).

> -Peff

Thanks,
Taylor

Re: [PATCH 4/4] repack: drop remove_temporary_files()

[Taylor Blau]

On Fri, Oct 21, 2022 at 05:48:38PM -0400, Jeff King wrote:
> The one case that could matter is if pack-objects generates an extension
> we don't know about, like ".tmp-pack-$$-$hash.some-new-ext". The current
> code will quietly delete such a file, while after this patch we'd leave
> it in place. In practice this doesn't happen, and would be indicative of
> a bug. Leaving the file as cruft is arguably a better behavior, as it
> means somebody is more likely to eventually notice and fix the bug.  If
> we really wanted to be paranoid, we could scan for and warn about such
> files, but that seems like overkill.

I agree, that would definitely be overkill.

> There's nothing to test with regard to the removal of this function. It
> was doing nothing, so the behavior should be the same.  However, we can
> verify (and protect) our assumption that "repack -ad" will eventually
> remove stray files by adding a test for that.

Thanks for adding such a test. This patch is beautiful :-).

Thanks,
Taylor

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Yeah, I actually considered writing it that way, but it felt a bit silly
>> > to use _ARRAY for something which is clearly meant to be a single item.
>> 
>> You are not alone.  I updated Réne's coccinelle rules to refrain
>> from converting xcalloc() to CALLOC_ARRAY() for a single element
>> case exactly because it feels silly to use array function for a
>> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).
>
> OK. Then I'll leave it as-is, then. :)
>
> -Peff

I do not think any "huh? array for a singleton" folks mind your

  #define CALLOC(x) CALLOC_ARRAY((x), 1)

though ;-)

Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

[Jeff King]

On Fri, Oct 21, 2022 at 07:24:46PM -0400, Jeff King wrote:

> > > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > >  			fname_old = mkpathdup("%s-%s%s",
> > >  					packtmp, item->string, exts[ext].name);
> > >  
> > > -			if (data->exts[ext]) {
> > > +			if (data->tempfiles[ext]) {
> > >  				struct stat statbuffer;
> > >  				if (!stat(fname_old, &statbuffer)) {
> > >  					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> > >  					chmod(fname_old, statbuffer.st_mode);
> > >  				}
> > >  
> > > -				if (rename(fname_old, fname))
> > > +				if (rename_tempfile(&data->tempfiles[ext], fname))
> > >  					die_errno(_("renaming '%s' failed"), fname_old);
> > 
> > It now got a bit confusing that we have 'fname', 'fname_old', and
> > the tempfile.  The path.buf used as the argument to register_tempfile()
> > matches what is used to compute fname_old above.  I wonder if tempfile
> > API does not give us that name so that we can stop using fname_old here?
> 
> It does, and we probably should use get_tempfile_path() in the error
> message here.

Gah, this is not quite true. If the rename fails, we clean up the
tempfile struct entirely, and that invalidates the pointer. I think it
is OK to just report "fname" in this case, though, which is what most
callers do. Arguably the tempfile API should leave the tempfile in place
on a failed rename, letting the callers decide themselves how to handle
things. In most cases they'll just exit anyway, which will clean up the
tempfile.

I also didn't notice that we do some mode-twiddling on fname_old. But I
think if the code becomes (inside this conditional block, once we stop
using it in the other half):

  const char *fname_old = get_tempfile_path(data->tempfiles[ext]);

then those lines don't even need to change.

-Peff

Re: [PATCH 3/4] repack: use tempfiles for signal cleanup

[Jeff King]

On Fri, Oct 21, 2022 at 07:45:28PM -0400, Taylor Blau wrote:

> TBH, I've always found fname_old to be a confusing name. It's not really
> "old", in fact we just had pack-objects write that file ;-). It really
> does pertain to the tempfile, and I think using get_tempfile_path() when
> we have a tempfile to rename is sensible.

I don't love it either, but I've kept it in what I'm preparing, just
because we need _some_ variable to avoid writing:

  get_tempfile_path(data->tempfiles[ext])

over and over. And using the same one keeps the diff minimal. If it's
too terrible we can rename it on top. :)

> I think that your proposed error message is good, too, and doubly so
> since it lets us get rid of fname_old entirely. Yay :-).

Thanks. I'm preparing something along those lines.

-Peff

Re: [PATCH 1/4] repack: convert "names" util bitfield to array

[Jeff King]

On Fri, Oct 21, 2022 at 04:51:44PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > Yeah, I actually considered writing it that way, but it felt a bit silly
> >> > to use _ARRAY for something which is clearly meant to be a single item.
> >> 
> >> You are not alone.  I updated Réne's coccinelle rules to refrain
> >> from converting xcalloc() to CALLOC_ARRAY() for a single element
> >> case exactly because it feels silly to use array function for a
> >> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).
> >
> > OK. Then I'll leave it as-is, then. :)
> >
> > -Peff
> 
> I do not think any "huh? array for a singleton" folks mind your
> 
>   #define CALLOC(x) CALLOC_ARRAY((x), 1)
> 
> though ;-)

I may follow-up on that, but let's leave it outside of this series.

-Peff

[PATCH v2 0/5] repack tempfile-cleanup signal deadlock

[Jeff King]

On Fri, Oct 21, 2022 at 05:41:02PM -0400, Jeff King wrote:

> Here's a series which should fix the deadlock Jan reported in:
> 
>   https://lore.kernel.org/git/00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz/

And here's a re-roll based on feedback on v1. Poor Jan; I hope this
thread isn't blowing up your inbox. :) But thank you for reporting the
problem!

The changes from v1 are:

  - clarified rationale in patch 2
  - new patch 3 improves an error message and reduces scope of fname_old
  - patch 4 cleans up fname_old to use tempfile path

Range diff is below.

  [1/5]: repack: convert "names" util bitfield to array
  [2/5]: repack: populate extension bits incrementally
  [3/5]: repack: expand error message for missing pack files
  [4/5]: repack: use tempfiles for signal cleanup
  [5/5]: repack: drop remove_temporary_files()

 builtin/repack.c  | 90 +++++++++++++++--------------------------------
 t/t7700-repack.sh | 16 +++++++++
 2 files changed, 45 insertions(+), 61 deletions(-)

1:  6de9bd9d71 = 1:  affb21442f repack: convert "names" util bitfield to array
2:  7d72ed9a39 ! 2:  18d8318881 repack: populate extension bits incrementally
    @@ Commit message
         iterate over the "names" list (which contains hashes of packs generated
         by pack-objects), and call populate_pack_exts() for each.
     
    -    There are two small problems with this:
    +    There's one small problem with this. In repack_promisor_objects(), we
    +    may add entries to "names" and call populate_pack_exts() for them.
    +    Calling it again is mostly just wasteful, as we'll stat() the filename
    +    with each possible extension, get the same result, and just overwrite
    +    our bits.
     
    -      - repack_promisor_objects() may have added entries to "names", and
    -        already called populate_pack_exts() for them. This is mostly just
    -        wasteful, as we'll stat() the filename with each possible extension,
    -        get the same result, and just overwrite our bits. But it makes the
    -        code flow confusing, and it will become a problem if we try to make
    -        populate_pack_exts() do more things.
    +    So we could drop the call there, and leave the final loop to populate
    +    all of the bits. But instead, this patch does the reverse: drops the
    +    final loop, and teaches the other two sites to populate the bits as they
    +    add entries.
     
    -      - it would be nice to record the generated filenames as soon as
    -        possible. We don't currently use them for cleaning up from a failed
    -        operation, but a future patch will do so.
    +    This makes the code easier to reason about, as you never have to worry
    +    about when the util field is valid; it is always valid for each entry.
    +
    +    It also serves my ulterior purpose: recording the generated filenames as
    +    soon as possible will make it easier for a future patch to use them for
    +    cleaning up from a failed operation.
     
         Signed-off-by: Jeff King <peff@peff.net>
     
-:  ---------- > 3:  bbd7f82f88 repack: expand error message for missing pack files
3:  16448019ba ! 4:  7cd89d1575 repack: use tempfiles for signal cleanup
    @@ Commit message
         pack. Before this patch, the .tmp-* file for the main pack would have
         been left, but now we correctly clean it up.
     
    +    Two small subtleties on the implementation:
    +
    +      - in the renaming loop, we can stop re-constructing fname_old; we only
    +        use it when we have a tempfile to rename, so we can just ask the
    +        tempfile for its path (which, barring bugs, should be identical)
    +
    +      - when renaming fails, our error message mentions fname_old. But since
    +        a failed rename_tempfile() invalidates the tempfile struct, we'll
    +        lose access to that string. Instead, let's mention the destination
    +        filename, which is what most other callers do.
    +
         Reported-by: Jan Pokorný <poki@fnusa.cz>
         Signed-off-by: Jeff King <peff@peff.net>
     
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      
      	show_progress = !po_args.quiet && isatty(2);
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    - 			fname_old = mkpathdup("%s-%s%s",
    - 					packtmp, item->string, exts[ext].name);
    + 		struct generated_pack_data *data = item->util;
    + 
    + 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
    +-			char *fname, *fname_old;
    ++			char *fname;
    + 
    + 			fname = mkpathdup("%s/pack-%s%s",
    + 					packdir, item->string, exts[ext].name);
    +-			fname_old = mkpathdup("%s-%s%s",
    +-					packtmp, item->string, exts[ext].name);
      
     -			if (data->exts[ext]) {
     +			if (data->tempfiles[ext]) {
    ++				const char *fname_old = get_tempfile_path(data->tempfiles[ext]);
      				struct stat statbuffer;
    ++
      				if (!stat(fname_old, &statbuffer)) {
      					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
      					chmod(fname_old, statbuffer.st_mode);
      				}
      
     -				if (rename(fname_old, fname))
    +-					die_errno(_("renaming '%s' failed"), fname_old);
     +				if (rename_tempfile(&data->tempfiles[ext], fname))
    - 					die_errno(_("renaming '%s' failed"), fname_old);
    ++					die_errno(_("renaming pack to '%s' failed"), fname);
      			} else if (!exts[ext].optional)
    - 				die(_("missing required file: %s"), fname_old);
    + 				die(_("pack-objects did not write a '%s' file for pack %s-%s"),
    + 				    exts[ext].name, packtmp, item->string);
    + 			else if (unlink(fname) < 0 && errno != ENOENT)
    + 				die_errno(_("could not unlink: %s"), fname);
    + 
    + 			free(fname);
    +-			free(fname_old);
    + 		}
    + 	}
    + 	/* End of pack replacement. */
     
      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success TTY '--quiet disables progress' '
4:  c7b0f3823d = 5:  d7e9225b8e repack: drop remove_temporary_files()

[PATCH v2 1/5] repack: convert "names" util bitfield to array

[Jeff King]

We keep a string_list "names" containing the hashes of packs generated
on our behalf by pack-objects. The util field of each item is treated as
a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are
present for each name.

Let's switch this to allocating a real array. That will give us room in
a future patch to store more data than just a single bit per extension.
And it makes the code a little easier to read, as we avoid casting back
and forth between uintptr_t and a void pointer.

Since the only thing we're storing is an array, we could just allocate
it directly. But instead I've put it into a named struct here. That
further increases readability around the casts, and in particular helps
differentiate us from other string_lists in the same file which use
their util field differently. E.g., the existing_*_packs lists still do
bit-twiddling, but their bits have different meaning than the ones in
"names". This makes it hard to grep around the code to see how the util
fields are used; now you can look for "generated_pack_data".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index a5bacc7797..8e71230bf7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -247,11 +247,15 @@ static struct {
 	{".idx"},
 };
 
-static unsigned populate_pack_exts(char *name)
+struct generated_pack_data {
+	char exts[ARRAY_SIZE(exts)];
+};
+
+static struct generated_pack_data *populate_pack_exts(const char *name)
 {
 	struct stat statbuf;
 	struct strbuf path = STRBUF_INIT;
-	unsigned ret = 0;
+	struct generated_pack_data *data = xcalloc(1, sizeof(*data));
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(exts); i++) {
@@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)
 		if (stat(path.buf, &statbuf))
 			continue;
 
-		ret |= (1 << i);
+		data->exts[i] = 1;
 	}
 
 	strbuf_release(&path);
-	return ret;
+	return data;
 }
 
 static void repack_promisor_objects(const struct pack_objects_args *args,
@@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 					  line.buf);
 		write_promisor_file(promisor_name, NULL, 0);
 
-		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
+		item->util = populate_pack_exts(item->string);
 
 		free(promisor_name);
 	}
@@ -994,7 +998,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_sort(&names);
 
 	for_each_string_list_item(item, &names) {
-		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
+		item->util = populate_pack_exts(item->string);
 	}
 
 	close_object_store(the_repository->objects);
@@ -1003,6 +1007,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	 * Ok we have prepared all new packfiles.
 	 */
 	for_each_string_list_item(item, &names) {
+		struct generated_pack_data *data = item->util;
+
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
 			char *fname, *fname_old;
 
@@ -1011,7 +1017,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			fname_old = mkpathdup("%s-%s%s",
 					packtmp, item->string, exts[ext].name);
 
-			if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {
+			if (data->exts[ext]) {
 				struct stat statbuffer;
 				if (!stat(fname_old, &statbuffer)) {
 					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
@@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_midx_file(get_object_directory(), NULL, NULL, flags);
 	}
 
-	string_list_clear(&names, 0);
+	string_list_clear(&names, 1);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
-- 
2.38.1.496.ga614b0e9bd

[PATCH v2 2/5] repack: populate extension bits incrementally

[Jeff King]

After generating the main pack and then any additional cruft packs, we
iterate over the "names" list (which contains hashes of packs generated
by pack-objects), and call populate_pack_exts() for each.

There's one small problem with this. In repack_promisor_objects(), we
may add entries to "names" and call populate_pack_exts() for them.
Calling it again is mostly just wasteful, as we'll stat() the filename
with each possible extension, get the same result, and just overwrite
our bits.

So we could drop the call there, and leave the final loop to populate
all of the bits. But instead, this patch does the reverse: drops the
final loop, and teaches the other two sites to populate the bits as they
add entries.

This makes the code easier to reason about, as you never have to worry
about when the util field is valid; it is always valid for each entry.

It also serves my ulterior purpose: recording the generated filenames as
soon as possible will make it easier for a future patch to use them for
cleaning up from a failed operation.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 8e71230bf7..b5bd9e5fed 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -714,10 +714,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,
 
 	out = xfdopen(cmd.out, "r");
 	while (strbuf_getline_lf(&line, out) != EOF) {
+		struct string_list_item *item;
+
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only "
 			      "from pack-objects."));
-		string_list_append(names, line.buf);
+
+		item = string_list_append(names, line.buf);
+		item->util = populate_pack_exts(line.buf);
 	}
 	fclose(out);
 
@@ -956,9 +960,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	out = xfdopen(cmd.out, "r");
 	while (strbuf_getline_lf(&line, out) != EOF) {
+		struct string_list_item *item;
+
 		if (line.len != the_hash_algo->hexsz)
 			die(_("repack: Expecting full hex object ID lines only from pack-objects."));
-		string_list_append(&names, line.buf);
+		item = string_list_append(&names, line.buf);
+		item->util = populate_pack_exts(item->string);
 	}
 	fclose(out);
 	ret = finish_command(&cmd);
@@ -997,10 +1004,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	string_list_sort(&names);
 
-	for_each_string_list_item(item, &names) {
-		item->util = populate_pack_exts(item->string);
-	}
-
 	close_object_store(the_repository->objects);
 
 	/*
-- 
2.38.1.496.ga614b0e9bd

[PATCH v2 3/5] repack: expand error message for missing pack files

[Jeff King]

If pack-objects tells us it generated pack $hash, we expect to find
.tmp-$$-pack-$hash.pack, .idx, .rev, and so on. Some of these files are
optional, but others are not. For the required ones, we'll bail with an
error if any of them is missing.

The error message is just "missing required file", which is a bit vague.
We should be more clear that it is not the user's fault, but rather that
the sub-pgoram we called is not operating as expected. In practice,
nobody should ever see this message, as it would generally only be
caused by a bug in Git.

It probably doesn't make sense to convert this to a BUG(), though, as
there are other (unlikely) possibilities, such as somebody else racily
deleting the files, filesystem errors causing stat() to fail, and so on.

A nice side effect here is that we stop relying on fname_old in this
code path, which will let us deal with it only in the first part of the
conditional.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index b5bd9e5fed..d1929bb3db 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1030,7 +1030,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				if (rename(fname_old, fname))
 					die_errno(_("renaming '%s' failed"), fname_old);
 			} else if (!exts[ext].optional)
-				die(_("missing required file: %s"), fname_old);
+				die(_("pack-objects did not write a '%s' file for pack %s-%s"),
+				    exts[ext].name, packtmp, item->string);
 			else if (unlink(fname) < 0 && errno != ENOENT)
 				die_errno(_("could not unlink: %s"), fname);
 
-- 
2.38.1.496.ga614b0e9bd

[PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Jeff King]

When git-repack exits due to a signal, it tries to clean up by calling
its remove_temporary_files() function, which walks through the packs dir
looking for ".tmp-$$-pack-*" files to delete (where "$$" is the pid of
the current process).

The biggest problem here is that remove_temporary_files() is not safe to
call in a signal handler. It uses opendir(), which isn't on the POSIX
async-signal-safe list. The details will be platform-specific, but a
likely issue is that it needs to allocate memory; if we receive a signal
while inside malloc(), etc, we'll conflict on the allocator lock and
deadlock with ourselves.

We can fix this by just cleaning up the files directly, without walking
the directory. We already know the complete list of .tmp-* files that
were generated, because we recorded them via populate_pack_exts(). When
we find files there, we can use register_tempfile() to record the
filenames. If we receive a signal, then the tempfile API will clean them
up for us, and it's async-safe and pretty battle-tested.

Note that this is slightly racier than the existing scheme. We don't
record the filenames until pack-objects tells us the hash over stdout.
So during the period between it generating the file and reporting the
hash, we'd fail to clean up. However, that period is very small. During
most of the pack generation process pack-objects is using its own
internal tempfiles. It's only at the very end that it moves them into
the names git-repack expects, and then it immediately reports the name
to us. Given that cleanup like this is best effort (after all, we may
get SIGKILL), this level of race is acceptable.

When we register the tempfiles, we'll record them locally and use the
result to call rename_tempfile(), rather than renaming by hand.  This
isn't strictly necessary, as once we've renamed the files they're gone,
and the tempfile API's cleanup unlink() would simply become a pointless
noop. But managing the lifetimes of the tempfile objects is the cleanest
thing to do, and the tempfile pointers naturally fill the same role as
the old booleans.

This patch also fixes another small problem. We only hook signals, and
don't set up an atexit handler. So if we see an error that causes us to
die(), we'll leave the .tmp-* files in place. But since the tempfile API
handles this for us, this is now fixed for free. The new test covers
this by stimulating a failure of pack-objects when generating a cruft
pack. Before this patch, the .tmp-* file for the main pack would have
been left, but now we correctly clean it up.

Two small subtleties on the implementation:

  - in the renaming loop, we can stop re-constructing fname_old; we only
    use it when we have a tempfile to rename, so we can just ask the
    tempfile for its path (which, barring bugs, should be identical)

  - when renaming fails, our error message mentions fname_old. But since
    a failed rename_tempfile() invalidates the tempfile struct, we'll
    lose access to that string. Instead, let's mention the destination
    filename, which is what most other callers do.

Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 26 ++++++++------------------
 t/t7700-repack.sh |  8 ++++++++
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index d1929bb3db..39f03c3a1d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -122,13 +122,6 @@ static void remove_temporary_files(void)
 	strbuf_release(&buf);
 }
 
-static void remove_pack_on_signal(int signo)
-{
-	remove_temporary_files();
-	sigchain_pop(signo);
-	raise(signo);
-}
-
 /*
  * Adds all packs hex strings to either fname_nonkept_list or
  * fname_kept_list based on whether each pack has a corresponding
@@ -248,7 +241,7 @@ static struct {
 };
 
 struct generated_pack_data {
-	char exts[ARRAY_SIZE(exts)];
+	struct tempfile *tempfiles[ARRAY_SIZE(exts)];
 };
 
 static struct generated_pack_data *populate_pack_exts(const char *name)
@@ -265,7 +258,7 @@ static struct generated_pack_data *populate_pack_exts(const char *name)
 		if (stat(path.buf, &statbuf))
 			continue;
 
-		data->exts[i] = 1;
+		data->tempfiles[i] = register_tempfile(path.buf);
 	}
 
 	strbuf_release(&path);
@@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		split_pack_geometry(geometry, geometric_factor);
 	}
 
-	sigchain_push_common(remove_pack_on_signal);
-
 	prepare_pack_objects(&cmd, &po_args);
 
 	show_progress = !po_args.quiet && isatty(2);
@@ -1013,30 +1004,29 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		struct generated_pack_data *data = item->util;
 
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
-			char *fname, *fname_old;
+			char *fname;
 
 			fname = mkpathdup("%s/pack-%s%s",
 					packdir, item->string, exts[ext].name);
-			fname_old = mkpathdup("%s-%s%s",
-					packtmp, item->string, exts[ext].name);
 
-			if (data->exts[ext]) {
+			if (data->tempfiles[ext]) {
+				const char *fname_old = get_tempfile_path(data->tempfiles[ext]);
 				struct stat statbuffer;
+
 				if (!stat(fname_old, &statbuffer)) {
 					statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
 					chmod(fname_old, statbuffer.st_mode);
 				}
 
-				if (rename(fname_old, fname))
-					die_errno(_("renaming '%s' failed"), fname_old);
+				if (rename_tempfile(&data->tempfiles[ext], fname))
+					die_errno(_("renaming pack to '%s' failed"), fname);
 			} else if (!exts[ext].optional)
 				die(_("pack-objects did not write a '%s' file for pack %s-%s"),
 				    exts[ext].name, packtmp, item->string);
 			else if (unlink(fname) < 0 && errno != ENOENT)
 				die_errno(_("could not unlink: %s"), fname);
 
 			free(fname);
-			free(fname_old);
 		}
 	}
 	/* End of pack replacement. */
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index ca45c4cd2c..592016f64a 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -432,6 +432,14 @@ test_expect_success TTY '--quiet disables progress' '
 	test_must_be_empty stderr
 '
 
+test_expect_success 'clean up .tmp-* packs on error' '
+	test_must_fail git \
+		-c repack.cruftwindow=bogus \
+		repack -ad --cruft &&
+	find $objdir/pack -name '.tmp-*' >tmpfiles &&
+	test_must_be_empty tmpfiles
+'
+
 test_expect_success 'setup for update-server-info' '
 	git init update-server-info &&
 	test_commit -C update-server-info message
-- 
2.38.1.496.ga614b0e9bd

[PATCH v2 5/5] repack: drop remove_temporary_files()

[Jeff King]

After we've successfully finished the repack, we call
remove_temporary_files(), which looks for and removes any files matching
".tmp-$$-pack-*", where $$ is the pid of the current process. But this
is pointless. If we make it this far in the process, we've already
renamed these tempfiles into place, and there is nothing left to delete.

Nor is there a point in trying to call it to clean up when we _aren't_
successful. It's not safe for using in a signal handler, and the
previous commit already handed that job over to the tempfile API.

It might seem like it would be useful to clean up stray .tmp files left
by other invocations of git-repack. But it won't clean those files; it
only matches ones with its pid, and leaves the rest. Fortunately, those
are cleaned up naturally by successive calls to git-repack; we'll
consider .tmp-*.pack the same as normal packfiles, so "repack -ad", etc,
will roll up their contents and eventually delete them.

The one case that could matter is if pack-objects generates an extension
we don't know about, like ".tmp-pack-$$-$hash.some-new-ext". The current
code will quietly delete such a file, while after this patch we'd leave
it in place. In practice this doesn't happen, and would be indicative of
a bug. Leaving the file as cruft is arguably a better behavior, as it
means somebody is more likely to eventually notice and fix the bug.  If
we really wanted to be paranoid, we could scan for and warn about such
files, but that seems like overkill.

There's nothing to test with regard to the removal of this function. It
was doing nothing, so the behavior should be the same.  However, we can
verify (and protect) our assumption that "repack -ad" will eventually
remove stray files by adding a test for that.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 32 --------------------------------
 t/t7700-repack.sh |  8 ++++++++
 2 files changed, 8 insertions(+), 32 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 39f03c3a1d..cd338b161f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -91,37 +91,6 @@ static int repack_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-/*
- * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
- */
-static void remove_temporary_files(void)
-{
-	struct strbuf buf = STRBUF_INIT;
-	size_t dirlen, prefixlen;
-	DIR *dir;
-	struct dirent *e;
-
-	dir = opendir(packdir);
-	if (!dir)
-		return;
-
-	/* Point at the slash at the end of ".../objects/pack/" */
-	dirlen = strlen(packdir) + 1;
-	strbuf_addstr(&buf, packtmp);
-	/* Hold the length of  ".tmp-%d-pack-" */
-	prefixlen = buf.len - dirlen;
-
-	while ((e = readdir(dir))) {
-		if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
-			continue;
-		strbuf_setlen(&buf, dirlen);
-		strbuf_addstr(&buf, e->d_name);
-		unlink(buf.buf);
-	}
-	closedir(dir);
-	strbuf_release(&buf);
-}
-
 /*
  * Adds all packs hex strings to either fname_nonkept_list or
  * fname_kept_list based on whether each pack has a corresponding
@@ -1106,7 +1075,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	if (run_update_server_info)
 		update_server_info(0);
-	remove_temporary_files();
 
 	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {
 		unsigned flags = 0;
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 592016f64a..edcda849b9 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -440,6 +440,14 @@ test_expect_success 'clean up .tmp-* packs on error' '
 	test_must_be_empty tmpfiles
 '
 
+test_expect_success 'repack -ad cleans up old .tmp-* packs' '
+	git rev-parse HEAD >input &&
+	git pack-objects $objdir/pack/.tmp-1234 <input &&
+	git repack -ad &&
+	find $objdir/pack -name '.tmp-*' >tmpfiles &&
+	test_must_be_empty tmpfiles
+'
+
 test_expect_success 'setup for update-server-info' '
 	git init update-server-info &&
 	test_commit -C update-server-info message
-- 
2.38.1.496.ga614b0e9bd

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Jeff King]

On Fri, Oct 21, 2022 at 08:21:54PM -0400, Jeff King wrote:

> +test_expect_success 'clean up .tmp-* packs on error' '
> +	test_must_fail git \
> +		-c repack.cruftwindow=bogus \
> +		repack -ad --cruft &&
> +	find $objdir/pack -name '.tmp-*' >tmpfiles &&
> +	test_must_be_empty tmpfiles
> +'

Ugh, I think this test is racy. I saw a failure via SIGPIPE on OS X in
CI, and running it locally with --stress confirmed. I think the problem
is in our method to trigger pack-objects to fail for the cruft pack. We
pass a bogus command line argument, so pack-objects exits more or less
immediately. But the parent git-repack process is trying to write to its
stdin (to name packs to keep/exclude). So that write may succeed or fail
based on how quickly the child dies.

Some options:

  - find a different way to convince repack to die. The most likely is
    probably corrupting the cruft objects in some way such that
    pack-objects dies, but not until it starts doing real work.

  - mark the test_must_fail with ok=sigpipe. In most case this is a
    band-aid, but here we still test what we want. The .tmp cleanup
    should kick in from a die() or from a signal death, so either is
    sufficient for our purposes.

  - teach git-repack to ignore sigpipe. We don't actually check the
    writes to pack-objects (since they're done by fprintf), but we'd
    notice its failing exit code. And arguably this is improving the
    user experience. Saying "pack-objects died with an error" is more
    useful than a sigpipe.

Thoughts?

-Peff

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ugh, I think this test is racy. I saw a failure via SIGPIPE on OS X in
> CI, and running it locally with --stress confirmed. I think the problem
> is in our method to trigger pack-objects to fail for the cruft pack. We
> pass a bogus command line argument, so pack-objects exits more or less
> immediately. But the parent git-repack process is trying to write to its
> stdin (to name packs to keep/exclude). So that write may succeed or fail
> based on how quickly the child dies.

Yeah, I saw flaky failures myself during my integration tests.

> Some options:
>
>   - find a different way to convince repack to die. The most likely is
>     probably corrupting the cruft objects in some way such that
>     pack-objects dies, but not until it starts doing real work.
>
>   - mark the test_must_fail with ok=sigpipe. In most case this is a
>     band-aid, but here we still test what we want. The .tmp cleanup
>     should kick in from a die() or from a signal death, so either is
>     sufficient for our purposes.
>
>   - teach git-repack to ignore sigpipe. We don't actually check the
>     writes to pack-objects (since they're done by fprintf), but we'd
>     notice its failing exit code. And arguably this is improving the
>     user experience. Saying "pack-objects died with an error" is more
>     useful than a sigpipe.
>
> Thoughts?

I agree that for this particular one the second one is a reasonable
thing to do in the context of the test.  The third one may actually
be a better fix, exactly for the reason you state here.

Thanks.

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Jeff King]

On Sat, Oct 22, 2022 at 05:14:33PM -0700, Junio C Hamano wrote:

> > Some options:
> >
> >   - find a different way to convince repack to die. The most likely is
> >     probably corrupting the cruft objects in some way such that
> >     pack-objects dies, but not until it starts doing real work.
> >
> >   - mark the test_must_fail with ok=sigpipe. In most case this is a
> >     band-aid, but here we still test what we want. The .tmp cleanup
> >     should kick in from a die() or from a signal death, so either is
> >     sufficient for our purposes.
> >
> >   - teach git-repack to ignore sigpipe. We don't actually check the
> >     writes to pack-objects (since they're done by fprintf), but we'd
> >     notice its failing exit code. And arguably this is improving the
> >     user experience. Saying "pack-objects died with an error" is more
> >     useful than a sigpipe.
> >
> > Thoughts?
> 
> I agree that for this particular one the second one is a reasonable
> thing to do in the context of the test.  The third one may actually
> be a better fix, exactly for the reason you state here.

Here's a patch on top of of jk/repack-tempfile-cleanup that adjusts the
test (and should make the immediate racy CI pain go away). It gives some
explanation why the third option isn't as interesting as you'd think. If
somebody later wants to add such a "pack-objects died" error, we can
adjust sigpipe handling there.

-- >8 --
Subject: [PATCH] t7700: annotate cruft-pack failure with ok=sigpipe

One of our tests intentionally causes the cruft-pack generation phase of
repack to fail, in order to stimulate an exit from repack at the desired
moment. It does so by feeding a bogus option argument to pack-objects.
This is a simple and reliable way to get pack-objects to fail, but it
has one downside: pack-objects will die before reading its stdin, which
means the caller repack may racily get SIGPIPE writing to it.

For the purposes of this test, that's OK. We are checking whether repack
cleans up already-created .tmp files, and it will do so whether it exits
or dies by signal (because the tempfile API hooks both).

But we have to tell test_must_fail that either outcome is OK, or it
complains about the signal. Arguably this is a workaround (compared to
fixing repack), as repack dying to SIGPIPE means that it loses the
opportunity to give a more detailed message. But we don't actually write
such a message anyway; we rely on pack-objects to have written something
useful to stderr, and it does. In either case (signal or exit), that is
the main thing the user will see.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7700-repack.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index edcda849b9..9164acbe02 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -433,7 +433,7 @@ test_expect_success TTY '--quiet disables progress' '
 '
 
 test_expect_success 'clean up .tmp-* packs on error' '
-	test_must_fail git \
+	test_must_fail ok=sigpipe git \
 		-c repack.cruftwindow=bogus \
 		repack -ad --cruft &&
 	find $objdir/pack -name '.tmp-*' >tmpfiles &&
-- 
2.38.1.500.ge50734af7a

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a patch on top of of jk/repack-tempfile-cleanup that adjusts the
> test (and should make the immediate racy CI pain go away). It gives some
> explanation why the third option isn't as interesting as you'd think. If
> somebody later wants to add such a "pack-objects died" error, we can
> adjust sigpipe handling there.

An extremely simplified alternative would be just to say !  instead
of test_must_fail, which essentially is ok=anycrash ;-)

I did try the same exact patch before going to bed last night but
t7700 somehow failed some other steps in my local tests and I gave
up digging further X-<.  One step at a time...

Will queue.  Thanks.

> -- >8 --
> Subject: [PATCH] t7700: annotate cruft-pack failure with ok=sigpipe
>
> One of our tests intentionally causes the cruft-pack generation phase of
> repack to fail, in order to stimulate an exit from repack at the desired
> moment. It does so by feeding a bogus option argument to pack-objects.
> This is a simple and reliable way to get pack-objects to fail, but it
> has one downside: pack-objects will die before reading its stdin, which
> means the caller repack may racily get SIGPIPE writing to it.
>
> For the purposes of this test, that's OK. We are checking whether repack
> cleans up already-created .tmp files, and it will do so whether it exits
> or dies by signal (because the tempfile API hooks both).
>
> But we have to tell test_must_fail that either outcome is OK, or it
> complains about the signal. Arguably this is a workaround (compared to
> fixing repack), as repack dying to SIGPIPE means that it loses the
> opportunity to give a more detailed message. But we don't actually write
> such a message anyway; we rely on pack-objects to have written something
> useful to stderr, and it does. In either case (signal or exit), that is
> the main thing the user will see.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t7700-repack.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index edcda849b9..9164acbe02 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -433,7 +433,7 @@ test_expect_success TTY '--quiet disables progress' '
>  '
>  
>  test_expect_success 'clean up .tmp-* packs on error' '
> -	test_must_fail git \
> +	test_must_fail ok=sigpipe git \
>  		-c repack.cruftwindow=bogus \
>  		repack -ad --cruft &&
>  	find $objdir/pack -name '.tmp-*' >tmpfiles &&

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Jeff King]

On Sun, Oct 23, 2022 at 11:08:26AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Here's a patch on top of of jk/repack-tempfile-cleanup that adjusts the
> > test (and should make the immediate racy CI pain go away). It gives some
> > explanation why the third option isn't as interesting as you'd think. If
> > somebody later wants to add such a "pack-objects died" error, we can
> > adjust sigpipe handling there.
> 
> An extremely simplified alternative would be just to say !  instead
> of test_must_fail, which essentially is ok=anycrash ;-)

I would have been tempted to do that if we hadn't already added
ok=sigpipe long ago. :)

> I did try the same exact patch before going to bed last night but
> t7700 somehow failed some other steps in my local tests and I gave
> up digging further X-<.  One step at a time...

Hopefully not my bits. :) FWIW, the tips of "master", "next", and "seen"
all pass just fine for me (minus this race on seen).

-Peff

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I did try the same exact patch before going to bed last night but
>> t7700 somehow failed some other steps in my local tests and I gave
>> up digging further X-<.  One step at a time...
>
> Hopefully not my bits. :) FWIW, the tips of "master", "next", and "seen"
> all pass just fine for me (minus this race on seen).

Not your bits.  What I am pushing out on 'seen' now has a few topics
excluded to avoid CI breakages, reproducible or flaky.

Thanks.