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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2023, 01:23 UTC
Message-ID: <xmqqfs1f3eji.fsf@gitster.g>
In-Reply-To: <898d3850-b0ca-485e-9489-320eee3121e4@github.com>

Victoria Dye <vdye@github.com> writes:

Show 15 quoted lines

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

In a repository without any tag-to-tag at tips of refs, would #2 above still be an issue? My assumption when I raised "isn't this simply a bug?" question was that the use of tag-to-tag is a mere intellectual curiosity, there is no serious use case, and they are not heavily used. Hence I was envisioning that #1 below (i.e., a mention in the Release Notes' backward compatibility notes section) would be sufficient.

If it weren't the case, then I do not think any "transition" would work, either.

And stepping back a bit, even though "peel only once" is how for-each-ref works, I do not think anybody who really cares about tag-to-tag and inspecting each level of peeled tag is helped by it all that much. Yes, you can get the result of single level peeling via "git format-patch --format=%(*objectname)", but then what would you do to dig further from that point? You cannot ask rev-parse to peel the result with "^{}", as that will peel all the way down.

You have to feed it to "git cat-file tag" and parse the contents of the tag obbject yourself to manually peel further levels of onion. Anybody who do care must already have such a machinery, and such a machinery does not depend on "git for-each-ref --format='%(*field)'" peeling just once, I would say. They would most likely learn the "%(objectname) %(objecttype) %(refname)" from the command, and for those that are tags, they would manually peel the object with such a machinery, because they have to do that for second and further levels anyway.

And that is why I am not so worried about "breaking" existing users in this particular case. Our existing support with tag-to-tag is so poor that those who truly need it would have invented necessary support without relying on for-each-ref's peeling (if such people did exist, that is).

But perhaps I am so overly optimistic against Hyrum's law.

Show 16 quoted lines

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