{"thread":{"id":"56351","subject":"[PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","startedAt":"2021-08-23T09:41:03Z","lastAt":"2021-08-23T18:00:19Z","messageCount":7,"participants":["Johannes Berg","Jeff King","Taylor Blau","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"433355","messageId":"20210823094049.44136-1-johannes@sipsolutions.net","threadId":"56351","inReplyTo":null,"subject":"[PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T09:40:49Z","receivedAt":"2021-08-23T09:41:03Z","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---\nDue to running inside git's tree, even with TEST_NO_CREATE_REPO=t\nI cannot reproduce the segfault in a test without the \"cd /\", so\nI've kept that. Yes, the test caught in that case that the *.rev\nfile wasn't cleaned up (due to being initialized to the wrong git\nrepo [git's] and cleaning up there!), but I wanted to test the\nsegfault too.\n---\n midx.c                      | 10 +++++-----\n t/t5319-multi-pack-index.sh | 19 +++++++++++++++++++\n 2 files changed, 24 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..3b6331f64113 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -201,6 +201,25 @@ 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+\t(\n+\t\tbase=\"$(pwd)\" &&\n+\t\tcd / && # run outside any git repo, including git itself\n+\t\tgit multi-pack-index --object-dir=\"$base/objdir-test-repo/$objdir\" write\n+\t) &&\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":"433453","messageId":"YSPHdofrDOQk3xmy@coredump.intra.peff.net","threadId":"56351","inReplyTo":"20210823094049.44136-1-johannes@sipsolutions.net","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-23T16:06:14Z","receivedAt":"2021-08-23T16:06:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 23, 2021 at 11:40:49AM +0200, Johannes Berg wrote:\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> 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\nI'm not entirely convinced that writing a midx when not \"inside\" a repo\nis something that we want to support. But if we do, then...\n\n> Due to running inside git's tree, even with TEST_NO_CREATE_REPO=t\n> I cannot reproduce the segfault in a test without the \"cd /\", so\n> I've kept that. Yes, the test caught in that case that the *.rev\n> file wasn't cleaned up (due to being initialized to the wrong git\n> repo [git's] and cleaning up there!), but I wanted to test the\n> segfault too.\n\n...there's a helper in the test suite for doing this kind of \"not in a\nrepo\" test:\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3b6331f641..3981bf96d0 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -211,11 +211,8 @@ test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '\n \t) &&\n \trev=\"objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev\" &&\n \ttouch $rev &&\n-\t(\n-\t\tbase=\"$(pwd)\" &&\n-\t\tcd / && # run outside any git repo, including git itself\n-\t\tgit multi-pack-index --object-dir=\"$base/objdir-test-repo/$objdir\" write\n-\t) &&\n+\tnongit git multi-pack-index \\\n+\t\t--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-Peff\n"},{"id":"433462","messageId":"be882704d7cf2a96a78c5c745c0bca2c53150a28.camel@sipsolutions.net","threadId":"56351","inReplyTo":"YSPHdofrDOQk3xmy@coredump.intra.peff.net","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Johannes Berg","fromEmail":"johannes@sipsolutions.net","sentAt":"2021-08-23T17:05:31Z","receivedAt":"2021-08-23T17:05:36Z","isPatch":true,"sender":{"key":"johannes@sipsolutions.net","avatar":"https://avatars.githubusercontent.com/u/5159728?v=4"},"body":"On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:\n> On Mon, Aug 23, 2021 at 11:40:49AM +0200, Johannes Berg wrote:\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> > 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> I'm not entirely convinced that writing a midx when not \"inside\" a repo\n> is something that we want to support. But if we do, then...\n\nSeemed like that was the point of --object-dir?\n\njohannes\n\n"},{"id":"433463","messageId":"YSPWQtOjKVgIKqsd@nand.local","threadId":"56351","inReplyTo":"be882704d7cf2a96a78c5c745c0bca2c53150a28.camel@sipsolutions.net","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-23T17:09:22Z","receivedAt":"2021-08-23T17:09:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:\n> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:\n> > I'm not entirely convinced that writing a midx when not \"inside\" a repo\n> > is something that we want to support. But if we do, then...\n>\n> Seemed like that was the point of --object-dir?\n\nStolee (cc'd) would know more as the original author, but as I recall\nthe point of `--object-dir` was to be able to write a midx in\ndirectories which were acting as Git repositories, but didn't contain a\n`.git` directory.\n\nIt's kind of a strange use-case, but I recall that it was important at\nthe time. Maybe he could shed more light on why. (Either way, we're\nstuck with it ;)).\n\nThanks,\nTaylor\n"},{"id":"433471","messageId":"YSPhTw6biUCxNrq6@coredump.intra.peff.net","threadId":"56351","inReplyTo":"YSPWQtOjKVgIKqsd@nand.local","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-23T17:56:31Z","receivedAt":"2021-08-23T17:56:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 23, 2021 at 01:09:22PM -0400, Taylor Blau wrote:\n\n> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:\n> > On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:\n> > > I'm not entirely convinced that writing a midx when not \"inside\" a repo\n> > > is something that we want to support. But if we do, then...\n> >\n> > Seemed like that was the point of --object-dir?\n> \n> Stolee (cc'd) would know more as the original author, but as I recall\n> the point of `--object-dir` was to be able to write a midx in\n> directories which were acting as Git repositories, but didn't contain a\n> `.git` directory.\n> \n> It's kind of a strange use-case, but I recall that it was important at\n> the time. Maybe he could shed more light on why. (Either way, we're\n> stuck with it ;)).\n\nAnd the point was that those directories would also be alternates of the\ncurrent repo (and that there is a current repo). That's one of the\nthings that your midx-bitmap series tightens.\n\n-Peff\n"},{"id":"433472","messageId":"xmqqk0kcqc0e.fsf@gitster.g","threadId":"56351","inReplyTo":"YSPWQtOjKVgIKqsd@nand.local","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-23T17:58:09Z","receivedAt":"2021-08-23T17:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:\n>> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:\n>> > I'm not entirely convinced that writing a midx when not \"inside\" a repo\n>> > is something that we want to support. But if we do, then...\n>>\n>> Seemed like that was the point of --object-dir?\n>\n> Stolee (cc'd) would know more as the original author, but as I recall\n> the point of `--object-dir` was to be able to write a midx in\n> directories which were acting as Git repositories, but didn't contain a\n> `.git` directory.\n>\n> It's kind of a strange use-case, but I recall that it was important at\n> the time. Maybe he could shed more light on why. (Either way, we're\n> stuck with it ;)).\n\nIt does sound strange.  \"git -C $there multi-pack-index write\"\nwould have felt more natural.\n"},{"id":"433474","messageId":"35654780-b0f7-d1b1-d7a1-0365e42f63b4@gmail.com","threadId":"56351","inReplyTo":"YSPWQtOjKVgIKqsd@nand.local","subject":"Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-23T18:00:13Z","receivedAt":"2021-08-23T18:00:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 1:09 PM, Taylor Blau wrote:\n> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:\n>> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:\n>>> I'm not entirely convinced that writing a midx when not \"inside\" a repo\n>>> is something that we want to support. But if we do, then...\n>>\n>> Seemed like that was the point of --object-dir?\n> \n> Stolee (cc'd) would know more as the original author, but as I recall\n> the point of `--object-dir` was to be able to write a midx in\n> directories which were acting as Git repositories, but didn't contain a\n> `.git` directory.\n> \n> It's kind of a strange use-case, but I recall that it was important at\n> the time. Maybe he could shed more light on why. (Either way, we're\n> stuck with it ;)).\n\nYes, the point was for how VFS for Git (and now Scalar) built the\n\"shared object cache\" directory. This is a directory that acts as\nan alternate for VFS for Git and Scalar clones so objects downloaded\nby one enlistment are immediately available in another.\n\nWhen the multi-pack-index was created, it was designed to handle\nthis shared object cache through the --object-dir parameter. There\nare custom patches in microsoft/git that shoehorn the option into\nbackground maintenance via a config value.\n\nIf I were to redesign the feature for use by Git, then I would make\nthe clones create a bare copy of the repository (if it doesn't already\nexist) and then create a worktree of that bare repo. The key is to\nallow users to delete individual worktrees without losing the object\ndata.\n\nIn summary, there is a reason for its design like this, although its\ndue to an external tool. But we are using it a lot, so I'd prefer that\nit stay.\n\nThanks,\n-Stolee\n"}]}