[RFC PATCH 0/3] Improve error reporting to mention "why" a directory is not a repository

[RFC PATCH 0/3] Improve error reporting to mention "why" a directory is not a repository

[Kaartic Sivaraam]

At the moment, there are a few scenarios where the error message for an
invalid Git repository is a bit blunt. For instance, when we point
GIT_OBJECT_DIRECTORY at a directory that does not exist, we get this:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'

Even though repo.git itself is a valid repository, we get this rather
puzzling error saying that it is not. The actual problem is the invalid
value given to GIT_OBJECT_DIRECTORY, and the user is left on their
to figure that out. This series aims to make such issues easier to
diagnose by saying why the specified repository was not considered
valid.

With this series, the same command outputs:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'
  reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

The following scenarios are covered when --git-dir is given explicitly:

  - HEAD is missing, or its path is not traversable
  - HEAD is a symlink that cannot be read
  - HEAD is a symlink whose target lives outside refs/
  - HEAD cannot be opened, or cannot be read
  - HEAD contains neither a ref under refs/ nor an object ID
  - $GIT_OBJECT_DIRECTORY is set to something we cannot access
  - the object directory in the common directory is inaccessible
  - the refs directory in the common directory is inaccessible

This series does not yet report a reason in the following cases.

The discovery walk, where we iterate up to the ceiling or the mount
point looking for a repository:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: not a git repository (or any parent up to mount point /)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)

The gitfile case, for instance when run from inside a submodule /
worktree:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: gitfile does not point to a valid repository: /path/to/super/sub/.git

GIT_ALTERNATE_OBJECT_DIRECTORIES is also left alone. Its diagnostic
are misleading in their own way, but they are emitted much later, from
the object database rather than from setup, so they need a separate
treatment.

I would be very interested in hearing what others think about this
direction. If it looks reasonable, I'm happy to cover the remaining
cases too, either in this series or separately.

Kaartic Sivaraam (3):
  t0009: add tests to cover more error reporting scenarios
  setup: introduce new helper 'is_git_directory_verbose'
  setup: communicate why a directory is not a valid git directory

 setup.c                       | 165 +++++++++++++++++++++++++---------
 t/t0009-git-dir-validation.sh |  38 ++++++++
 2 files changed, 161 insertions(+), 42 deletions(-)

-- 
2.56.0.rc1.12.g2c9c8d64bb

[RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios

[Kaartic Sivaraam]

Introduce few more tests to t0009 to cover error reporting scenarios
when --git-dir is used.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 t/t0009-git-dir-validation.sh | 36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index 4cba478e50..244dc07c0e 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -74,4 +74,40 @@ test_expect_success 'setup: .git as an empty directory is ignored' '
 	)
 '
 
