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

[Patrick Steinhardt <ps@pks.im>]

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

On Mon, Apr 29, 2024 at 05:52:41PM -0500, Justin Tobler wrote:
> On 24/04/29 03:41PM, Patrick Steinhardt wrote:
> > diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> > index d71b199955..4275918fa0 100644
> > --- a/Documentation/glossary-content.txt
> > +++ b/Documentation/glossary-content.txt
> > @@ -497,20 +497,28 @@ exclude;;
> >  	unusual refs.
> >  
> >  [[def_pseudoref]]pseudoref::
> > -	Pseudorefs are a class of files under `$GIT_DIR` which behave
> > -	like refs for the purposes of rev-parse, but which are treated
> > -	specially by git.  Pseudorefs both have names that are all-caps,
> > -	and always start with a line consisting of a
> > -	<<def_SHA1,SHA-1>> followed by whitespace.  So, HEAD is not a
> > -	pseudoref, because it is sometimes a symbolic ref.  They might
> 
> We remove the example here about HEAD not being a pseudoref. This
> example seems helpful to indicate that a pseudoref cannot be a symbolic
> ref. Is this no longer the case and the change intended?

I just don't see why we would want to have this restriction. Honestly,
the more I think about this whole topic the more I want to go into the
direction I've hinted at in the cover letter: drop "special refs" and
define pseudo refs as either FETCH_HEAD or MERGE_HEAD. Everything else
is just a normal ref, even though some of those may live in the root
directory if they conform to a set of strict rules:

  - All upppercase characters plus underscores.

  - Must end with "_HEAD", except a list of known irregular root refs.

I feel like the world would be better like this.

> > -	optionally contain some additional data.  `MERGE_HEAD` and
> > -	`CHERRY_PICK_HEAD` are examples.  Unlike
> > -	<<def_per_worktree_ref,per-worktree refs>>, these files cannot
> > -	be symbolic refs, and never have reflogs.  They also cannot be
> > -	updated through the normal ref update machinery.  Instead,
> > -	they are updated by directly writing to the files.  However,
> > -	they can be read as if they were refs, so `git rev-parse
> > -	MERGE_HEAD` will work.
> > +	Pseudorefs are references that live in the root of the reference
> > +	hierarchy, outside of the usual "refs/" hierarchy. Pseudorefs have an
> > +	all-uppercase name and must end with a "_HEAD" suffix, for example
> > +	"`BISECT_HEAD`". Other than that, pseudorefs behave the exact same as
> > +	any other reference and can be both read and written via regular Git
> > +	tooling.
> 
> Pseudorefs behaving the same and using the same tooling seems to
> contridict the previous documentation. I assume the previous information
> was out-of-date, but it might be nice to explain this in the message.

Yes, and I actually want to change this. We never enforced restrictions
for pseudorefs anyway, they can be symrefs just fine. And neither would
I see any reason why that should be the case in the first place.

> > ++
> > +<<def_special_ref>,Special refs>> are not pseudorefs.
> > ++
> > +Due to historic reasons, Git has several irregular pseudo refs that do not
> > +follow above rules. The following list of irregular pseudo refs is exhaustive
> 
> We seem to be inconsistent between using "pseudoref" and "pseudo ref".
> Not sure it we want to be consistent here. 

Makes sense.

Patrick

> -Justin
> 
> > +and shall not be extended in the future:
> > +
> > + - "`AUTO_MERGE`"
> > +
> > + - "`BISECT_EXPECTED_REV`"
> > +
> > + - "`NOTES_MERGE_PARTIAL`"
> > +
> > + - "`NOTES_MERGE_REF`"
> > +
> > + - "`MERGE_AUTOSTASH`"
> >  
> >  [[def_pull]]pull::
> >  	Pulling a <<def_branch,branch>> means to <<def_fetch,fetch>> it and
> > diff --git a/refs.c b/refs.c
> > index c64f66bff9..567c6fc6ff 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -905,6 +905,8 @@ int is_pseudoref(struct ref_store *refs, const char *refname)
> >  
> >  	if (!is_pseudoref_syntax(refname))
> >  		return 0;
> > +	if (is_special_ref(refname))
> > +		return 0;
> >  
> >  	if (ends_with(refname, "_HEAD")) {
> >  		refs_resolve_ref_unsafe(refs, refname,
> > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> > index 948f1bb5f4..8c92fbde79 100755
> > --- a/t/t6302-for-each-ref-filter.sh
> > +++ b/t/t6302-for-each-ref-filter.sh
> > @@ -52,6 +52,23 @@ test_expect_success '--include-root-refs pattern prints pseudorefs' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success '--include-root-refs pattern does not print special refs' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		test_commit initial &&
> > +		git rev-parse HEAD >.git/MERGE_HEAD &&
> > +		git for-each-ref --format="%(refname)" --include-root-refs >actual &&
> > +		cat >expect <<-EOF &&
> > +		HEAD
> > +		$(git symbolic-ref HEAD)
> > +		refs/tags/initial
> > +		EOF
> > +		test_cmp expect actual
> > +	)
> > +'
> > +
> >  test_expect_success '--include-root-refs with other patterns' '
> >  	cat >expect <<-\EOF &&
> >  	HEAD
> > -- 
> > 2.45.0-rc1
> > 
> 
> 

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