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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 30, 2024, 09:59 UTC
Message-ID: <e9c70778-3414-48ae-8a7d-6b6f0dfaad63@gmail.com>
In-Reply-To: <ZjCd_drNT6kyjNa6@tanuki>

Hi Patrick

On 30/04/2024 08:30, Patrick Steinhardt wrote:

Show 17 quoted lines

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

Show 12 quoted lines

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

Show 13 quoted lines

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