index-pack outside of repository?

index-pack outside of repository?

[Jeff King]

Running git on 'next', you can trigger a BUG:

  $ cd /some/repo
  $ git pack-objects --all --stdout </dev/null >/tmp/foo.pack
  $ cd /not-a-git-repo
  $ git index-pack --stdin </tmp/foo.pack
  fatal: BUG: setup_git_env called without repository

This obviously comes from my b1ef400eec (setup_git_env: avoid blind
fall-back to ".git", 2016-10-20). What's going on is that index-pack
uses RUN_SETUP_GENTLY, but never actually handles the out-of-repo case.
When we use the internal git_dir to make "objects/pack/pack-xxx.pack",
it barfs.

In older versions of git will just blindly write into
".git/objects/pack", even though there's no repository there.

So I think complaining to the user is the right thing to do here. I
started to write a patch to have index-pack notice when it needs a repo
and doesn't have one, but the logic is actually a bit unclear.  Do we
need to complain early _just_ when --stdin is specified, or does that
miss somes cases?  Likewise, are there cases where --stdin can operate
without a repo? I couldn't think of any.

I'm actually wondering if the way it calls die() in 'next' is a pretty
reasonable way for things to work in general. It happens when we lazily
try to ask for the repository directory. So we don't have to replicate
logic to say "are we going to need a repo"; at the moment we need it, we
notice we don't have it and die. The only problem is that it says "BUG"
and not "this operation must be run in a git repository".

That strategy _might_ be a problem for some programs, which would want
to notice the issue early before doing work. But it seems like a
reasonable outcome for index-pack. Thoughts?

-Peff

Re: index-pack outside of repository?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> In older versions of git will just blindly write into
> ".git/objects/pack", even though there's no repository there.
>
> So I think complaining to the user is the right thing to do here. I
> started to write a patch to have index-pack notice when it needs a repo
> and doesn't have one, but the logic is actually a bit unclear.  Do we
> need to complain early _just_ when --stdin is specified, or does that
> miss somes cases?  Likewise, are there cases where --stdin can operate
> without a repo? I couldn't think of any.

I think there are two and only two major modes; --stdin wants to put
the result in the repository it is working on, while the other mode
takes a filename to deposit the result in, so the latter does not
technically need a repository.

> I'm actually wondering if the way it calls die() in 'next' is a pretty
> reasonable way for things to work in general. It happens when we lazily
> try to ask for the repository directory. So we don't have to replicate
> logic to say "are we going to need a repo"; at the moment we need it, we
> notice we don't have it and die. The only problem is that it says "BUG"
> and not "this operation must be run in a git repository".

Isn't what we currently have is a good way to discover which
codepaths we missed to add a check to issue the latter error?

> That strategy _might_ be a problem for some programs, which would want
> to notice the issue early before doing work. But it seems like a
> reasonable outcome for index-pack. Thoughts?

That is, once we know which codepaths should require a repository, I
think it is reasonable to add a check that is done earlier than the
place where we currently try to see where we have one (which could
be deep in the callchain).  But we are all human and can miss things,
so the BUG() thing is probably fine.  We are cooking it exactly because
we would want to find such corner cases we missed, no?

Re: index-pack outside of repository?

[Jeff King]

On Thu, Dec 15, 2016 at 04:13:38PM -0800, Junio C Hamano wrote:

> > So I think complaining to the user is the right thing to do here. I
> > started to write a patch to have index-pack notice when it needs a repo
> > and doesn't have one, but the logic is actually a bit unclear.  Do we
> > need to complain early _just_ when --stdin is specified, or does that
> > miss somes cases?  Likewise, are there cases where --stdin can operate
> > without a repo? I couldn't think of any.
> 
> I think there are two and only two major modes; --stdin wants to put
> the result in the repository it is working on, while the other mode
> takes a filename to deposit the result in, so the latter does not
> technically need a repository.

OK. That's easy to check for, then.  Reverse-engineering that logic from
the actual calls in index-pack.c:final() is complicated.  But certainly
basing it on --stdin is what I would have expected.

> > That strategy _might_ be a problem for some programs, which would want
> > to notice the issue early before doing work. But it seems like a
> > reasonable outcome for index-pack. Thoughts?
> 
> That is, once we know which codepaths should require a repository, I
> think it is reasonable to add a check that is done earlier than the
> place where we currently try to see where we have one (which could
> be deep in the callchain).  But we are all human and can miss things,
> so the BUG() thing is probably fine.  We are cooking it exactly because
> we would want to find such corner cases we missed, no?

