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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 2, 2026, 18:57 UTC
Message-ID: <ac68ME2j5CXzVgxF@pks.im>
In-Reply-To: <ac6K5UnVdw67Rfpy@gmail.com>

On Thu, Apr 02, 2026 at 08:03:45PM +0300, Burak Kaan Karaçay wrote:

Show 37 quoted lines

> 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.

Show 12 quoted lines

> 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/

If we cannot make it work in this patch series, the next question is whether we actually want to give the false sense of `check_branch_ref()` being independent of global state, or whether we want to leave it as-is for now and then do a follow-up patch series where we fix the issue and adapt the interface.

Patrick