+test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
+	test_when_finished "rm -rf parent/empty-dir" &&
+	mkdir -p parent/empty-dir &&
+	(
+		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
+test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
+	test_when_finished "rm -rf parent/head-as-link-to-garbage" &&
+	mkdir -p parent/head-as-link-to-garbage &&
+	(
+		cd parent/head-as-link-to-garbage &&
+		git init --bare real-repo &&
+		touch garbage &&
+		rm real-repo/HEAD &&
+		ln -s ../garbage real-repo/HEAD &&
+		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
+test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTORY configuration is rejected' '
+	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
+	mkdir -p parent/invalid-git-object-directory-config &&
+	(
+		cd parent/invalid-git-object-directory-config &&
+		git init --bare real-repo &&
+		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
+			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
+
 test_done
-- 
2.56.0.rc1.12.g2c9c8d64bb

Re: [RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> Introduce few more tests to t0009 to cover error reporting scenarios
> when --git-dir is used.
>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
>  t/t0009-git-dir-validation.sh | 36 +++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
> index 4cba478e50..244dc07c0e 100755
> --- a/t/t0009-git-dir-validation.sh
> +++ b/t/t0009-git-dir-validation.sh
> @@ -74,4 +74,40 @@ test_expect_success 'setup: .git as an empty directory is ignored' '
>  	)
>  '
>  
> +test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
> +	test_when_finished "rm -rf parent/empty-dir" &&
> +	mkdir -p parent/empty-dir &&
> +	(
> +		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
> +		test_grep "not a git repository" stderr
> +	)
> +'

Why subshell?

> +
> +test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
> +	test_when_finished "rm -rf parent/head-as-link-to-garbage" &&
> +	mkdir -p parent/head-as-link-to-garbage &&
> +	(
> +		cd parent/head-as-link-to-garbage &&
> +		git init --bare real-repo &&
> +		touch garbage &&
> +		rm real-repo/HEAD &&
> +		ln -s ../garbage real-repo/HEAD &&
> +		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
> +		test_grep "not a git repository" stderr
> +	)
> +'
> +
> +test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTORY configuration is rejected' '
> +	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
> +	mkdir -p parent/invalid-git-object-directory-config &&
> +	(
> +		cd parent/invalid-git-object-directory-config &&
> +		git init --bare real-repo &&
> +		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
> +			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
> +		test_grep "not a git repository" stderr
> +	)
> +'
> +
> +
>  test_done

Re: [RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios

[Kaartic Sivaraam]

On 9/25/26 03:38, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> Introduce few more tests to t0009 to cover error reporting scenarios
>> when --git-dir is used.
>>
>> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>> ---
>>   t/t0009-git-dir-validation.sh | 36 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
>> index 4cba478e50..244dc07c0e 100755
>> --- a/t/t0009-git-dir-validation.sh
>> +++ b/t/t0009-git-dir-validation.sh
>> @@ -74,4 +74,40 @@ test_expect_success 'setup: .git as an empty directory is ignored' '
>>   	)
>>   '
>>   
>> +test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
>> +	test_when_finished "rm -rf parent/empty-dir" &&
>> +	mkdir -p parent/empty-dir &&
>> +	(
>> +		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
>> +		test_grep "not a git repository" stderr
>> +	)
>> +'
> 
> Why subshell?
>

Good catch. It is unnecessary. An earlier iteration used to cd into 
parent/empty-dir. This is no longer the case. So, I'll avoid the 
sub-shell in this test.

-- 
Sivaraam

[RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose'

[Kaartic Sivaraam]

Introduce a new helper is_git_directory_verbose() as a
counterpart to the existing is_git_directory().

is_git_directory_verbose() also populates an optional
string strbuf with reasoning around why the given suspect
is not a valid git directory. This strbuf in turn can be
used to improve the error reporting which is currently blunt:

  fatal: not a git repository

This is not helpful as the user does not get any hint about "why"
the repository is not considered valid.

Call-site(s) will be made to use this helper in a follow-up commit.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c | 152 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 112 insertions(+), 40 deletions(-)

diff --git a/setup.c b/setup.c
index 0d157ac254..b3b53a1cfc 100644
--- a/setup.c
+++ b/setup.c
@@ -347,7 +347,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
 	return ret;
 }
 
-static int validate_headref(const char *path)
+static int validate_headref(const char *path, struct strbuf *err)
 {
 	struct stat st;
 	char buffer[256];
@@ -356,14 +356,32 @@ static int validate_headref(const char *path)
 	int fd;
 	ssize_t len;
 
-	if (lstat(path, &st) < 0)
+	if (lstat(path, &st) < 0) {
+		if (err)
+			strbuf_addf(
+				err, _("could not stat HEAD at '%s'"), path
+			);
 		return -1;
+	}
 
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
 		if (len >= 5 && !memcmp("refs/", buffer, 5))
 			return 0;
+		if (len == -1 && err)
+			strbuf_addf(
+				err,
+				_("could not read the symlink HEAD at '%s'"),
+				path
+			);
+		else if (err)
+			strbuf_addf(
+				err,
+				_("HEAD is a symlink ('%s') but target"
+				  " lives outside refs/"),
+				path
+			);
 		return -1;
 	}
 
@@ -371,13 +389,23 @@ static int validate_headref(const char *path)
 	 * Anything else, just open it and try to see if it is a symbolic ref.
 	 */
 	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (fd < 0) {
+		if (err)
+			strbuf_addf(
+				err, _("could not open HEAD at '%s'"), path
+			);
 		return -1;
+	}
 	len = read_in_full(fd, buffer, sizeof(buffer)-1);
 	close(fd);
 
-	if (len < 0)
+	if (len < 0) {
+		if (err)
+			strbuf_addf(
+				err, _("could not read HEAD at '%s'"), path
+			);
 		return -1;
+	}
 	buffer[len] = '\0';
 
 	/*
@@ -396,9 +424,88 @@ static int validate_headref(const char *path)
 	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
 		return 0;
 
+	if (err)
+		strbuf_addf(
+			err,
+			_("HEAD at '%s' does not point to a valid"
+			  " symbolic link or an object ID"),
+			path
+		);
+
 	return -1;
 }
 
+/*
+ * A variant of is_git_directory that gives additional
+ * context via 'err' about why a given suspect is not
+ * a valid git repository.
+ */
+static int is_git_directory_verbose(const char *suspect, struct strbuf *err)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *objdir;
+	int ret = 0;
+	size_t len;
+
+	/* Check worktree-related signatures */
+	strbuf_addstr(&path, suspect);
+	strbuf_complete(&path, '/');
+	strbuf_addstr(&path, "HEAD");
+	if (validate_headref(path.buf, err))
+		goto done;
+
+	strbuf_reset(&path);
+	get_common_dir(&path, suspect);
+	len = path.len;
+
+	/* Check non-worktree-related signatures */
+	objdir = getenv(DB_ENVIRONMENT);
+	if (objdir) {
+		if (access(objdir, X_OK)) {
+			if (err)
+				strbuf_addf(
+					err,
+					_("cannot access object directory '%s'"
+					  " set via $%s\n"),
+				objdir,
+				DB_ENVIRONMENT
+			);
+			goto done;
+		}
+	}
+	else {
+		strbuf_setlen(&path, len);
+		strbuf_addstr(&path, "/objects");
+		if (access(path.buf, X_OK)) {
+			if (err)
+				strbuf_addf(
+					err,
+					_("cannot access object directory '%s'"),
+					path.buf
+				);
+			goto done;
+		}
+	}
+
+	strbuf_setlen(&path, len);
+	strbuf_addstr(&path, "/refs");
+	if (access(path.buf, X_OK)) {
+		if (err)
+			strbuf_addf(
+				err,
+				_("cannot access refs directory '%s'"),
+				path.buf
+			);
+		goto done;
+	}
+
+	ret = 1;
+done:
+	strbuf_release(&path);
+	return ret;
+
+}
+
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
@@ -412,42 +519,7 @@ static int validate_headref(const char *path)
  */
 int is_git_directory(const char *suspect)
 {
-	struct strbuf path = STRBUF_INIT;
-	int ret = 0;
-	size_t len;
-
-	/* Check worktree-related signatures */
-	strbuf_addstr(&path, suspect);
-	strbuf_complete(&path, '/');
-	strbuf_addstr(&path, "HEAD");
-	if (validate_headref(path.buf))
-		goto done;
-
-	strbuf_reset(&path);
-	get_common_dir(&path, suspect);
-	len = path.len;
-
-	/* Check non-worktree-related signatures */
-	if (getenv(DB_ENVIRONMENT)) {
-		if (access(getenv(DB_ENVIRONMENT), X_OK))
-			goto done;
-	}
-	else {
-		strbuf_setlen(&path, len);
-		strbuf_addstr(&path, "/objects");
-		if (access(path.buf, X_OK))
-			goto done;
-	}
-
-	strbuf_setlen(&path, len);
-	strbuf_addstr(&path, "/refs");
-	if (access(path.buf, X_OK))
-		goto done;
-
-	ret = 1;
-done:
-	strbuf_release(&path);
-	return ret;
+	return is_git_directory_verbose(suspect, NULL);
 }
 
 int is_nonbare_repository_dir(struct strbuf *path)
-- 
2.56.0.rc1.12.g2c9c8d64bb

Re: [RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose'

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> Introduce a new helper is_git_directory_verbose() as a
> counterpart to the existing is_git_directory().
>
> is_git_directory_verbose() also populates an optional
> string strbuf with reasoning around why the given suspect
> is not a valid git directory. This strbuf in turn can be
> used to improve the error reporting which is currently blunt:
>
>   fatal: not a git repository
>
> This is not helpful as the user does not get any hint about "why"
> the repository is not considered valid.
>
> Call-site(s) will be made to use this helper in a follow-up commit.
>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
>  setup.c | 152 +++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 112 insertions(+), 40 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 0d157ac254..b3b53a1cfc 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -347,7 +347,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
>  	return ret;
>  }
>  
> -static int validate_headref(const char *path)
> +static int validate_headref(const char *path, struct strbuf *err)
>  {
>  	struct stat st;
>  	char buffer[256];
> @@ -356,14 +356,32 @@ static int validate_headref(const char *path)
>  	int fd;
>  	ssize_t len;
>  
> -	if (lstat(path, &st) < 0)
> +	if (lstat(path, &st) < 0) {
> +		if (err)
> +			strbuf_addf(
> +				err, _("could not stat HEAD at '%s'"), path
> +			);
>  		return -1;
> +	}
>  
>  	/* Make sure it is a "refs/.." symlink */
>  	if (S_ISLNK(st.st_mode)) {
>  		len = readlink(path, buffer, sizeof(buffer)-1);
>  		if (len >= 5 && !memcmp("refs/", buffer, 5))
>  			return 0;
> +		if (len == -1 && err)
> +			strbuf_addf(
> +				err,
> +				_("could not read the symlink HEAD at '%s'"),
> +				path
> +			);
> +		else if (err)
> +			strbuf_addf(
> +				err,
> +				_("HEAD is a symlink ('%s') but target"
> +				  " lives outside refs/"),
> +				path
> +			);
>  		return -1;
>  	}

All of the above (and below---ellided) look fairly funny way to
indent them.  If you are trying ot match the style used in the
existing code around the same area, I wouldn't complain, but I
didn't look beyond what is visible in the patch.

> +	}
> +	else {

Style: "} else {" go on a single line.

Re: [RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose'

[Kaartic Sivaraam]

On 9/25/26 03:41, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> diff --git a/setup.c b/setup.c
>> index 0d157ac254..b3b53a1cfc 100644
>>   
>>   	/* Make sure it is a "refs/.." symlink */
>>   	if (S_ISLNK(st.st_mode)) {
>>   		len = readlink(path, buffer, sizeof(buffer)-1);
>>   		if (len >= 5 && !memcmp("refs/", buffer, 5))
>>   			return 0;
>> +		if (len == -1 && err)
>> +			strbuf_addf(
>> +				err,
>> +				_("could not read the symlink HEAD at '%s'"),
>> +				path
>> +			);
>> +		else if (err)
>> +			strbuf_addf(
>> +				err,
>> +				_("HEAD is a symlink ('%s') but target"
>> +				  " lives outside refs/"),
>> +				path
>> +			);
>>   		return -1;
>>   	}
> 
> All of the above (and below---ellided) look fairly funny way to
> indent them.  If you are trying ot match the style used in the
> existing code around the same area, I wouldn't complain, but I
> didn't look beyond what is visible in the patch.
> 

Indeed. My bad. Does the following look like a good indentation style 
for shorter messages?

	if (lstat(path, &st) < 0) {
		if (err)
			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);
		return -1;
          }

... and this for messages that are a bit longer:

	if (len == -1 && err)
		strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
			    path);
	else if (err)
		strbuf_addf(err, _("HEAD is a symlink ('%s') but target lives"
				   " outside refs/"), path);


-- 
Sivaraam

PS: I'm not yet very sure if my MUA will send this as intended. Will 
resend if it doesn't. Excuse the noise in advance.

[RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory

[Kaartic Sivaraam]

At the moment, there are a few scenarios in which the error message
surrounding an invalid Git repository is a bit blunt:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'

In this case, even though repo.git is a valid Git repository,
we get an output saying it is not since the GIT_OBJECT_DIRECTORY
does not point to a valid object directory. At the moment, the
user is on their own in figuring this out.

Instead, make it more easy for users to figure such issues
particularly in cases where they have explicitly specified
a Git directory. This intends to improve the error reporting UX
by clarifying why the specified repository is not considered valid.

We achieve this by means of using the new helper
is_git_directory_verbose() that has been introduced. With the
same, we get a more helpful error message as follows:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'
  reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c                       | 13 +++++++++++--
 t/t0009-git-dir-validation.sh | 10 ++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/setup.c b/setup.c
index b3b53a1cfc..3e99141474 100644
--- a/setup.c
+++ b/setup.c
@@ -1225,6 +1225,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 					  int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
+	struct strbuf invalid_gitdir_reason = STRBUF_INIT;
 	char *gitfile;
 	int offset;
 
@@ -1237,12 +1238,19 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 		gitdirenv = gitfile;
 	}
 
-	if (!is_git_directory(gitdirenv)) {
+	if (!is_git_directory_verbose(gitdirenv, &invalid_gitdir_reason)) {
+		struct strbuf die_msg = STRBUF_INIT;
 		if (nongit_ok) {
 			*nongit_ok = 1;
 			goto out;
 		}
-		die(_("not a git repository: '%s'"), gitdirenv);
+
+		strbuf_addf(&die_msg, _("not a git repository: '%s'"), gitdirenv);
+		strbuf_addch(&die_msg, '\n');
+		strbuf_addf(&die_msg, _("reason: %s"), invalid_gitdir_reason.buf);
+		die("%s", die_msg.buf);
+
+		strbuf_release(&die_msg);
 	}
 
 	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
@@ -1304,6 +1312,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 	repo_discovery_set_gitdir(discovery, gitdirenv, 0);
 
 out:
+	strbuf_release(&invalid_gitdir_reason);
 	free(gitfile);
 }
 
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index 244dc07c0e..411aac1d9e 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -79,7 +79,8 @@ test_expect_success 'setup: custom git directory with missing HEAD is rejected'
 	mkdir -p parent/empty-dir &&
 	(
 		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "reason: could not stat HEAD at" stderr
 	)
 '
 
@@ -93,7 +94,8 @@ test_expect_success 'setup: custom git directory with HEAD as a symlink outside
 		rm real-repo/HEAD &&
 		ln -s ../garbage real-repo/HEAD &&
 		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "reason: HEAD is a symlink .* but target lives outside refs" stderr
 	)
 '
 
@@ -105,9 +107,9 @@ test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTO
 		git init --bare real-repo &&
 		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
 			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "reason: cannot access object directory .* set via \$GIT_OBJECT_DIRECTORY"   stderr
 	)
 '
 
-
 test_done
-- 
2.56.0.rc1.12.g2c9c8d64bb

