{"thread":{"id":"58671","subject":"[PATCH 0/4] repack tempfile-cleanup signal deadlock","startedAt":"2022-10-21T21:41:09Z","lastAt":"2022-10-23T21:48:16Z","messageCount":34,"participants":["Jeff King","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"465510","messageId":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","threadId":"58671","inReplyTo":null,"subject":"[PATCH 0/4] repack tempfile-cleanup signal deadlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T21:41:01Z","receivedAt":"2022-10-21T21:41:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a series which should fix the deadlock Jan reported in:\n\n https://lore.kernel.org/git/00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz/\n\nI split it into a new thread since the problem here is distinct from the\none fixed in that thread (but they're indeed the same class of problem).\nThe fix is along the same lines as what I showed there, switching to\nusing the tempfile API. But this has the cleanups I noted there, plus\nnice things like commit messages and tests.\n\nAs I noted there, pack-objects is still happy to leave its internal\ntempfiles sitting around. I didn't tackle that in this series, since it\nreally is orthogonal, at least in terms of implementation.\n\nI've cc'd Taylor for review. The main problem here goes all the way back\nto a1bbc6c017 (repack: rewrite the shell script in C, 2013-09-15), but\nthe solution is enabled by your more recent populate_pack_ext(), etc.\n\n [1/4]: repack: convert \"names\" util bitfield to array\n [2/4]: repack: populate extension bits incrementally\n [3/4]: repack: use tempfiles for signal cleanup\n [4/4]: repack: drop remove_temporary_files()\n\n builtin/repack.c | 78 ++++++++++++++---------------------------------\n t/t7700-repack.sh | 16 ++++++++++\n 2 files changed, 39 insertions(+), 55 deletions(-)\n\n-Peff\n"},{"id":"465511","messageId":"Y1MSWAx+baTklfLL@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","subject":"[PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T21:42:48Z","receivedAt":"2022-10-21T21:42:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a string_list \"names\" containing the hashes of packs generated\non our behalf by pack-objects. The util field of each item is treated as\na bitfield that tells us which extensions (.pack, .idx, .rev, etc) are\npresent for each name.\n\nLet's switch this to allocating a real array. That will give us room in\na future patch to store more data than just a single bit per extension.\nAnd it makes the code a little easier to read, as we avoid casting back\nand forth between uintptr_t and a void pointer.\n\nSince the only thing we're storing is an array, we could just allocate\nit directly. But instead I've put it into a named struct here. That\nfurther increases readability around the casts, and in particular helps\ndifferentiate us from other string_lists in the same file which use\ntheir util field differently. E.g., the existing_*_packs lists still do\nbit-twiddling, but their bits have different meaning than the ones in\n\"names\". This makes it hard to grep around the code to see how the util\nfields are used; now you can look for \"generated_pack_data\".\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..8e71230bf7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -247,11 +247,15 @@ static struct {\n \t{\".idx\"},\n };\n \n-static unsigned populate_pack_exts(char *name)\n+struct generated_pack_data {\n+\tchar exts[ARRAY_SIZE(exts)];\n+};\n+\n+static struct generated_pack_data *populate_pack_exts(const char *name)\n {\n \tstruct stat statbuf;\n \tstruct strbuf path = STRBUF_INIT;\n-\tunsigned ret = 0;\n+\tstruct generated_pack_data *data = xcalloc(1, sizeof(*data));\n \tint i;\n \n \tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n@@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)\n \t\tif (stat(path.buf, &statbuf))\n \t\t\tcontinue;\n \n-\t\tret |= (1 << i);\n+\t\tdata->exts[i] = 1;\n \t}\n \n \tstrbuf_release(&path);\n-\treturn ret;\n+\treturn data;\n }\n \n static void repack_promisor_objects(const struct pack_objects_args *args,\n@@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t\t line.buf);\n \t\twrite_promisor_file(promisor_name, NULL, 0);\n \n-\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\t\titem->util = populate_pack_exts(item->string);\n \n \t\tfree(promisor_name);\n \t}\n@@ -994,7 +998,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_sort(&names);\n \n \tfor_each_string_list_item(item, &names) {\n-\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\t\titem->util = populate_pack_exts(item->string);\n \t}\n \n \tclose_object_store(the_repository->objects);\n@@ -1003,6 +1007,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t * Ok we have prepared all new packfiles.\n \t */\n \tfor_each_string_list_item(item, &names) {\n+\t\tstruct generated_pack_data *data = item->util;\n+\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n \t\t\tchar *fname, *fname_old;\n \n@@ -1011,7 +1017,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n \n-\t\t\tif (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {\n+\t\t\tif (data->exts[ext]) {\n \t\t\t\tstruct stat statbuffer;\n \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n@@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n \t}\n \n-\tstring_list_clear(&names, 0);\n+\tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465512","messageId":"Y1MSkubbDBh0Ma1G@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","subject":"[PATCH 2/4] repack: populate extension bits incrementally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T21:43:46Z","receivedAt":"2022-10-21T21:43:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After generating the main pack and then any additional cruft packs, we\niterate over the \"names\" list (which contains hashes of packs generated\nby pack-objects), and call populate_pack_exts() for each.\n\nThere are two small problems with this:\n\n - repack_promisor_objects() may have added entries to \"names\", and\n already called populate_pack_exts() for them. This is mostly just\n wasteful, as we'll stat() the filename with each possible extension,\n get the same result, and just overwrite our bits. But it makes the\n code flow confusing, and it will become a problem if we try to make\n populate_pack_exts() do more things.\n\n - it would be nice to record the generated filenames as soon as\n possible. We don't currently use them for cleaning up from a failed\n operation, but a future patch will do so.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8e71230bf7..b5bd9e5fed 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -714,10 +714,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tstruct string_list_item *item;\n+\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only \"\n \t\t\t \"from pack-objects.\"));\n-\t\tstring_list_append(names, line.buf);\n+\n+\t\titem = string_list_append(names, line.buf);\n+\t\titem->util = populate_pack_exts(line.buf);\n \t}\n \tfclose(out);\n \n@@ -956,9 +960,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tstruct string_list_item *item;\n+\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n-\t\tstring_list_append(&names, line.buf);\n+\t\titem = string_list_append(&names, line.buf);\n+\t\titem->util = populate_pack_exts(item->string);\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\n@@ -997,10 +1004,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tstring_list_sort(&names);\n \n-\tfor_each_string_list_item(item, &names) {\n-\t\titem->util = populate_pack_exts(item->string);\n-\t}\n-\n \tclose_object_store(the_repository->objects);\n \n \t/*\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465513","messageId":"Y1MTJz3wy5xDEPEH@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","subject":"[PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T21:46:15Z","receivedAt":"2022-10-21T21:46:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-repack exits due to a signal, it tries to clean up by calling\nits remove_temporary_files() function, which walks through the packs dir\nlooking for \".tmp-$$-pack-*\" files to delete (where \"$$\" is the pid of\nthe current process).\n\nThe biggest problem here is that remove_temporary_files() is not safe to\ncall in a signal handler. It uses opendir(), which isn't on the POSIX\nasync-signal-safe list. The details will be platform-specific, but a\nlikely issue is that it needs to allocate memory; if we receive a signal\nwhile inside malloc(), etc, we'll conflict on the allocator lock and\ndeadlock with ourselves.\n\nWe can fix this by just cleaning up the files directly, without walking\nthe directory. We already know the complete list of .tmp-* files that\nwere generated, because we recorded them via populate_pack_exts(). When\nwe find files there, we can use register_tempfile() to record the\nfilenames. If we receive a signal, then the tempfile API will clean them\nup for us, and it's async-safe and pretty battle-tested.\n\nNote that this is slightly racier than the existing scheme. We don't\nrecord the filenames until pack-objects tells us the hash over stdout.\nSo during the period between it generating the file and reporting the\nhash, we'd fail to clean up. However, that period is very small. During\nmost of the pack generation process pack-objects is using its own\ninternal tempfiles. It's only at the very end that it moves them into\nthe names git-repack expects, and then it immediately reports the name\nto us. Given that cleanup like this is best effort (after all, we may\nget SIGKILL), this level of race is acceptable.\n\nWhen we register the tempfiles, we'll record them locally and use the\nresult to call rename_tempfile(), rather than renaming by hand. This\nisn't strictly necessary, as once we've renamed the files they're gone,\nand the tempfile API's cleanup unlink() would simply become a pointless\nnoop. But managing the lifetimes of the tempfile objects is the cleanest\nthing to do, and the tempfile pointers naturally fill the same role as\nthe old booleans.\n\nThis patch also fixes another small problem. We only hook signals, and\ndon't set up an atexit handler. So if we see an error that causes us to\ndie(), we'll leave the .tmp-* files in place. But since the tempfile API\nhandles this for us, this is now fixed for free. The new test covers\nthis by stimulating a failure of pack-objects when generating a cruft\npack. Before this patch, the .tmp-* file for the main pack would have\nbeen left, but now we correctly clean it up.\n\nReported-by: Jan Pokorný \nSigned-off-by: Jeff King \n---\n builtin/repack.c | 17 ++++-------------\n t/t7700-repack.sh | 8 ++++++++\n 2 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex b5bd9e5fed..15b6f24626 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -122,13 +122,6 @@ static void remove_temporary_files(void)\n \tstrbuf_release(&buf);\n }\n \n-static void remove_pack_on_signal(int signo)\n-{\n-\tremove_temporary_files();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n /*\n * Adds all packs hex strings to either fname_nonkept_list or\n * fname_kept_list based on whether each pack has a corresponding\n@@ -248,7 +241,7 @@ static struct {\n };\n \n struct generated_pack_data {\n-\tchar exts[ARRAY_SIZE(exts)];\n+\tstruct tempfile *tempfiles[ARRAY_SIZE(exts)];\n };\n \n static struct generated_pack_data *populate_pack_exts(const char *name)\n@@ -265,7 +258,7 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \t\tif (stat(path.buf, &statbuf))\n \t\t\tcontinue;\n \n-\t\tdata->exts[i] = 1;\n+\t\tdata->tempfiles[i] = register_tempfile(path.buf);\n \t}\n \n \tstrbuf_release(&path);\n@@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \n-\tsigchain_push_common(remove_pack_on_signal);\n-\n \tprepare_pack_objects(&cmd, &po_args);\n \n \tshow_progress = !po_args.quiet && isatty(2);\n@@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n \n-\t\t\tif (data->exts[ext]) {\n+\t\t\tif (data->tempfiles[ext]) {\n \t\t\t\tstruct stat statbuffer;\n \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n \t\t\t\t}\n \n-\t\t\t\tif (rename(fname_old, fname))\n+\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n \t\t\t} else if (!exts[ext].optional)\n \t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..592016f64a 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -432,6 +432,14 @@ test_expect_success TTY '--quiet disables progress' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'clean up .tmp-* packs on error' '\n+\ttest_must_fail git \\\n+\t\t-c repack.cruftwindow=bogus \\\n+\t\trepack -ad --cruft &&\n+\tfind $objdir/pack -name '.tmp-*' >tmpfiles &&\n+\ttest_must_be_empty tmpfiles\n+'\n+\n test_expect_success 'setup for update-server-info' '\n \tgit init update-server-info &&\n \ttest_commit -C update-server-info message\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465515","messageId":"Y1MTtgOOWNovPawH@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","subject":"[PATCH 4/4] repack: drop remove_temporary_files()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T21:48:38Z","receivedAt":"2022-10-21T21:48:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After we've successfully finished the repack, we call\nremove_temporary_files(), which looks for and removes any files matching\n\".tmp-$$-pack-*\", where $$ is the pid of the current process. But this\nis pointless. If we make it this far in the process, we've already\nrenamed these tempfiles into place, and there is nothing left to delete.\n\nNor is there a point in trying to call it to clean up when we _aren't_\nsuccessful. It's not safe for using in a signal handler, and the\nprevious commit already handed that job over to the tempfile API.\n\nIt might seem like it would be useful to clean up stray .tmp files left\nby other invocations of git-repack. But it won't clean those files; it\nonly matches ones with its pid, and leaves the rest. Fortunately, those\nare cleaned up naturally by successive calls to git-repack; we'll\nconsider .tmp-*.pack the same as normal packfiles, so \"repack -ad\", etc,\nwill roll up their contents and eventually delete them.\n\nThe one case that could matter is if pack-objects generates an extension\nwe don't know about, like \".tmp-pack-$$-$hash.some-new-ext\". The current\ncode will quietly delete such a file, while after this patch we'd leave\nit in place. In practice this doesn't happen, and would be indicative of\na bug. Leaving the file as cruft is arguably a better behavior, as it\nmeans somebody is more likely to eventually notice and fix the bug. If\nwe really wanted to be paranoid, we could scan for and warn about such\nfiles, but that seems like overkill.\n\nThere's nothing to test with regard to the removal of this function. It\nwas doing nothing, so the behavior should be the same. However, we can\nverify (and protect) our assumption that \"repack -ad\" will eventually\nremove stray files by adding a test for that.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 32 --------------------------------\n t/t7700-repack.sh | 8 ++++++++\n 2 files changed, 8 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 15b6f24626..68294c7bc7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -91,37 +91,6 @@ static int repack_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-/*\n- * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.\n- */\n-static void remove_temporary_files(void)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tsize_t dirlen, prefixlen;\n-\tDIR *dir;\n-\tstruct dirent *e;\n-\n-\tdir = opendir(packdir);\n-\tif (!dir)\n-\t\treturn;\n-\n-\t/* Point at the slash at the end of \".../objects/pack/\" */\n-\tdirlen = strlen(packdir) + 1;\n-\tstrbuf_addstr(&buf, packtmp);\n-\t/* Hold the length of \".tmp-%d-pack-\" */\n-\tprefixlen = buf.len - dirlen;\n-\n-\twhile ((e = readdir(dir))) {\n-\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n-\t\t\tcontinue;\n-\t\tstrbuf_setlen(&buf, dirlen);\n-\t\tstrbuf_addstr(&buf, e->d_name);\n-\t\tunlink(buf.buf);\n-\t}\n-\tclosedir(dir);\n-\tstrbuf_release(&buf);\n-}\n-\n /*\n * Adds all packs hex strings to either fname_nonkept_list or\n * fname_kept_list based on whether each pack has a corresponding\n@@ -1106,7 +1075,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (run_update_server_info)\n \t\tupdate_server_info(0);\n-\tremove_temporary_files();\n \n \tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {\n \t\tunsigned flags = 0;\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 592016f64a..edcda849b9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -440,6 +440,14 @@ test_expect_success 'clean up .tmp-* packs on error' '\n \ttest_must_be_empty tmpfiles\n '\n \n+test_expect_success 'repack -ad cleans up old .tmp-* packs' '\n+\tgit rev-parse HEAD >input &&\n+\tgit pack-objects $objdir/pack/.tmp-1234 tmpfiles &&\n+\ttest_must_be_empty tmpfiles\n+'\n+\n test_expect_success 'setup for update-server-info' '\n \tgit init update-server-info &&\n \ttest_commit -C update-server-info message\n-- \n2.38.1.496.ga614b0e9bd\n"},{"id":"465524","messageId":"xmqqtu3wizuc.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1MSWAx+baTklfLL@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T22:19:55Z","receivedAt":"2022-10-21T22:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We keep a string_list \"names\" containing the hashes of packs generated\n> on our behalf by pack-objects. The util field of each item is treated as\n> a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are\n> present for each name.\n>\n> Let's switch this to allocating a real array. That will give us room in\n> a future patch to store more data than just a single bit per extension.\n> And it makes the code a little easier to read, as we avoid casting back\n> and forth between uintptr_t and a void pointer.\n>\n> Since the only thing we're storing is an array, we could just allocate\n> it directly. But instead I've put it into a named struct here. That\n> further increases readability around the casts, and in particular helps\n> differentiate us from other string_lists in the same file which use\n> their util field differently. E.g., the existing_*_packs lists still do\n> bit-twiddling, but their bits have different meaning than the ones in\n> \"names\". This makes it hard to grep around the code to see how the util\n> fields are used; now you can look for \"generated_pack_data\".\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/repack.c | 22 ++++++++++++++--------\n> 1 file changed, 14 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index a5bacc7797..8e71230bf7 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -247,11 +247,15 @@ static struct {\n> \t{\".idx\"},\n> };\n> \n> -static unsigned populate_pack_exts(char *name)\n> +struct generated_pack_data {\n> +\tchar exts[ARRAY_SIZE(exts)];\n> +};\n\nOK, so instead of a single \"unsigned\" word holding six bits (the\nnumber of elements in the exts[] array), we have one byte per the\nfile extension. Since ...\n\n> +static struct generated_pack_data *populate_pack_exts(const char *name)\n> {\n> \tstruct stat statbuf;\n> \tstruct strbuf path = STRBUF_INIT;\n> -\tunsigned ret = 0;\n> +\tstruct generated_pack_data *data = xcalloc(1, sizeof(*data));\n\n... this is allocated a real piece of storage and the function\nreturns a pointer to it, ...\n\n> -\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n> +\t\titem->util = populate_pack_exts(item->string);\n\n... we no longer need to cast but the value can go straight to the\n.util member, and ...\n\n> -\tstring_list_clear(&names, 0);\n> +\tstring_list_clear(&names, 1);\n\n... we now need to free these structs pointed at by the .util\nmember.\n\nAll make sense.\n"},{"id":"465533","messageId":"xmqqmt9oizcq.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1MTJz3wy5xDEPEH@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T22:30:29Z","receivedAt":"2022-10-21T22:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This patch also fixes another small problem. We only hook signals, and\n> don't set up an atexit handler. So if we see an error that causes us to\n> die(), we'll leave the .tmp-* files in place. But since the tempfile API\n> handles this for us, this is now fixed for free. The new test covers\n> this by stimulating a failure of pack-objects when generating a cruft\n> pack. Before this patch, the .tmp-* file for the main pack would have\n> been left, but now we correctly clean it up.\n>\n> Reported-by: Jan Pokorný \n> Signed-off-by: Jeff King \n> ---\n> builtin/repack.c | 17 ++++-------------\n> t/t7700-repack.sh | 8 ++++++++\n> 2 files changed, 12 insertions(+), 13 deletions(-)\n\nNice. \n\n> struct generated_pack_data {\n> -\tchar exts[ARRAY_SIZE(exts)];\n> +\tstruct tempfile *tempfiles[ARRAY_SIZE(exts)];\n> ...\n> -\t\tdata->exts[i] = 1;\n> +\t\tdata->tempfiles[i] = register_tempfile(path.buf);\n\nOK.\n\n> \t}\n> \n> \tstrbuf_release(&path);\n> @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\tsplit_pack_geometry(geometry, geometric_factor);\n> \t}\n> \n> -\tsigchain_push_common(remove_pack_on_signal);\n> -\n> \tprepare_pack_objects(&cmd, &po_args);\n> \n> \tshow_progress = !po_args.quiet && isatty(2);\n> @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> \n> -\t\t\tif (data->exts[ext]) {\n> +\t\t\tif (data->tempfiles[ext]) {\n> \t\t\t\tstruct stat statbuffer;\n> \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> \t\t\t\t}\n> \n> -\t\t\t\tif (rename(fname_old, fname))\n> +\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n> \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n\nIt now got a bit confusing that we have 'fname', 'fname_old', and\nthe tempfile. The path.buf used as the argument to register_tempfile()\nmatches what is used to compute fname_old above. I wonder if tempfile\nAPI does not give us that name so that we can stop using fname_old here?\n\n"},{"id":"465534","messageId":"Y1Mm9zTX3XwN3IWu@nand.local","threadId":"58671","inReplyTo":"Y1MSWAx+baTklfLL@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T23:10:47Z","receivedAt":"2022-10-21T23:10:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 05:42:48PM -0400, Jeff King wrote:\n> We keep a string_list \"names\" containing the hashes of packs generated\n> on our behalf by pack-objects. The util field of each item is treated as\n> a bitfield that tells us which extensions (.pack, .idx, .rev, etc) are\n> present for each name.\n>\n> Let's switch this to allocating a real array. That will give us room in\n> a future patch to store more data than just a single bit per extension.\n> And it makes the code a little easier to read, as we avoid casting back\n> and forth between uintptr_t and a void pointer.\n\nThanks, this is a nice summary that matches my own recollection from\nworking in this area.\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index a5bacc7797..8e71230bf7 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -247,11 +247,15 @@ static struct {\n> \t{\".idx\"},\n> };\n>\n> -static unsigned populate_pack_exts(char *name)\n> +struct generated_pack_data {\n> +\tchar exts[ARRAY_SIZE(exts)];\n> +};\n\nMakes sense, this replaces the individual bits of the `void *` we were\ntreating as an array of \"which extensions associated with this pack were\ngenerated by pack-objects\"?\n\n> +\n> +static struct generated_pack_data *populate_pack_exts(const char *name)\n> {\n> \tstruct stat statbuf;\n> \tstruct strbuf path = STRBUF_INIT;\n> -\tunsigned ret = 0;\n> +\tstruct generated_pack_data *data = xcalloc(1, sizeof(*data));\n\nI'm nitpicking, but we could replace this with;\n\n struct generated_pack_data *data;\n\n CALLOC_ARRAY(data, 1);\n\nso that we don't have to rely on calling sizeof(*data). But\nsizeof(*data) will always give us the right answer anyway, even if the\nname of data's type changed, so what you have is fine, too.\n\n> @@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)\n> \t\tif (stat(path.buf, &statbuf))\n> \t\t\tcontinue;\n>\n> -\t\tret |= (1 << i);\n> +\t\tdata->exts[i] = 1;\n\nGreat. I'm happy to see this bit-twiddling go away, but..\n\n> @@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> \t\t\t\t\t line.buf);\n> \t\twrite_promisor_file(promisor_name, NULL, 0);\n>\n> -\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n> +\t\titem->util = populate_pack_exts(item->string);\n\nI'm even happier to see this go away ;-).\n\n> @@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n> \t}\n>\n> -\tstring_list_clear(&names, 0);\n> +\tstring_list_clear(&names, 1);\n\nGood. Now that we're actually allocating memory in the `->util` field,\nwe care about freeing it, too. Thanks for remembering this.\n\nThanks,\nTaylor\n"},{"id":"465535","messageId":"Y1MpUHBQtq8uP5Uy@nand.local","threadId":"58671","inReplyTo":"Y1MSkubbDBh0Ma1G@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] repack: populate extension bits incrementally","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T23:20:48Z","receivedAt":"2022-10-21T23:20:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:\n> There are two small problems with this:\n>\n> - repack_promisor_objects() may have added entries to \"names\", and\n> already called populate_pack_exts() for them. This is mostly just\n> wasteful, as we'll stat() the filename with each possible extension,\n> get the same result, and just overwrite our bits. But it makes the\n> code flow confusing, and it will become a problem if we try to make\n> populate_pack_exts() do more things.\n\nHmm. I agree with you that repack_promisor_objects() calling\npopulate_pack_exts() itself is at best weird, and at worst wasteful.\n>\n> - it would be nice to record the generated filenames as soon as\n> possible. We don't currently use them for cleaning up from a failed\n> operation, but a future patch will do so.\n\nThat said, I think that it's worth having a single spot within\ncmd_repack() that is responsible for populating the generated pack\nextensions, since it protects against a caller forgetting to do so (and\nthen tricking repack into thinking that we didn't generate *any* of the\ncorresponding extensions).\n\nBut I'm sure future patch you're referring to cares about knowing\nthese as soon as possible, since that's the point of this series ;-).\n\nI think a reasonable middle ground here is to do something like the\nfollowing on top of this patch:\n\n--- >8 ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex b5bd9e5fed..16a941f48b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1002,6 +1002,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\treturn ret;\n \t}\n\n+\tfor_each_string_list_item(item, &names) {\n+\t\tif (!item->util)\n+\t\t\tBUG(\"missing generated_pack_data for pack %s\",\n+\t\t\t item->string);\n+\t}\n+\n \tstring_list_sort(&names);\n\n \tclose_object_store(the_repository->objects);\n--- 8< ---\n\nwhich still lets you eagerly keep track of the generated pack extensions\nwhile also protecting against forgetful callers. Obviously we're relying\non a runtime check which is going to be somewhat weaker. But I think\n\nThe patch itself looks good.\n\nThanks,\nTaylor\n"},{"id":"465536","messageId":"Y1MqPjnYoWcOgXo6@coredump.intra.peff.net","threadId":"58671","inReplyTo":"xmqqmt9oizcq.fsf@gitster.g","subject":"Re: [PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T23:24:46Z","receivedAt":"2022-10-21T23:24:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 03:30:29PM -0700, Junio C Hamano wrote:\n\n> > @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\tsplit_pack_geometry(geometry, geometric_factor);\n> > \t}\n> > \n> > -\tsigchain_push_common(remove_pack_on_signal);\n> > -\n> > \tprepare_pack_objects(&cmd, &po_args);\n> > \n> > \tshow_progress = !po_args.quiet && isatty(2);\n> > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> > \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> > \n> > -\t\t\tif (data->exts[ext]) {\n> > +\t\t\tif (data->tempfiles[ext]) {\n> > \t\t\t\tstruct stat statbuffer;\n> > \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> > \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> > \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> > \t\t\t\t}\n> > \n> > -\t\t\t\tif (rename(fname_old, fname))\n> > +\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n> > \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> \n> It now got a bit confusing that we have 'fname', 'fname_old', and\n> the tempfile. The path.buf used as the argument to register_tempfile()\n> matches what is used to compute fname_old above. I wonder if tempfile\n> API does not give us that name so that we can stop using fname_old here?\n\nIt does, and we probably should use get_tempfile_path() in the error\nmessage here. But sadly we can't get rid of fname_old entirely, as it's\nused below this for the second block in the if-else chain:\n\n if (data->tempfiles[ext]) {\n ...do the rename ...\n } else if (!exts[ext].optional)\n\t die(_(\"missing required file: %s\"), fname_old);\n else if (unlink(fname) < 0 && errno != ENOENT)\n\t die_errno(_(\"could not unlink: %s\"), fname);\n\nOTOH, it would probably be equally readable (or perhaps even better) for\nthat second block to say:\n\n die(\"pack-objects did not generate a '%s' file for pack %s\",\n exts[ext].name, item->string);\n\nAnd then we could drop fname_old entirely. Which is nice, because it\ngets rid of the implicit assumption that the tempfile matches what is in\nfname_old (which is always true, but since they are generated by\nindividual lines far apart from each other, it's possible for that to\nchange).\n\n-Peff\n"},{"id":"465537","messageId":"Y1MrXoobkVKngYL1@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1Mm9zTX3XwN3IWu@nand.local","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T23:29:34Z","receivedAt":"2022-10-21T23:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 07:10:47PM -0400, Taylor Blau wrote:\n\n> > +\tstruct generated_pack_data *data = xcalloc(1, sizeof(*data));\n> \n> I'm nitpicking, but we could replace this with;\n> \n> struct generated_pack_data *data;\n> \n> CALLOC_ARRAY(data, 1);\n> \n> so that we don't have to rely on calling sizeof(*data). But\n> sizeof(*data) will always give us the right answer anyway, even if the\n> name of data's type changed, so what you have is fine, too.\n\nYeah, I actually considered writing it that way, but it felt a bit silly\nto use _ARRAY for something which is clearly meant to be a single item.\nGrepping for \"CALLOC_ARRAY([^)]*, 1)\" does seem to turn up quite a few\nhits, though, so maybe it is just me.\n\nWe could also introduce:\n\n #define CALLOC(x) CALLOC_ARRAY((x), 1)\n\nbut even if we think that is a good idea, it should not be in this\nseries.\n\nIt looks I may re-roll for the fname_old stuff in the other part of the\nthread, so I'll take your suggestion.\n\n-Peff\n"},{"id":"465538","messageId":"Y1MsdAgL8fdIRtxH@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MpUHBQtq8uP5Uy@nand.local","subject":"Re: [PATCH 2/4] repack: populate extension bits incrementally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T23:34:12Z","receivedAt":"2022-10-21T23:34:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:\n\n> On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:\n> > There are two small problems with this:\n> >\n> > - repack_promisor_objects() may have added entries to \"names\", and\n> > already called populate_pack_exts() for them. This is mostly just\n> > wasteful, as we'll stat() the filename with each possible extension,\n> > get the same result, and just overwrite our bits. But it makes the\n> > code flow confusing, and it will become a problem if we try to make\n> > populate_pack_exts() do more things.\n> \n> Hmm. I agree with you that repack_promisor_objects() calling\n> populate_pack_exts() itself is at best weird, and at worst wasteful.\n\nI don't think it's weird, really. It is setting up the entries in the\nstring-list completely when we add them, rather than annotating later.\nIf there were some performance gain from doing them all at once, I could\nsee it, but otherwise I like that it means the entries are always in a\nconsistent state.\n\n> But I'm sure future patch you're referring to cares about knowing\n> these as soon as possible, since that's the point of this series ;-).\n\nYes. :)\n\n> I think a reasonable middle ground here is to do something like the\n> following on top of this patch:\n> \n> --- >8 ---\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index b5bd9e5fed..16a941f48b 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1002,6 +1002,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\treturn ret;\n> \t}\n> \n> +\tfor_each_string_list_item(item, &names) {\n> +\t\tif (!item->util)\n> +\t\t\tBUG(\"missing generated_pack_data for pack %s\",\n> +\t\t\t item->string);\n> +\t}\n> +\n> \tstring_list_sort(&names);\n> \n> \tclose_object_store(the_repository->objects);\n> --- 8< ---\n> \n> which still lets you eagerly keep track of the generated pack extensions\n> while also protecting against forgetful callers. Obviously we're relying\n> on a runtime check which is going to be somewhat weaker. But I think\n\nI don't think we need that. The renaming loop a few lines below will\nhappily segfault if anybody forgot to populate it. With a less nice\nmessage, obviously, but if the point is to notice a bug, it will get the\njob done.\n\n-Peff\n"},{"id":"465539","messageId":"xmqqfsfgiwcq.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1MrXoobkVKngYL1@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T23:35:17Z","receivedAt":"2022-10-21T23:35:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I actually considered writing it that way, but it felt a bit silly\n> to use _ARRAY for something which is clearly meant to be a single item.\n\nYou are not alone. I updated Réne's coccinelle rules to refrain\nfrom converting xcalloc() to CALLOC_ARRAY() for a single element\ncase exactly because it feels silly to use array function for a\nsingleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).\n\n"},{"id":"465540","messageId":"Y1MuMJT0ntvoTeUZ@nand.local","threadId":"58671","inReplyTo":"Y1MsdAgL8fdIRtxH@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] repack: populate extension bits incrementally","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T23:41:36Z","receivedAt":"2022-10-21T23:41:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 07:34:12PM -0400, Jeff King wrote:\n> > which still lets you eagerly keep track of the generated pack extensions\n> > while also protecting against forgetful callers. Obviously we're relying\n> > on a runtime check which is going to be somewhat weaker. But I think\n>\n> I don't think we need that. The renaming loop a few lines below will\n> happily segfault if anybody forgot to populate it. With a less nice\n> message, obviously, but if the point is to notice a bug, it will get the\n> job done.\n\nThat's reasonable, I think. I'm happy to abandon that suggestion :-).\n\nThanks,\nTaylor\n"},{"id":"465541","messageId":"Y1MuYQSo7rb6WCE8@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MsdAgL8fdIRtxH@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] repack: populate extension bits incrementally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T23:42:25Z","receivedAt":"2022-10-21T23:42:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 07:34:13PM -0400, Jeff King wrote:\n\n> On Fri, Oct 21, 2022 at 07:20:48PM -0400, Taylor Blau wrote:\n> \n> > On Fri, Oct 21, 2022 at 05:43:46PM -0400, Jeff King wrote:\n> > > There are two small problems with this:\n> > >\n> > > - repack_promisor_objects() may have added entries to \"names\", and\n> > > already called populate_pack_exts() for them. This is mostly just\n> > > wasteful, as we'll stat() the filename with each possible extension,\n> > > get the same result, and just overwrite our bits. But it makes the\n> > > code flow confusing, and it will become a problem if we try to make\n> > > populate_pack_exts() do more things.\n> > \n> > Hmm. I agree with you that repack_promisor_objects() calling\n> > populate_pack_exts() itself is at best weird, and at worst wasteful.\n> \n> I don't think it's weird, really. It is setting up the entries in the\n> string-list completely when we add them, rather than annotating later.\n> If there were some performance gain from doing them all at once, I could\n> see it, but otherwise I like that it means the entries are always in a\n> consistent state.\n\nI think my original didn't explain my thinking very well. And its \"two\nsmall problems\" is really a bit of a lie. It is really one small\nproblem, and a tweak I want in order to make a future patch work. :)\n\nSo here's what I wrote instead:\n\n-- >8 --\nThere's one small problem with this. In repack_promisor_objects(), we\nmay add entries to \"names\" and call populate_pack_exts() for them.\nCalling it again is mostly just wasteful, as we'll stat() the filename\nwith each possible extension, get the same result, and just overwrite\nour bits.\n\nSo we could drop the call there, and leave the final loop to populate\nall of the bits. But instead, this patch does the reverse: drops the\nfinal loop, and teaches the other two sites to populate the bits as they\nadd entries.\n\nThis makes the code easier to reason about, as you never have to worry\nabout when the util field is valid; it is always valid for each entry.\n\nIt also serves my ulterior purpose: recording the generated filenames as\nsoon as possible will make it easier for a future patch to use them for\ncleaning up from a failed operation.\n-- >8 --\n\n-Peff\n"},{"id":"465542","messageId":"Y1MumtN5fcCGD5+G@coredump.intra.peff.net","threadId":"58671","inReplyTo":"xmqqfsfgiwcq.fsf@gitster.g","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T23:43:22Z","receivedAt":"2022-10-21T23:43:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, I actually considered writing it that way, but it felt a bit silly\n> > to use _ARRAY for something which is clearly meant to be a single item.\n> \n> You are not alone. I updated Réne's coccinelle rules to refrain\n> from converting xcalloc() to CALLOC_ARRAY() for a single element\n> case exactly because it feels silly to use array function for a\n> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).\n\nOK. Then I'll leave it as-is, then. :)\n\n-Peff\n"},{"id":"465543","messageId":"Y1MvGIVz9bTTsS25@nand.local","threadId":"58671","inReplyTo":"Y1MqPjnYoWcOgXo6@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T23:45:28Z","receivedAt":"2022-10-21T23:45:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 07:24:46PM -0400, Jeff King wrote:\n> On Fri, Oct 21, 2022 at 03:30:29PM -0700, Junio C Hamano wrote:\n>\n> > > @@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > > \t\tsplit_pack_geometry(geometry, geometric_factor);\n> > > \t}\n> > >\n> > > -\tsigchain_push_common(remove_pack_on_signal);\n> > > -\n> > > \tprepare_pack_objects(&cmd, &po_args);\n> > >\n> > > \tshow_progress = !po_args.quiet && isatty(2);\n> > > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > > \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> > > \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> > >\n> > > -\t\t\tif (data->exts[ext]) {\n> > > +\t\t\tif (data->tempfiles[ext]) {\n> > > \t\t\t\tstruct stat statbuffer;\n> > > \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> > > \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> > > \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> > > \t\t\t\t}\n> > >\n> > > -\t\t\t\tif (rename(fname_old, fname))\n> > > +\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n> > > \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> >\n> > It now got a bit confusing that we have 'fname', 'fname_old', and\n> > the tempfile. The path.buf used as the argument to register_tempfile()\n> > matches what is used to compute fname_old above. I wonder if tempfile\n> > API does not give us that name so that we can stop using fname_old here?\n>\n> It does, and we probably should use get_tempfile_path() in the error\n> message here. But sadly we can't get rid of fname_old entirely, as it's\n> used below this for the second block in the if-else chain:\n>\n> if (data->tempfiles[ext]) {\n> ...do the rename ...\n> } else if (!exts[ext].optional)\n> \t die(_(\"missing required file: %s\"), fname_old);\n> else if (unlink(fname) < 0 && errno != ENOENT)\n> \t die_errno(_(\"could not unlink: %s\"), fname);\n>\n> OTOH, it would probably be equally readable (or perhaps even better) for\n> that second block to say:\n>\n> die(\"pack-objects did not generate a '%s' file for pack %s\",\n> exts[ext].name, item->string);\n>\n> And then we could drop fname_old entirely. Which is nice, because it\n> gets rid of the implicit assumption that the tempfile matches what is in\n> fname_old (which is always true, but since they are generated by\n> individual lines far apart from each other, it's possible for that to\n> change).\n\nTBH, I've always found fname_old to be a confusing name. It's not really\n\"old\", in fact we just had pack-objects write that file ;-). It really\ndoes pertain to the tempfile, and I think using get_tempfile_path() when\nwe have a tempfile to rename is sensible.\n\nI think that your proposed error message is good, too, and doubly so\nsince it lets us get rid of fname_old entirely. Yay :-).\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"465544","messageId":"Y1Mwhdn83MzNMsUx@nand.local","threadId":"58671","inReplyTo":"Y1MTtgOOWNovPawH@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] repack: drop remove_temporary_files()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T23:51:33Z","receivedAt":"2022-10-21T23:51:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 21, 2022 at 05:48:38PM -0400, Jeff King wrote:\n> The one case that could matter is if pack-objects generates an extension\n> we don't know about, like \".tmp-pack-$$-$hash.some-new-ext\". The current\n> code will quietly delete such a file, while after this patch we'd leave\n> it in place. In practice this doesn't happen, and would be indicative of\n> a bug. Leaving the file as cruft is arguably a better behavior, as it\n> means somebody is more likely to eventually notice and fix the bug. If\n> we really wanted to be paranoid, we could scan for and warn about such\n> files, but that seems like overkill.\n\nI agree, that would definitely be overkill.\n\n> There's nothing to test with regard to the removal of this function. It\n> was doing nothing, so the behavior should be the same. However, we can\n> verify (and protect) our assumption that \"repack -ad\" will eventually\n> remove stray files by adding a test for that.\n\nThanks for adding such a test. This patch is beautiful :-).\n\nThanks,\nTaylor\n"},{"id":"465545","messageId":"xmqq8rl8ivlb.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1MumtN5fcCGD5+G@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T23:51:44Z","receivedAt":"2022-10-21T23:51:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Yeah, I actually considered writing it that way, but it felt a bit silly\n>> > to use _ARRAY for something which is clearly meant to be a single item.\n>> \n>> You are not alone. I updated Réne's coccinelle rules to refrain\n>> from converting xcalloc() to CALLOC_ARRAY() for a single element\n>> case exactly because it feels silly to use array function for a\n>> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).\n>\n> OK. Then I'll leave it as-is, then. :)\n>\n> -Peff\n\nI do not think any \"huh? array for a singleton\" folks mind your\n\n #define CALLOC(x) CALLOC_ARRAY((x), 1)\n\nthough ;-)\n\n"},{"id":"465547","messageId":"Y1M1HFbmygJDZWYh@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MqPjnYoWcOgXo6@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:11:08Z","receivedAt":"2022-10-22T00:11:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 07:24:46PM -0400, Jeff King wrote:\n\n> > > @@ -1020,14 +1011,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > > \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n> > > \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n> > > \n> > > -\t\t\tif (data->exts[ext]) {\n> > > +\t\t\tif (data->tempfiles[ext]) {\n> > > \t\t\t\tstruct stat statbuffer;\n> > > \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n> > > \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> > > \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> > > \t\t\t\t}\n> > > \n> > > -\t\t\t\tif (rename(fname_old, fname))\n> > > +\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n> > > \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> > \n> > It now got a bit confusing that we have 'fname', 'fname_old', and\n> > the tempfile. The path.buf used as the argument to register_tempfile()\n> > matches what is used to compute fname_old above. I wonder if tempfile\n> > API does not give us that name so that we can stop using fname_old here?\n> \n> It does, and we probably should use get_tempfile_path() in the error\n> message here.\n\nGah, this is not quite true. If the rename fails, we clean up the\ntempfile struct entirely, and that invalidates the pointer. I think it\nis OK to just report \"fname\" in this case, though, which is what most\ncallers do. Arguably the tempfile API should leave the tempfile in place\non a failed rename, letting the callers decide themselves how to handle\nthings. In most cases they'll just exit anyway, which will clean up the\ntempfile.\n\nI also didn't notice that we do some mode-twiddling on fname_old. But I\nthink if the code becomes (inside this conditional block, once we stop\nusing it in the other half):\n\n const char *fname_old = get_tempfile_path(data->tempfiles[ext]);\n\nthen those lines don't even need to change.\n\n-Peff\n"},{"id":"465549","messageId":"Y1M1bFNV8DybuJtt@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MvGIVz9bTTsS25@nand.local","subject":"Re: [PATCH 3/4] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:12:28Z","receivedAt":"2022-10-22T00:12:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 07:45:28PM -0400, Taylor Blau wrote:\n\n> TBH, I've always found fname_old to be a confusing name. It's not really\n> \"old\", in fact we just had pack-objects write that file ;-). It really\n> does pertain to the tempfile, and I think using get_tempfile_path() when\n> we have a tempfile to rename is sensible.\n\nI don't love it either, but I've kept it in what I'm preparing, just\nbecause we need _some_ variable to avoid writing:\n\n get_tempfile_path(data->tempfiles[ext])\n\nover and over. And using the same one keeps the diff minimal. If it's\ntoo terrible we can rename it on top. :)\n\n> I think that your proposed error message is good, too, and doubly so\n> since it lets us get rid of fname_old entirely. Yay :-).\n\nThanks. I'm preparing something along those lines.\n\n-Peff\n"},{"id":"465550","messageId":"Y1M1ilGpIQUKghzG@coredump.intra.peff.net","threadId":"58671","inReplyTo":"xmqq8rl8ivlb.fsf@gitster.g","subject":"Re: [PATCH 1/4] repack: convert \"names\" util bitfield to array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:12:58Z","receivedAt":"2022-10-22T00:13:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 04:51:44PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Oct 21, 2022 at 04:35:17PM -0700, Junio C Hamano wrote:\n> >\n> >> Jeff King writes:\n> >> \n> >> > Yeah, I actually considered writing it that way, but it felt a bit silly\n> >> > to use _ARRAY for something which is clearly meant to be a single item.\n> >> \n> >> You are not alone. I updated Réne's coccinelle rules to refrain\n> >> from converting xcalloc() to CALLOC_ARRAY() for a single element\n> >> case exactly because it feels silly to use array function for a\n> >> singleton with 1c57cc70 (cocci: allow xcalloc(1, size), 2021-03-15).\n> >\n> > OK. Then I'll leave it as-is, then. :)\n> >\n> > -Peff\n> \n> I do not think any \"huh? array for a singleton\" folks mind your\n> \n> #define CALLOC(x) CALLOC_ARRAY((x), 1)\n> \n> though ;-)\n\nI may follow-up on that, but let's leave it outside of this series.\n\n-Peff\n"},{"id":"465551","messageId":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1MR7V8kGolLd8eh@coredump.intra.peff.net","subject":"[PATCH v2 0/5] repack tempfile-cleanup signal deadlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:17Z","receivedAt":"2022-10-22T00:21:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 05:41:02PM -0400, Jeff King wrote:\n\n> Here's a series which should fix the deadlock Jan reported in:\n> \n> https://lore.kernel.org/git/00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz/\n\nAnd here's a re-roll based on feedback on v1. Poor Jan; I hope this\nthread isn't blowing up your inbox. :) But thank you for reporting the\nproblem!\n\nThe changes from v1 are:\n\n - clarified rationale in patch 2\n - new patch 3 improves an error message and reduces scope of fname_old\n - patch 4 cleans up fname_old to use tempfile path\n\nRange diff is below.\n\n [1/5]: repack: convert \"names\" util bitfield to array\n [2/5]: repack: populate extension bits incrementally\n [3/5]: repack: expand error message for missing pack files\n [4/5]: repack: use tempfiles for signal cleanup\n [5/5]: repack: drop remove_temporary_files()\n\n builtin/repack.c | 90 +++++++++++++++--------------------------------\n t/t7700-repack.sh | 16 +++++++++\n 2 files changed, 45 insertions(+), 61 deletions(-)\n\n1: 6de9bd9d71 = 1: affb21442f repack: convert \"names\" util bitfield to array\n2: 7d72ed9a39 ! 2: 18d8318881 repack: populate extension bits incrementally\n @@ Commit message\n iterate over the \"names\" list (which contains hashes of packs generated\n by pack-objects), and call populate_pack_exts() for each.\n \n - There are two small problems with this:\n + There's one small problem with this. In repack_promisor_objects(), we\n + may add entries to \"names\" and call populate_pack_exts() for them.\n + Calling it again is mostly just wasteful, as we'll stat() the filename\n + with each possible extension, get the same result, and just overwrite\n + our bits.\n \n - - repack_promisor_objects() may have added entries to \"names\", and\n - already called populate_pack_exts() for them. This is mostly just\n - wasteful, as we'll stat() the filename with each possible extension,\n - get the same result, and just overwrite our bits. But it makes the\n - code flow confusing, and it will become a problem if we try to make\n - populate_pack_exts() do more things.\n + So we could drop the call there, and leave the final loop to populate\n + all of the bits. But instead, this patch does the reverse: drops the\n + final loop, and teaches the other two sites to populate the bits as they\n + add entries.\n \n - - it would be nice to record the generated filenames as soon as\n - possible. We don't currently use them for cleaning up from a failed\n - operation, but a future patch will do so.\n + This makes the code easier to reason about, as you never have to worry\n + about when the util field is valid; it is always valid for each entry.\n +\n + It also serves my ulterior purpose: recording the generated filenames as\n + soon as possible will make it easier for a future patch to use them for\n + cleaning up from a failed operation.\n \n Signed-off-by: Jeff King \n \n-: ---------- > 3: bbd7f82f88 repack: expand error message for missing pack files\n3: 16448019ba ! 4: 7cd89d1575 repack: use tempfiles for signal cleanup\n @@ Commit message\n pack. Before this patch, the .tmp-* file for the main pack would have\n been left, but now we correctly clean it up.\n \n + Two small subtleties on the implementation:\n +\n + - in the renaming loop, we can stop re-constructing fname_old; we only\n + use it when we have a tempfile to rename, so we can just ask the\n + tempfile for its path (which, barring bugs, should be identical)\n +\n + - when renaming fails, our error message mentions fname_old. But since\n + a failed rename_tempfile() invalidates the tempfile struct, we'll\n + lose access to that string. Instead, let's mention the destination\n + filename, which is what most other callers do.\n +\n Reported-by: Jan Pokorný \n Signed-off-by: Jeff King \n \n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n \n \tshow_progress = !po_args.quiet && isatty(2);\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n - \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n - \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n + \t\tstruct generated_pack_data *data = item->util;\n + \n + \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n +-\t\t\tchar *fname, *fname_old;\n ++\t\t\tchar *fname;\n + \n + \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n + \t\t\t\t\tpackdir, item->string, exts[ext].name);\n +-\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n +-\t\t\t\t\tpacktmp, item->string, exts[ext].name);\n \n -\t\t\tif (data->exts[ext]) {\n +\t\t\tif (data->tempfiles[ext]) {\n ++\t\t\t\tconst char *fname_old = get_tempfile_path(data->tempfiles[ext]);\n \t\t\t\tstruct stat statbuffer;\n ++\n \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n \t\t\t\t}\n \n -\t\t\t\tif (rename(fname_old, fname))\n +-\t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n +\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n - \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n ++\t\t\t\t\tdie_errno(_(\"renaming pack to '%s' failed\"), fname);\n \t\t\t} else if (!exts[ext].optional)\n - \t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n + \t\t\t\tdie(_(\"pack-objects did not write a '%s' file for pack %s-%s\"),\n + \t\t\t\t exts[ext].name, packtmp, item->string);\n + \t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n + \t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n + \n + \t\t\tfree(fname);\n +-\t\t\tfree(fname_old);\n + \t\t}\n + \t}\n + \t/* End of pack replacement. */\n \n ## t/t7700-repack.sh ##\n @@ t/t7700-repack.sh: test_expect_success TTY '--quiet disables progress' '\n4: c7b0f3823d = 5: d7e9225b8e repack: drop remove_temporary_files()\n"},{"id":"465552","messageId":"Y1M3megP6ma+xMpi@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","subject":"[PATCH v2 1/5] repack: convert \"names\" util bitfield to array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:45Z","receivedAt":"2022-10-22T00:21:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a string_list \"names\" containing the hashes of packs generated\non our behalf by pack-objects. The util field of each item is treated as\na bitfield that tells us which extensions (.pack, .idx, .rev, etc) are\npresent for each name.\n\nLet's switch this to allocating a real array. That will give us room in\na future patch to store more data than just a single bit per extension.\nAnd it makes the code a little easier to read, as we avoid casting back\nand forth between uintptr_t and a void pointer.\n\nSince the only thing we're storing is an array, we could just allocate\nit directly. But instead I've put it into a named struct here. That\nfurther increases readability around the casts, and in particular helps\ndifferentiate us from other string_lists in the same file which use\ntheir util field differently. E.g., the existing_*_packs lists still do\nbit-twiddling, but their bits have different meaning than the ones in\n\"names\". This makes it hard to grep around the code to see how the util\nfields are used; now you can look for \"generated_pack_data\".\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..8e71230bf7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -247,11 +247,15 @@ static struct {\n \t{\".idx\"},\n };\n \n-static unsigned populate_pack_exts(char *name)\n+struct generated_pack_data {\n+\tchar exts[ARRAY_SIZE(exts)];\n+};\n+\n+static struct generated_pack_data *populate_pack_exts(const char *name)\n {\n \tstruct stat statbuf;\n \tstruct strbuf path = STRBUF_INIT;\n-\tunsigned ret = 0;\n+\tstruct generated_pack_data *data = xcalloc(1, sizeof(*data));\n \tint i;\n \n \tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n@@ -261,11 +265,11 @@ static unsigned populate_pack_exts(char *name)\n \t\tif (stat(path.buf, &statbuf))\n \t\t\tcontinue;\n \n-\t\tret |= (1 << i);\n+\t\tdata->exts[i] = 1;\n \t}\n \n \tstrbuf_release(&path);\n-\treturn ret;\n+\treturn data;\n }\n \n static void repack_promisor_objects(const struct pack_objects_args *args,\n@@ -320,7 +324,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t\t line.buf);\n \t\twrite_promisor_file(promisor_name, NULL, 0);\n \n-\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\t\titem->util = populate_pack_exts(item->string);\n \n \t\tfree(promisor_name);\n \t}\n@@ -994,7 +998,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstring_list_sort(&names);\n \n \tfor_each_string_list_item(item, &names) {\n-\t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n+\t\titem->util = populate_pack_exts(item->string);\n \t}\n \n \tclose_object_store(the_repository->objects);\n@@ -1003,6 +1007,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t * Ok we have prepared all new packfiles.\n \t */\n \tfor_each_string_list_item(item, &names) {\n+\t\tstruct generated_pack_data *data = item->util;\n+\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n \t\t\tchar *fname, *fname_old;\n \n@@ -1011,7 +1017,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n \t\t\t\t\tpacktmp, item->string, exts[ext].name);\n \n-\t\t\tif (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) {\n+\t\t\tif (data->exts[ext]) {\n \t\t\t\tstruct stat statbuffer;\n \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n@@ -1115,7 +1121,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n \t}\n \n-\tstring_list_clear(&names, 0);\n+\tstring_list_clear(&names, 1);\n \tstring_list_clear(&existing_nonkept_packs, 0);\n \tstring_list_clear(&existing_kept_packs, 0);\n \tclear_pack_geometry(geometry);\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465553","messageId":"Y1M3nMNTOGAH0L82@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","subject":"[PATCH v2 2/5] repack: populate extension bits incrementally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:48Z","receivedAt":"2022-10-22T00:21:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After generating the main pack and then any additional cruft packs, we\niterate over the \"names\" list (which contains hashes of packs generated\nby pack-objects), and call populate_pack_exts() for each.\n\nThere's one small problem with this. In repack_promisor_objects(), we\nmay add entries to \"names\" and call populate_pack_exts() for them.\nCalling it again is mostly just wasteful, as we'll stat() the filename\nwith each possible extension, get the same result, and just overwrite\nour bits.\n\nSo we could drop the call there, and leave the final loop to populate\nall of the bits. But instead, this patch does the reverse: drops the\nfinal loop, and teaches the other two sites to populate the bits as they\nadd entries.\n\nThis makes the code easier to reason about, as you never have to worry\nabout when the util field is valid; it is always valid for each entry.\n\nIt also serves my ulterior purpose: recording the generated filenames as\nsoon as possible will make it easier for a future patch to use them for\ncleaning up from a failed operation.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8e71230bf7..b5bd9e5fed 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -714,10 +714,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tstruct string_list_item *item;\n+\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only \"\n \t\t\t \"from pack-objects.\"));\n-\t\tstring_list_append(names, line.buf);\n+\n+\t\titem = string_list_append(names, line.buf);\n+\t\titem->util = populate_pack_exts(line.buf);\n \t}\n \tfclose(out);\n \n@@ -956,9 +960,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tstruct string_list_item *item;\n+\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n-\t\tstring_list_append(&names, line.buf);\n+\t\titem = string_list_append(&names, line.buf);\n+\t\titem->util = populate_pack_exts(item->string);\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\n@@ -997,10 +1004,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tstring_list_sort(&names);\n \n-\tfor_each_string_list_item(item, &names) {\n-\t\titem->util = populate_pack_exts(item->string);\n-\t}\n-\n \tclose_object_store(the_repository->objects);\n \n \t/*\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465554","messageId":"Y1M3nhOqdYP7QoEv@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","subject":"[PATCH v2 3/5] repack: expand error message for missing pack files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:50Z","receivedAt":"2022-10-22T00:22:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If pack-objects tells us it generated pack $hash, we expect to find\n.tmp-$$-pack-$hash.pack, .idx, .rev, and so on. Some of these files are\noptional, but others are not. For the required ones, we'll bail with an\nerror if any of them is missing.\n\nThe error message is just \"missing required file\", which is a bit vague.\nWe should be more clear that it is not the user's fault, but rather that\nthe sub-pgoram we called is not operating as expected. In practice,\nnobody should ever see this message, as it would generally only be\ncaused by a bug in Git.\n\nIt probably doesn't make sense to convert this to a BUG(), though, as\nthere are other (unlikely) possibilities, such as somebody else racily\ndeleting the files, filesystem errors causing stat() to fail, and so on.\n\nA nice side effect here is that we stop relying on fname_old in this\ncode path, which will let us deal with it only in the first part of the\nconditional.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex b5bd9e5fed..d1929bb3db 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1030,7 +1030,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (rename(fname_old, fname))\n \t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n \t\t\t} else if (!exts[ext].optional)\n-\t\t\t\tdie(_(\"missing required file: %s\"), fname_old);\n+\t\t\t\tdie(_(\"pack-objects did not write a '%s' file for pack %s-%s\"),\n+\t\t\t\t exts[ext].name, packtmp, item->string);\n \t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n \t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n \n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465555","messageId":"Y1M3oie5dPrRaOni@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","subject":"[PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:54Z","receivedAt":"2022-10-22T00:22:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git-repack exits due to a signal, it tries to clean up by calling\nits remove_temporary_files() function, which walks through the packs dir\nlooking for \".tmp-$$-pack-*\" files to delete (where \"$$\" is the pid of\nthe current process).\n\nThe biggest problem here is that remove_temporary_files() is not safe to\ncall in a signal handler. It uses opendir(), which isn't on the POSIX\nasync-signal-safe list. The details will be platform-specific, but a\nlikely issue is that it needs to allocate memory; if we receive a signal\nwhile inside malloc(), etc, we'll conflict on the allocator lock and\ndeadlock with ourselves.\n\nWe can fix this by just cleaning up the files directly, without walking\nthe directory. We already know the complete list of .tmp-* files that\nwere generated, because we recorded them via populate_pack_exts(). When\nwe find files there, we can use register_tempfile() to record the\nfilenames. If we receive a signal, then the tempfile API will clean them\nup for us, and it's async-safe and pretty battle-tested.\n\nNote that this is slightly racier than the existing scheme. We don't\nrecord the filenames until pack-objects tells us the hash over stdout.\nSo during the period between it generating the file and reporting the\nhash, we'd fail to clean up. However, that period is very small. During\nmost of the pack generation process pack-objects is using its own\ninternal tempfiles. It's only at the very end that it moves them into\nthe names git-repack expects, and then it immediately reports the name\nto us. Given that cleanup like this is best effort (after all, we may\nget SIGKILL), this level of race is acceptable.\n\nWhen we register the tempfiles, we'll record them locally and use the\nresult to call rename_tempfile(), rather than renaming by hand. This\nisn't strictly necessary, as once we've renamed the files they're gone,\nand the tempfile API's cleanup unlink() would simply become a pointless\nnoop. But managing the lifetimes of the tempfile objects is the cleanest\nthing to do, and the tempfile pointers naturally fill the same role as\nthe old booleans.\n\nThis patch also fixes another small problem. We only hook signals, and\ndon't set up an atexit handler. So if we see an error that causes us to\ndie(), we'll leave the .tmp-* files in place. But since the tempfile API\nhandles this for us, this is now fixed for free. The new test covers\nthis by stimulating a failure of pack-objects when generating a cruft\npack. Before this patch, the .tmp-* file for the main pack would have\nbeen left, but now we correctly clean it up.\n\nTwo small subtleties on the implementation:\n\n - in the renaming loop, we can stop re-constructing fname_old; we only\n use it when we have a tempfile to rename, so we can just ask the\n tempfile for its path (which, barring bugs, should be identical)\n\n - when renaming fails, our error message mentions fname_old. But since\n a failed rename_tempfile() invalidates the tempfile struct, we'll\n lose access to that string. Instead, let's mention the destination\n filename, which is what most other callers do.\n\nReported-by: Jan Pokorný \nSigned-off-by: Jeff King \n---\n builtin/repack.c | 26 ++++++++------------------\n t/t7700-repack.sh | 8 ++++++++\n 2 files changed, 16 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d1929bb3db..39f03c3a1d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -122,13 +122,6 @@ static void remove_temporary_files(void)\n \tstrbuf_release(&buf);\n }\n \n-static void remove_pack_on_signal(int signo)\n-{\n-\tremove_temporary_files();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n /*\n * Adds all packs hex strings to either fname_nonkept_list or\n * fname_kept_list based on whether each pack has a corresponding\n@@ -248,7 +241,7 @@ static struct {\n };\n \n struct generated_pack_data {\n-\tchar exts[ARRAY_SIZE(exts)];\n+\tstruct tempfile *tempfiles[ARRAY_SIZE(exts)];\n };\n \n static struct generated_pack_data *populate_pack_exts(const char *name)\n@@ -265,7 +258,7 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \t\tif (stat(path.buf, &statbuf))\n \t\t\tcontinue;\n \n-\t\tdata->exts[i] = 1;\n+\t\tdata->tempfiles[i] = register_tempfile(path.buf);\n \t}\n \n \tstrbuf_release(&path);\n@@ -867,8 +860,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \n-\tsigchain_push_common(remove_pack_on_signal);\n-\n \tprepare_pack_objects(&cmd, &po_args);\n \n \tshow_progress = !po_args.quiet && isatty(2);\n@@ -1013,30 +1004,29 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstruct generated_pack_data *data = item->util;\n \n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tchar *fname, *fname_old;\n+\t\t\tchar *fname;\n \n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\",\n \t\t\t\t\tpackdir, item->string, exts[ext].name);\n-\t\t\tfname_old = mkpathdup(\"%s-%s%s\",\n-\t\t\t\t\tpacktmp, item->string, exts[ext].name);\n \n-\t\t\tif (data->exts[ext]) {\n+\t\t\tif (data->tempfiles[ext]) {\n+\t\t\t\tconst char *fname_old = get_tempfile_path(data->tempfiles[ext]);\n \t\t\t\tstruct stat statbuffer;\n+\n \t\t\t\tif (!stat(fname_old, &statbuffer)) {\n \t\t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n \t\t\t\t\tchmod(fname_old, statbuffer.st_mode);\n \t\t\t\t}\n \n-\t\t\t\tif (rename(fname_old, fname))\n-\t\t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n+\t\t\t\tif (rename_tempfile(&data->tempfiles[ext], fname))\n+\t\t\t\t\tdie_errno(_(\"renaming pack to '%s' failed\"), fname);\n \t\t\t} else if (!exts[ext].optional)\n \t\t\t\tdie(_(\"pack-objects did not write a '%s' file for pack %s-%s\"),\n \t\t\t\t exts[ext].name, packtmp, item->string);\n \t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n \t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n \n \t\t\tfree(fname);\n-\t\t\tfree(fname_old);\n \t\t}\n \t}\n \t/* End of pack replacement. */\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..592016f64a 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -432,6 +432,14 @@ test_expect_success TTY '--quiet disables progress' '\n \ttest_must_be_empty stderr\n '\n \n+test_expect_success 'clean up .tmp-* packs on error' '\n+\ttest_must_fail git \\\n+\t\t-c repack.cruftwindow=bogus \\\n+\t\trepack -ad --cruft &&\n+\tfind $objdir/pack -name '.tmp-*' >tmpfiles &&\n+\ttest_must_be_empty tmpfiles\n+'\n+\n test_expect_success 'setup for update-server-info' '\n \tgit init update-server-info &&\n \ttest_commit -C update-server-info message\n-- \n2.38.1.496.ga614b0e9bd\n\n"},{"id":"465556","messageId":"Y1M3pjuA2po83LU7@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3fVnixJHvKiSg@coredump.intra.peff.net","subject":"[PATCH v2 5/5] repack: drop remove_temporary_files()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T00:21:58Z","receivedAt":"2022-10-22T00:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After we've successfully finished the repack, we call\nremove_temporary_files(), which looks for and removes any files matching\n\".tmp-$$-pack-*\", where $$ is the pid of the current process. But this\nis pointless. If we make it this far in the process, we've already\nrenamed these tempfiles into place, and there is nothing left to delete.\n\nNor is there a point in trying to call it to clean up when we _aren't_\nsuccessful. It's not safe for using in a signal handler, and the\nprevious commit already handed that job over to the tempfile API.\n\nIt might seem like it would be useful to clean up stray .tmp files left\nby other invocations of git-repack. But it won't clean those files; it\nonly matches ones with its pid, and leaves the rest. Fortunately, those\nare cleaned up naturally by successive calls to git-repack; we'll\nconsider .tmp-*.pack the same as normal packfiles, so \"repack -ad\", etc,\nwill roll up their contents and eventually delete them.\n\nThe one case that could matter is if pack-objects generates an extension\nwe don't know about, like \".tmp-pack-$$-$hash.some-new-ext\". The current\ncode will quietly delete such a file, while after this patch we'd leave\nit in place. In practice this doesn't happen, and would be indicative of\na bug. Leaving the file as cruft is arguably a better behavior, as it\nmeans somebody is more likely to eventually notice and fix the bug. If\nwe really wanted to be paranoid, we could scan for and warn about such\nfiles, but that seems like overkill.\n\nThere's nothing to test with regard to the removal of this function. It\nwas doing nothing, so the behavior should be the same. However, we can\nverify (and protect) our assumption that \"repack -ad\" will eventually\nremove stray files by adding a test for that.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 32 --------------------------------\n t/t7700-repack.sh | 8 ++++++++\n 2 files changed, 8 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 39f03c3a1d..cd338b161f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -91,37 +91,6 @@ static int repack_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-/*\n- * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.\n- */\n-static void remove_temporary_files(void)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tsize_t dirlen, prefixlen;\n-\tDIR *dir;\n-\tstruct dirent *e;\n-\n-\tdir = opendir(packdir);\n-\tif (!dir)\n-\t\treturn;\n-\n-\t/* Point at the slash at the end of \".../objects/pack/\" */\n-\tdirlen = strlen(packdir) + 1;\n-\tstrbuf_addstr(&buf, packtmp);\n-\t/* Hold the length of \".tmp-%d-pack-\" */\n-\tprefixlen = buf.len - dirlen;\n-\n-\twhile ((e = readdir(dir))) {\n-\t\tif (strncmp(e->d_name, buf.buf + dirlen, prefixlen))\n-\t\t\tcontinue;\n-\t\tstrbuf_setlen(&buf, dirlen);\n-\t\tstrbuf_addstr(&buf, e->d_name);\n-\t\tunlink(buf.buf);\n-\t}\n-\tclosedir(dir);\n-\tstrbuf_release(&buf);\n-}\n-\n /*\n * Adds all packs hex strings to either fname_nonkept_list or\n * fname_kept_list based on whether each pack has a corresponding\n@@ -1106,7 +1075,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (run_update_server_info)\n \t\tupdate_server_info(0);\n-\tremove_temporary_files();\n \n \tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {\n \t\tunsigned flags = 0;\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 592016f64a..edcda849b9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -440,6 +440,14 @@ test_expect_success 'clean up .tmp-* packs on error' '\n \ttest_must_be_empty tmpfiles\n '\n \n+test_expect_success 'repack -ad cleans up old .tmp-* packs' '\n+\tgit rev-parse HEAD >input &&\n+\tgit pack-objects $objdir/pack/.tmp-1234 tmpfiles &&\n+\ttest_must_be_empty tmpfiles\n+'\n+\n test_expect_success 'setup for update-server-info' '\n \tgit init update-server-info &&\n \ttest_commit -C update-server-info message\n-- \n2.38.1.496.ga614b0e9bd\n"},{"id":"465576","messageId":"Y1RUI8ny2mexxwKm@coredump.intra.peff.net","threadId":"58671","inReplyTo":"Y1M3oie5dPrRaOni@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T20:35:47Z","receivedAt":"2022-10-22T20:38:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 08:21:54PM -0400, Jeff King wrote:\n\n> +test_expect_success 'clean up .tmp-* packs on error' '\n> +\ttest_must_fail git \\\n> +\t\t-c repack.cruftwindow=bogus \\\n> +\t\trepack -ad --cruft &&\n> +\tfind $objdir/pack -name '.tmp-*' >tmpfiles &&\n> +\ttest_must_be_empty tmpfiles\n> +'\n\nUgh, I think this test is racy. I saw a failure via SIGPIPE on OS X in\nCI, and running it locally with --stress confirmed. I think the problem\nis in our method to trigger pack-objects to fail for the cruft pack. We\npass a bogus command line argument, so pack-objects exits more or less\nimmediately. But the parent git-repack process is trying to write to its\nstdin (to name packs to keep/exclude). So that write may succeed or fail\nbased on how quickly the child dies.\n\nSome options:\n\n - find a different way to convince repack to die. The most likely is\n probably corrupting the cruft objects in some way such that\n pack-objects dies, but not until it starts doing real work.\n\n - mark the test_must_fail with ok=sigpipe. In most case this is a\n band-aid, but here we still test what we want. The .tmp cleanup\n should kick in from a die() or from a signal death, so either is\n sufficient for our purposes.\n\n - teach git-repack to ignore sigpipe. We don't actually check the\n writes to pack-objects (since they're done by fprintf), but we'd\n notice its failing exit code. And arguably this is improving the\n user experience. Saying \"pack-objects died with an error\" is more\n useful than a sigpipe.\n\nThoughts?\n\n-Peff\n"},{"id":"465590","messageId":"xmqqtu3vflau.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1RUI8ny2mexxwKm@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-23T00:14:33Z","receivedAt":"2022-10-23T00:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ugh, I think this test is racy. I saw a failure via SIGPIPE on OS X in\n> CI, and running it locally with --stress confirmed. I think the problem\n> is in our method to trigger pack-objects to fail for the cruft pack. We\n> pass a bogus command line argument, so pack-objects exits more or less\n> immediately. But the parent git-repack process is trying to write to its\n> stdin (to name packs to keep/exclude). So that write may succeed or fail\n> based on how quickly the child dies.\n\nYeah, I saw flaky failures myself during my integration tests.\n\n> Some options:\n>\n> - find a different way to convince repack to die. The most likely is\n> probably corrupting the cruft objects in some way such that\n> pack-objects dies, but not until it starts doing real work.\n>\n> - mark the test_must_fail with ok=sigpipe. In most case this is a\n> band-aid, but here we still test what we want. The .tmp cleanup\n> should kick in from a die() or from a signal death, so either is\n> sufficient for our purposes.\n>\n> - teach git-repack to ignore sigpipe. We don't actually check the\n> writes to pack-objects (since they're done by fprintf), but we'd\n> notice its failing exit code. And arguably this is improving the\n> user experience. Saying \"pack-objects died with an error\" is more\n> useful than a sigpipe.\n>\n> Thoughts?\n\nI agree that for this particular one the second one is a reasonable\nthing to do in the context of the test. The third one may actually\nbe a better fix, exactly for the reason you state here.\n\nThanks.\n"},{"id":"465598","messageId":"Y1VzPY4zQyZbVAsm@coredump.intra.peff.net","threadId":"58671","inReplyTo":"xmqqtu3vflau.fsf@gitster.g","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-23T17:00:45Z","receivedAt":"2022-10-23T17:00:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2022 at 05:14:33PM -0700, Junio C Hamano wrote:\n\n> > Some options:\n> >\n> > - find a different way to convince repack to die. The most likely is\n> > probably corrupting the cruft objects in some way such that\n> > pack-objects dies, but not until it starts doing real work.\n> >\n> > - mark the test_must_fail with ok=sigpipe. In most case this is a\n> > band-aid, but here we still test what we want. The .tmp cleanup\n> > should kick in from a die() or from a signal death, so either is\n> > sufficient for our purposes.\n> >\n> > - teach git-repack to ignore sigpipe. We don't actually check the\n> > writes to pack-objects (since they're done by fprintf), but we'd\n> > notice its failing exit code. And arguably this is improving the\n> > user experience. Saying \"pack-objects died with an error\" is more\n> > useful than a sigpipe.\n> >\n> > Thoughts?\n> \n> I agree that for this particular one the second one is a reasonable\n> thing to do in the context of the test. The third one may actually\n> be a better fix, exactly for the reason you state here.\n\nHere's a patch on top of of jk/repack-tempfile-cleanup that adjusts the\ntest (and should make the immediate racy CI pain go away). It gives some\nexplanation why the third option isn't as interesting as you'd think. If\nsomebody later wants to add such a \"pack-objects died\" error, we can\nadjust sigpipe handling there.\n\n-- >8 --\nSubject: [PATCH] t7700: annotate cruft-pack failure with ok=sigpipe\n\nOne of our tests intentionally causes the cruft-pack generation phase of\nrepack to fail, in order to stimulate an exit from repack at the desired\nmoment. It does so by feeding a bogus option argument to pack-objects.\nThis is a simple and reliable way to get pack-objects to fail, but it\nhas one downside: pack-objects will die before reading its stdin, which\nmeans the caller repack may racily get SIGPIPE writing to it.\n\nFor the purposes of this test, that's OK. We are checking whether repack\ncleans up already-created .tmp files, and it will do so whether it exits\nor dies by signal (because the tempfile API hooks both).\n\nBut we have to tell test_must_fail that either outcome is OK, or it\ncomplains about the signal. Arguably this is a workaround (compared to\nfixing repack), as repack dying to SIGPIPE means that it loses the\nopportunity to give a more detailed message. But we don't actually write\nsuch a message anyway; we rely on pack-objects to have written something\nuseful to stderr, and it does. In either case (signal or exit), that is\nthe main thing the user will see.\n\nSigned-off-by: Jeff King \n---\n t/t7700-repack.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex edcda849b9..9164acbe02 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -433,7 +433,7 @@ test_expect_success TTY '--quiet disables progress' '\n '\n \n test_expect_success 'clean up .tmp-* packs on error' '\n-\ttest_must_fail git \\\n+\ttest_must_fail ok=sigpipe git \\\n \t\t-c repack.cruftwindow=bogus \\\n \t\trepack -ad --cruft &&\n \tfind $objdir/pack -name '.tmp-*' >tmpfiles &&\n-- \n2.38.1.500.ge50734af7a\n\n"},{"id":"465599","messageId":"xmqqlep6fm5h.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1VzPY4zQyZbVAsm@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-23T18:08:26Z","receivedAt":"2022-10-23T18:08:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's a patch on top of of jk/repack-tempfile-cleanup that adjusts the\n> test (and should make the immediate racy CI pain go away). It gives some\n> explanation why the third option isn't as interesting as you'd think. If\n> somebody later wants to add such a \"pack-objects died\" error, we can\n> adjust sigpipe handling there.\n\nAn extremely simplified alternative would be just to say ! instead\nof test_must_fail, which essentially is ok=anycrash ;-)\n\nI did try the same exact patch before going to bed last night but\nt7700 somehow failed some other steps in my local tests and I gave\nup digging further X-<. One step at a time...\n\nWill queue. Thanks.\n\n> -- >8 --\n> Subject: [PATCH] t7700: annotate cruft-pack failure with ok=sigpipe\n>\n> One of our tests intentionally causes the cruft-pack generation phase of\n> repack to fail, in order to stimulate an exit from repack at the desired\n> moment. It does so by feeding a bogus option argument to pack-objects.\n> This is a simple and reliable way to get pack-objects to fail, but it\n> has one downside: pack-objects will die before reading its stdin, which\n> means the caller repack may racily get SIGPIPE writing to it.\n>\n> For the purposes of this test, that's OK. We are checking whether repack\n> cleans up already-created .tmp files, and it will do so whether it exits\n> or dies by signal (because the tempfile API hooks both).\n>\n> But we have to tell test_must_fail that either outcome is OK, or it\n> complains about the signal. Arguably this is a workaround (compared to\n> fixing repack), as repack dying to SIGPIPE means that it loses the\n> opportunity to give a more detailed message. But we don't actually write\n> such a message anyway; we rely on pack-objects to have written something\n> useful to stderr, and it does. In either case (signal or exit), that is\n> the main thing the user will see.\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t7700-repack.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index edcda849b9..9164acbe02 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -433,7 +433,7 @@ test_expect_success TTY '--quiet disables progress' '\n> '\n> \n> test_expect_success 'clean up .tmp-* packs on error' '\n> -\ttest_must_fail git \\\n> +\ttest_must_fail ok=sigpipe git \\\n> \t\t-c repack.cruftwindow=bogus \\\n> \t\trepack -ad --cruft &&\n> \tfind $objdir/pack -name '.tmp-*' >tmpfiles &&\n"},{"id":"465601","messageId":"Y1WqX7YRrSpkmyxH@coredump.intra.peff.net","threadId":"58671","inReplyTo":"xmqqlep6fm5h.fsf@gitster.g","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-23T20:55:59Z","receivedAt":"2022-10-23T20:56:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 23, 2022 at 11:08:26AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Here's a patch on top of of jk/repack-tempfile-cleanup that adjusts the\n> > test (and should make the immediate racy CI pain go away). It gives some\n> > explanation why the third option isn't as interesting as you'd think. If\n> > somebody later wants to add such a \"pack-objects died\" error, we can\n> > adjust sigpipe handling there.\n> \n> An extremely simplified alternative would be just to say ! instead\n> of test_must_fail, which essentially is ok=anycrash ;-)\n\nI would have been tempted to do that if we hadn't already added\nok=sigpipe long ago. :)\n\n> I did try the same exact patch before going to bed last night but\n> t7700 somehow failed some other steps in my local tests and I gave\n> up digging further X-<. One step at a time...\n\nHopefully not my bits. :) FWIW, the tips of \"master\", \"next\", and \"seen\"\nall pass just fine for me (minus this race on seen).\n\n-Peff\n"},{"id":"465602","messageId":"xmqqh6zufbze.fsf@gitster.g","threadId":"58671","inReplyTo":"Y1WqX7YRrSpkmyxH@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-23T21:48:05Z","receivedAt":"2022-10-23T21:48:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I did try the same exact patch before going to bed last night but\n>> t7700 somehow failed some other steps in my local tests and I gave\n>> up digging further X-<. One step at a time...\n>\n> Hopefully not my bits. :) FWIW, the tips of \"master\", \"next\", and \"seen\"\n> all pass just fine for me (minus this race on seen).\n\nNot your bits. What I am pushing out on 'seen' now has a few topics\nexcluded to avoid CI breakages, reproducible or flaky.\n\nThanks.\n"}]}