Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags

From: Victoria Dye <vdye@github.com>
Date: Nov 8, 2023, 18:02 UTC
Message-ID: <898d3850-b0ca-485e-9489-320eee3121e4@github.com>
In-Reply-To: <xmqq4jhx7x8l.fsf@gitster.g>

Junio C Hamano wrote:

Show 5 quoted lines

> Victoria Dye <vdye@github.com> writes:
> 
>> I think `^{}fieldname` would be a good candidate, but it's *extremely*
> 
> Gaah.  Why?  fieldname^{} I may understand, but in the prefix form?

'fieldname^{}' seemed like more of a misuse of "^{}" than the prefixed form, since we're not peeling "fieldname" but instead getting the value of "fieldname" from the peeled tag. But then we're not dereferencing "fieldname" in '*fieldname' either, so 'fieldname^{}' is no worse than what already exists.

Show 7 quoted lines

> 
> In any case, has anybody considered that we may be better off to
> declare that "*field" peeling a tag only once is a longstanding bug?
> 
> IOW, can we not add "fully peel" command line option or a new syntax
> and instead just "fix" the bug to fully peel when "*field" is asked
> for?

I'd certainly prefer that from a technical standpoint; it simplifies this patch if I can just replace 'get_tagged_oid' with 'peel_iterated_oid'. The two things that make me hesitate are:

1. There isn't a straightforward 1:1 substitute available for getting info
   on the immediate target of a list of tags. 
2. The performance of a recursive peel can be worse than that of a single
   tag dereference, since (unless the formatting is done in a ref_iterator
   iteration *and* the tag is a packed ref) the dereferenced object needs to
   be resolved to determine whether it's another tag or not.

#1 may not be an issue in practice, but I don't have enough information on how applications use that formatting atom to say for sure. #2 is a bigger issue, IMO, since one of the goals of this series was to improve performance for some cases of 'for-each-ref' without hurting it in others.

Show 25 quoted lines

> An application that cares about handling a chain of annotatetd tags
> would want to be able to say "this is the outermost tag's
> information; one level down, the tag was signed by this person;
> another level down, the tag was signed by this person, etc."  which
> would mean either
> 
>  * we have a syntax that shows the information from all levels
>    (e.g., "**taggername" may say "Victoria\nPatrick\nGitster")
> 
>  * we have a syntax that allows to specify how many levels to peel,
>    (e.g., "*0*taggername" may be the same as "taggername",
>    "*1*taggername" may be the same as "*taggername") plus some
>    programming construct like variables and loops.
> 
> but the repertoire being proposed that consists only of "peel only
> once" and "peel all levels" is way too insufficient.
> 
> Note that I do not advocate for allowing inspection of each levels
> separately.  Quite the contrary.  I would say that --format=<>
> placeholder should not be a programming language to satisify such a
> niche need.  And my conclusion from that stance is "peel once" plus
> "peel all" are already one level too many, and "peel once" was a
> very flawed implementation from day one, when 9f613ddd (Add
> git-for-each-ref: helper for language bindings, 2006-09-15)
> introduced it.

I can (and would like to) deprecate the "peel once" behavior and replace it with "peel all", but with how long it's been around and the potential performance impact, such a change should probably be clearly communicated. How that happens depends on how aggressively we want to cut over. We could:

1. Change the behavior of '*' from single dereference to recursive
   dereference, make a note of it in the documentation.
2. Same as #1, but also add an option like '--no-recursive-dereference' or
   something to use the old behavior. Remove the option after 1-2 release
   cycles?
3. Add a new format specifier '^{}', note that '*' is deprecated in the
   docs.
4. Same as #3, but also show a warning/advice if '*' is used.
5. Same as #3, but die() if '*' is used.

I'm open to other options, those were just the first few I could think of.