Bug: All git operations fail when .git contains a non-existent gitdir

What did you do before the bug happened? (Steps to reproduce your issue)

Add a git submodule to a git repository.
Overlay-mount that submodule to another place in the filesystem.
Attempt any git operation in the overlay-mounted path.

What did you expect to happen? (Expected behaviour)

git operations that don't depend on a repository should succeed.

What happened instead? (Actual behaviour)

All git operations fail.  Even `git bugreport` fails.  More importantly,
`git ls-remote` on an unrelated repository fails.

$ git bugreport
fatal: not a git repository:
/home/tkcook/git/Veea/demo.git/VHP-3375-veeadb-on-vhe09/build/iesv10/programs/veeadb/../../../../../../.git/worktree>

What's different between what you expected and what actually happened?

Commands that should succeed actually fail with the above message.

Anything else you want to add:

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.30.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.13.0 #1 SMP Mon Jun 28 11:38:29 UTC 2021 x86_64
compiler info: gnuc: 10.2
libc info: glibc: 2.33
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]
not run from a git repository - no hooks to show

On 2021-07-21 at 09:17:36, Tom Cook wrote:

What did you do before the bug happened? (Steps to reproduce your issue)

Add a git submodule to a git repository.
Overlay-mount that submodule to another place in the filesystem.
Attempt any git operation in the overlay-mounted path.

I'm not sure about what you mean by an overlay-mount operation.  Can you
provide some specific commands that we can run at a shell that reproduce
the issue?
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

The easiest way to reproduce it is this:

$ mkdir test
$ cd test
$ echo "gitdir: /foo/bar" > .git
$ git ls-remote https://github.com/torvalds/linux

We happen to use overlay mounts in our build system in a way that maps
a git submodule from one place to another so that its "gitdir" is
invalid and then attempt a `git ls-remote` from that location which
unexpectedly fails.  But the above reproduces the problem well enough.

Regards,
Tom

On Wed, Jul 21, 2021 at 11:59 PM brian m. carlson
[off-list ref] wrote:

On 2021-07-21 at 09:17:36, Tom Cook wrote:

quoted

What did you do before the bug happened? (Steps to reproduce your issue)

Add a git submodule to a git repository.
Overlay-mount that submodule to another place in the filesystem.
Attempt any git operation in the overlay-mounted path.

I'm not sure about what you mean by an overlay-mount operation.  Can you
provide some specific commands that we can run at a shell that reproduce
the issue?
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

Change RUN_SETUP_GENTLY to stop dying if e.g. the .git is "not a
repo". This means that we now recover in cases like:

    $ echo "gitdir: /foo/bar" > .git
    $ git ls-remote https://github.com/torvalds/linux
    [... ls-remote output ...]

But not (as intended):

    $ git rev-parse HEAD
    fatal: not a git repository: /foo/bar

The relevant setup_git_directory_gently_1() invocation was added in
01017dce546 (setup_git_directory_gently_1(): avoid die()ing,
2017-03-13), but I could reproduce this as far back as Git v1.8.0. I
don't know if this ever worked, but it should.

Let's also use the compiler to check enum arms for us, instead of
having a "default" fall-though case, this changes code added in
ce9b8aab5d9 (setup_git_directory_1(): avoid changing global state,
2017-03-13).

Reported-by: Tom Cook <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 setup.c            | 27 ++++++++++++++++++++++-----
 t/t0002-gitfile.sh |  8 ++++++--
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/setup.c b/setup.c
index eb9367ca5c..6ff145d58b 100644
--- a/setup.c
+++ b/setup.c