Re: [RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> +		strbuf_addf(&die_msg, _("not a git repository: '%s'"), gitdirenv);
> +		strbuf_addch(&die_msg, '\n');
> +		strbuf_addf(&die_msg, _("reason: %s"), invalid_gitdir_reason.buf);
> +		die("%s", die_msg.buf);
> +
> +		strbuf_release(&die_msg);

You just called die(); nobody will execute this strbuf_release() for
you, and because die() is marked with NORETURN, smart enough compilers
would scold you for introducing dead code.

Why are you lego-assembling localized message yourself, instead of
doing something like ...

	die(_("not a git repository: '%s'\nreason: %s"),
	    gitdirenv, invalid_gitdir_reason.buf);

... which is what is usually done?

Re: [RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory

[Kaartic Sivaraam]

On 9/25/26 03:46, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> +		strbuf_addf(&die_msg, _("not a git repository: '%s'"), gitdirenv);
>> +		strbuf_addch(&die_msg, '\n');
>> +		strbuf_addf(&die_msg, _("reason: %s"), invalid_gitdir_reason.buf);
>> +		die("%s", die_msg.buf);
>> +
>> +		strbuf_release(&die_msg);
> 
> You just called die(); nobody will execute this strbuf_release() for
> you, and because die() is marked with NORETURN, smart enough compilers
> would scold you for introducing dead code.
>

Oops. It should indeed warn me. I did a quick check and seems gcc (which 
I was using) lost its ability to report this long ago[1]. Sad. Clang 
clearly reports this.

[1]: https://gcc.gnu.org/legacy-ml/gcc-help/2011-05/msg00360.html

> Why are you lego-assembling localized message yourself, instead of
> doing something like ...
> 
> 	die(_("not a git repository: '%s'\nreason: %s"),
> 	    gitdirenv, invalid_gitdir_reason.buf);
> 
> ... which is what is usually done?
> 

That was my attempt at trying to save translators some work of 
translating the "not a git repository" part. I was thinking this lego 
was not a problem as it was not so complex. If we just want to avoid 
legos totally, I'm cool with doing it the usual way.

-- 
Sivaraam

[RFC PATCH v2 0/4] Improve error reporting to mention "why" a directory is not a repository

[Kaartic Sivaraam]

At the moment, there are a few scenarios where the error message for an
invalid Git repository is a bit blunt. For instance, when we point
GIT_OBJECT_DIRECTORY at a directory that does not exist, we get this:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'

Even though repo.git itself is a valid repository, we get this rather
puzzling error saying that it is not. The actual problem is the invalid
value given to GIT_OBJECT_DIRECTORY, and the user is left on their
to figure that out. This series aims to make such issues easier to
diagnose by saying why the specified repository was not considered
valid.

With this series, the same command outputs:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'
  reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

The following scenarios are covered when --git-dir is given explicitly:

  - HEAD is missing, or its path is not traversable
  - HEAD is a symlink that cannot be read
  - HEAD is a symlink whose target lives outside refs/
  - HEAD cannot be opened, or cannot be read
  - HEAD contains neither a ref under refs/ nor an object ID
  - $GIT_OBJECT_DIRECTORY is set to something we cannot access
  - the object directory in the common directory is inaccessible
  - the refs directory in the common directory is inaccessible

This series does not yet report a reason in the following cases.

The discovery walk, where we iterate up to the ceiling or the mount
point looking for a repository:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: not a git repository (or any parent up to mount point /)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)

The gitfile case, for instance when run from inside a submodule /
worktree:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: gitfile does not point to a valid repository: /path/to/super/sub/.git

GIT_ALTERNATE_OBJECT_DIRECTORIES is also left alone. Its diagnostic
are misleading in their own way, but they are emitted much later, from
the object database rather than from setup, so they need a separate
treatment.

I would be very interested in hearing what others think about this
direction. If it looks reasonable, I'm happy to cover the remaining
cases too, either in this series or separately.

CI:

One symlink related test appears to be failing in Windows. I'll check the same.
All others pass.

See here: https://github.com/sivaraam/git/actions/runs/36548531586

Changes since v2:

- Included 1/4 which fixes a style nit
- Dropped the sub-shell from the test that was unnecessary
- Fixed some style issue and tried to improve the code intentation corresponding
  to the strbuf construction code
- Replaced sentence lego with a single string even though that means
  additional work for translators. On the plus side, avoiding sentence
  lego provides them more context about the message.

Range-diff between v1 and v2

-:  ---------- > 1:  17b72331d0 setup: normalize an if-else to follow our convention
1:  b4d7a4efa2 ! 2:  962418c9f3 t0009: add tests to cover more error reporting scenarios
    @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: .git as an empty dire
     +test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
     +	test_when_finished "rm -rf parent/empty-dir" &&
     +	mkdir -p parent/empty-dir &&
    -+	(
    -+		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    -+		test_grep "not a git repository" stderr
    -+	)
    ++	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    ++	test_grep "not a git repository" stderr
     +'
     +
     +test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
2:  8591dc4162 ! 3:  4a1503a6dc setup: introduce new helper 'is_git_directory_verbose'
    @@ setup.c: static int validate_headref(const char *path)
     -	if (lstat(path, &st) < 0)
     +	if (lstat(path, &st) < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not stat HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);
      		return -1;
     +	}
      
    @@ setup.c: static int validate_headref(const char *path)
      		if (len >= 5 && !memcmp("refs/", buffer, 5))
      			return 0;
     +		if (len == -1 && err)
    -+			strbuf_addf(
    -+				err,
    -+				_("could not read the symlink HEAD at '%s'"),
    -+				path
    -+			);
    ++			strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
    ++				    path);
     +		else if (err)
    -+			strbuf_addf(
    -+				err,
    -+				_("HEAD is a symlink ('%s') but target"
    -+				  " lives outside refs/"),
    -+				path
    -+			);
    ++			strbuf_addf(err, _("HEAD is a symlink ('%s') but target lives"
    ++					   " outside refs/"), path);
      		return -1;
      	}
      
    @@ setup.c: static int validate_headref(const char *path)
     -	if (fd < 0)
     +	if (fd < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not open HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not open HEAD at '%s'"), path);
      		return -1;
     +	}
      	len = read_in_full(fd, buffer, sizeof(buffer)-1);
    @@ setup.c: static int validate_headref(const char *path)
     -	if (len < 0)
     +	if (len < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not read HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not read HEAD at '%s'"), path);
      		return -1;
     +	}
      	buffer[len] = '\0';
    @@ setup.c: static int validate_headref(const char *path)
      		return 0;
      
     +	if (err)
    -+		strbuf_addf(
    -+			err,
    -+			_("HEAD at '%s' does not point to a valid"
    -+			  " symbolic link or an object ID"),
    -+			path
    -+		);
    ++		strbuf_addf(err, _("HEAD at '%s' does not point to a valid symbolic"
    ++				   " link or an object ID"), path);
     +
      	return -1;
      }
    @@ setup.c: static int validate_headref(const char *path)
     +	if (objdir) {
     +		if (access(objdir, X_OK)) {
     +			if (err)
    -+				strbuf_addf(
    -+					err,
    -+					_("cannot access object directory '%s'"
    -+					  " set via $%s\n"),
    -+				objdir,
    -+				DB_ENVIRONMENT
    -+			);
    ++				strbuf_addf(err, _("cannot access object directory '%s'"
    ++						   "set via $%s\n"), objdir, DB_ENVIRONMENT);
     +			goto done;
     +		}
    -+	}
    -+	else {
    ++	} else {
     +		strbuf_setlen(&path, len);
     +		strbuf_addstr(&path, "/objects");
     +		if (access(path.buf, X_OK)) {
     +			if (err)
    -+				strbuf_addf(
    -+					err,
    -+					_("cannot access object directory '%s'"),
    -+					path.buf
    -+				);
    ++				strbuf_addf(err, _("cannot access object directory '%s'"),
    ++					    path.buf);
     +			goto done;
     +		}
     +	}
    @@ setup.c: static int validate_headref(const char *path)
     +	strbuf_addstr(&path, "/refs");
     +	if (access(path.buf, X_OK)) {
     +		if (err)
    -+			strbuf_addf(
    -+				err,
    -+				_("cannot access refs directory '%s'"),
    -+				path.buf
    -+			);
    ++			strbuf_addf(err, _("cannot access refs directory '%s'"), path.buf);
     +		goto done;
     +	}
     +
3:  2c9c8d64bb ! 4:  27a9f668ab setup: communicate why a directory is not a valid git directory
    @@ setup.c: static void repo_discover_explicit_gitdir(struct repo_discovery *discov
      		}
     -		die(_("not a git repository: '%s'"), gitdirenv);
     +
    -+		strbuf_addf(&die_msg, _("not a git repository: '%s'"), gitdirenv);
    -+		strbuf_addch(&die_msg, '\n');
    -+		strbuf_addf(&die_msg, _("reason: %s"), invalid_gitdir_reason.buf);
    ++		strbuf_addf(&die_msg, _("not a git repository: '%s'\nreason: %s"),
    ++			    gitdirenv, invalid_gitdir_reason.buf);
     +		die("%s", die_msg.buf);
    -+
    -+		strbuf_release(&die_msg);
      	}
      
      	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
    @@ setup.c: static void repo_discover_explicit_gitdir(struct repo_discovery *discov
     
      ## t/t0009-git-dir-validation.sh ##
     @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: custom git directory with missing HEAD is rejected'
    + 	test_when_finished "rm -rf parent/empty-dir" &&
      	mkdir -p parent/empty-dir &&
    - 	(
    - 		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    --		test_grep "not a git repository" stderr
    -+		test_grep "not a git repository" stderr &&
    -+		test_grep "reason: could not stat HEAD at" stderr
    - 	)
    + 	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    +-	test_grep "not a git repository" stderr
    ++	test_grep "not a git repository" stderr &&
    ++	test_grep "reason: could not stat HEAD at" stderr
      '
      
    + test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
     @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: custom git directory with HEAD as a symlink outside
      		rm real-repo/HEAD &&
      		ln -s ../garbage real-repo/HEAD &&

Kaartic Sivaraam (4):
  setup: normalize an if-else to follow our convention
  t0009: add tests to cover more error reporting scenarios
  setup: introduce new helper 'is_git_directory_verbose'
  setup: communicate why a directory is not a valid git directory

 setup.c                       | 135 +++++++++++++++++++++++-----------
 t/t0009-git-dir-validation.sh |  36 +++++++++
 2 files changed, 127 insertions(+), 44 deletions(-)

-- 
2.56.0.rc1.12.g2c9c8d64bb

[RFC PATCH v2 1/4] setup: normalize an if-else to follow our convention

[Kaartic Sivaraam]

The else case was not stuck with the corresponding if's
closing brace which is not in-line with our convention.
Fix the same.

This is a style-only change; no behaviour change intended.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 0d157ac254..e9a9ecda19 100644
--- a/setup.c
+++ b/setup.c
@@ -1879,8 +1879,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
 		if (chdir(used_path.buf))
 			return NULL;
 		path = validated_path.buf;
-	}
-	else {
+	} else {
 		const char *gitfile = read_gitfile(path);
 		if (!(flags & ENTER_REPO_ANY_OWNER_OK))
 			die_upon_dubious_ownership(gitfile, NULL, path);
-- 
2.56.0.rc1.12.g2c9c8d64bb

[RFC PATCH v2 2/4] t0009: add tests to cover more error reporting scenarios

[Kaartic Sivaraam]

Introduce few more tests to t0009 to cover error reporting scenarios
when --git-dir is used.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 t/t0009-git-dir-validation.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index 4cba478e50..6c40925aa4 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -74,4 +74,38 @@ test_expect_success 'setup: .git as an empty directory is ignored' '
 	)
 '
 
