Re: [PATCH] ref-filter: handle nested tags in --points-at option

On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:

That seems reasonable to me. It is changing the definition of --points-at slightly, but I think in a way that should be less surprising to users (i.e., we can consider the old behavior a bug). The existing documentation is sufficiently vague about "points" that I don't think it needs to be updated (though arguably we could improve that here, too).

Note that most other tag-peeling in Git (like the peeled values returned by upload-pack) errs in the opposite direction: they peel completely, and don't show the intermediate values. We _could_ switch to that here, but I think it would be a behavior regression (but see below on why we might entertain the thought).

My biggest question would be whether this introduces any performance penalty for the more common cases (lightweight tags and single-level annotated tags). The answer is "no", I think; we are already paying the cost to parse every object to find out if it's a tag, and your new loop only does an extra parse if we see a tag-of-tag. Good.

  Let me go off on a tangent here, since I'm looking at the performance
  of this function. The current code is already rather pessimal here, as
  we could probably avoid parsing non-tags entirely. Some strategies
  there are:

    1. We could check the object type via oid_object_info() before
       parsing. This carries a small penalty (two lookups) for tags but
       a big win (avoiding loading the object contents) for non-tags.

       An easy way to do this is to replace the parse_object() with
       parse_object_with_flags(PARSE_OBJECT_SKIP_HASH_CHECK), which
       tries to avoid loading object contents (especially using the
       commit-graph for commits, which presumably covers most non-tag
       refs).

    2. We could be using the peel_iterated_oid() interface (this is the
       peel_ref() thing mentioned in the comment you touched, but it has
       since been renamed). But it does the "peel all the way" thing
       mentioned above (both because of its interface, but also because
       that's what the packed-refs peel lines store).

       So to do that we'd either have to enhance the packed-refs store
       (which would not be too hard to do in a backwards-compatible
       way), or switch --points-at to only match either the direct ref
       value or the fully-peeled value.

  I don't think either of those is something your patch needs to deal
  with. It is not making these kinds of optimizations any harder (it is
  the existing "peel only once" behavior that does so). I mostly wanted
  to get it written down while we are all looking at this function.

Great, thanks for cleaning up this comment.

OK, so we are doing the usual peeling loop here. I wondered if we might be able to use peel_object(), but it again suffers from the "peel all the way" syndrome. So we have to loop ourselves so that we can check at each level. Good.

This will now trigger if refname points to a broken object, or if its tag does. I think the resulting message is OK in either case (and presumably lower level code would produce extra error messages, too).

This code is moved into the loop body, but your version there drops the "if (tagged_oid)" check. I think that is OK (and even preferable), though. In get_tagged_oid() we will die() if the tagged object is NULL (though even before switching to that function this check was questionable, because it is "tag->tagged" that may be NULL, and we were dereferencing that unconditionally).

So the code looks good.

And the test looks good, too. It is nice that we can rely on the existing setup for the doubly-* tags.

Thanks for an easy-to-review patch. :)

-Peff