Right, that was my original intent in adding the BUG(): to catch
unhandled cases, and then do the appropriate thing earlier. I was just
questioning whether the appropriate thing in some cases might be dying
at the BUG(), just with a more friendly message. That has the benefit of
being very easy to implement, and never wrong (e.g., forbidding a case
that actually _doesn't_ need to look at the repo).

But if this case really is just "if (from_stdin)" that's quite easy,
too.

-Peff

Re: index-pack outside of repository?

[Jeff King]

On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:

> But if this case really is just "if (from_stdin)" that's quite easy,
> too.

So here is that patch (with some associated refactoring and cleanups).
This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,
though it should be fine to merge with that topic. The BUG will actually
pass the new test, because it calls die, too. I wonder if we should die
with a unique error code on BUGs, and catch them in test_must_fail
similar to the way we catch signal death.

  [1/3]: t5000: extract nongit function to test-lib-functions.sh
  [2/3]: index-pack: complain when --stdin is used outside of a repo
  [3/3]: t: use nongit() function where applicable

 builtin/index-pack.c     |  2 ++
 t/t1308-config-set.sh    | 10 ++--------
 t/t5000-tar-tree.sh      | 14 --------------
 t/t5300-pack-object.sh   | 15 +++++++++++++++
 t/t9100-git-svn-basic.sh | 17 ++---------------
 t/t9902-completion.sh    |  7 +------
 t/test-lib-functions.sh  | 14 ++++++++++++++
 7 files changed, 36 insertions(+), 43 deletions(-)

-Peff

[PATCH 1/3] t5000: extract nongit function to test-lib-functions.sh

[Jeff King]

This function abstracts the idea of running a command
outside of any repository (which is slightly awkward to do
because even if you make a non-repo directory, git may keep
walking up outside of the trash directory). There are
several scripts that use the same technique, so let's make
the function available for everyone.

Signed-off-by: Jeff King <peff@peff.net>
---
I waffled on the name. Something like test_outside_repo() is more
descriptive, but as this is prepended to existing commands, the lines
already end up quite long.

 t/t5000-tar-tree.sh     | 14 --------------
 t/test-lib-functions.sh | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 830bf2a2f6..886b6953e4 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,20 +94,6 @@ check_tar() {
 	'
 }
 
-# run "$@" inside a non-git directory
-nongit () {
-	test -d non-repo ||
-	mkdir non-repo ||
-	return 1
-
-	(
-		GIT_CEILING_DIRECTORIES=$(pwd) &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd non-repo &&
-		"$@"
-	)
-}
-
 test_expect_success \
     'populate workdir' \
     'mkdir a &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..adab7f51f4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -994,3 +994,17 @@ test_copy_bytes () {
 		}
 	' - "$1"
 }
+
+# run "$@" inside a non-git directory
+nongit () {
+	test -d non-repo ||
+	mkdir non-repo ||
+	return 1
+
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non-repo &&
+		"$@"
+	)
+}
-- 
2.11.0.348.g960a0b554

[PATCH 2/3] index-pack: complain when --stdin is used outside of a repo

[Jeff King]

The index-pack builtin is marked as RUN_SETUP_GENTLY,
because it's perfectly fine to index a pack in the
filesystem outside of any repository. However, --stdin mode
will write the result to the object database, which does not
make sense outside of a repository. Doing so creates a bogus
".git" directory with nothing in it except the newly-created
pack and its index.

Instead, let's flag this as an error and abort.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c   |  2 ++
 t/t5300-pack-object.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0a27bab11b..d450a6ada2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1730,6 +1730,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 		usage(index_pack_usage);
 	if (fix_thin_pack && !from_stdin)
 		die(_("--fix-thin cannot be used without --stdin"));
+	if (from_stdin && !startup_info->have_repository)
+		die(_("--stdin requires a git repository"));
 	if (!index_name && pack_name)
 		index_name = derive_filename(pack_name, ".idx", &index_name_buf);
 	if (keep_msg && !keep_name && pack_name)
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 899e52d50f..43a672c345 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -406,6 +406,21 @@ test_expect_success 'verify resulting packs' '
 	git verify-pack test-11-*.pack
 '
 