+test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
+	test_when_finished "rm -rf parent/empty-dir" &&
+	mkdir -p parent/empty-dir &&
+	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
+	test_grep "not a git repository" stderr
+'
+
+test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
+	test_when_finished "rm -rf parent/head-as-link-to-garbage" &&
+	mkdir -p parent/head-as-link-to-garbage &&
+	(
+		cd parent/head-as-link-to-garbage &&
+		git init --bare real-repo &&
+		touch garbage &&
+		rm real-repo/HEAD &&
+		ln -s ../garbage real-repo/HEAD &&
+		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
+test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTORY configuration is rejected' '
+	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
+	mkdir -p parent/invalid-git-object-directory-config &&
+	(
+		cd parent/invalid-git-object-directory-config &&
+		git init --bare real-repo &&
+		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
+			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
+
 test_done
-- 
2.56.0.rc1.12.g2c9c8d64bb

[RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'

[Kaartic Sivaraam]

Introduce a new helper is_git_directory_verbose() as a
counterpart to the existing is_git_directory().

is_git_directory_verbose() also populates an optional
string strbuf with reasoning around why the given suspect
is not a valid git directory. This strbuf in turn can be
used to improve the error reporting which is currently blunt:

  fatal: not a git repository

This is not helpful as the user does not get any hint about "why"
the repository is not considered valid.

Call-site(s) will be made to use this helper in a follow-up commit.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c | 122 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 82 insertions(+), 40 deletions(-)

diff --git a/setup.c b/setup.c
index e9a9ecda19..a0fb68f7f6 100644
--- a/setup.c
+++ b/setup.c
@@ -347,7 +347,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
 	return ret;
 }
 
-static int validate_headref(const char *path)
+static int validate_headref(const char *path, struct strbuf *err)
 {
 	struct stat st;
 	char buffer[256];
@@ -356,14 +356,23 @@ static int validate_headref(const char *path)
 	int fd;
 	ssize_t len;
 
-	if (lstat(path, &st) < 0)
+	if (lstat(path, &st) < 0) {
+		if (err)
+			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);
 		return -1;
+	}
 
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
 		if (len >= 5 && !memcmp("refs/", buffer, 5))
 			return 0;
+		if (len == -1 && err)
+			strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
+				    path);
+		else if (err)
+			strbuf_addf(err, _("HEAD is a symlink ('%s') but target lives"
+					   " outside refs/"), path);
 		return -1;
 	}
 
@@ -371,13 +380,19 @@ static int validate_headref(const char *path)
 	 * Anything else, just open it and try to see if it is a symbolic ref.
 	 */
 	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (fd < 0) {
+		if (err)
+			strbuf_addf(err, _("could not open HEAD at '%s'"), path);
 		return -1;
+	}
 	len = read_in_full(fd, buffer, sizeof(buffer)-1);
 	close(fd);
 
-	if (len < 0)
+	if (len < 0) {
+		if (err)
+			strbuf_addf(err, _("could not read HEAD at '%s'"), path);
 		return -1;
+	}
 	buffer[len] = '\0';
 
 	/*
@@ -396,9 +411,71 @@ static int validate_headref(const char *path)
 	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
 		return 0;
 
+	if (err)
+		strbuf_addf(err, _("HEAD at '%s' does not point to a valid symbolic"
+				   " link or an object ID"), path);
+
 	return -1;
 }
 
+/*
+ * A variant of is_git_directory that gives additional
+ * context via 'err' about why a given suspect is not
+ * a valid git repository.
+ */
+static int is_git_directory_verbose(const char *suspect, struct strbuf *err)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *objdir;
+	int ret = 0;
+	size_t len;
+
+	/* Check worktree-related signatures */
+	strbuf_addstr(&path, suspect);
+	strbuf_complete(&path, '/');
+	strbuf_addstr(&path, "HEAD");
+	if (validate_headref(path.buf, err))
+		goto done;
+
+	strbuf_reset(&path);
+	get_common_dir(&path, suspect);
+	len = path.len;
+
+	/* Check non-worktree-related signatures */
+	objdir = getenv(DB_ENVIRONMENT);
+	if (objdir) {
+		if (access(objdir, X_OK)) {
+			if (err)
+				strbuf_addf(err, _("cannot access object directory '%s'"
+						   " set via $%s\n"), objdir, DB_ENVIRONMENT);
+			goto done;
+		}
+	} else {
+		strbuf_setlen(&path, len);
+		strbuf_addstr(&path, "/objects");
+		if (access(path.buf, X_OK)) {
+			if (err)
+				strbuf_addf(err, _("cannot access object directory '%s'"),
+					    path.buf);
+			goto done;
+		}
+	}
+
+	strbuf_setlen(&path, len);
+	strbuf_addstr(&path, "/refs");
+	if (access(path.buf, X_OK)) {
+		if (err)
+			strbuf_addf(err, _("cannot access refs directory '%s'"), path.buf);
+		goto done;
+	}
+
+	ret = 1;
+done:
+	strbuf_release(&path);
+	return ret;
+
+}
+
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
@@ -412,42 +489,7 @@ static int validate_headref(const char *path)
  */
 int is_git_directory(const char *suspect)
 {
-	struct strbuf path = STRBUF_INIT;
-	int ret = 0;
-	size_t len;
-
-	/* Check worktree-related signatures */
-	strbuf_addstr(&path, suspect);
-	strbuf_complete(&path, '/');
-	strbuf_addstr(&path, "HEAD");
-	if (validate_headref(path.buf))
-		goto done;
-
-	strbuf_reset(&path);
-	get_common_dir(&path, suspect);
-	len = path.len;
-
-	/* Check non-worktree-related signatures */
-	if (getenv(DB_ENVIRONMENT)) {
-		if (access(getenv(DB_ENVIRONMENT), X_OK))
-			goto done;
-	}
-	else {
-		strbuf_setlen(&path, len);
-		strbuf_addstr(&path, "/objects");
-		if (access(path.buf, X_OK))
-			goto done;
-	}
-
-	strbuf_setlen(&path, len);
-	strbuf_addstr(&path, "/refs");
-	if (access(path.buf, X_OK))
-		goto done;
-
-	ret = 1;
-done:
-	strbuf_release(&path);
-	return ret;
+	return is_git_directory_verbose(suspect, NULL);
 }
 
 int is_nonbare_repository_dir(struct strbuf *path)
-- 
2.56.0.rc1.12.g2c9c8d64bb

Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'

[Patrick Steinhardt]

On Tue, Sep 29, 2026 at 03:55:09PM +0530, Kaartic Sivaraam wrote:
> diff --git a/setup.c b/setup.c
> index e9a9ecda19..a0fb68f7f6 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -347,7 +347,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
>  	return ret;
>  }
>  
> -static int validate_headref(const char *path)
> +static int validate_headref(const char *path, struct strbuf *err)
>  {
>  	struct stat st;
>  	char buffer[256];

If only we had structured errors.

> @@ -356,14 +356,23 @@ static int validate_headref(const char *path)
>  	int fd;
>  	ssize_t len;
>  
> -	if (lstat(path, &st) < 0)
> +	if (lstat(path, &st) < 0) {
> +		if (err)
> +			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);

Shouldn't this also include `strerror(errno)`? Otherwise you're still
not that much wiser what the root cause of this is.

>  		return -1;
> +	}
>  
>  	/* Make sure it is a "refs/.." symlink */
>  	if (S_ISLNK(st.st_mode)) {
>  		len = readlink(path, buffer, sizeof(buffer)-1);
>  		if (len >= 5 && !memcmp("refs/", buffer, 5))
>  			return 0;
> +		if (len == -1 && err)
> +			strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
> +				    path);

Same here, we should include `errno`. Other sites should probably be
updated, too.

> @@ -396,9 +411,71 @@ static int validate_headref(const char *path)
>  	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
>  		return 0;
>  
> +	if (err)
> +		strbuf_addf(err, _("HEAD at '%s' does not point to a valid symbolic"
> +				   " link or an object ID"), path);
> +
>  	return -1;
>  }
>  
> +/*
> + * A variant of is_git_directory that gives additional
> + * context via 'err' about why a given suspect is not
> + * a valid git repository.
> + */
> +static int is_git_directory_verbose(const char *suspect, struct strbuf *err)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	char *objdir;
> +	int ret = 0;
> +	size_t len;
> +
> +	/* Check worktree-related signatures */
> +	strbuf_addstr(&path, suspect);
> +	strbuf_complete(&path, '/');
> +	strbuf_addstr(&path, "HEAD");
> +	if (validate_headref(path.buf, err))
> +		goto done;
> +
> +	strbuf_reset(&path);
> +	get_common_dir(&path, suspect);
> +	len = path.len;
> +
> +	/* Check non-worktree-related signatures */
> +	objdir = getenv(DB_ENVIRONMENT);
> +	if (objdir) {
> +		if (access(objdir, X_OK)) {
> +			if (err)
> +				strbuf_addf(err, _("cannot access object directory '%s'"
> +						   " set via $%s\n"), objdir, DB_ENVIRONMENT);
> +			goto done;
> +		}
> +	} else {
> +		strbuf_setlen(&path, len);
> +		strbuf_addstr(&path, "/objects");
> +		if (access(path.buf, X_OK)) {
> +			if (err)
> +				strbuf_addf(err, _("cannot access object directory '%s'"),
> +					    path.buf);
> +			goto done;
> +		}
> +	}
> +
> +	strbuf_setlen(&path, len);
> +	strbuf_addstr(&path, "/refs");
> +	if (access(path.buf, X_OK)) {
> +		if (err)
> +			strbuf_addf(err, _("cannot access refs directory '%s'"), path.buf);
> +		goto done;
> +	}
> +
> +	ret = 1;
> +done:
> +	strbuf_release(&path);
> +	return ret;
> +
> +}
> +
>  /*
>   * Test if it looks like we're at a git directory.
>   * We want to see:

It would've been helpful to move the function up in a separate commit.
Like this it's hard to see what exactly has changed.

Patrick

Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'

[Kaartic Sivaraam]

On 9/30/26 21:33, Patrick Steinhardt wrote:
> On Tue, Sep 29, 2026 at 03:55:09PM +0530, Kaartic Sivaraam wrote:
>> diff --git a/setup.c b/setup.c
>> index e9a9ecda19..a0fb68f7f6 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -347,7 +347,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
>>   	return ret;
>>   }
>>   
>> -static int validate_headref(const char *path)
>> +static int validate_headref(const char *path, struct strbuf *err)
>>   {
>>   	struct stat st;
>>   	char buffer[256];
> 
> If only we had structured errors.
>

Indeed.
>> @@ -356,14 +356,23 @@ static int validate_headref(const char *path)
>>   	int fd;
>>   	ssize_t len;
>>   
>> -	if (lstat(path, &st) < 0)
>> +	if (lstat(path, &st) < 0) {
>> +		if (err)
>> +			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);
> 
> Shouldn't this also include `strerror(errno)`? Otherwise you're still
> not that much wiser what the root cause of this is.
> 

That would of course be an improvement as it helps provide more context. 
Will check on it.
  >>   		return -1;
>> +	}
>>   
>>   	/* Make sure it is a "refs/.." symlink */
>>   	if (S_ISLNK(st.st_mode)) {
>>   		len = readlink(path, buffer, sizeof(buffer)-1);
>>   		if (len >= 5 && !memcmp("refs/", buffer, 5))
>>   			return 0;
>> +		if (len == -1 && err)
>> +			strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
>> +				    path);
> 
> Same here, we should include `errno`. Other sites should probably be
> updated, too.
> 

Noted.

> 
> It would've been helpful to move the function up in a separate commit.
> Like this it's hard to see what exactly has changed.
> 