@@ -1033,7 +1033,8 @@ enum discovery_result {
 	/* these are errors */
 	GIT_DIR_HIT_CEILING = -1,
 	GIT_DIR_HIT_MOUNT_POINT = -2,
-	GIT_DIR_INVALID_GITFILE = -3
+	GIT_DIR_INVALID_GITFILE = -3,
+	GIT_DIR_GITFILE_NOT_A_REPO = -4,
 };
 
 /*

@@ -1118,8 +1119,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 				/* NEEDSWORK: fail if .git is not file nor dir */
 				if (is_git_directory(dir->buf))
 					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+			} else if (error_code == READ_GITFILE_ERR_NOT_A_REPO) {
+				return GIT_DIR_GITFILE_NOT_A_REPO;
+			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
 				return GIT_DIR_INVALID_GITFILE;
+			}
 		}
 		strbuf_setlen(dir, offset);
 		if (gitdirenv) {

@@ -1209,6 +1213,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
 	const char *prefix = NULL;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+	int die_on_error = !nongit_ok;
+	enum discovery_result discovery;
 
 	/*
 	 * We may have read an incomplete configuration before

@@ -1231,7 +1237,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die_errno(_("Unable to read current working directory"));
 	strbuf_addbuf(&dir, &cwd);
 
-	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
+	discovery = setup_git_directory_gently_1(&dir, &gitdir, die_on_error);
+
+	switch (discovery) {
 	case GIT_DIR_EXPLICIT:
 		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
 		break;

@@ -1259,6 +1267,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			    dir.buf);
 		*nongit_ok = 1;
 		break;
+	case GIT_DIR_GITFILE_NOT_A_REPO:
+		if (!nongit_ok)
+			die(_("not a git repository: %s"), dir.buf);
+		*nongit_ok = 1;
+		break;
+	case GIT_DIR_INVALID_GITFILE:
+		if (!nongit_ok)
+			die(_("invalid .git file: %s"), dir.buf);
+		*nongit_ok = 1;
+		break;
 	case GIT_DIR_NONE:
 		/*
 		 * As a safeguard against setup_git_directory_gently_1 returning

@@ -1266,8 +1284,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		 * set startup_info->have_repository to 1 when we did nothing to
 		 * find a repository.
 		 */
-	default:
-		BUG("unhandled setup_git_directory_1() result");
+		BUG("setup_git_directory_1() should not return GIT_DIR_NONE");
 	}
 
 	/*

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 8440e6add1..176dc8c9dc 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh

@@ -21,13 +21,17 @@ test_expect_success 'initial setup' '
 test_expect_success 'bad setup: invalid .git file format' '
 	echo "gitdir $REAL" >.git &&
 	test_must_fail git rev-parse 2>.err &&
-	test_i18ngrep "invalid gitfile format" .err
+	test_i18ngrep "invalid gitfile format" .err &&
+
+	git ls-remote "file://$REAL"
 '
 
 test_expect_success 'bad setup: invalid .git file path' '
 	echo "gitdir: $REAL.not" >.git &&
 	test_must_fail git rev-parse 2>.err &&
-	test_i18ngrep "not a git repository" .err
+	test_i18ngrep "not a git repository" .err &&
+
+	git ls-remote "file://$REAL"
 '
 
 test_expect_success 'final setup + check rev-parse --git-dir' '

-- 
2.32.0.954.g5a3a1483ade

On 22/07/2021 16:07, Ævar Arnfjörð Bjarmason wrote:

Variable die_on_error could be used in two `if`s above.

On 22-Jul-2021, at 18:43, Tom Cook [off-list ref] wrote:

On Wed, Jul 21, 2021 at 11:59 PM brian m. carlson
[off-list ref] wrote:

quoted

On 2021-07-21 at 09:17:36, Tom Cook wrote:

quoted

What did you do before the bug happened? (Steps to reproduce your issue)

Add a git submodule to a git repository.
Overlay-mount that submodule to another place in the filesystem.
Attempt any git operation in the overlay-mounted path.

I'm not sure about what you mean by an overlay-mount operation.  Can you
provide some specific commands that we can run at a shell that reproduce
the issue?
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

The easiest way to reproduce it is this:

$ mkdir test
$ cd test
$ echo "gitdir: /foo/bar" > .git
$ git ls-remote https://github.com/torvalds/linux

We happen to use overlay mounts in our build system in a way that maps
a git submodule from one place to another so that its "gitdir" is
invalid and then attempt a `git ls-remote` from that location which
unexpectedly fails.  But the above reproduces the problem well enough.

'ls-remote' needs a valid git directory for the case where the URL is not
explicitly supplied (to read the git config and learn the default remote).

Making a special case for when an explicit URL is not given is not as
straightforward as it seems, because by the time 'ls-remote' even knows about
its arguments, it already takes a worktree prefix and sets up the environment,
for which a valid Git repository path is required.

I am not too familiar with this area, and I don't know how feasible it is to
delay setting up the environment until after looking at the 'ls-remote'
arguments. At a cursory glance, it looks difficult to do without large
structural changes to the code.

This might have been less of a problem with old-form submodules, where '.git'
was an actual directory, rather than a text file pointer [1], but newer
versions of Git discourage their usage.

[1] https://git-scm.com/docs/gitsubmodules#_forms

PS: we prefer bottom posting or inline replies :)

---
Pointers for others who might be interested in looking into this:

The immediate cause of this seems to be 'setup.c:setup_gitdir_gently()' [2]
which calls 'setup_gitdir_gently_1()' with the 'die_on_error' argument set
to true. This function then calls 'read_gitfile_gently()' with the same flag,
which errors out when it runs 'is_git_directory()' [3], because the path in
the gitfile is not a valid repository.

[2] https://github.com/git/git/blob/eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687/setup.c#L1234
[3] https://github.com/git/git/blob/eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687/setup.c#L784-L799

---
Atharva Raykar
ಅಥರ್ವ ರಾಯ್ಕರ್
अथर्व रायकर

On Fri, Jul 23, 2021 at 9:23 AM Atharva Raykar [off-list ref] wrote:

I am not too familiar with this area, and I don't know how feasible it is to
delay setting up the environment until after looking at the 'ls-remote'
arguments. At a cursory glance, it looks difficult to do without large
structural changes to the code.

Understood.  This happened to bite us particularly hard, because the
context where we found it is during a `go mod download` and `go`
swallows the error, attempts something else and then reports that the
something else failed with no indication what the underlying error is.
It took us a while to track down what was going on (see
https://github.com/golang/go/issues/47311).

On Thu, Jul 22 2021, Andrei Rybak wrote:

On 22/07/2021 16:07, Ævar Arnfjörð Bjarmason wrote:

quoted

Change RUN_SETUP_GENTLY to stop dying if e.g. the .git is "not a
repo". This means that we now recover in cases like:
     $ echo "gitdir: /foo/bar" > .git
     $ git ls-remote https://github.com/torvalds/linux
     [... ls-remote output ...]
But not (as intended):
     $ git rev-parse HEAD
     fatal: not a git repository: /foo/bar
The relevant setup_git_directory_gently_1() invocation was added in
01017dce546 (setup_git_directory_gently_1(): avoid die()ing,
2017-03-13), but I could reproduce this as far back as Git v1.8.0. I
don't know if this ever worked, but it should.
Let's also use the compiler to check enum arms for us, instead of
having a "default" fall-though case, this changes code added in
ce9b8aab5d9 (setup_git_directory_1(): avoid changing global state,
2017-03-13).
Reported-by: Tom Cook <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
  setup.c            | 27 ++++++++++++++++++++++-----
  t/t0002-gitfile.sh |  8 ++++++--
  2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/setup.c b/setup.c
index eb9367ca5c..6ff145d58b 100644
--- a/setup.c
+++ b/setup.c

@@ -1033,7 +1033,8 @@ enum discovery_result {
  	/* these are errors */
  	GIT_DIR_HIT_CEILING = -1,
  	GIT_DIR_HIT_MOUNT_POINT = -2,
