[PATCH 0/2] index-pack out-of-repo fixups

[PATCH 0/2] index-pack out-of-repo fixups

[Jeff King]

Coverity reported that the recently-added call to add_packed_git() in
index-pack.c actually does nothing. It's right, and it turns out this is
a minor bug in the .gitmodules fsck patches in v2.17.1. I say minor
because it errs on the side of complaining too much (so it's not a
security problem) and it comes up only in what I can imagine is a pretty
obscure case.

The second patch here fixes that. But while looking into it, I notice a
much older breakage in running "index-pack --strict" outside of a
repository entirely. That's fixed by the first patch.

  [1/2]: prepare_commit_graft: treat non-repository as a noop
  [2/2]: index-pack: handle --strict checks of non-repo packs

 builtin/index-pack.c       |  8 ++++++--
 commit.c                   |  3 +++
 t/t5300-pack-object.sh     |  6 ++++++
 t/t7415-submodule-names.sh | 10 ++++++++++
 4 files changed, 25 insertions(+), 2 deletions(-)

-Peff

[PATCH 1/2] prepare_commit_graft: treat non-repository as a noop

[Jeff King]

The parse_commit_buffer() function consults lookup_commit_graft()
to see if we need to rewrite parents. The latter will look
at $GIT_DIR/info/grafts. If you're outside of a repository,
then this will trigger a BUG() as of b1ef400eec (setup_git_env:
avoid blind fall-back to ".git", 2016-10-20).

It's probably uncommon to actually parse a commit outside of
a repository, but you can see it in action with:

  cd /not/a/git/repo
  git index-pack --strict /some/file.pack

This works fine without --strict, but the fsck checks will
try to parse any commits, triggering the BUG(). We can fix
that by teaching the graft code to behave as if there are no
grafts when we aren't in a repository.

Arguably index-pack (and fsck) are wrong to consider grafts
at all. So another solution is to disable grafts entirely
for those commands. But given that the graft feature is
deprecated anyway, it's not worth even thinking through the
ramifications that might have.

There is one other corner case I considered here. What
should:

  cd /not/a/git/repo
  export GIT_GRAFT_FILE=/file/with/grafts
  git index-pack --strict /some/file.pack

do? We don't have a repository, but the user has pointed us
directly at a graft file, which we could respect. I believe
this case did work that way prior to b1ef400eec. However,
fixing it now would be pretty invasive. Back then we would
just call into setup_git_env() even without a repository.
But these days it actually takes a git_dir argument. So
there would be a fair bit of refactoring of the setup code
involved.

Given the obscurity of this case, plus the fact that grafts
are deprecated and probably shouldn't work under index-pack
anyway, it's not worth pursuing further. This patch at least
un-breaks the common case where you're _not_ using grafts,
but we BUG() anyway trying to even find that out.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.c               | 3 +++
 t/t5300-pack-object.sh | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/commit.c b/commit.c
index b0e57cc440..0030e79940 100644
--- a/commit.c
+++ b/commit.c
@@ -207,6 +207,9 @@ static void prepare_commit_graft(void)
 
 	if (commit_graft_prepared)
 		return;
+	if (!startup_info->have_repository)
+		return;
+
 	graft_file = get_graft_file();
 	read_graft_file(graft_file);
 	/* make sure shallows are read */
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 87a590c4a9..2336d09dcc 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -421,6 +421,12 @@ test_expect_success 'index-pack <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
+test_expect_success 'index-pack --strict <pack> works in non-repo' '
+	rm -f foo.idx &&
+	nongit git index-pack --strict ../foo.pack &&
+	test_path_is_file foo.idx
+'
+
 test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
 	test_must_fail git index-pack --threads=2 2>err &&
 	grep ^warning: err >warnings &&
-- 
2.17.1.1443.gd58a3f03b7

[PATCH 2/2] index-pack: handle --strict checks of non-repo packs

[Jeff King]

Commit 73c3f0f704 (index-pack: check .gitmodules files with
--strict, 2018-05-04) added a call to add_packed_git(), with
the intent that the newly-indexed objects would be available
to the process when we run fsck_finish().  But that's not
what add_packed_git() does. It only allocates the struct,
and you must install_packed_git() on the result. So that
call was effectively doing nothing (except leaking a
struct).

But wait, we passed all of the tests! Does that mean we
don't need the call at all?

For normal cases, no. When we run "index-pack --stdin"
inside a repository, we write the new pack into the object
directory. If fsck_finish() needs to access one of the new
objects, then our initial lookup will fail to find it, but
we'll follow up by running reprepare_packed_git() and
looking again. That logic was meant to handle somebody else
repacking simultaneously, but it ends up working for us
here.

But there is a case that does need this, that we were not
testing. You can run "git index-pack foo.pack" on any file,
even when it is not inside the object directory. Or you may
not even be in a repository at all! This case fails without
doing the proper install_packed_git() call.

We can make this work by adding the install call.

Note that we should be prepared to handle add_packed_git()
failing. We can just silently ignore this case, though. If
fsck_finish() later needs the objects and they're not
available, it will complain itself. And if it doesn't
(because we were able to resolve the whole fsck in the first
pass), then it actually isn't an interesting error at all.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c       |  8 ++++++--
 t/t7415-submodule-names.sh | 10 ++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4ab31ed388..74fe2973e1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1482,8 +1482,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	} else
 		chmod(final_index_name, 0444);
 
-	if (do_fsck_object)
-		add_packed_git(final_index_name, strlen(final_index_name), 0);
+	if (do_fsck_object) {
+		struct packed_git *p;
+		p = add_packed_git(final_index_name, strlen(final_index_name), 0);
+		if (p)
+			install_packed_git(the_repository, p);
+	}
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(hash));
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index a770d92a55..4157e1a134 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -122,6 +122,16 @@ test_expect_success 'transfer.fsckObjects handles odd pack (index)' '
 	test_must_fail git -C dst.git index-pack --strict --stdin <odd.pack
 '
 
+test_expect_success 'index-pack --strict works for non-repo pack' '
+	rm -rf dst.git &&
+	git init --bare dst.git &&
+	cp odd.pack dst.git &&
+	test_must_fail git -C dst.git index-pack --strict odd.pack 2>output &&
+	# Make sure we fail due to bad gitmodules content, not because we
+	# could not read the blob in the first place.
+	grep gitmodulesName output
+'
+
 test_expect_success 'fsck detects symlinked .gitmodules file' '
 	git init symlink &&
 	(
-- 
2.17.1.1443.gd58a3f03b7