Re: [PATCH v3 1/5] refs: add struct repository parameter to branchname helpers

[Tian Yuchen <a3205153416@gmail.com>]

On 4/3/26 01:03, Burak Kaan Karaçay wrote:
> 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

You’re absolutely right.

Actually, I’d already spotted the error when I wrote that bit of code, 
which is why I said 'I dunno' - simply to give an idea of what I was 
trying to achieve. It looks like we’ll have to go with a much uglier 
solution. ;)

One possible approach (albeit temporary and useless) is to wrap the code 
in a macro or an inline function (in repository.h, I guess?):

> static inline int repo_has_repository(struct repository *repo)
> {
> 	/*  
 >	* NEEDSWORK...
>	*/> 	if (repo == the_repository)
> 		return startup_info->have_repository;
> 
> 	return repo && repo->gitdir;
> }

This will remove a large amount of startup_info->have_repository...But 
that doesn’t solve anything, just as stuffing rubbish into a bin doesn’t 
change the fact that it’s rubbish. The 'startup_info->have_repository' 
still requires a significant amount of effort to refactor.

So still, I dunno.

Regards, Yuchen