Indeed. I will improve it in the next iteration.

-- 
Sivaraam

Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> Introduce a new helper is_git_directory_verbose() as a
> counterpart to the existing is_git_directory().
>
> is_git_directory_verbose() also populates an optional
> string strbuf with reasoning around why the given suspect
> is not a valid git directory. This strbuf in turn can be
> used to improve the error reporting which is currently blunt:
>
>   fatal: not a git repository
>
> This is not helpful as the user does not get any hint about "why"
> the repository is not considered valid.

I suspect that it is much less the failure to report the reason that
may confuse users than the failure to report which directory was
inspected and why we picked that directory.  Once we make it clear
which directory was used as the repository to run the Git operation
the user specified, they can visit the directory themselves and see
that there is no HEAD, etc.  The user may have a leftover GIT_DIR in
the environment that is interfering with their operation without
their remembering they still had it, for example.  For this reason,
I am not so enthused to see this much code churn to tell the user
the subtle differences among a dangling symlink HEAD, a HEAD
pointing outside refs, and a missing HEAD.

What would be preferable is a much less invasive patch that reports
which path was assumed to be the repository and where it came from
(among GIT_DIR, auto-discovery stopped at GIT_CEILING_DIRECTORIES,
etc.).  It would help the user figure out why the directory they
thought was the Git directory is not what the git binary inspected.

This patch does report which path we thought HEAD should be at in its
messages, but does not explain where that assumption came from,
unlike the verification of the objects/ directory, which mentions the
environment variable if it is involved.  This feels uneven.

Thanks.

Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'

[Kaartic Sivaraam]

On 10/1/26 00:02, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> Introduce a new helper is_git_directory_verbose() as a
>> counterpart to the existing is_git_directory().
>>
>> is_git_directory_verbose() also populates an optional
>> string strbuf with reasoning around why the given suspect
>> is not a valid git directory. This strbuf in turn can be
>> used to improve the error reporting which is currently blunt:
>>
>>    fatal: not a git repository
>>
>> This is not helpful as the user does not get any hint about "why"
>> the repository is not considered valid.
> 
> I suspect that it is much less the failure to report the reason that
> may confuse users than the failure to report which directory was
> inspected and why we picked that directory.  Once we make it clear
> which directory was used as the repository to run the Git operation
> the user specified, they can visit the directory themselves and see
> that there is no HEAD, etc.  The user may have a leftover GIT_DIR in
> the environment that is interfering with their operation without
> their remembering they still had it, for example.  For this reason,
> I am not so enthused to see this much code churn to tell the user
> the subtle differences among a dangling symlink HEAD, a HEAD
> pointing outside refs, and a missing HEAD.
> 
> What would be preferable is a much less invasive patch that reports
> which path was assumed to be the repository and where it came from
> (among GIT_DIR, auto-discovery stopped at GIT_CEILING_DIRECTORIES,
> etc.).  It would help the user figure out why the directory they
> thought was the Git directory is not what the git binary inspected.
> 
> This patch does report which path we thought HEAD should be at in its
> messages, but does not explain where that assumption came from,
> unlike the verification of the objects/ directory, which mentions the
> environment variable if it is involved.  This feels uneven.
>

Thank you for your insights and excuse the delay. I have tried to take 
this into consideration and just sent a v3. Kindly check the same and 
let me know your thoughts.

   v3: <20261028115412.169748-1-kaartic.sivaraam@gmail.com/>

-- 
Sivaraam

[RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory

[Kaartic Sivaraam]

At the moment, there are a few scenarios in which the error message
surrounding an invalid Git repository is a bit blunt:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'

In this case, even though repo.git is a valid Git repository,
we get an output saying it is not since the GIT_OBJECT_DIRECTORY
does not point to a valid object directory. At the moment, the
user is on their own in figuring this out.

Instead, make it more easy for users to figure such issues
particularly in cases where they have explicitly specified
a Git directory. This intends to improve the error reporting UX
by clarifying why the specified repository is not considered valid.

We achieve this by means of using the new helper
is_git_directory_verbose() that has been introduced. With the
same, we get a more helpful error message as follows:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'
  reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c                       | 10 ++++++++--
 t/t0009-git-dir-validation.sh | 10 ++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/setup.c b/setup.c
index a0fb68f7f6..a0d3c0c5bb 100644
--- a/setup.c
+++ b/setup.c
@@ -1195,6 +1195,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 					  int *nongit_ok)
 {
 	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
+	struct strbuf invalid_gitdir_reason = STRBUF_INIT;
 	char *gitfile;
 	int offset;
 
@@ -1207,12 +1208,16 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 		gitdirenv = gitfile;
 	}
 
-	if (!is_git_directory(gitdirenv)) {
+	if (!is_git_directory_verbose(gitdirenv, &invalid_gitdir_reason)) {
+		struct strbuf die_msg = STRBUF_INIT;
 		if (nongit_ok) {
 			*nongit_ok = 1;
 			goto out;
 		}
-		die(_("not a git repository: '%s'"), gitdirenv);
+
+		strbuf_addf(&die_msg, _("not a git repository: '%s'\nreason: %s"),
+			    gitdirenv, invalid_gitdir_reason.buf);
+		die("%s", die_msg.buf);
 	}
 
 	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
@@ -1274,6 +1279,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 	repo_discovery_set_gitdir(discovery, gitdirenv, 0);
 
 out:
+	strbuf_release(&invalid_gitdir_reason);
 	free(gitfile);
 }
 
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index 6c40925aa4..1f8ac3fad5 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -78,7 +78,8 @@ test_expect_success 'setup: custom git directory with missing HEAD is rejected'
 	test_when_finished "rm -rf parent/empty-dir" &&
 	mkdir -p parent/empty-dir &&
 	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
-	test_grep "not a git repository" stderr
+	test_grep "not a git repository" stderr &&
+	test_grep "reason: could not stat HEAD at" stderr
 '
 
 test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
@@ -91,7 +92,8 @@ test_expect_success 'setup: custom git directory with HEAD as a symlink outside
 		rm real-repo/HEAD &&
 		ln -s ../garbage real-repo/HEAD &&
 		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "reason: HEAD is a symlink .* but target lives outside refs" stderr
 	)
 '
 
@@ -103,9 +105,9 @@ test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTO
 		git init --bare real-repo &&
 		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
 			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "reason: cannot access object directory .* set via \$GIT_OBJECT_DIRECTORY"   stderr
 	)
 '
 
-
 test_done
-- 
2.56.0.rc1.12.g2c9c8d64bb

Re: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory

[Patrick Steinhardt]

On Tue, Sep 29, 2026 at 03:55:10PM +0530, Kaartic Sivaraam wrote:
> At the moment, there are a few scenarios in which the error message
> surrounding an invalid Git repository is a bit blunt:
> 
>   $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
>   fatal: not a git repository: 'repo.git'
> 
> In this case, even though repo.git is a valid Git repository,
> we get an output saying it is not since the GIT_OBJECT_DIRECTORY
> does not point to a valid object directory. At the moment, the
> user is on their own in figuring this out.
> 
> Instead, make it more easy for users to figure such issues

s/more easy/easier/

> particularly in cases where they have explicitly specified
> a Git directory. This intends to improve the error reporting UX
> by clarifying why the specified repository is not considered valid.

Which I think is a good motivation.

> We achieve this by means of using the new helper
> is_git_directory_verbose() that has been introduced. With the
> same, we get a more helpful error message as follows:
> 
>   $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
>   fatal: not a git repository: 'repo.git'
>   reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

Having a separate "reason:" line feels a bit off to me, but that may be
subjective. I'd have preferred to have it on the same line, or maybe
first have "error:" followed by "fatal:".

> diff --git a/setup.c b/setup.c
> index a0fb68f7f6..a0d3c0c5bb 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1195,6 +1195,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
>  					  int *nongit_ok)
>  {
>  	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
> +	struct strbuf invalid_gitdir_reason = STRBUF_INIT;

Nit, please feel free to ignore: I'd just have called this `errbuf`.

Patrick

Re: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory

[Kaartic Sivaraam]

On 9/30/26 21:33, Patrick Steinhardt wrote:
> On Tue, Sep 29, 2026 at 03:55:10PM +0530, Kaartic Sivaraam wrote:
>> At the moment, there are a few scenarios in which the error message
>> surrounding an invalid Git repository is a bit blunt:
>>
>>    $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
>>    fatal: not a git repository: 'repo.git'
>>
>> In this case, even though repo.git is a valid Git repository,
>> we get an output saying it is not since the GIT_OBJECT_DIRECTORY
>> does not point to a valid object directory. At the moment, the
>> user is on their own in figuring this out.
>>
>> Instead, make it more easy for users to figure such issues
> 
> s/more easy/easier/
>

Ah. Will correct.

>> particularly in cases where they have explicitly specified
>> a Git directory. This intends to improve the error reporting UX
>> by clarifying why the specified repository is not considered valid.
> 
> Which I think is a good motivation.
> 
>> We achieve this by means of using the new helper
>> is_git_directory_verbose() that has been introduced. With the
>> same, we get a more helpful error message as follows:
>>
>>    $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
>>    fatal: not a git repository: 'repo.git'
>>    reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY
> 
> Having a separate "reason:" line feels a bit off to me, but that may be
> subjective. I'd have preferred to have it on the same line, or maybe
> first have "error:" followed by "fatal:".
> 

Hmm. Let me think which of these I could adopt. Thank you for the 
suggestion.

>> diff --git a/setup.c b/setup.c
>> index a0fb68f7f6..a0d3c0c5bb 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -1195,6 +1195,7 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
>>   					  int *nongit_ok)
>>   {
>>   	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
>> +	struct strbuf invalid_gitdir_reason = STRBUF_INIT;
> 
> Nit, please feel free to ignore: I'd just have called this `errbuf`.
> 

Noted.

-- 
Sivaraam

[PATCH v3 0/5] Improve error reporting to mention how a Git repository is discovered

[Kaartic Sivaraam]

At the moment, when Git gives up with "fatal: not a git repository",
it does not say which directory it actually inspected or where that
directory came from. This is especially confusing when a leftover
environment variable is steering Git somewhere the user did not
expect.

In v2 I tried to address this by explaining why the inspected
directory was not a valid repository (missing HEAD, HEAD outside
refs/, and so on). Junio pointed out that this is less useful than
saying which directory was used as the repository and where that
came from, since once the user knows that they can go look at the
directory themselves. He also pointed out that v2 was uneven, as it
named $GIT_OBJECT_DIRECTORY but did not say where the git directory
itself came from.

So this iteration takes that direction instead. The validity checks
in is_git_directory() and validate_headref() are left untouched, and
we only add information at the points where we are about to die.

With this series:

 - An explicitly given git directory says where it came from:

     $ cd /srv/git
     $ export GIT_DIR=project.git
     ...
     $ cd ~/src/myproject            # later, same terminal
     $ git status
     fatal: not a git repository: 'project.git'
     this path was taken from $GIT_DIR (which can also be set with --git-dir or --bare)

   --git-dir and --bare both just set GIT_DIR, so by the time setup
   runs we cannot tell them apart. I went with a single wording that
   is true for all three rather than plumbing that information
   through. Happy to revisit if it is felt to be worth it.

 - The discovery walk says where it started, where it stopped and
   what GIT_CEILING_DIRECTORIES is set to:

     $ echo 'export GIT_CEILING_DIRECTORIES=$HOME' >> ~/.bashrc
     ...weeks later...
     $ cd ~/.config/nvim
     $ git status
     fatal: not a git repository (or any of the parent directories): .git
     searched from '/home/user/.config/nvim' up to '/home/user/.config'
     $GIT_CEILING_DIRECTORIES is set to '/home/user'

 - An unusable $GIT_OBJECT_DIRECTORY, which was the original
   motivation for this series, is reported wherever we die for want
   of a repository (explicit git directory, ceiling and mount point):

     $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
     fatal: not a git repository: 'repo.git'
     this path was taken from $GIT_DIR (which can also be set with --git-dir or --bare)
     cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY: No such file or directory

Commands that are fine running outside a repository stay silent and
do not build these messages at all.

This series does not handle the following, which I think are better
done separately:

 - The gitfile case (for instance from inside a submodule or a
   worktree). A gitfile pointing at an invalid repository dies
   earlier in read_gitfile() with its own message.

 - GIT_ALTERNATE_OBJECT_DIRECTORIES. It never leads to "not a git
   repository". Its diagnostics are misleading in their own way (one
   of them points at .git/objects/info/alternates even when the entry
   came from the environment), but they come from the object database
   rather than from setup.

 - The "$GIT_CEILING_DIRECTORIES is set to" line is printed whenever
   the variable is set, even if none of its entries is above the
   current directory and the walk went all the way to '/'. The
   "searched from ... up to '/'" line makes that visible, and getting
   it exact would mean passing more state out of the discovery walk.

