git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* Bug: All git operations fail when .git contains a non-existent gitdir
@ 2021-07-21  9:17 Tom Cook
  2021-07-21 22:58 ` brian m. carlson
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Cook @ 2021-07-21  9:17 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-21  9:17 Bug: All git operations fail when .git contains a non-existent gitdir Tom Cook
@ 2021-07-21 22:58 ` brian m. carlson
  2021-07-22 13:13   ` Tom Cook
  0 siblings, 1 reply; 15+ messages in thread
From: brian m. carlson @ 2021-07-21 22:58 UTC (permalink / raw)
  To: Tom Cook; +Cc: git

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-21 22:58 ` brian m. carlson
@ 2021-07-22 13:13   ` Tom Cook
  2021-07-22 14:07     ` [PATCH] setup: only die on invalid .git under RUN_SETUP Ævar Arnfjörð Bjarmason
  2021-07-23  8:23     ` Bug: All git operations fail when .git contains a non-existent gitdir Atharva Raykar
  0 siblings, 2 replies; 15+ messages in thread
From: Tom Cook @ 2021-07-22 13:13 UTC (permalink / raw)
  To: brian m. carlson, Tom Cook; +Cc: git

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 13:13   ` Tom Cook
@ 2021-07-22 14:07     ` Ævar Arnfjörð Bjarmason
  2021-07-22 19:06       ` Junio C Hamano
  2021-07-22 20:50       ` Andrei Rybak
  2021-07-23  8:23     ` Bug: All git operations fail when .git contains a non-existent gitdir Atharva Raykar
  1 sibling, 2 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-22 14:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Tom Cook, brian m . carlson,
	Æ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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 14:07     ` [PATCH] setup: only die on invalid .git under RUN_SETUP Ævar Arnfjörð Bjarmason
@ 2021-07-22 19:06       ` Junio C Hamano
  2021-07-22 21:08         ` Ævar Arnfjörð Bjarmason
  2021-07-22 20:50       ` Andrei Rybak
  1 sibling, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2021-07-22 19:06 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Tom Cook, brian m . carlson

Æ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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 14:07     ` [PATCH] setup: only die on invalid .git under RUN_SETUP Ævar Arnfjörð Bjarmason
  2021-07-22 19:06       ` Junio C Hamano
@ 2021-07-22 20:50       ` Andrei Rybak
  2021-07-23  9:33         ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 15+ messages in thread
From: Andrei Rybak @ 2021-07-22 20:50 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Tom Cook, brian m . carlson

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 19:06       ` Junio C Hamano
@ 2021-07-22 21:08         ` Ævar Arnfjörð Bjarmason
  2021-07-23  1:59           ` Junio C Hamano
  2021-07-23  8:42           ` Tom Cook
  0 siblings, 2 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-22 21:08 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Tom Cook, brian m . carlson


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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 21:08         ` Ævar Arnfjörð Bjarmason
@ 2021-07-23  1:59           ` Junio C Hamano
  2021-07-23  8:42           ` Tom Cook
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2021-07-23  1:59 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Tom Cook, brian m . carlson

Æ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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-22 13:13   ` Tom Cook
  2021-07-22 14:07     ` [PATCH] setup: only die on invalid .git under RUN_SETUP Ævar Arnfjörð Bjarmason
@ 2021-07-23  8:23     ` Atharva Raykar
  2021-07-23  8:39       ` Tom Cook
  2021-07-23 15:47       ` Ævar Arnfjörð Bjarmason
  1 sibling, 2 replies; 15+ messages in thread
From: Atharva Raykar @ 2021-07-23  8:23 UTC (permalink / raw)
  To: Tom Cook; +Cc: brian m. carlson, git

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-23  8:23     ` Bug: All git operations fail when .git contains a non-existent gitdir Atharva Raykar
@ 2021-07-23  8:39       ` Tom Cook
  2021-07-23 15:47       ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 15+ messages in thread
From: Tom Cook @ 2021-07-23  8:39 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: brian m. carlson, git

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).

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 21:08         ` Ævar Arnfjörð Bjarmason
  2021-07-23  1:59           ` Junio C Hamano
@ 2021-07-23  8:42           ` Tom Cook
  1 sibling, 0 replies; 15+ messages in thread
From: Tom Cook @ 2021-07-23  8:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, brian m . carlson

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-22 20:50       ` Andrei Rybak
@ 2021-07-23  9:33         ` Ævar Arnfjörð Bjarmason
  2021-07-23 15:21           ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-23  9:33 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: git, Junio C Hamano, Tom Cook, brian m . carlson


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?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] setup: only die on invalid .git under RUN_SETUP
  2021-07-23  9:33         ` Ævar Arnfjörð Bjarmason
@ 2021-07-23 15:21           ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2021-07-23 15:21 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Andrei Rybak, git, Tom Cook, brian m . carlson

Æ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).



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-23  8:23     ` Bug: All git operations fail when .git contains a non-existent gitdir Atharva Raykar
  2021-07-23  8:39       ` Tom Cook
@ 2021-07-23 15:47       ` Ævar Arnfjörð Bjarmason
  2021-07-23 17:02         ` Atharva Raykar
  1 sibling, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-23 15:47 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: Tom Cook, brian m. carlson, git


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/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Bug: All git operations fail when .git contains a non-existent gitdir
  2021-07-23 15:47       ` Ævar Arnfjörð Bjarmason
@ 2021-07-23 17:02         ` Atharva Raykar
  0 siblings, 0 replies; 15+ messages in thread
From: Atharva Raykar @ 2021-07-23 17:02 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Tom Cook, brian m. carlson, git

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-07-23 17:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21  9:17 Bug: All git operations fail when .git contains a non-existent gitdir Tom Cook
2021-07-21 22:58 ` brian m. carlson
2021-07-22 13:13   ` Tom Cook
2021-07-22 14:07     ` [PATCH] setup: only die on invalid .git under RUN_SETUP Ævar Arnfjörð Bjarmason
2021-07-22 19:06       ` Junio C Hamano
2021-07-22 21:08         ` Ævar Arnfjörð Bjarmason
2021-07-23  1:59           ` Junio C Hamano
2021-07-23  8:42           ` Tom Cook
2021-07-22 20:50       ` Andrei Rybak
2021-07-23  9:33         ` Ævar Arnfjörð Bjarmason
2021-07-23 15:21           ` Junio C Hamano
2021-07-23  8:23     ` Bug: All git operations fail when .git contains a non-existent gitdir Atharva Raykar
2021-07-23  8:39       ` Tom Cook
2021-07-23 15:47       ` Ævar Arnfjörð Bjarmason
2021-07-23 17:02         ` Atharva Raykar

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git