From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 30 Sep 2026 16:03:12 GMT
Subject: Re: [RFC PATCH v2 4/4] setup: communicate why a directory is not a valid git directory
Message-ID: <ar0ywAlMDuzK1Hvb@pks.im>
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