[PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Johannes Berg]

If using --object-dir to point into a repo while the current
working dir is outside, such as

  git init /repo
  git -C /repo ... # add some objects
  cd /non-repo
  git multi-pack-index --object-dir /repo/.git/objects/ write

the binary will segfault trying to access the object-dir via
the repo it found, but that's not fully initialized. Fix it
to use the object_dir properly to clean up the *.rev files,
this avoids the crash and cleans up the *.rev files for the
now rewritten multi-pack-index properly.

Fixes: 38ff7cabb6b8 ("pack-revindex: write multi-pack reverse indexes")
Cc: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Due to running inside git's tree, even with TEST_NO_CREATE_REPO=t
I cannot reproduce the segfault in a test without the "cd /", so
I've kept that. Yes, the test caught in that case that the *.rev
file wasn't cleaned up (due to being initialized to the wrong git
repo [git's] and cleaning up there!), but I wanted to test the
segfault too.
---
 midx.c                      | 10 +++++-----
 t/t5319-multi-pack-index.sh | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/midx.c b/midx.c
index 321c6fdd2f18..902e1a7a7d9d 100644
--- a/midx.c
+++ b/midx.c
@@ -882,7 +882,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 	strbuf_release(&buf);
 }
 
-static void clear_midx_files_ext(struct repository *r, const char *ext,
+static void clear_midx_files_ext(const char *object_dir, const char *ext,
 				 unsigned char *keep_hash);
 
 static int midx_checksum_valid(struct multi_pack_index *m)
@@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
 
 	if (flags & MIDX_WRITE_REV_INDEX)
 		write_midx_reverse_index(midx_name, midx_hash, &ctx);
-	clear_midx_files_ext(the_repository, ".rev", midx_hash);
+	clear_midx_files_ext(object_dir, ".rev", midx_hash);
 
 	commit_lock_file(&lk);
 
@@ -1135,7 +1135,7 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len,
 		die_errno(_("failed to remove %s"), full_path);
 }
 
-static void clear_midx_files_ext(struct repository *r, const char *ext,
+static void clear_midx_files_ext(const char *object_dir, const char *ext,
 				 unsigned char *keep_hash)
 {
 	struct clear_midx_data data;
@@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,
 				    hash_to_hex(keep_hash), ext);
 	data.ext = ext;
 
-	for_each_file_in_pack_dir(r->objects->odb->path,
+	for_each_file_in_pack_dir(object_dir,
 				  clear_midx_file_ext,
 				  &data);
 
@@ -1165,7 +1165,7 @@ void clear_midx_file(struct repository *r)
 	if (remove_path(midx))
 		die(_("failed to clear multi-pack-index at %s"), midx);
 
-	clear_midx_files_ext(r, ".rev", NULL);
+	clear_midx_files_ext(r->objects->odb->path, ".rev", NULL);
 
 	free(midx);
 }
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 3d4d9f10c31b..3b6331f64113 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -201,6 +201,25 @@ test_expect_success 'write midx with twelve packs' '
 
 compare_results_with_midx "twelve packs"
 
+test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '
+	git init objdir-test-repo &&
+	test_when_finished "rm -rf objdir-test-repo" &&
+	(
+		cd objdir-test-repo &&
+		test_commit base &&
+		git repack -d
+	) &&
+	rev="objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev" &&
+	touch $rev &&
+	(
+		base="$(pwd)" &&
+		cd / && # run outside any git repo, including git itself
+		git multi-pack-index --object-dir="$base/objdir-test-repo/$objdir" write
+	) &&
+	test_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&
+	test_path_is_missing $rev
+'
+
 test_expect_success 'warn on improper hash version' '
 	git init --object-format=sha1 sha1 &&
 	(
-- 
2.31.1

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Jeff King]

On Mon, Aug 23, 2021 at 11:40:49AM +0200, Johannes Berg wrote:

> If using --object-dir to point into a repo while the current
> working dir is outside, such as
> 
>   git init /repo
>   git -C /repo ... # add some objects
>   cd /non-repo
>   git multi-pack-index --object-dir /repo/.git/objects/ write
> 
> the binary will segfault trying to access the object-dir via
> the repo it found, but that's not fully initialized. Fix it
> to use the object_dir properly to clean up the *.rev files,
> this avoids the crash and cleans up the *.rev files for the
> now rewritten multi-pack-index properly.

I'm not entirely convinced that writing a midx when not "inside" a repo
is something that we want to support. But if we do, then...

> Due to running inside git's tree, even with TEST_NO_CREATE_REPO=t
> I cannot reproduce the segfault in a test without the "cd /", so
> I've kept that. Yes, the test caught in that case that the *.rev
> file wasn't cleaned up (due to being initialized to the wrong git
> repo [git's] and cleaning up there!), but I wanted to test the
> segfault too.

...there's a helper in the test suite for doing this kind of "not in a
repo" test:

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 3b6331f641..3981bf96d0 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -211,11 +211,8 @@ test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' '
 	) &&
 	rev="objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev" &&
 	touch $rev &&
-	(
-		base="$(pwd)" &&
-		cd / && # run outside any git repo, including git itself
-		git multi-pack-index --object-dir="$base/objdir-test-repo/$objdir" write
-	) &&
+	nongit git multi-pack-index \
+		--object-dir="$PWD/objdir-test-repo/$objdir" write &&
 	test_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&
 	test_path_is_missing $rev
 '

-Peff

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Johannes Berg]

