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

[Junio C Hamano <gitster@pobox.com>]

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.