Junio C Hamano wrote:
> Victoria Dye <vdye@xxxxxxxxxx> 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.
>
> 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.
> 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.