On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:
> On Mon, Aug 23, 2021 at 11:40:49AM +0200, Johannes Berg wrote:
> 
> > If using --object-dir to point into a repo while the current
> > working dir is outside, such as
> > 
> >   git init /repo
> >   git -C /repo ... # add some objects
> >   cd /non-repo
> >   git multi-pack-index --object-dir /repo/.git/objects/ write
> > 
> > the binary will segfault trying to access the object-dir via
> > the repo it found, but that's not fully initialized. Fix it
> > to use the object_dir properly to clean up the *.rev files,
> > this avoids the crash and cleans up the *.rev files for the
> > now rewritten multi-pack-index properly.
> 
> I'm not entirely convinced that writing a midx when not "inside" a repo
> is something that we want to support. But if we do, then...

Seemed like that was the point of --object-dir?

johannes

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Taylor Blau]

On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:
> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:
> > I'm not entirely convinced that writing a midx when not "inside" a repo
> > is something that we want to support. But if we do, then...
>
> Seemed like that was the point of --object-dir?

Stolee (cc'd) would know more as the original author, but as I recall
the point of `--object-dir` was to be able to write a midx in
directories which were acting as Git repositories, but didn't contain a
`.git` directory.

It's kind of a strange use-case, but I recall that it was important at
the time. Maybe he could shed more light on why. (Either way, we're
stuck with it ;)).

Thanks,
Taylor

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Jeff King]

On Mon, Aug 23, 2021 at 01:09:22PM -0400, Taylor Blau wrote:

> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:
> > On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:
> > > I'm not entirely convinced that writing a midx when not "inside" a repo
> > > is something that we want to support. But if we do, then...
> >
> > Seemed like that was the point of --object-dir?
> 
> Stolee (cc'd) would know more as the original author, but as I recall
> the point of `--object-dir` was to be able to write a midx in
> directories which were acting as Git repositories, but didn't contain a
> `.git` directory.
> 
> It's kind of a strange use-case, but I recall that it was important at
> the time. Maybe he could shed more light on why. (Either way, we're
> stuck with it ;)).

And the point was that those directories would also be alternates of the
current repo (and that there is a current repo). That's one of the
things that your midx-bitmap series tightens.

-Peff

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:
>> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:
>> > I'm not entirely convinced that writing a midx when not "inside" a repo
>> > is something that we want to support. But if we do, then...
>>
>> Seemed like that was the point of --object-dir?
>
> Stolee (cc'd) would know more as the original author, but as I recall
> the point of `--object-dir` was to be able to write a midx in
> directories which were acting as Git repositories, but didn't contain a
> `.git` directory.
>
> It's kind of a strange use-case, but I recall that it was important at
> the time. Maybe he could shed more light on why. (Either way, we're
> stuck with it ;)).

It does sound strange.  "git -C $there multi-pack-index write"
would have felt more natural.

Re: [PATCH v2] multi-pack-index: fix *.rev cleanups with --object-dir

[Derrick Stolee]

On 8/23/2021 1:09 PM, Taylor Blau wrote:
> On Mon, Aug 23, 2021 at 07:05:31PM +0200, Johannes Berg wrote:
>> On Mon, 2021-08-23 at 12:06 -0400, Jeff King wrote:
>>> I'm not entirely convinced that writing a midx when not "inside" a repo
>>> is something that we want to support. But if we do, then...
>>
>> Seemed like that was the point of --object-dir?
> 
> Stolee (cc'd) would know more as the original author, but as I recall
> the point of `--object-dir` was to be able to write a midx in
> directories which were acting as Git repositories, but didn't contain a
> `.git` directory.
> 
> It's kind of a strange use-case, but I recall that it was important at
> the time. Maybe he could shed more light on why. (Either way, we're
> stuck with it ;)).

Yes, the point was for how VFS for Git (and now Scalar) built the
"shared object cache" directory. This is a directory that acts as
an alternate for VFS for Git and Scalar clones so objects downloaded
by one enlistment are immediately available in another.

When the multi-pack-index was created, it was designed to handle
this shared object cache through the --object-dir parameter. There
are custom patches in microsoft/git that shoehorn the option into
background maintenance via a config value.

If I were to redesign the feature for use by Git, then I would make
the clones create a bare copy of the repository (if it doesn't already
exist) and then create a worktree of that bare repo. The key is to
allow users to delete individual worktrees without losing the object
data.

In summary, there is a reason for its design like this, although its
due to an external tool. But we are using it a lot, so I'd prefer that
it stay.

Thanks,
-Stolee