CI:

They seem to pass.

- GitLab: https://gitlab.com/gitlab-org/git/-/merge_requests/672

- GitHub: https://github.com/sivaraam/git/actions/runs/40676055975

Changes since v2:

- Dropped is_git_directory_verbose() and the HEAD-related reasons, as
  per Junio's review. is_git_directory() and validate_headref() are
  now unchanged.
- New: say where an explicitly given git directory came from.
- New: say where the discovery walk started and stopped, and what
  GIT_CEILING_DIRECTORIES is set to.
- Reworked the GIT_OBJECT_DIRECTORY reporting to ask once at the
  point of failure, and extended it to the discovery walk (ceiling
  and mount point).
- Dropped the test for HEAD being a symlink outside refs/, as the
  series no longer cares about it. This test was failing on Windows
  in v2.
- Reworked the t0009 tests to check the new messages, including that
  a command which does not need a repository stays silent.
- Kept each message as a single translatable string and added
  TRANSLATORS comments describing the placeholders.

Range-diff versus v2:
(This may not be very helpful because I threw out 3/4 and 4/4 from v2 and have 3 new
 patches in place of them.)

1:  0b062456eb = 1:  b37136baa4 setup: normalize an if-else to follow our convention
2:  7e8dab0078 ! 2:  08f3584c3a t0009: add tests to cover more error reporting scenarios
    @@ Metadata
     Author: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>

      ## Commit message ##
    -    t0009: add tests to cover more error reporting scenarios
    +    t0009: add tests to cover more repository setup scenarios

    -    Introduce few more tests to t0009 to cover error reporting scenarios
    -    when --git-dir is used.
    +    Introduce a few more tests to t0009 to cover repository setup
    +    scenarios. We introduce these as we intend to do some change
    +    in upcoming commits which would benefit from this additional
    +    test coverage.

         Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>

    @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: .git as an empty dire
      	)
      '

    -+test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
    ++test_expect_success 'setup: --git-dir pointing to an invalid directory is rejected' '
     +	test_when_finished "rm -rf parent/empty-dir" &&
     +	mkdir -p parent/empty-dir &&
     +	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
     +	test_grep "not a git repository" stderr
     +'
     +
    -+test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
    -+	test_when_finished "rm -rf parent/head-as-link-to-garbage" &&
    -+	mkdir -p parent/head-as-link-to-garbage &&
    ++test_expect_success 'setup: discovery fails when GIT_DIR points to an invalid directory' '
    ++	test_must_fail env GIT_DIR="/path/to/does-not-exist" \
    ++		git rev-parse --is-bare-repository 2>stderr &&
    ++	test_grep "not a git repository" stderr
    ++'
    ++
    ++test_expect_success 'setup: command that does not need a repository stays silent for invalid GIT_DIR' '
    ++	echo x | GIT_DIR="/path/to/does-not-exist" git hash-object --stdin 2>stderr &&
    ++	test_must_be_empty stderr
    ++'
    ++
    ++test_expect_success 'setup: discovery fails when a valid git directory not found within GIT_CEILING_DIRECTORIES' '
    ++	test_when_finished "rm -rf parent/no-valid-git-directory-within-ceiling" &&
    ++	mkdir -p parent/no-valid-git-directory-within-ceiling/ceil/sub &&
     +	(
    -+		cd parent/head-as-link-to-garbage &&
    -+		git init --bare real-repo &&
    -+		touch garbage &&
    -+		rm real-repo/HEAD &&
    -+		ln -s ../garbage real-repo/HEAD &&
    -+		test_must_fail git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
    -+		test_grep "not a git repository" stderr
    ++		cd parent/no-valid-git-directory-within-ceiling/ceil/sub &&
    ++		test_must_fail env GIT_CEILING_DIRECTORIES="$(pwd)/../../" \
    ++			git rev-parse --is-bare-repository 2>stderr &&
    ++		test_grep "not a git repository (or any of the parent directories)" stderr
     +	)
     +'
     +
    -+test_expect_success 'setup: custom git directory with invalid GIT_OBJECT_DIRECTORY configuration is rejected' '
    ++test_expect_success 'setup: discovery fails with unusable $GIT_OBJECT_DIRECTORY and explicit gitdir' '
     +	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
     +	mkdir -p parent/invalid-git-object-directory-config &&
     +	(
     +		cd parent/invalid-git-object-directory-config &&
     +		git init --bare real-repo &&
    -+		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
    ++		test_must_fail env GIT_OBJECT_DIRECTORY="/path/to/does-not-exist" \
     +			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
     +		test_grep "not a git repository" stderr
     +	)
     +'
    -+
     +
      test_done
3:  312b861c6c < -:  ---------- setup: introduce new helper 'is_git_directory_verbose'
4:  29191f463e < -:  ---------- setup: communicate why a directory is not a valid git directory
-:  ---------- > 3:  34a110da6d setup: provide hint about the source of the explicit git directory
-:  ---------- > 4:  9986739fec setup: provide hint about auto-discovery boundaries
-:  ---------- > 5:  7d13445134 setup: report when GIT_OBJECT_DIRECTORY is what we could not use

Kaartic Sivaraam (5):
  setup: normalize an if-else to follow our convention
  t0009: add tests to cover more repository discovery scenarios
  setup: provide hint about the source of the explicit git directory
  setup: provide hint about auto-discovery boundaries
  setup: report when GIT_OBJECT_DIRECTORY is what we could not use

 setup.c                       | 104 ++++++++++++++++++++++++++++++----
 t/t0009-git-dir-validation.sh |  77 +++++++++++++++++++++++++
 2 files changed, 171 insertions(+), 10 deletions(-)

-- 
2.55.0.1003.g10538fe699

[PATCH v3 1/5] setup: normalize an if-else to follow our convention

[Kaartic Sivaraam]

The else case was not stuck with the corresponding if's
closing brace which is not in-line with our convention.
Fix the same.

This is a style-only change; no behaviour change intended.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 61bcdafe82..4789b2d79d 100644
--- a/setup.c
+++ b/setup.c
@@ -1905,8 +1905,7 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
 		if (chdir(used_path.buf))
 			return NULL;
 		path = validated_path.buf;
-	}
-	else {
+	} else {
 		const char *gitfile = read_gitfile(path);
 		if (!(flags & ENTER_REPO_ANY_OWNER_OK))
 			die_upon_dubious_ownership(gitfile, NULL, path);
-- 
2.55.0.1003.g10538fe699

[PATCH v3 2/5] t0009: add tests to cover more repository discovery scenarios

[Kaartic Sivaraam]

Introduce a few more tests to t0009 to cover repository discovery
scenarios. We introduce these as we intend to do some change
in upcoming commits which would benefit from this additional
test coverage.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 t/t0009-git-dir-validation.sh | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index 4cba478e50..bc607e6c2a 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -74,4 +74,45 @@ test_expect_success 'setup: .git as an empty directory is ignored' '
 	)
 '
 
+test_expect_success 'setup: --git-dir pointing to an invalid directory is rejected' '
+	test_when_finished "rm -rf parent/empty-dir" &&
+	mkdir -p parent/empty-dir &&
+	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
+	test_grep "not a git repository" stderr
+'
+
+test_expect_success 'setup: discovery fails when GIT_DIR points to an invalid directory' '
+	test_must_fail env GIT_DIR="/path/to/does-not-exist" \
+		git rev-parse --is-bare-repository 2>stderr &&
+	test_grep "not a git repository" stderr
+'
+
+test_expect_success 'setup: command that does not need a repository stays silent for invalid GIT_DIR' '
+	echo x | GIT_DIR="/path/to/does-not-exist" git hash-object --stdin 2>stderr &&
+	test_must_be_empty stderr
+'
+
+test_expect_success 'setup: discovery fails when a valid git directory not found within GIT_CEILING_DIRECTORIES' '
+	test_when_finished "rm -rf parent/no-valid-git-directory-within-ceiling" &&
+	mkdir -p parent/no-valid-git-directory-within-ceiling/ceil/sub &&
+	(
+		cd parent/no-valid-git-directory-within-ceiling/ceil/sub &&
+		test_must_fail env GIT_CEILING_DIRECTORIES="$(pwd)/../../" \
+			git rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository (or any of the parent directories)" stderr
+	)
+'
+
+test_expect_success 'setup: discovery fails with unusable $GIT_OBJECT_DIRECTORY and explicit gitdir' '
+	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
+	mkdir -p parent/invalid-git-object-directory-config &&
+	(
+		cd parent/invalid-git-object-directory-config &&
+		git init --bare real-repo &&
+		test_must_fail env GIT_OBJECT_DIRECTORY="/path/to/does-not-exist" \
+			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
+		test_grep "not a git repository" stderr
+	)
+'
+
 test_done
