From: Burak Kaan Karaçay <bkkaracay@gmail.com>
Date: Thu, 02 Apr 2026 17:03:45 GMT
Subject: Re: [PATCH v3 1/5] refs: add struct repository parameter to branchname helpers
Message-ID: <ac6K5UnVdw67Rfpy@gmail.com>
In-Reply-To: <ac4aZRveWXjOtxgB@pks.im>

Hi,

On Thu, Apr 02, 2026 at 09:27:33AM +0200, Patrick Steinhardt wrote:
>On Sun, Mar 29, 2026 at 03:46:39PM +0530, Shreyansh Paliwal wrote:
>> diff --git a/refs.c b/refs.c
>> index 685a0c247b..5cdc8858c5 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -758,10 +758,10 @@ void copy_branchname(struct strbuf *sb, const char *name,
>>  	strbuf_add(sb, name + used, len - used);
>>  }
>>
>> -int check_branch_ref(struct strbuf *sb, const char *name)
>> +int check_branch_ref(struct repository *repo, struct strbuf *sb, const char *name)
>>  {
>>  	if (startup_info->have_repository)
>> -		copy_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
>> +		copy_branchname(repo, sb, name, INTERPRET_BRANCH_LOCAL);
>>  	else
>>  		strbuf_addstr(sb, name);
>>
>
>I have to agree with Tian's comment on v2, this part here looks wrong. I
>don't think we should depend on `startup_info` here, but we should
>exclusively rely on whether or not the caller has passed in a
>repository. And that will likely require a bit more scrutiny to figure
>out whether there are any callers that shouldn't pass in a repository
>because it's not initialized.
>
>Alternatively, we could go with Tian's suggestion of checking for `repo
>&& repo->gitdir`.
>
>Patrick

This approach actually leads to a bug and segfault in a specific edge
case when running 'git check-ref-format'. The current tests don't cover
this scenario, but they can be extended to catch it.

If GIT_DIR is set to a non-existent path,
'startup_info->have_repository' becomes '0' but 'repo->gitdir' still
holds the invalid path. As a result, the code enters the first condition
and crashes. The case can be tested with this command:

$ git --git-dir='non-existing' check-ref-format --branch @{-1}

Modifying the behavior of 'repo->gitdir' might solve the issue, but I
belive that falls outside the scope of this patch. After a quick search,
I found a prophecy from Peff about the 'startup_info->have_repository':

[1] https://lore.kernel.org/git/20190806124954.GA13649@sigill.intra.peff.net/

Thanks,
Burak Kaan Karaçay