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

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

[Tom Cook]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Tom Cook]

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
<sandals@crustytoothpaste.net> wrote:
>
> 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

[PATCH] setup: only die on invalid .git under RUN_SETUP

[Ævar Arnfjörð Bjarmason]

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 <tom.k.cook@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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

I am of two minds.  ls-remote is benign in that it behaves more or
less identically when given certain types of args, and the above may
be a strict improvement (but it does fail if you did not use URL but
use a remote nickname you thought you configured in the repository
in such a situation).  There however are a few niche commands that
work inside and outside a repository and they work differently.  For
example, if you do

  $ git diff file1 file2

in such a corrupt repository, I'd prefer to see the command _fail_
to nudge the user to look into the situation, instead of taking the
output (which degenerates to "git diff --no-index file1 file2"
outside a repository) blindly as a patch that shows the changes
relative to the index for these two paths.

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 22 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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
>
> I am of two minds.  ls-remote is benign in that it behaves more or
> less identically when given certain types of args, and the above may
> be a strict improvement (but it does fail if you did not use URL but
> use a remote nickname you thought you configured in the repository
> in such a situation).  There however are a few niche commands that
> work inside and outside a repository and they work differently.  For
> example, if you do
>
>   $ git diff file1 file2
>
> in such a corrupt repository, I'd prefer to see the command _fail_
> to nudge the user to look into the situation, instead of taking the
> output (which degenerates to "git diff --no-index file1 file2"
> outside a repository) blindly as a patch that shows the changes
> relative to the index for these two paths.

Yes it comes down to what we think RUN_SETUP_GENTLY and
setup_git_directory_gently() should be doing.

I.e. is &nongit_ok supposed to be a binary "repo you can use"/"[...]
can't use", or a tri-state of that plus "this looks like it's supposed
to be a repo, but it's broken, so let's die".

Anyway, if you're not happy with this pretty much as-is consider it
dropped from my side, because I think the next step would be to do some
more work on e.g. split up RUN_SETUP_GENTLY into a mode that makes sense
for "diff", and another for "bugreport" and "ls-remote". I figured this
was an easy bugfix, but if not perhaps Tom Cooks wants to pick this
up...

I guess another easy alternative would be to issue a warning() in this
case, which is a relatively light refactoring of passing an error
message up from the relevant function(s) instead of having it die()
directly.

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I guess another easy alternative would be to issue a warning() in this
> case, which is a relatively light refactoring of passing an error
> message up from the relevant function(s) instead of having it die()
> directly.

I think the end users deserve a warning even when they are running
ls-remote or bugreport in such a corrupt repository-lookalike.  It
might be sufficient to make "git diff [--no-index]" safe enough.

Thanks.

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Tom Cook]

On Thu, Jul 22, 2021 at 10:19 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:

> Anyway, if you're not happy with this pretty much as-is consider it
> dropped from my side, because I think the next step would be to do some
> more work on e.g. split up RUN_SETUP_GENTLY into a mode that makes sense
> for "diff", and another for "bugreport" and "ls-remote". I figured this
> was an easy bugfix, but if not perhaps Tom Cooks wants to pick this
> up...

Just to note that I am following this conversation.  I don't have any
time available to look into this immediately but probably will in a
couple of weeks.  If it's not resolved by then, then I can pick it up.
But please continue the conversation; I've no familiarity at all with
git internals so the more detail that comes out here, the better.

Regards,
Tom

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Andrei Rybak]

On 22/07/2021 16:07, Ævar Arnfjörð Bjarmason wrote:
> 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 <tom.k.cook@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   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.

> +			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' '
> 

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 22 2021, Andrei Rybak wrote:

> On 22/07/2021 16:07, Ævar Arnfjörð Bjarmason wrote:
>> 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 <tom.k.cook@gmail.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   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?

Re: [PATCH] setup: only die on invalid .git under RUN_SETUP

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> +	int die_on_error = !nongit_ok;
>>> +	enum discovery_result discovery;
>>> ...    	/*
>>> +	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?

I think "not X_ok" means we do not consider X is OK, and agree with
you that the code is clearer without an extra indirection (I do not
know if you meant to address me, though).

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

[Atharva Raykar]

On 22-Jul-2021, at 18:43, Tom Cook <tom.k.cook@gmail.com> wrote:
> On Wed, Jul 21, 2021 at 11:59 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>> 
>> 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.

'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
ಅಥರ್ವ ರಾಯ್ಕರ್
अथर्व रायकर

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

[Tom Cook]

On Fri, Jul 23, 2021 at 9:23 AM Atharva Raykar <raykar.ath@gmail.com> 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).

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 23 2021, Atharva Raykar wrote:

> On 22-Jul-2021, at 18:43, Tom Cook <tom.k.cook@gmail.com> wrote:
>> On Wed, Jul 21, 2021 at 11:59 PM brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>>> 
>>> 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.
>
> '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/

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

[Atharva Raykar]

On 23-Jul-2021, at 21:17, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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

[David Aguilar]

On Thu, Jul 22, 2021 at 6:16 AM Tom Cook <tom.k.cook@gmail.com> 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

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

[Tom Cook]

On Mon, Aug 30, 2021 at 1:38 AM David Aguilar <davvid@gmail.com> 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