From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Date: Fri, 03 Apr 2026 10:39:02 GMT
Subject: Re: [PATCH v3 1/5] refs: add struct repository parameter to branchname helpers
Message-ID: <CAPYXD646gcj-fmy0fqZUrKsSt1=+ZW4iRsVuJoLf0yUyUddigQ@mail.gmail.com>
In-Reply-To: <ac68ME2j5CXzVgxF@pks.im>

On Fri, Apr 3, 2026 at 12:28 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Apr 02, 2026 at 08:03:45PM +0300, 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/
>
> 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.

I think it makes sense to drop patch 1/5 from this series for now, which
introduces changes to the branch name helper functions.
It would be much better to address this separately after replacing
startup_info->have_repository.
For now, I'll reroll the series with the remaining patches and send this
part later as an rfc.

Thanks everyone,
Shreyansh