Re: [PATCH 3/3] refs: fix segfault in `is_pseudoref()` when ref cannot be resolved

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 30, 2024, 07:30 UTC
Message-ID: <ZjCeAxbL9bufwGEN@tanuki>
In-Reply-To: <CAOLa=ZTCCgtQiQbm_GU--1+Yq-qYJANzTvRmvVrqNmzchkVevw@mail.gmail.com>

On Mon, Apr 29, 2024 at 11:57:53AM -0700, Karthik Nayak wrote:

Show 22 quoted lines

> > It is somewhat questionable in the first place that we declare a ref to
> > be a pseudorefe depending on whether it resolves to an object ID or not.
> 
> s/pseudorefe/pseudoref
> 
> [snip]
> 
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> via the uninitialized value when calling `is_null_oid()`, which causes
> >> us to segfault.
> >>
> >> It is somewhat questionable in the first place that we declare a ref to
> >> be a pseudorefe depending on whether it resolves to an object ID or not.
> >
> > If I remember rightly Karthik added that check to avoid the files
> > backend calling a file with a name that matched the pseudoref syntax a
> > pseudoref when it wasn't actually a pseudoref.
> 
> Not sure I follow. I think it was strictly done to ensure we don't
> consider symrefs as pseudorefs [1].
> 
> [1]: https://lore.kernel.org/git/xmqqfrymeega.fsf@gitster.g/

And that's fair from a terminology perspective. But honestly, I really doubt that any user will understand that REBASE_HEAD is a pseudoref when it contains an object ID, but is not a pseudoref when it is a symref.

Anyway, as I've said in parallel mails, I want to change the definition of what a pseudoref is. I just think that the current mess is understood by nobody and doesn't make any sense.

I'll thus implicitly address this in my v2.

Patrick