Re: [RFC PATCH 2/3] 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.
>
> 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.