+test_expect_success 'set up pack for non-repo tests' '
+	# make sure we have a pack with no matching index file
+	cp test-1-*.pack foo.pack
+'
+
+test_expect_success 'index-pack --stdin complains of non-repo' '
+	nongit test_must_fail git index-pack --stdin <foo.pack &&
+	test_path_is_missing non-repo/.git
+'
+
+test_expect_success 'index-pack <pack> works in non-repo' '
+	nongit git index-pack ../foo.pack &&
+	test_path_is_file foo.idx
+'
+
 #
 # WARNING!
 #
-- 
2.11.0.348.g960a0b554

[PATCH 3/3] t: use nongit() function where applicable

[Jeff King]

Many tests want to run a command outside of any git repo;
with the nongit() function this is now a one-liner. It saves
a few lines, but more importantly, it's immediately obvious
what the code is trying to accomplish.

This doesn't convert every such case in the test suite; it
just covers those that want to do a one-off command. Other
cases, such as the ones in t4035, are part of a larger
scheme of outside-repo files, and it's less confusing for
them to stay consistent with the surrounding tests.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is obviously not necessary for the rest of the series, but the
diffstat is certainly pleasing.

 t/t1308-config-set.sh    | 10 ++--------
 t/t9100-git-svn-basic.sh | 17 ++---------------
 t/t9902-completion.sh    |  7 +------
 3 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7655c94c28..ff50960cca 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -219,14 +219,8 @@ test_expect_success 'check line errors for malformed values' '
 '
 
 test_expect_success 'error on modifying repo config without repo' '
-	mkdir no-repo &&
-	(
-		GIT_CEILING_DIRECTORIES=$(pwd) &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd no-repo &&
-		test_must_fail git config a.b c 2>err &&
-		grep "not in a git directory" err
-	)
+	nongit test_must_fail git config a.b c 2>err &&
+	grep "not in a git directory" err
 '
 
 cmdline_config="'foo.bar=from-cmdline'"
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 92a3aa8063..8a8ba65a2a 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -17,25 +17,12 @@ case "$GIT_SVN_LC_ALL" in
 	;;
 esac
 
-deepdir=nothing-above
-ceiling=$PWD
-
 test_expect_success 'git svn --version works anywhere' '
-	mkdir -p "$deepdir" && (
-		GIT_CEILING_DIRECTORIES="$ceiling" &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd "$deepdir" &&
-		git svn --version
-	)
+	nongit git svn --version
 '
 
 test_expect_success 'git svn help works anywhere' '
-	mkdir -p "$deepdir" && (
-		GIT_CEILING_DIRECTORIES="$ceiling" &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd "$deepdir" &&
-		git svn help
-	)
+	nongit git svn help
 '
 
 test_expect_success \
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2ba62fbc17..a34e55f874 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -257,12 +257,7 @@ test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
 '
 
 test_expect_success '__gitdir - not a git repository' '
-	(
-		cd subdir/subsubdir &&
-		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
-		export GIT_CEILING_DIRECTORIES &&
-		test_must_fail __gitdir
-	)
+	nongit test_must_fail __gitdir
 '
 
 test_expect_success '__gitcomp - trailing space - options' '
-- 
2.11.0.348.g960a0b554

Re: index-pack outside of repository?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:
>
>> But if this case really is just "if (from_stdin)" that's quite easy,
>> too.
>
> So here is that patch (with some associated refactoring and cleanups).
> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,
> though it should be fine to merge with that topic. The BUG will actually
> pass the new test, because it calls die, too. I wonder if we should die
> with a unique error code on BUGs, and catch them in test_must_fail
> similar to the way we catch signal death.
>
>   [1/3]: t5000: extract nongit function to test-lib-functions.sh
>   [2/3]: index-pack: complain when --stdin is used outside of a repo
>   [3/3]: t: use nongit() function where applicable

I think 2/3 is a good change to ensure we get a reasonable error for
"index-pack --stdin", and 3/3 is a very good cleanup.  Both of them
of course are enabled by 1/3.

We still fail "nongit git index-pack tmp.pack" with a BUG: though.

