# [RFC PATCH 0/3] Improve error reporting to mention "why" a directory is not a repository 20 messages from 2026-09-24 to 2026-10-05. Participants: Kaartic Sivaraam, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/66383 ## Kaartic Sivaraam, 2026-09-24 12:02 Subject: [RFC PATCH 0/3] Improve error reporting to mention "why" a directory is not a repository Message-ID: <20260924120502.2642141-1-kaartic.sivaraam@gmail.com> ``` 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 ``` ## Kaartic Sivaraam, 2026-09-24 12:02 Subject: [RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios Message-ID: <20260924120502.2642141-2-kaartic.sivaraam@gmail.com> In-Reply-To: <20260924120502.2642141-1-kaartic.sivaraam@gmail.com> ``` Introduce few more tests to t0009 to cover error reporting scenarios when --git-dir is used. Signed-off-by: Kaartic Sivaraam --- 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 ``` ## Kaartic Sivaraam, 2026-09-24 12:02 Subject: [RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose' Message-ID: <20260924120502.2642141-3-kaartic.sivaraam@gmail.com> In-Reply-To: <20260924120502.2642141-1-kaartic.sivaraam@gmail.com> ``` 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 --- 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 ``` ## Kaartic Sivaraam, 2026-09-24 12:02 Subject: [RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory Message-ID: <20260924120502.2642141-4-kaartic.sivaraam@gmail.com> In-Reply-To: <20260924120502.2642141-1-kaartic.sivaraam@gmail.com> ``` 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 --- 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 ``` ## Junio C Hamano, 2026-09-24 22:08 Subject: Re: [RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios Message-ID: In-Reply-To: <20260924120502.2642141-2-kaartic.sivaraam@gmail.com> ``` Kaartic Sivaraam writes: > Introduce few more tests to t0009 to cover error reporting scenarios > when --git-dir is used. > > Signed-off-by: Kaartic Sivaraam > --- > 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 ``` ## Junio C Hamano, 2026-09-24 22:11 Subject: Re: [RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose' Message-ID: In-Reply-To: <20260924120502.2642141-3-kaartic.sivaraam@gmail.com> ``` Kaartic Sivaraam 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 > --- > 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. ``` ## Junio C Hamano, 2026-09-24 22:16 Subject: Re: [RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory Message-ID: In-Reply-To: <20260924120502.2642141-4-kaartic.sivaraam@gmail.com> ``` Kaartic Sivaraam 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? ``` ## Kaartic Sivaraam, 2026-09-25 13:46 Subject: Re: [RFC PATCH 1/3] t0009: add tests to cover more error reporting scenarios Message-ID: In-Reply-To: ``` On 9/25/26 03:38, Junio C Hamano wrote: > Kaartic Sivaraam writes: > >> Introduce few more tests to t0009 to cover error reporting scenarios >> when --git-dir is used. >> >> Signed-off-by: Kaartic Sivaraam >> --- >> 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 ``` ## Kaartic Sivaraam, 2026-09-25 14:33 Subject: Re: [RFC PATCH 3/3] setup: communicate why a directory is not a valid git directory Message-ID: In-Reply-To: ``` On 9/25/26 03:46, Junio C Hamano wrote: > Kaartic Sivaraam 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 ``` ## Kaartic Sivaraam, 2026-09-25 17:51 Subject: Re: [RFC PATCH 2/3] setup: introduce new helper 'is_git_directory_verbose' Message-ID: In-Reply-To: ``` On 9/25/26 03:41, Junio C Hamano wrote: > Kaartic Sivaraam 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. ``` ## Kaartic Sivaraam, 2026-09-29 10:25 Subject: [RFC PATCH v2 1/4] setup: normalize an if-else to follow our convention Message-ID: <20260929102513.712181-2-kaartic.sivaraam@gmail.com> In-Reply-To: <20260929102513.712181-1-kaartic.sivaraam@gmail.com> ``` 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 --- 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 ``` ## Kaartic Sivaraam, 2026-09-29 10:25 Subject: [RFC PATCH v2 0/4] Improve error reporting to mention "why" a directory is not a repository Message-ID: <20260929102513.712181-1-kaartic.sivaraam@gmail.com> In-Reply-To: <20260924120502.2642141-1-kaartic.sivaraam@gmail.com> ``` 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 ``` ## Kaartic Sivaraam, 2026-09-29 10:25 Subject: [RFC PATCH v2 2/4] t0009: add tests to cover more error reporting scenarios Message-ID: <20260929102513.712181-3-kaartic.sivaraam@gmail.com> In-Reply-To: <20260929102513.712181-1-kaartic.sivaraam@gmail.com> ``` Introduce few more tests to t0009 to cover error reporting scenarios when --git-dir is used. Signed-off-by: Kaartic Sivaraam --- 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 ``` ## Kaartic Sivaraam, 2026-09-29 10:25 Subject: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose' Message-ID: <20260929102513.712181-4-kaartic.sivaraam@gmail.com> In-Reply-To: <20260929102513.712181-1-kaartic.sivaraam@gmail.com> ``` 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 --- 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 ``` ## Kaartic Sivaraam, 2026-09-29 10:25 Subject: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory Message-ID: <20260929102513.712181-5-kaartic.sivaraam@gmail.com> In-Reply-To: <20260929102513.712181-1-kaartic.sivaraam@gmail.com> ``` 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 --- 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 ``` ## Patrick Steinhardt, 2026-09-30 16:03 Subject: Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose' Message-ID: In-Reply-To: <20260929102513.712181-4-kaartic.sivaraam@gmail.com> ``` 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 ``` ## Patrick Steinhardt, 2026-09-30 16:03 Subject: Re: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory Message-ID: In-Reply-To: <20260929102513.712181-5-kaartic.sivaraam@gmail.com> ``` 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 ``` ## Junio C Hamano, 2026-09-30 18:32 Subject: Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose' Message-ID: In-Reply-To: <20260929102513.712181-4-kaartic.sivaraam@gmail.com> ``` Kaartic Sivaraam 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. ``` ## Kaartic Sivaraam, 2026-10-05 12:14 Subject: Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose' Message-ID: <168ac5aa-a05b-43df-9cf4-78c4295e4faa@gmail.com> In-Reply-To: ``` 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 ``` ## Kaartic Sivaraam, 2026-10-05 12:29 Subject: Re: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory Message-ID: In-Reply-To: ``` 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 ```