{"thread":{"id":"56355","subject":"[PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","startedAt":"2021-08-23T17:10:20Z","lastAt":"2021-08-24T19:01:54Z","messageCount":6,"participants":["Johannes Berg","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"433464","messageId":"20210823171011.80588-1-johannes@sipsolutions.net","threadId":"56355","inReplyTo":null,"subject":"[PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T17:10:11Z","receivedAt":"2021-08-23T17:10:20Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"If using --object-dir to point into a repo while the current\nworking dir is outside, such as\n\n  git init /repo\n  git -C /repo ... # add some objects\n  cd /non-repo\n  git multi-pack-index --object-dir /repo/.git/objects/ write\n\nthe binary will segfault trying to access the object-dir via\nthe repo it found, but that's not fully initialized. Fix it\nto use the object_dir properly to clean up the *.rev files,\nthis avoids the crash and cleans up the *.rev files for the\nnow rewritten multi-pack-index properly.\n\nFixes: 38ff7cabb6b8 (\"pack-revindex: write multi-pack reverse indexes\")\nCc: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Johannes Berg <johannes@sipsolutions.net>\n---\nv3:\n - use nongit\n---\n midx.c                      | 10 +++++-----\n t/t5319-multi-pack-index.sh | 15 +++++++++++++++\n 2 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 321c6fdd2f18..902e1a7a7d9d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -882,7 +882,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \tstrbuf_release(&buf);\n }\n \n-static void clear_midx_files_ext(struct repository *r, const char *ext,\n+static void clear_midx_files_ext(const char *object_dir, const char *ext,\n \t\t\t\t unsigned char *keep_hash);\n \n static int midx_checksum_valid(struct multi_pack_index *m)\n@@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \n \tif (flags & MIDX_WRITE_REV_INDEX)\n \t\twrite_midx_reverse_index(midx_name, midx_hash, &ctx);\n-\tclear_midx_files_ext(the_repository, \".rev\", midx_hash);\n+\tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n \n \tcommit_lock_file(&lk);\n \n@@ -1135,7 +1135,7 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len,\n \t\tdie_errno(_(\"failed to remove %s\"), full_path);\n }\n \n-static void clear_midx_files_ext(struct repository *r, const char *ext,\n+static void clear_midx_files_ext(const char *object_dir, const char *ext,\n \t\t\t\t unsigned char *keep_hash)\n {\n \tstruct clear_midx_data data;\n@@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,\n \t\t\t\t    hash_to_hex(keep_hash), ext);\n \tdata.ext = ext;\n \n-\tfor_each_file_in_pack_dir(r->objects->odb->path,\n+\tfor_each_file_in_pack_dir(object_dir,\n \t\t\t\t  clear_midx_file_ext,\n \t\t\t\t  &data);\n \n@@ -1165,7 +1165,7 @@ void clear_midx_file(struct repository *r)\n \tif (remove_path(midx))\n \t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n \n-\tclear_midx_files_ext(r, \".rev\", NULL);\n+\tclear_midx_files_ext(r->objects->odb->path, \".rev\", NULL);\n \n \tfree(midx);\n }\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3d4d9f10c31b..665c6d64a0ab 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -201,6 +201,21 @@ test_expect_success 'write midx with twelve packs' '\n \n compare_results_with_midx \"twelve packs\"\n \n+test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '\n+\tgit init objdir-test-repo &&\n+\ttest_when_finished \"rm -rf objdir-test-repo\" &&\n+\t(\n+\t\tcd objdir-test-repo &&\n+\t\ttest_commit base &&\n+\t\tgit repack -d\n+\t) &&\n+\trev=\"objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev\" &&\n+\ttouch $rev &&\n+\tnongit git multi-pack-index --object-dir=\"$(pwd)/objdir-test-repo/$objdir\" write &&\n+\ttest_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&\n+\ttest_path_is_missing $rev\n+'\n+\n test_expect_success 'warn on improper hash version' '\n \tgit init --object-format=sha1 sha1 &&\n \t(\n-- \n2.31.1\n\n"},{"id":"433502","messageId":"xmqqeeajpyrc.fsf@gitster.g","threadId":"56355","inReplyTo":"20210823171011.80588-1-johannes@sipsolutions.net","subject":"Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T22:44:23Z","receivedAt":"2021-08-23T22:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg <johannes@sipsolutions.net> writes:\n\n> If using --object-dir to point into a repo while the current\n> working dir is outside, such as\n>\n>   git init /repo\n>   git -C /repo ... # add some objects\n>   cd /non-repo\n>   git multi-pack-index --object-dir /repo/.git/objects/ write\n>\n> the binary will segfault trying to access the object-dir via\n> the repo it found, but that's not fully initialized. Fix it\n\nOK, so write_midx_internal() was given an object_dir to work in,\nmade various changes to that directory, but at the very end of the\nsequence, instead of clearing the revindex in the object_dir we have\nbeen working in, cleared the odb associated with the repository.\n\nInitialized or not, that indeed is very wrong.\n\nAnd the new code looks obviously correct, with minimal changes.\n\nThanks for finding and fixing.\n\n> to use the object_dir properly to clean up the *.rev files,\n> this avoids the crash and cleans up the *.rev files for the\n> now rewritten multi-pack-index properly.\n>\n> Fixes: 38ff7cabb6b8 (\"pack-revindex: write multi-pack reverse indexes\")\n> Cc: Taylor Blau <me@ttaylorr.com>\n> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>\n> ---\n> v3:\n>  - use nongit\n> ---\n>  midx.c                      | 10 +++++-----\n>  t/t5319-multi-pack-index.sh | 15 +++++++++++++++\n>  2 files changed, 20 insertions(+), 5 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 321c6fdd2f18..902e1a7a7d9d 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -882,7 +882,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n>  \tstrbuf_release(&buf);\n>  }\n>  \n> -static void clear_midx_files_ext(struct repository *r, const char *ext,\n> +static void clear_midx_files_ext(const char *object_dir, const char *ext,\n>  \t\t\t\t unsigned char *keep_hash);\n>  \n>  static int midx_checksum_valid(struct multi_pack_index *m)\n> @@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n>  \n>  \tif (flags & MIDX_WRITE_REV_INDEX)\n>  \t\twrite_midx_reverse_index(midx_name, midx_hash, &ctx);\n> -\tclear_midx_files_ext(the_repository, \".rev\", midx_hash);\n> +\tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n>  \n>  \tcommit_lock_file(&lk);\n>  \n> @@ -1135,7 +1135,7 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len,\n>  \t\tdie_errno(_(\"failed to remove %s\"), full_path);\n>  }\n>  \n> -static void clear_midx_files_ext(struct repository *r, const char *ext,\n> +static void clear_midx_files_ext(const char *object_dir, const char *ext,\n>  \t\t\t\t unsigned char *keep_hash)\n>  {\n>  \tstruct clear_midx_data data;\n> @@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,\n>  \t\t\t\t    hash_to_hex(keep_hash), ext);\n>  \tdata.ext = ext;\n>  \n> -\tfor_each_file_in_pack_dir(r->objects->odb->path,\n> +\tfor_each_file_in_pack_dir(object_dir,\n>  \t\t\t\t  clear_midx_file_ext,\n>  \t\t\t\t  &data);\n>  \n> @@ -1165,7 +1165,7 @@ void clear_midx_file(struct repository *r)\n>  \tif (remove_path(midx))\n>  \t\tdie(_(\"failed to clear multi-pack-index at %s\"), midx);\n>  \n> -\tclear_midx_files_ext(r, \".rev\", NULL);\n> +\tclear_midx_files_ext(r->objects->odb->path, \".rev\", NULL);\n>  \n>  \tfree(midx);\n>  }\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3d4d9f10c31b..665c6d64a0ab 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -201,6 +201,21 @@ test_expect_success 'write midx with twelve packs' '\n>  \n>  compare_results_with_midx \"twelve packs\"\n>  \n> +test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '\n> +\tgit init objdir-test-repo &&\n> +\ttest_when_finished \"rm -rf objdir-test-repo\" &&\n> +\t(\n> +\t\tcd objdir-test-repo &&\n> +\t\ttest_commit base &&\n> +\t\tgit repack -d\n> +\t) &&\n> +\trev=\"objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev\" &&\n> +\ttouch $rev &&\n> +\tnongit git multi-pack-index --object-dir=\"$(pwd)/objdir-test-repo/$objdir\" write &&\n> +\ttest_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&\n> +\ttest_path_is_missing $rev\n> +'\n> +\n>  test_expect_success 'warn on improper hash version' '\n>  \tgit init --object-format=sha1 sha1 &&\n>  \t(\n"},{"id":"433504","messageId":"YSQ7wVKbE2HTkEz0@nand.local","threadId":"56355","inReplyTo":"20210823171011.80588-1-johannes@sipsolutions.net","subject":"Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-24T00:22:25Z","receivedAt":"2021-08-24T00:22:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 23, 2021 at 07:10:11PM +0200, Johannes Berg wrote:\n> If using --object-dir to point into a repo while the current\n> working dir is outside, such as\n>\n>   git init /repo\n>   git -C /repo ... # add some objects\n>   cd /non-repo\n>   git multi-pack-index --object-dir /repo/.git/objects/ write\n>\n> the binary will segfault trying to access the object-dir via\n> the repo it found, but that's not fully initialized. Fix it\n> to use the object_dir properly to clean up the *.rev files,\n> this avoids the crash and cleans up the *.rev files for the\n> now rewritten multi-pack-index properly.\n\nThanks for splitting this up and clarifying the bug. This matches my\nunderstanding, and is careful to get the \"--object-dir write\" vs \"write\n--object-dir\" (where the former segfaults, and the latter triggers a\nBUG()) distinction right.\n\nThis looks good to me, but I had one suggestion for an additional test\nwe should consider before picking this up.\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3d4d9f10c31b..665c6d64a0ab 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -201,6 +201,21 @@ test_expect_success 'write midx with twelve packs' '\n>\n>  compare_results_with_midx \"twelve packs\"\n>\n> +test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '\n> +\tgit init objdir-test-repo &&\n> +\ttest_when_finished \"rm -rf objdir-test-repo\" &&\n> +\t(\n> +\t\tcd objdir-test-repo &&\n> +\t\ttest_commit base &&\n> +\t\tgit repack -d\n> +\t) &&\n> +\trev=\"objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev\" &&\n> +\ttouch $rev &&\n\nThis is the only non-obvious part of the patch, but is necessary because\nthere's no way to trigger the MIDX code to write a reverse index\n(thankfully so, since this means that we're not affecting anybody in the\nwild cleaning up .rev's that we shouldn't be).\n\nIt may be worth returning to this in the future when we have support for\nMIDX bitmaps (which will trigger writing a .rev file), but this is\nabsolutely the right thing to do in the meantime.\n\n> +\tnongit git multi-pack-index --object-dir=\"$(pwd)/objdir-test-repo/$objdir\" write &&\n> +\ttest_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&\n> +\ttest_path_is_missing $rev\n\nMakes sense. There's no point in testing that we ignore a .rev file in\nthe outer repository, since we're using nongit to trigger this bug.\n\nBut it may be worth adding an additional test which doesn't use nongit,\nand instead invokes 'git multi-pack-index' from a Git repository, but\npoints at another repo's object directory. That should give us some\nconfidence that we're not deleting .rev files that we shouldn't.\n\nThanks,\nTaylor\n"},{"id":"433511","messageId":"255fb1277db09f66e5cfddc6bbe34181effca3dc.camel@sipsolutions.net","threadId":"56355","inReplyTo":"YSQ7wVKbE2HTkEz0@nand.local","subject":"Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-24T07:50:41Z","receivedAt":"2021-08-24T07:51:41Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Mon, 2021-08-23 at 20:22 -0400, Taylor Blau wrote:\n> \n> > +\trev=\"objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev\" &&\n> > +\ttouch $rev &&\n> \n> This is the only non-obvious part of the patch, but is necessary because\n> there's no way to trigger the MIDX code to write a reverse index\n> (thankfully so, since this means that we're not affecting anybody in the\n> wild cleaning up .rev's that we shouldn't be).\n> \n> It may be worth returning to this in the future when we have support for\n> MIDX bitmaps (which will trigger writing a .rev file)\n\nNo argument there, though it doesn't matter much for this test how you\narrive at a repo that has a .rev file.\n\n> > +\tnongit git multi-pack-index --object-dir=\"$(pwd)/objdir-test-repo/$objdir\" write &&\n> > +\ttest_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&\n> > +\ttest_path_is_missing $rev\n> \n> Makes sense. There's no point in testing that we ignore a .rev file in\n> the outer repository, since we're using nongit to trigger this bug.\n> \n> But it may be worth adding an additional test which doesn't use nongit,\n> and instead invokes 'git multi-pack-index' from a Git repository, but\n> points at another repo's object directory. That should give us some\n> confidence that we're not deleting .rev files that we shouldn't.\n\nMaybe you can just send that as a separate follow-up patch? :)\n\nI'm not _entirely_ sure what you'd want to test, you could do at least\nthese things:\n\n * test like this that the correct file is deleted, from another repo\n   instead of nongit\n * additionally arrange the *other* repo to have a .rev file and check\n   that it's *not* deleted?\n\nBut to me all of the three (including my test) seem quite equivalent, at\nleast as long as we assume that the code won't grow a \"try to delete all\nthe .rev files anywhere I can find\" thing :)\n\njohannes\n\n\n"},{"id":"433512","messageId":"f398645c2c946ea0d7cc6d8f603962dde5f7c4e0.camel@sipsolutions.net","threadId":"56355","inReplyTo":"xmqqeeajpyrc.fsf@gitster.g","subject":"Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-24T07:59:02Z","receivedAt":"2021-08-24T07:59:09Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Mon, 2021-08-23 at 15:44 -0700, Junio C Hamano wrote:\n> Johannes Berg <johannes@sipsolutions.net> writes:\n> \n> > If using --object-dir to point into a repo while the current\n> > working dir is outside, such as\n> > \n> >   git init /repo\n> >   git -C /repo ... # add some objects\n> >   cd /non-repo\n> >   git multi-pack-index --object-dir /repo/.git/objects/ write\n> > \n> > the binary will segfault trying to access the object-dir via\n> > the repo it found, but that's not fully initialized. Fix it\n> \n> OK, so write_midx_internal() was given an object_dir to work in,\n> made various changes to that directory, but at the very end of the\n> sequence, instead of clearing the revindex in the object_dir we have\n> been working in, cleared the odb associated with the repository.\n\nI'm not sure I'd claim \"cleared the odb\" but it's also not entirely\nclear to me what you mean by that.\n\nSpecifically, what happened is that it cleared out all the .rev files in\nthe objects/pack folder associated with the repository. And if there\nwasn't actually a repository, it would NULL-ptr-deref instead.\n\nFeel free to rewrite the commit log, or I can if you really want me to.\nI was more concerned with the segfault, but I can also understand that\nyou'd be more concerned with the on-disk correctness issue this causes.\n\njohannes\n\n"},{"id":"433611","messageId":"xmqqr1eioee8.fsf@gitster.g","threadId":"56355","inReplyTo":"f398645c2c946ea0d7cc6d8f603962dde5f7c4e0.camel@sipsolutions.net","subject":"Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-24T19:01:51Z","receivedAt":"2021-08-24T19:01:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Berg <johannes@sipsolutions.net> writes:\n\n> On Mon, 2021-08-23 at 15:44 -0700, Junio C Hamano wrote:\n>> Johannes Berg <johannes@sipsolutions.net> writes:\n>> \n>> > If using --object-dir to point into a repo while the current\n>> > working dir is outside, such as\n>> > \n>> >   git init /repo\n>> >   git -C /repo ... # add some objects\n>> >   cd /non-repo\n>> >   git multi-pack-index --object-dir /repo/.git/objects/ write\n>> > \n>> > the binary will segfault trying to access the object-dir via\n>> > the repo it found, but that's not fully initialized. Fix it\n>> \n>> OK, so write_midx_internal() was given an object_dir to work in,\n>> made various changes to that directory, but at the very end of the\n>> sequence, instead of clearing the revindex in the object_dir we have\n>> been working in, cleared the odb associated with the repository.\n>\n> I'm not sure I'd claim \"cleared the odb\" but it's also not entirely\n> clear to me what you mean by that.\n\n\"cleared the revindex in the wrong odb\" is what I meant to say.\n"}]}