Re: index-pack outside of repository?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:
>>
>>> But if this case really is just "if (from_stdin)" that's quite easy,
>>> too.
>>
>> So here is that patch (with some associated refactoring and cleanups).
>> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,
>> though it should be fine to merge with that topic. The BUG will actually
>> pass the new test, because it calls die, too. I wonder if we should die
>> with a unique error code on BUGs, and catch them in test_must_fail
>> similar to the way we catch signal death.
>>
>>   [1/3]: t5000: extract nongit function to test-lib-functions.sh
>>   [2/3]: index-pack: complain when --stdin is used outside of a repo
>>   [3/3]: t: use nongit() function where applicable
>
> I think 2/3 is a good change to ensure we get a reasonable error for
> "index-pack --stdin", and 3/3 is a very good cleanup.  Both of them
> of course are enabled by 1/3.
>
> We still fail "nongit git index-pack tmp.pack" with a BUG: though.

Wait.  

This only happens with a stalled-and-to-be-discarded topic on 'pu'.
Please don't waste time digging it (yet).

Re: index-pack outside of repository?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> On Thu, Dec 15, 2016 at 08:37:28PM -0500, Jeff King wrote:
>>>
>>>> But if this case really is just "if (from_stdin)" that's quite easy,
>>>> too.
>>>
>>> So here is that patch (with some associated refactoring and cleanups).
>>> This is conceptually independent of jk/no-looking-at-dotgit-outside-repo-final,
>>> though it should be fine to merge with that topic. The BUG will actually
>>> pass the new test, because it calls die, too. I wonder if we should die
>>> with a unique error code on BUGs, and catch them in test_must_fail
>>> similar to the way we catch signal death.
>>>
>>>   [1/3]: t5000: extract nongit function to test-lib-functions.sh
>>>   [2/3]: index-pack: complain when --stdin is used outside of a repo
>>>   [3/3]: t: use nongit() function where applicable
>>
>> I think 2/3 is a good change to ensure we get a reasonable error for
>> "index-pack --stdin", and 3/3 is a very good cleanup.  Both of them
>> of course are enabled by 1/3.
>>
>> We still fail "nongit git index-pack tmp.pack" with a BUG: though.
>
> Wait.  
>
> This only happens with a stalled-and-to-be-discarded topic on 'pu'.
> Please don't waste time digging it (yet).

Don't wait ;-).  My mistake.  We can see t5300 broken with this
change and b1ef400eec ("setup_git_env: avoid blind fall-back to
".git"", 2016-10-20) without anything else.  We still need to
address it.

Re: index-pack outside of repository?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
> ...
>> I'm actually wondering if the way it calls die() in 'next' is a pretty
>> reasonable way for things to work in general. It happens when we lazily
>> try to ask for the repository directory. So we don't have to replicate
>> logic to say "are we going to need a repo"; at the moment we need it, we
>> notice we don't have it and die. The only problem is that it says "BUG"
>> and not "this operation must be run in a git repository".
>
> Isn't what we currently have is a good way to discover which
> codepaths we missed to add a check to issue the latter error?

I am tempted to suggest an intermediate step that comes before
b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
2016-10-20), which is the attached, and publish that as part of an
official release.  That way, we'll see what is broken without
hurting people too much (unless they or their scripts care about
extra message given to the standard error stream).  I suspect that
released Git has a slightly larger user base than what is cooked on
'next'.

 environment.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/environment.c b/environment.c
index 0935ec696e..88f857331e 100644
--- a/environment.c
+++ b/environment.c
@@ -167,8 +167,11 @@ static void setup_git_env(void)
 	const char *replace_ref_base;
 
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
-	if (!git_dir)
+	if (!git_dir) {
+		if (!startup_info->have_repository)
+			warning("BUG: please report this at git@vger.kernel.org");
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	}
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
 	if (get_common_dir(&sb, git_dir))

Re: index-pack outside of repository?

[Jeff King]

On Fri, Dec 16, 2016 at 10:01:49AM -0800, Junio C Hamano wrote:

> >> I think 2/3 is a good change to ensure we get a reasonable error for
> >> "index-pack --stdin", and 3/3 is a very good cleanup.  Both of them
> >> of course are enabled by 1/3.
> >>
> >> We still fail "nongit git index-pack tmp.pack" with a BUG: though.
> >
> > Wait.  
> >
> > This only happens with a stalled-and-to-be-discarded topic on 'pu'.
> > Please don't waste time digging it (yet).
> 
> Don't wait ;-).  My mistake.  We can see t5300 broken with this
> change and b1ef400eec ("setup_git_env: avoid blind fall-back to
> ".git"", 2016-10-20) without anything else.  We still need to
> address it.

Ah, I only checked that it did not do anything terrible like write into
".git". But it looks like it still looks at the git_dir value as part of
the collision check.

Here's a patch, on top of the rest of the series.

-- >8 --
Subject: [PATCH] index-pack: skip collision check when not in repository

You can run "git index-pack path/to/foo.pack" outside of a
repository to generate an index file, or just to verify the
contents. There's no point in doing a collision check, since
we obviously do not have any objects to collide with.

The current code will blindly look in .git/objects based on
the result of setup_git_env(). That effectively gives us the
right answer (since we won't find any objects), but it's a
waste of time, and it conflicts with our desire to
eventually get rid of the "fallback to .git" behavior of
setup_git_env().

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't do a test, as this doesn't really have any user-visible
behavior change. I guess technically if you had a non-repo with
".git/objects/12/3456..." in it we would erroneously read it, but that's
kind of bizarre. The interesting test is that when merged with
jk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't
die.

 builtin/index-pack.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d450a6ada2..f4b87c6c9f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
 			const unsigned char *sha1)
 {
 	void *new_data = NULL;
-	int collision_test_needed;
+	int collision_test_needed = 0;
 
 	assert(data || obj_entry);
 
-	read_lock();
-	collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
-	read_unlock();
+	if (startup_info->have_repository) {
+		read_lock();
+		collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
+		read_unlock();
+	}
 
 	if (collision_test_needed && !data) {
 		read_lock();
-- 
2.11.0.348.g960a0b554

Re: index-pack outside of repository?

[Jeff King]

On Fri, Dec 16, 2016 at 10:54:13AM -0800, Junio C Hamano wrote:

> I am tempted to suggest an intermediate step that comes before
> b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
> 2016-10-20), which is the attached, and publish that as part of an
> official release.  That way, we'll see what is broken without
> hurting people too much (unless they or their scripts care about
> extra message given to the standard error stream).  I suspect that
> released Git has a slightly larger user base than what is cooked on
> 'next'.
> 
>  environment.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/environment.c b/environment.c
> index 0935ec696e..88f857331e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -167,8 +167,11 @@ static void setup_git_env(void)
>  	const char *replace_ref_base;
>  
>  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
> -	if (!git_dir)
> +	if (!git_dir) {
> +		if (!startup_info->have_repository)
> +			warning("BUG: please report this at git@vger.kernel.org");
>  		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> +	}

Yes, I think this is a nice way to ease into the change. I wish I had
thought of it when doing the original series, and we could have shipped
it in v2.11. :)

-Peff

Re: index-pack outside of repository?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah, I only checked that it did not do anything terrible like write into
> ".git". But it looks like it still looks at the git_dir value as part of
> the collision check.
>
> Here's a patch, on top of the rest of the series.

Thanks for a quick turnaround, as always.

> -- >8 --
> Subject: [PATCH] index-pack: skip collision check when not in repository
>
> You can run "git index-pack path/to/foo.pack" outside of a
> repository to generate an index file, or just to verify the
> contents. There's no point in doing a collision check, since
> we obviously do not have any objects to collide with.
>
> The current code will blindly look in .git/objects based on
> the result of setup_git_env(). That effectively gives us the
> right answer (since we won't find any objects), but it's a
> waste of time, and it conflicts with our desire to
> eventually get rid of the "fallback to .git" behavior of
> setup_git_env().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I didn't do a test, as this doesn't really have any user-visible
> behavior change.
>
> I guess technically if you had a non-repo with
> ".git/objects/12/3456..." in it we would erroneously read it, but that's
> kind of bizarre. The interesting test is that when merged with
> jk/no-looking-at-dotgit-outside-repo-final, the test in t5300 doesn't
> die.

Yes.

>
>  builtin/index-pack.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index d450a6ada2..f4b87c6c9f 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -787,13 +787,15 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>  			const unsigned char *sha1)
>  {
>  	void *new_data = NULL;
> -	int collision_test_needed;
> +	int collision_test_needed = 0;
>  
>  	assert(data || obj_entry);
>  
> -	read_lock();
> -	collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
> -	read_unlock();
> +	if (startup_info->have_repository) {
> +		read_lock();
> +		collision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);
> +		read_unlock();
> +	}
>  
>  	if (collision_test_needed && !data) {
>  		read_lock();