[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

[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

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

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

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

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