From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Mon, 05 Oct 2026 12:14:56 GMT
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: <ar0yutZ9ksSvaVmM@pks.im>

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