From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 30 Sep 2026 16:03:06 GMT
Subject: Re: [RFC PATCH v2 3/4] setup: introduce new helper 'is_git_directory_verbose'
Message-ID: <ar0yutZ9ksSvaVmM@pks.im>
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