-- 
2.55.0.1003.g10538fe699

[PATCH v3 3/5] setup: provide hint about the source of the explicit git directory

[Kaartic Sivaraam]

Git directories can be specified explicitly by setting the GIT_DIR
environment variable or by passing --git-dir / --bare before a
sub-command. When discovery fails in these cases, the error message
that we print is a bit unhelpful. This is especially more pronounced
when someone sets GIT_DIR and forgets about it.

  $ cd /srv/git
  $ export GIT_DIR=project.git    # saves typing --git-dir on every command
  $ git log --oneline -3
  $ git for-each-ref --sort=-committerdate refs/heads | head
  $ cd ~/src/myproject            # later, same terminal
  $ git status
  fatal: not a git repository: 'project.git'

The blunt message does not help the user understand where 'project.git'
came from. They are on their own in figuring this out.

Improve this error reporting UX by giving an additional hint to the
user that the path comes via GIT_DIR. With this, the user would see
the more helpful hint:

  ...
  $ cd ~/src/myproject            # later, same terminal
  $ git status
  fatal: not a git repository: 'project.git'
  this path was taken from $GIT_DIR (which can also be set with --git-dir or --bare)

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c                       | 8 +++++++-
 t/t0009-git-dir-validation.sh | 6 ++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index 4789b2d79d..8f6b609240 100644
--- a/setup.c
+++ b/setup.c
@@ -1152,7 +1152,13 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 			*nongit_ok = 1;
 			goto out;
 		}
-		die(_("not a git repository: '%s'"), gitdirenv);
+		/*
+		 * TRANSLATORS: The first %s is a path, the second %s is the
+		 * name of the environment variable it was taken from.
+		 */
+		die(_("not a git repository: '%s'\n"
+		      "this path was taken from $%s (which can also be set with --git-dir or --bare)"),
+		    gitdirenv, GIT_DIR_ENVIRONMENT);
 	}
 
 	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index bc607e6c2a..fed1b045bb 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -78,13 +78,15 @@ test_expect_success 'setup: --git-dir pointing to an invalid directory is reject
 	test_when_finished "rm -rf parent/empty-dir" &&
 	mkdir -p parent/empty-dir &&
 	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
-	test_grep "not a git repository" stderr
+	test_grep "not a git repository" stderr &&
+	test_grep "taken from \$GIT_DIR" stderr
 '
 
 test_expect_success 'setup: discovery fails when GIT_DIR points to an invalid directory' '
 	test_must_fail env GIT_DIR="/path/to/does-not-exist" \
 		git rev-parse --is-bare-repository 2>stderr &&
-	test_grep "not a git repository" stderr
+	test_grep "not a git repository" stderr &&
+	test_grep "taken from \$GIT_DIR" stderr
 '
 
 test_expect_success 'setup: command that does not need a repository stays silent for invalid GIT_DIR' '
-- 
2.55.0.1003.g10538fe699

[PATCH v3 4/5] setup: provide hint about auto-discovery boundaries

[Kaartic Sivaraam]

In the same spirit as the previous commit, we want to improve the UX
of the Git repository discovery case when the GIT_CEILING_DIRECTORIES
environment varaible is set. For instance, consider the following
scenario:

  $ cd ~ && git init                    # dotfiles repo at ~/.git
  $ echo 'export GIT_CEILING_DIRECTORIES=$HOME' >> ~/.bashrc
                                        # stop ~/.git swallowing every
                                        # non-repo directory under ~
  ...weeks later...
  $ cd ~/.config/nvim
  $ vim init.lua
  $ git status
  fatal: not a git repository (or any of the parent directories): .git

The error message is a bit unhelpful as the gut reaction of the user
would be to wonder why their ~/.git directory was not discovered even
though the message says it checked the parent directories. We can't
really blame the user for not remembering that they had
GIT_CEILING_DIRECTORIES configured in their ~/.bashrc.

Improve this error reporting UX by giving an additional hint about the
ceiling that auto-discovery was bounded by and also mention what
GIT_CEILING_DIRECTORIES was configured to. With this, the user would
see the following:

  ...
  $ git status
  fatal: not a git repository (or any of the parent directories): .git
  searched from '/home/user/.config/nvim' up to '/home/user/.config'
  $GIT_CEILING_DIRECTORIES is set to '/home/user'

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c                       | 29 ++++++++++++++++++++++++++---
 t/t0009-git-dir-validation.sh |  4 +++-
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 8f6b609240..87fcead87e 100644
--- a/setup.c
+++ b/setup.c
@@ -1997,9 +1997,32 @@ static void repo_discover(struct repo_discovery *discovery, int *nongit_ok)
 		repo_discover_bare_gitdir(discovery, &cwd, dir.len, nongit_ok);
 		break;
 	case GIT_DIR_HIT_CEILING:
-		if (!nongit_ok)
-			die(_("not a git repository (or any of the parent directories): %s"),
-			    DEFAULT_GIT_DIR_ENVIRONMENT);
+		if (!nongit_ok) {
+			struct strbuf message = STRBUF_INIT;
+			const char *ceiling = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
+
+			strbuf_addf(&message,
+				    /*
+				     * TRANSLATORS: The first %s is ".git", the
+				     * second and third %s are the directories
+				     * where the search for a repository started
+				     * and where it stopped.
+				     */
+				    _("not a git repository (or any of the parent directories): %s\n"
+				      "searched from '%s' up to '%s'"),
+				    DEFAULT_GIT_DIR_ENVIRONMENT, cwd.buf, dir.buf);
+			if (ceiling && *ceiling) {
+				strbuf_addch(&message, '\n');
+				/*
+				 * TRANSLATORS: The first %s is the name of
+				 * the environment variable, the second %s is
+				 * its value.
+				 */
+				strbuf_addf(&message, _("$%s is set to '%s'"),
+					    CEILING_DIRECTORIES_ENVIRONMENT, ceiling);
+			}
+			die("%s", message.buf);
+		}
 		*nongit_ok = 1;
 		break;
 	case GIT_DIR_HIT_MOUNT_POINT:
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index fed1b045bb..ab3d2593aa 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -101,7 +101,9 @@ test_expect_success 'setup: discovery fails when a valid git directory not found
 		cd parent/no-valid-git-directory-within-ceiling/ceil/sub &&
 		test_must_fail env GIT_CEILING_DIRECTORIES="$(pwd)/../../" \
 			git rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository (or any of the parent directories)" stderr
+		test_grep "not a git repository (or any of the parent directories)" stderr &&
+		test_grep "searched from .*ceil/sub.*up to.*ceil." stderr &&
+		test_grep "GIT_CEILING_DIRECTORIES is set to" stderr
 	)
 '
 
-- 
2.55.0.1003.g10538fe699

Re: [PATCH v3 4/5] setup: provide hint about auto-discovery boundaries

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> In the same spirit as the previous commit, we want to improve the UX
> of the Git repository discovery case when the GIT_CEILING_DIRECTORIES
> environment varaible is set. For instance, consider the following
> scenario:
>
>   $ cd ~ && git init                    # dotfiles repo at ~/.git
>   $ echo 'export GIT_CEILING_DIRECTORIES=$HOME' >> ~/.bashrc
>                                         # stop ~/.git swallowing every
>                                         # non-repo directory under ~
>   ...weeks later...
>   $ cd ~/.config/nvim
>   $ vim init.lua
>   $ git status
>   fatal: not a git repository (or any of the parent directories): .git
>
> The error message is a bit unhelpful as the gut reaction of the user
> would be to wonder why their ~/.git directory was not discovered even
> though the message says it checked the parent directories. We can't
> really blame the user for not remembering that they had
> GIT_CEILING_DIRECTORIES configured in their ~/.bashrc.
>
> Improve this error reporting UX by giving an additional hint about the
> ceiling that auto-discovery was bounded by and also mention what
> GIT_CEILING_DIRECTORIES was configured to. With this, the user would
> see the following:
>
>   ...
>   $ git status
>   fatal: not a git repository (or any of the parent directories): .git
>   searched from '/home/user/.config/nvim' up to '/home/user/.config'
>   $GIT_CEILING_DIRECTORIES is set to '/home/user'

The second line of the output gives a much better information than
before.  Stepping back a bit, the "(or any of the ...)" was a weak
and ineffective attempt to say that ".git" we claim is not a valid
repository is not necessarily in the current directory.  But would
it be still needed with the more explicit "we looked for it from
here to there"?  Can we combine both into a single statement somehow
to make it more concise and understandable?

Here is my attempt.  Would this

    fatal: no valid repository ".git" found in "/home/user/.config/nvim"
    or any of its parent directories up to "/home/user/.config".

or something like that be easier to read?

Of course, such a change would break scripts that attempt to parse
this error message, so even if it is easier for humans, there could
be downside.

> -		test_grep "not a git repository (or any of the parent directories)" stderr
> +		test_grep "not a git repository (or any of the parent directories)" stderr &&
> +		test_grep "searched from .*ceil/sub.*up to.*ceil." stderr &&
> +		test_grep "GIT_CEILING_DIRECTORIES is set to" stderr
>  	)
>  '

[PATCH v3 5/5] setup: report when GIT_OBJECT_DIRECTORY is what we could not use

[Kaartic Sivaraam]

In the same spirit as the last two commits, we want to improve the UX
of the Git repository discovery case when the GIT_OBJECT_DIRECTORY
environment variable is set to an invalid directory. For instance,
consider the following scenario:

  $ echo 'export GIT_OBJECT_DIRECTORY=/tmp/git-objects' >> ~/.bashrc
                                     # shared object store across clones
  ...after a reboot clears /tmp...
  $ cd ~/src/myproject
  $ git status
  fatal: not a git repository (or any parent up to mount point /)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

We get an error here because when GIT_OBJECT_DIRECTORY points at a
directory that cannot be used, Git refuses to work with any repository
at all, but says so in a way that blames the repository.