-	GIT_DIR_INVALID_GITFILE = -3
+	GIT_DIR_INVALID_GITFILE = -3,
+	GIT_DIR_GITFILE_NOT_A_REPO = -4,
  };
    /*

@@ -1118,8 +1119,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
  				/* NEEDSWORK: fail if .git is not file nor dir */
  				if (is_git_directory(dir->buf))
  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+			} else if (error_code == READ_GITFILE_ERR_NOT_A_REPO) {
+				return GIT_DIR_GITFILE_NOT_A_REPO;
+			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
  				return GIT_DIR_INVALID_GITFILE;
+			}
  		}
  		strbuf_setlen(dir, offset);
  		if (gitdirenv) {

@@ -1209,6 +1213,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
  	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
  	const char *prefix = NULL;
  	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+	int die_on_error = !nongit_ok;
+	enum discovery_result discovery;
    	/*
  	 * We may have read an incomplete configuration before

@@ -1231,7 +1237,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
  		die_errno(_("Unable to read current working directory"));
  	strbuf_addbuf(&dir, &cwd);
  -	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
+	discovery = setup_git_directory_gently_1(&dir, &gitdir, die_on_error);
+
+	switch (discovery) {
  	case GIT_DIR_EXPLICIT:
  		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
  		break;

@@ -1259,6 +1267,16 @@ const char *setup_git_directory_gently(int *nongit_ok)
  			    dir.buf);
  		*nongit_ok = 1;
  		break;
+	case GIT_DIR_GITFILE_NOT_A_REPO:
+		if (!nongit_ok)
+			die(_("not a git repository: %s"), dir.buf);
+		*nongit_ok = 1;
+		break;
+	case GIT_DIR_INVALID_GITFILE:
+		if (!nongit_ok)

Variable die_on_error could be used in two `if`s above.

Re-reading my own code I think it's better just to drop die_on_error
entirely and use !nongit_ok consistently, as the rest of the function
does. What do yo think?

On Fri, Jul 23 2021, Atharva Raykar wrote:

On 22-Jul-2021, at 18:43, Tom Cook [off-list ref] wrote:

quoted

On Wed, Jul 21, 2021 at 11:59 PM brian m. carlson
[off-list ref] wrote:

quoted

On 2021-07-21 at 09:17:36, Tom Cook wrote:

quoted

What did you do before the bug happened? (Steps to reproduce your issue)

Add a git submodule to a git repository.
Overlay-mount that submodule to another place in the filesystem.
Attempt any git operation in the overlay-mounted path.

I'm not sure about what you mean by an overlay-mount operation.  Can you
provide some specific commands that we can run at a shell that reproduce
the issue?
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

The easiest way to reproduce it is this:

$ mkdir test
$ cd test
$ echo "gitdir: /foo/bar" > .git
$ git ls-remote https://github.com/torvalds/linux

We happen to use overlay mounts in our build system in a way that maps
a git submodule from one place to another so that its "gitdir" is
invalid and then attempt a `git ls-remote` from that location which
unexpectedly fails.  But the above reproduces the problem well enough.

'ls-remote' needs a valid git directory for the case where the URL is not
explicitly supplied (to read the git config and learn the default remote).

Making a special case for when an explicit URL is not given is not as
straightforward as it seems, because by the time 'ls-remote' even knows about
its arguments, it already takes a worktree prefix and sets up the environment,
for which a valid Git repository path is required.

I am not too familiar with this area, and I don't know how feasible it is to
delay setting up the environment until after looking at the 'ls-remote'
arguments. At a cursory glance, it looks difficult to do without large
structural changes to the code.

This might have been less of a problem with old-form submodules, where '.git'
was an actual directory, rather than a text file pointer [1], but newer
versions of Git discourage their usage.

[1] https://git-scm.com/docs/gitsubmodules#_forms

PS: we prefer bottom posting or inline replies :)

---
Pointers for others who might be interested in looking into this:

The immediate cause of this seems to be 'setup.c:setup_gitdir_gently()' [2]
which calls 'setup_gitdir_gently_1()' with the 'die_on_error' argument set
to true. This function then calls 'read_gitfile_gently()' with the same flag,
which errors out when it runs 'is_git_directory()' [3], because the path in
the gitfile is not a valid repository.

[2] https://github.com/git/git/blob/eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687/setup.c#L1234
[3] https://github.com/git/git/blob/eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687/setup.c#L784-L799

The timing of this reply after [1] suggests that you may not have seen
that patch that fixes this issue (sans Junio's outstanding comments on
it).

Perhaps your E-Mail client is forcing threading by subject only, not
In-Reply-To chains?

1. https://lore.kernel.org/git/patch-1.1-fc26c46d39-20210722T140648Z-avarab@gmail.com/

On 23-Jul-2021, at 21:17, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

[...]
The timing of this reply after [1] suggests that you may not have seen
that patch that fixes this issue (sans Junio's outstanding comments on
it).

Perhaps your E-Mail client is forcing threading by subject only, not
In-Reply-To chains?

1. https://lore.kernel.org/git/patch-1.1-fc26c46d39-20210722T140648Z-avarab@gmail.com/

Yes, that is exactly what happened, and I noticed your patch a few minutes 
after I sent that message.

On Thu, Jul 22, 2021 at 6:16 AM Tom Cook [off-list ref] wrote:

The easiest way to reproduce it is this:

$ mkdir test
$ cd test
$ echo "gitdir: /foo/bar" > .git
$ git ls-remote https://github.com/torvalds/linux

We happen to use overlay mounts in our build system in a way that maps
a git submodule from one place to another so that its "gitdir" is
invalid and then attempt a `git ls-remote` from that location which
unexpectedly fails.  But the above reproduces the problem well enough.

I'm making a guess about your setup. To me it sounds like tweaking the
overlayfs or bind mounts to also map the .git directory of the parent
repository (at just the right relative location inside the build
environment) is the right approach towards making the .git-file links
happy.

Submodules point to the parent repository's .git/modules/ directory
these days so poking those paths through into the mapped file system
is the solution that's going to lead to the least amount of
hard-to-diagnose issues in the future.

The .git-file uses relative paths so it is possible to remap and
relocate them as you're doing, but it's going to require another mount
in the right relative location to do it.

It seems doable, even for most edge cases. For example if you end up
mapping some/deeply/nested/submod into /shallow inside your
container(?) and the .git-file contains
"../../../../.git/modules/submod", that might seem impossible to
resolve from a "/shallow" directory, but actually it's fine.

The solution in that situation is to map the parent repo's .git/ dir
to the "/.git" filesystem root inside the build environment. Unix
treats "/../../../../../../" as equivalent to "/" so if the .git-file
has a bunch of "../../../" parent-dir traversing going on then it'll
still work from a one-level deep "/shallow" directory. The parent git
repo directory would be expected to be found at "/.git" (with a
"modules" directory inside it) because the parent directory of "/" is
also "/".

This should mesh right in with the setup you described because you're
already using overlay mounts to synthesize a filesystem.

There are edge cases where this approach breaks down, though. One of
these cases is submodules inside submodules. Luckily it doesn't sound
like you have that problem.

There are patches now that special-case being able to run ls-remote
<url> in a broken setup, but the real fix is to extend the build
environment's filesystem view so that the submodules have access to
the underlying .git/modules/ storage.

Hopefully it's just a --mount or --volume docker(?) command-line flag
that has to be extended to poke the parent .git/ repository contents
into the environment. That's the avenue I would explore if I were
already down a path of using mount tricks.

I hope that helps and maybe sparks an idea that you can run with. I
certainly don't mean it to be a dismissive, "go change your setup"
kind of advice, but it would be interesting to know more details since
it seems like achieving correctness is going to require doing that
based on the limited information in this thread about the specifics of
the setup.

Also, I wouldn't be surprised if, even after the "git ls-remote"
robustness fixes are applied, you'll just stumble onto the next git
command that can only be resolved by mapping in the parent repo's .git
directory.

--
David

On Mon, Aug 30, 2021 at 1:38 AM David Aguilar [off-list ref] wrote:

I hope that helps and maybe sparks an idea that you can run with. I
certainly don't mean it to be a dismissive, "go change your setup"
kind of advice, but it would be interesting to know more details since
it seems like achieving correctness is going to require doing that
based on the limited information in this thread about the specifics of
the setup.

Also, I wouldn't be surprised if, even after the "git ls-remote"
robustness fixes are applied, you'll just stumble onto the next git
command that can only be resolved by mapping in the parent repo's .git
directory.

We map the source code into a build tree in order to build
executables.  We don't expect to be using git on that build tree.  But
one of our executables is written in go and uses go modules.  In our
situation, `go mod download` was trying to use `git ls-remote` to
inspect dependencies before downloading them with `git clone`,
swallowing the git error and then dying horribly on something else
that appeared to be unrelated.  I raised a related bug report on `go`
(I think I linked it elsewhere in this thread).  So for us, easily the
worst thing about these two related bugs was just figuring out what
was wrong in the first place, and the go bug was definitely more
severe than the git bug.

We could modify our build system to make the git local repo valid but
there just doesn't seem any point - we never do anything to the local
repo in the build tree.  The next engineer who comes along and looks
at our build system would inevitably ask why on earth we need to make
git work in the build tree and it would be a fair question.

As an end user, it still appears to make little sense for a git
operation that will complete happily with no local git repo at all and
which doesn't depend on the local repo in any way to nonetheless die
because the local repo is not valid.  As a software engineer I can see
why it's that way but as a user it's unhelpful.

Regards,
Tom