Re: [PATCH 2/3] refs: do not label special refs as pseudo refs

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4721 bytes --]

On Tue, Apr 30, 2024 at 10:59:36AM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 30/04/2024 08:30, Patrick Steinhardt wrote:
> > On Mon, Apr 29, 2024 at 04:12:37PM +0100, Phillip Wood wrote:
> > 
> > > This changes the definition to allow pseudorefs to by symbolic refs. When
> > > is_pseudoref() was introduced Junio and I had a brief discussion about this
> > > restriction and he was not in favor of allowing pseudorefs to be symbolic
> > > refs [1].
> > 
> > So the reason why pseudorefs exist is that some refs behave like a ref
> > sometimes, but not always. And in my book that really only applies to
> > MERGE_HEAD and FETCH_HEAD, because those contain additional metadata
> > that makes them not-a-ref. And for those I very much see that they
> > should not ever be a symref.
> > 
> > But everyhing else living in the root of the ref hierarchy is not
> > special in any way, at least not in my opinion. We have never enforced
> > that those cannot be symrefs, and it makes our terminology needlessly
> > confusing.
> 
> I agree HEAD not being a pseudoref and having special refs as well as
> pseudorefs refs is confusing. I do have some sympathy for the argument that
> pseudorefs should not be symbolic refs though as AUTO_MERGE,
> CHERRY_PICK_HEAD, ORIG_HEAD etc. are all pointers to a commit and it would
> be a bug for them to be a symbolic ref. It is unfortunate that in the move
> away from assessing those refs as files we lost the check that they are not
> symbolic refs.

While I agree that conceptually these should always be "regular" refs, I
feel like that is higher-level logic that belongs into the respective
subsystems that write those. I just don't see why the ref backend should
care about the particular usecases that those higher-level subsystems
have, and I can very much see that there might eventually be another
subsystem that actually wants a specific ref to be a symref.

No we could of course start to hard code all kinds of refs into the ref
layer. But I think that this is the wrong way to go, and treating the
ref store as just that, a generic store where you can store refs,
without attaching specific meaning to any of the refs, is the proper way
to go.

> > I think I'm going to reroll this patch series and go down the nuclear
> > path that I've hinted at in the cover letter:
> > 
> >    - Pseudo refs can only be either FETCH_HEAD or MERGE_HEAD.
> > 
> >    - Refs starting with "refs/" are just plain normal refs.
> > 
> >    - Refs living in the root of the ref hierarchy need to conform to a
> >      set of strict rules, as Peff is starting to enforce in a separate
> >      patch series. These are just normal refs, as well, even though we
> >      may call them "root ref" in our tooling as they live in the root of
> >      the ref hierarchy.
> 
> That would certainly be simpler.
> 
> > I just don't think that the current state makes sense to anybody. It's
> > majorly confusing -- I've spent the last 8 months working in our refs
> > code almost exclusively and still forget what's what. How are our users
> > expected to understand this?
> 
> The current state is confusing but arguably there is a logic to the various
> distinctions - whether those distinctions are useful in practice is open to
> debate though. I wonder how much users really care about these distinctions
> and whether it affects their use of git. I was unaware of the distinction
> between HEAD and pseudorefs until I reviewed Karthik's for-each-ref series a
> couple of months ago and I don't think that lack of knowledge had caused me
> any trouble when using git.

There is some logic, that's true enough. I just don't think that anybody
understands the logic.

Patrick

> > > Are there any practical implications of the changes in this patch for users
> > > running commands like "git log FETCH_HEAD" (I can't think of any off the top
> > > of my head but it would be good to have some reassurance on that point in
> > > the commit message)
> > 
> > Not really, no. We have never been doing a good job at enforcing the
> > difference between pseudo refs or normal refs anyway. Pseudo refs can be
> > symrefs just fine, and our tooling won't complain. The only exception
> > where I want us to become stricter is in how we enforce the syntax rules
> > for root refs (which is handled by Peff in a separate patch series), and
> > that we start to not treat FETCH_HEAD and MERGE_HEAD as proper refs.
> > They should still resolve when you ask git-rev-parse(1), but when you
> > iterate through refs they should not be surfaced as they _aren't_ refs.
> 
> That's good
> 
> Thanks
> 
> Phillip
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]