Improve this error reporting UX by explaining the real cause wherever
we are about to die. Because the check does not depend on the candidate
that is given to is_git_directory(), it is enough to ask once, at the
point of failure, whether the configured object directory is to blame.
With this, the user would see the following:

  ...
  $ git status
  fatal: not a git repository (or any parent up to mount point /)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
  cannot access object directory '/tmp/git-objects' set via
    $GIT_OBJECT_DIRECTORY: No such file or directory

Note that pointing GIT_OBJECT_DIRECTORY at a directory that does not
exist yet is legitimate for "git init", which goes on to create it.
Diagnosing only where we would have died anyway leaves that case alone.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 setup.c                       | 80 +++++++++++++++++++++++++++++------
 t/t0009-git-dir-validation.sh | 36 +++++++++++++++-
 2 files changed, 102 insertions(+), 14 deletions(-)

diff --git a/setup.c b/setup.c
index 87fcead87e..7b1e67e0dc 100644
--- a/setup.c
+++ b/setup.c
@@ -450,6 +450,52 @@ int is_git_directory(const char *suspect)
 	return ret;
 }
 
+/*
+ * If $GIT_OBJECT_DIRECTORY names a directory we cannot use, describe why in
+ * `out` and return 1, otherwise return 0.
+ *
+ * Note that the check is not specific to any one candidate directory: when the
+ * variable is set but unusable, is_git_directory() rejects every directory it
+ * is given, so repository discovery cannot succeed anywhere.
+ */
+static int describe_unusable_object_dir(struct strbuf *out)
+{
+	const char *objdir = getenv(DB_ENVIRONMENT);
+	int saved_errno;
+
+	if (!objdir || !access(objdir, X_OK))
+		return 0;
+	saved_errno = errno;
+
+	/*
+	 * TRANSLATORS: The first %s is a path, the second %s is the
+	 * name of the environment variable that set it, and the third
+	 * %s is the system error message.
+	 */
+	strbuf_addf(out, _("cannot access object directory '%s' set via $%s: %s"),
+		    objdir, DB_ENVIRONMENT, strerror(saved_errno));
+	return 1;
+}
+
+/*
+ * Die with `message`, which reports that no repository could be used. When an
+ * unusable $GIT_OBJECT_DIRECTORY is what stood in the way, say so as well: on
+ * its own the message blames the repository we were pointed at, which may be
+ * perfectly fine.
+ *
+ * `message` is not released, as we are about to exit anyway.
+ */
+static void NORETURN die_no_repository(struct strbuf *message)
+{
+	struct strbuf objdir = STRBUF_INIT;
+
+	if (describe_unusable_object_dir(&objdir))
+		strbuf_addf(message, "\n%s", objdir.buf);
+	strbuf_release(&objdir);
+
+	die("%s", message->buf);
+}
+
 int is_nonbare_repository_dir(struct strbuf *path)
 {
 	int ret = 0;
@@ -1148,17 +1194,22 @@ static void repo_discover_explicit_gitdir(struct repo_discovery *discovery,
 	}
 
 	if (!is_git_directory(gitdirenv)) {
+		struct strbuf message = STRBUF_INIT;
+
 		if (nongit_ok) {
 			*nongit_ok = 1;
 			goto out;
 		}
-		/*
-		 * TRANSLATORS: The first %s is a path, the second %s is the
-		 * name of the environment variable it was taken from.
-		 */
-		die(_("not a git repository: '%s'\n"
-		      "this path was taken from $%s (which can also be set with --git-dir or --bare)"),
-		    gitdirenv, GIT_DIR_ENVIRONMENT);
+		strbuf_addf(&message,
+			    /*
+			     * TRANSLATORS: The first %s is a path, the second
+			     * %s is the name of the environment variable it
+			     * was taken from.
+			     */
+			    _("not a git repository: '%s'\n"
+			      "this path was taken from $%s (which can also be set with --git-dir or --bare)"),
+			    gitdirenv, GIT_DIR_ENVIRONMENT);
+		die_no_repository(&message);
 	}
 
 	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
@@ -2021,15 +2072,20 @@ static void repo_discover(struct repo_discovery *discovery, int *nongit_ok)
 				strbuf_addf(&message, _("$%s is set to '%s'"),
 					    CEILING_DIRECTORIES_ENVIRONMENT, ceiling);
 			}
-			die("%s", message.buf);
+			die_no_repository(&message);
 		}
 		*nongit_ok = 1;
 		break;
 	case GIT_DIR_HIT_MOUNT_POINT:
-		if (!nongit_ok)
-			die(_("not a git repository (or any parent up to mount point %s)\n"
-			      "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
-			    dir.buf);
+		if (!nongit_ok) {
+			struct strbuf message = STRBUF_INIT;
+
+			strbuf_addf(&message,
+				    _("not a git repository (or any parent up to mount point %s)\n"
+				      "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
+				    dir.buf);
+			die_no_repository(&message);
+		}
 		*nongit_ok = 1;
 		break;
 	case GIT_DIR_INVALID_OWNERSHIP:
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
index ab3d2593aa..94ef10edcd 100755
--- a/t/t0009-git-dir-validation.sh
+++ b/t/t0009-git-dir-validation.sh
@@ -107,7 +107,7 @@ test_expect_success 'setup: discovery fails when a valid git directory not found
 	)
 '
 
-test_expect_success 'setup: discovery fails with unusable $GIT_OBJECT_DIRECTORY and explicit gitdir' '
+test_expect_success 'setup: unusable $GIT_OBJECT_DIRECTORY is reported with explicit gitdir' '
 	test_when_finished "rm -rf parent/invalid-git-object-directory-config" &&
 	mkdir -p parent/invalid-git-object-directory-config &&
 	(
@@ -115,8 +115,40 @@ test_expect_success 'setup: discovery fails with unusable $GIT_OBJECT_DIRECTORY
 		git init --bare real-repo &&
 		test_must_fail env GIT_OBJECT_DIRECTORY="/path/to/does-not-exist" \
 			git --git-dir real-repo rev-parse --is-bare-repository 2>stderr &&
-		test_grep "not a git repository" stderr
+		test_grep "not a git repository" stderr &&
+		test_grep "cannot access object directory .*does-not-exist" stderr &&
+		test_grep "GIT_OBJECT_DIRECTORY" stderr
 	)
 '
 
+test_expect_success 'setup: unusable $GIT_OBJECT_DIRECTORY is reported during discovery' '
+	test_when_finished "rm -rf objdir-discovery" &&
+	git init objdir-discovery &&
+	(
+		cd objdir-discovery &&
+		test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/does-not-exist" \
+			git rev-parse --git-dir 2>stderr &&
+		test_grep "not a git repository" stderr &&
+		test_grep "cannot access object directory .*does-not-exist" stderr &&
+		test_grep "GIT_OBJECT_DIRECTORY" stderr
+	)
+'
+
+test_expect_success 'setup: usable $GIT_OBJECT_DIRECTORY is not reported' '
+	test_when_finished "rm -rf objdir-usable.git objdir-usable-odb" &&
+	git init --bare objdir-usable.git &&
+	mkdir objdir-usable-odb &&
+	GIT_OBJECT_DIRECTORY="$(pwd)/objdir-usable-odb" \
+		git --git-dir objdir-usable.git rev-parse --is-bare-repository >actual 2>stderr &&
+	echo true >expect &&
+	test_cmp expect actual &&
+	test_grep ! "not a git repository" stderr &&
+	test_grep ! "cannot access object directory" stderr &&
+
+	test_must_fail env GIT_OBJECT_DIRECTORY="$(pwd)/objdir-usable-odb" \
+		git --git-dir does-not-exist.git rev-parse --is-bare-repository 2>stderr &&
+	test_grep "not a git repository" stderr &&
+	test_grep ! "cannot access object directory" stderr
+'
+
 test_done
-- 
2.55.0.1003.g10538fe699

Re: [PATCH v3 5/5] setup: report when GIT_OBJECT_DIRECTORY is what we could not use

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> Improve this error reporting UX by explaining the real cause wherever
> we are about to die. Because the check does not depend on the candidate
> that is given to is_git_directory(), it is enough to ask once, at the
> point of failure, whether the configured object directory is to blame.
> With this, the user would see the following:
>
>   ...
>   $ git status
>   fatal: not a git repository (or any parent up to mount point /)
>   Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
>   cannot access object directory '/tmp/git-objects' set via
>     $GIT_OBJECT_DIRECTORY: No such file or directory

Hmph, this is only a half of the real story isn't it?

    $ git status ;# to show I am in a healthy repository
    on branch master
    nothing to commit (use -u to show untracked files)

In such a healthy repository, if I have a bogus DB_ENVIRONMENT,
discovery does not have to fail.

    $ GIT_OBJECT_DIRECTORY=/ git status
    fatal: bad object HEAD

On the other hand, if I am truly in a bogus non-repository,
it does not matter where DB_ENVIRONMENT points at.

    $ (cd / && GIT_OBJECT_DIRECTORY=. git status)
    fatal: not a git repository (or any of the parent directories): .git

Even when DB_ENVIRONMENT points at a valid object directory

    $ (cd / && GIT_OBJECT_DIRECTORY=$HOME/w/git.git/.git/objects git status)
    fatal: not a git repository (or any of the parent directories): .git

the command fails because it cannot find a valid repository.

> +static int describe_unusable_object_dir(struct strbuf *out)
> +{
> +	const char *objdir = getenv(DB_ENVIRONMENT);
> +	int saved_errno;
> +
> +	if (!objdir || !access(objdir, X_OK))
> +		return 0;
> +	saved_errno = errno;
> +
> +	/*
> +	 * TRANSLATORS: The first %s is a path, the second %s is the
> +	 * name of the environment variable that set it, and the third
> +	 * %s is the system error message.
> +	 */
> +	strbuf_addf(out, _("cannot access object directory '%s' set via $%s: %s"),
> +		    objdir, DB_ENVIRONMENT, strerror(saved_errno));
> +	return 1;
> +}

As this is called ONLY when we cannot figure out a usable GIT_DIR,
we do not have to worry about a case where $GIT_OBJECT_DIRECTORY is
not set and $GIT_DIR/objects/ is not readable.  So I do not think
this additional message hurts much, but I am not sure how much it
would help.  It would make the message more confusing in the "no
valid repository, DB_ENVIRONMENT pointing at a healthy object
directory" situation in the above example.

Re: [PATCH v3 0/5] Improve error reporting to mention how a Git repository is discovered

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> At the moment, when Git gives up with "fatal: not a git repository",
> it does not say which directory it actually inspected or where that
> directory came from. This is especially confusing when a leftover
> environment variable is steering Git somewhere the user did not
> expect.

I think [1/5]..[4/5] make sense but I am not sure about [5/5].

Thanks.