{"thread":{"id":"59941","subject":"[PATCH] ref-filter: handle nested tags in --points-at option","startedAt":"2023-07-01T21:07:15Z","lastAt":"2023-07-05T20:15:44Z","messageCount":15,"participants":["Jan Klötzke","Jeff King","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"479086","messageId":"20230701205703.1172505-1-jan@kloetzke.net","threadId":"59941","inReplyTo":null,"subject":"[PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jan Klötzke","fromEmail":"jan@kloetzke.net","sentAt":"2023-07-01T20:57:02Z","receivedAt":"2023-07-01T21:07:15Z","isPatch":true,"sender":{"key":"jan@kloetzke.net","avatar":"https://avatars.githubusercontent.com/u/234300?v=4"},"body":"Tags are dereferenced until reaching a different object type to handle\nnested tags, e.g. on checkout. In contrast, \"git tag --points-at=...\"\nfails to list such nested tags because only one level of indirection is\nobtained in filter_refs(). Implement the recursive dereferencing for the\n\"--points-at\" option when filtering refs to unify the behaviour.\n\nSigned-off-by: Jan Klötzke <jan@kloetzke.net>\n---\n ref-filter.c                   | 16 +++++++---------\n t/t6302-for-each-ref-filter.sh |  2 ++\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e0d03a9f8e..ad7f244414 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2211,10 +2211,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n  * of oids. If the given ref is a tag, check if the given tag points\n  * at one of the oids in the given oid array.\n  * NEEDSWORK:\n- * 1. Only a single level of indirection is obtained, we might want to\n- * change this to account for multiple levels (e.g. annotated tags\n- * pointing to annotated tags pointing to a commit.)\n- * 2. As the refs are cached we might know what refname peels to without\n+ * As the refs are cached we might know what refname peels to without\n  * the need to parse the object via parse_object(). peel_ref() might be a\n  * more efficient alternative to obtain the pointee.\n  */\n@@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n \t\t\t\t\t       const struct object_id *oid,\n \t\t\t\t\t       const char *refname)\n {\n-\tconst struct object_id *tagged_oid = NULL;\n \tstruct object *obj;\n \n \tif (oid_array_lookup(points_at, oid) >= 0)\n \t\treturn oid;\n \tobj = parse_object(the_repository, oid);\n+\twhile (obj && obj->type == OBJ_TAG) {\n+\t\toid = get_tagged_oid((struct tag *)obj);\n+\t\tif (oid_array_lookup(points_at, oid) >= 0)\n+\t\t\treturn oid;\n+\t\tobj = parse_object(the_repository, oid);\n+\t}\n \tif (!obj)\n \t\tdie(_(\"malformed object at '%s'\"), refname);\n-\tif (obj->type == OBJ_TAG)\n-\t\ttagged_oid = get_tagged_oid((struct tag *)obj);\n-\tif (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)\n-\t\treturn tagged_oid;\n \treturn NULL;\n }\n \ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 1ce5f490e9..af223e44d6 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -45,6 +45,8 @@ test_expect_success 'check signed tags with --points-at' '\n \tsed -e \"s/Z$//\" >expect <<-\\EOF &&\n \trefs/heads/side Z\n \trefs/tags/annotated-tag four\n+\trefs/tags/doubly-annotated-tag An annotated tag\n+\trefs/tags/doubly-signed-tag A signed tag\n \trefs/tags/four Z\n \trefs/tags/signed-tag four\n \tEOF\n-- \n2.39.2\n\n"},{"id":"479091","messageId":"20230702125611.GA1036686@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230701205703.1172505-1-jan@kloetzke.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T12:56:11Z","receivedAt":"2023-07-02T12:56:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:\n\n> Tags are dereferenced until reaching a different object type to handle\n> nested tags, e.g. on checkout. In contrast, \"git tag --points-at=...\"\n> fails to list such nested tags because only one level of indirection is\n> obtained in filter_refs(). Implement the recursive dereferencing for the\n> \"--points-at\" option when filtering refs to unify the behaviour.\n\nThat seems reasonable to me. It is changing the definition of\n--points-at slightly, but I think in a way that should be less\nsurprising to users (i.e., we can consider the old behavior a bug).  The\nexisting documentation is sufficiently vague about \"points\" that I don't\nthink it needs to be updated (though arguably we could improve that\nhere, too).\n\nNote that most other tag-peeling in Git (like the peeled values returned\nby upload-pack) errs in the opposite direction: they peel completely,\nand don't show the intermediate values. We _could_ switch to that here,\nbut I think it would be a behavior regression (but see below on why we\nmight entertain the thought).\n\nMy biggest question would be whether this introduces any performance\npenalty for the more common cases (lightweight tags and single-level\nannotated tags). The answer is \"no\", I think; we are already paying the\ncost to parse every object to find out if it's a tag, and your new loop\nonly does an extra parse if we see a tag-of-tag. Good.\n\n  Let me go off on a tangent here, since I'm looking at the performance\n  of this function. The current code is already rather pessimal here, as\n  we could probably avoid parsing non-tags entirely. Some strategies\n  there are:\n\n    1. We could check the object type via oid_object_info() before\n       parsing. This carries a small penalty (two lookups) for tags but\n       a big win (avoiding loading the object contents) for non-tags.\n\n       An easy way to do this is to replace the parse_object() with\n       parse_object_with_flags(PARSE_OBJECT_SKIP_HASH_CHECK), which\n       tries to avoid loading object contents (especially using the\n       commit-graph for commits, which presumably covers most non-tag\n       refs).\n\n    2. We could be using the peel_iterated_oid() interface (this is the\n       peel_ref() thing mentioned in the comment you touched, but it has\n       since been renamed). But it does the \"peel all the way\" thing\n       mentioned above (both because of its interface, but also because\n       that's what the packed-refs peel lines store).\n\n       So to do that we'd either have to enhance the packed-refs store\n       (which would not be too hard to do in a backwards-compatible\n       way), or switch --points-at to only match either the direct ref\n       value or the fully-peeled value.\n\n  I don't think either of those is something your patch needs to deal\n  with. It is not making these kinds of optimizations any harder (it is\n  the existing \"peel only once\" behavior that does so). I mostly wanted\n  to get it written down while we are all looking at this function.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index e0d03a9f8e..ad7f244414 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2211,10 +2211,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n>   * of oids. If the given ref is a tag, check if the given tag points\n>   * at one of the oids in the given oid array.\n>   * NEEDSWORK:\n> - * 1. Only a single level of indirection is obtained, we might want to\n> - * change this to account for multiple levels (e.g. annotated tags\n> - * pointing to annotated tags pointing to a commit.)\n> - * 2. As the refs are cached we might know what refname peels to without\n> + * As the refs are cached we might know what refname peels to without\n>   * the need to parse the object via parse_object(). peel_ref() might be a\n>   * more efficient alternative to obtain the pointee.\n>   */\n\nGreat, thanks for cleaning up this comment.\n\n> @@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n>  \t\t\t\t\t       const struct object_id *oid,\n>  \t\t\t\t\t       const char *refname)\n>  {\n> -\tconst struct object_id *tagged_oid = NULL;\n>  \tstruct object *obj;\n>  \n>  \tif (oid_array_lookup(points_at, oid) >= 0)\n>  \t\treturn oid;\n>  \tobj = parse_object(the_repository, oid);\n> +\twhile (obj && obj->type == OBJ_TAG) {\n> +\t\toid = get_tagged_oid((struct tag *)obj);\n> +\t\tif (oid_array_lookup(points_at, oid) >= 0)\n> +\t\t\treturn oid;\n> +\t\tobj = parse_object(the_repository, oid);\n> +\t}\n\nOK, so we are doing the usual peeling loop here. I wondered if we might\nbe able to use peel_object(), but it again suffers from the \"peel all\nthe way\" syndrome. So we have to loop ourselves so that we can check at\neach level. Good.\n\n>  \tif (!obj)\n>  \t\tdie(_(\"malformed object at '%s'\"), refname);\n\nThis will now trigger if refname points to a broken object, or if its\ntag does. I think the resulting message is OK in either case (and\npresumably lower level code would produce extra error messages, too).\n\n> -\tif (obj->type == OBJ_TAG)\n> -\t\ttagged_oid = get_tagged_oid((struct tag *)obj);\n> -\tif (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)\n> -\t\treturn tagged_oid;\n\nThis code is moved into the loop body, but your version there drops the\n\"if (tagged_oid)\" check. I think that is OK (and even preferable),\nthough. In get_tagged_oid() we will die() if the tagged object is NULL\n(though even before switching to that function this check was\nquestionable, because it is \"tag->tagged\" that may be NULL, and we were\ndereferencing that unconditionally).\n\nSo the code looks good.\n\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 1ce5f490e9..af223e44d6 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -45,6 +45,8 @@ test_expect_success 'check signed tags with --points-at' '\n>  \tsed -e \"s/Z$//\" >expect <<-\\EOF &&\n>  \trefs/heads/side Z\n>  \trefs/tags/annotated-tag four\n> +\trefs/tags/doubly-annotated-tag An annotated tag\n> +\trefs/tags/doubly-signed-tag A signed tag\n>  \trefs/tags/four Z\n>  \trefs/tags/signed-tag four\n>  \tEOF\n\nAnd the test looks good, too. It is nice that we can rely on the\nexisting setup for the doubly-* tags.\n\nThanks for an easy-to-review patch. :)\n\n-Peff\n"},{"id":"479096","messageId":"df912fc4-601b-4d43-c6e4-aa5f74ec031f@web.de","threadId":"59941","inReplyTo":"20230702125611.GA1036686@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-07-02T16:25:13Z","receivedAt":"2023-07-02T16:25:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.07.23 um 14:56 schrieb Jeff King:\n> On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:\n>\n>> @@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n>>  \t\t\t\t\t       const struct object_id *oid,\n>>  \t\t\t\t\t       const char *refname)\n>>  {\n>> -\tconst struct object_id *tagged_oid = NULL;\n>>  \tstruct object *obj;\n>>\n>>  \tif (oid_array_lookup(points_at, oid) >= 0)\n>>  \t\treturn oid;\n>>  \tobj = parse_object(the_repository, oid);\n>> +\twhile (obj && obj->type == OBJ_TAG) {\n>> +\t\toid = get_tagged_oid((struct tag *)obj);\n>> +\t\tif (oid_array_lookup(points_at, oid) >= 0)\n>> +\t\t\treturn oid;\n>> +\t\tobj = parse_object(the_repository, oid);\n>> +\t}\n>\n> OK, so we are doing the usual peeling loop here. I wondered if we might\n> be able to use peel_object(), but it again suffers from the \"peel all\n> the way\" syndrome. So we have to loop ourselves so that we can check at\n> each level. Good.\n>\n>>  \tif (!obj)\n>>  \t\tdie(_(\"malformed object at '%s'\"), refname);\n>\n> This will now trigger if refname points to a broken object, or if its\n> tag does. I think the resulting message is OK in either case (and\n> presumably lower level code would produce extra error messages, too).\n>\n>> -\tif (obj->type == OBJ_TAG)\n>> -\t\ttagged_oid = get_tagged_oid((struct tag *)obj);\n>> -\tif (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)\n>> -\t\treturn tagged_oid;\n>\n> This code is moved into the loop body, but your version there drops the\n> \"if (tagged_oid)\" check. I think that is OK (and even preferable),\n> though. In get_tagged_oid() we will die() if the tagged object is NULL\n> (though even before switching to that function this check was\n> questionable, because it is \"tag->tagged\" that may be NULL, and we were\n> dereferencing that unconditionally).\n\nThe check is necessary in the current code because tagged_oid is NULL if\nobj is not a tag.  The new code no longer needs it.\n\n> So the code looks good.\n\nI agree.\n\nRené\n"},{"id":"479100","messageId":"20230702202722.GA1052071@coredump.intra.peff.net","threadId":"59941","inReplyTo":"df912fc4-601b-4d43-c6e4-aa5f74ec031f@web.de","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T20:27:22Z","receivedAt":"2023-07-02T20:27:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 02, 2023 at 06:25:13PM +0200, René Scharfe wrote:\n\n> >> -\tif (obj->type == OBJ_TAG)\n> >> -\t\ttagged_oid = get_tagged_oid((struct tag *)obj);\n> >> -\tif (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)\n> >> -\t\treturn tagged_oid;\n> >\n> > This code is moved into the loop body, but your version there drops the\n> > \"if (tagged_oid)\" check. I think that is OK (and even preferable),\n> > though. In get_tagged_oid() we will die() if the tagged object is NULL\n> > (though even before switching to that function this check was\n> > questionable, because it is \"tag->tagged\" that may be NULL, and we were\n> > dereferencing that unconditionally).\n> \n> The check is necessary in the current code because tagged_oid is NULL if\n> obj is not a tag.  The new code no longer needs it.\n\nOh, right. Probably:\n\n\tif (obj->type == OBJ_TAG) {\n\t\tconst struct object_id *tagged_oid = get_tagged_oid((struct tag *)obj);\n\t\tif (oid_array_lookup(points_at, tagged_oid) >= 0)\n\t\t\treturn tagged_oid;\n\t}\n\nwould have been a better way to write the original, but the new one with\nthe loop is better still. ;)\n\nI also notice that the function returns a pointer to an oid, even though\nthe sole caller only cares about a boolean result. Not that big a deal,\nthough the memory lifetime of the return value is confusing.  We might\nreturn \"tagged_oid\" which points to a struct that will live forever, but\nwe might also return \"oid\" directly, which points to memory passed in\nfrom the caller with a limited lifetime.\n\n-Peff\n"},{"id":"479101","messageId":"20230702220243.GA1534980@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230702125611.GA1036686@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T22:02:43Z","receivedAt":"2023-07-02T22:02:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 02, 2023 at 08:56:11AM -0400, Jeff King wrote:\n\n> My biggest question would be whether this introduces any performance\n> penalty for the more common cases (lightweight tags and single-level\n> annotated tags). The answer is \"no\", I think; we are already paying the\n> cost to parse every object to find out if it's a tag, and your new loop\n> only does an extra parse if we see a tag-of-tag. Good.\n\nReading more carefully, I think this does actually change the\nperformance a bit, because we end up parsing the pointed-to commits, as\nwell. So here's before and after your patch running \"git for-each-ref\n--points-at=HEAD\" on linux.git (785 refs, all but 3 are tags):\n\n  Benchmark 1: ./git.old for-each-ref --points-at=HEAD\n    Time (mean ± σ):      11.4 ms ±   0.2 ms    [User: 6.5 ms, System: 4.9 ms]\n    Range (min … max):    11.0 ms …  12.3 ms    239 runs\n  \n  Benchmark 2: ./git.new for-each-ref --points-at=HEAD\n    Time (mean ± σ):      20.6 ms ±   0.5 ms    [User: 10.4 ms, System: 10.2 ms]\n    Range (min … max):    19.8 ms …  22.7 ms    133 runs\n  \n  Summary\n    './git.old for-each-ref --points-at=HEAD' ran\n      1.80 ± 0.06 times faster than './git.new for-each-ref --points-at=HEAD'\n\nThe absolute numbers are pretty small, but the percent change isn't\ngreat. I'll send some patches in a minute that can be applied on top to\nimprove this case, as well as fix the other issues I pointed out in the\nexisting code.\n\n-Peff\n"},{"id":"479102","messageId":"20230702223342.GA1598765@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230702220243.GA1534980@coredump.intra.peff.net","subject":"[PATCH 0/3] a few --points-at optimizations/cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T22:33:42Z","receivedAt":"2023-07-02T22:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 02, 2023 at 06:02:43PM -0400, Jeff King wrote:\n\n> I'll send some patches in a minute that can be applied on top to\n> improve this case, as well as fix the other issues I pointed out in the\n> existing code.\n\nHere they are, on top of your patch.\n\n  [1/3]: ref-filter: avoid parsing tagged objects in match_points_at()\n  [2/3]: ref-filter: avoid parsing non-tags in match_points_at()\n  [3/3]: ref-filter: simplify return type of match_points_at\n\n ref-filter.c | 32 +++++++++++++++++++++-----------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"479103","messageId":"20230702223540.GA3494279@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230702223342.GA1598765@coredump.intra.peff.net","subject":"[PATCH 1/3] ref-filter: avoid parsing tagged objects in match_points_at()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T22:35:40Z","receivedAt":"2023-07-02T22:35:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we peel tags to check if they match a --points-at oid, we\nrecursively parse the tagged object to see if it is also a tag. But\nsince the tag itself tells us the type of the object it points to (and\neven gives us the appropriate object struct via its \"tagged\" member), we\ncan use that directly.\n\nWe do still have to make sure to call parse_tag() before looking at each\ntag. This is redundant for the outermost tag (since we did call\nparse_object() to find its type), but that's OK; parse_tag() is smart\nenough to make this a noop when the tag has already been parsed.\n\nIn my clone of linux.git, with 782 tags (and only 3 non-tags), this\nyields a significant speedup (bringing us back where we were before the\ncommit before this one started recursively dereferencing tags):\n\n  Benchmark 1: ./git.old for-each-ref --points-at=HEAD --format=\"%(refname)\"\n    Time (mean ± σ):      20.3 ms ±   0.5 ms    [User: 11.1 ms, System: 9.1 ms]\n    Range (min … max):    19.6 ms …  21.5 ms    141 runs\n\n  Benchmark 2: ./git.new for-each-ref --points-at=HEAD --format=\"%(refname)\"\n    Time (mean ± σ):      11.4 ms ±   0.2 ms    [User: 6.3 ms, System: 5.0 ms]\n    Range (min … max):    11.0 ms …  12.2 ms    250 runs\n\n  Summary\n    './git.new for-each-ref --points-at=HEAD --format=\"%(refname)\"' ran\n      1.79 ± 0.05 times faster than './git.old for-each-ref --points-at=HEAD --format=\"%(refname)\"'\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis could optionally be squashed into the original. If we leave it as\nits own patch, it might be worth replacing \"the commit before this one\"\nwith the actual oid (once Junio has picked it up and it's stable).\n\n ref-filter.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ad7f244414..e091f056ab 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2225,10 +2225,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n \t\treturn oid;\n \tobj = parse_object(the_repository, oid);\n \twhile (obj && obj->type == OBJ_TAG) {\n-\t\toid = get_tagged_oid((struct tag *)obj);\n+\t\tstruct tag *tag = (struct tag *)obj;\n+\n+\t\tif (parse_tag(tag) < 0) {\n+\t\t\tobj = NULL;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\toid = get_tagged_oid(tag);\n \t\tif (oid_array_lookup(points_at, oid) >= 0)\n \t\t\treturn oid;\n-\t\tobj = parse_object(the_repository, oid);\n+\n+\t\tobj = tag->tagged;\n \t}\n \tif (!obj)\n \t\tdie(_(\"malformed object at '%s'\"), refname);\n-- \n2.41.0.586.g3c0cc15bc7\n\n"},{"id":"479104","messageId":"20230702223747.GB3494279@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230702223342.GA1598765@coredump.intra.peff.net","subject":"[PATCH 2/3] ref-filter: avoid parsing non-tags in match_points_at()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T22:37:47Z","receivedAt":"2023-07-02T22:37:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When handling --points-at, we have to try to peel each ref to see if\nit's a tag that points at a requested oid. We start this process by\ncalling parse_object() on the oid pointed to by each ref.\n\nThe cost of parsing each object adds up, especially in an output that\ndoesn't otherwise need to open the objects at all. Ideally we'd use\npeel_iterated_oid() here, which uses the cached information in the\npacked-refs file. But we can't, because our --points-at must match not\nonly the fully peeled value, but any interim values (so if tag A points\nto tag B which points to commit C, we should match --points-at=B, but\npeel_iterated_oid() will only tell us about C).\n\nSo the best we can do (absent changes to the packed-refs peel traits) is\nto avoid parsing non-tags. The obvious way to do that is to call\noid_object_info() to check the type before parsing. But there are a few\ngotchas there, like checking if the object has already been parsed.\n\nInstead we can just tell parse_object() that we are OK skipping the hash\ncheck, which lets it turn on several optimizations. Commits can be\nloaded via the commit graph (so it's both fast and we have the benefit\nof the parsed data if we need it later at the output stage). Blobs are\nnot loaded at all. Trees are still loaded, but it's rather rare to have\na ref point directly to a tree (and since this is just an optimization,\nkicking in 99% of the time is OK).\n\nEven though we're paying for an extra lookup, the cost to avoid parsing\nthe non-tags is a net benefit. In my git.git repository with 941 tags\nand 1440 other refs pointing to commits, this significantly cuts the\nruntime:\n\n  Benchmark 1: ./git.old for-each-ref --points-at=HEAD\n    Time (mean ± σ):      26.8 ms ±   0.5 ms    [User: 24.5 ms, System: 2.2 ms]\n    Range (min … max):    25.9 ms …  29.2 ms    107 runs\n\n  Benchmark 2: ./git.new for-each-ref --points-at=HEAD\n    Time (mean ± σ):       9.1 ms ±   0.3 ms    [User: 6.8 ms, System: 2.2 ms]\n    Range (min … max):     8.6 ms …  10.2 ms    308 runs\n\n  Summary\n    './git.new for-each-ref --points-at=HEAD' ran\n      2.96 ± 0.10 times faster than './git.old for-each-ref --points-at=HEAD'\n\nIn a repository that is mostly annotated tags, we'd expect less\nimprovement (we might still skip a few object loads, but that's balanced\nby the extra lookups). In my clone of linux.git, which has 782 tags and\n3 branches, the run-time is about the same (it's actually ~1% faster on\naverage after this patch, but that's within the run-to-run noise).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e091f056ab..f5ac486430 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2223,7 +2223,8 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n \n \tif (oid_array_lookup(points_at, oid) >= 0)\n \t\treturn oid;\n-\tobj = parse_object(the_repository, oid);\n+\tobj = parse_object_with_flags(the_repository, oid,\n+\t\t\t\t      PARSE_OBJECT_SKIP_HASH_CHECK);\n \twhile (obj && obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *)obj;\n \n-- \n2.41.0.586.g3c0cc15bc7\n\n"},{"id":"479105","messageId":"20230702223829.GC3494279@coredump.intra.peff.net","threadId":"59941","inReplyTo":"20230702223342.GA1598765@coredump.intra.peff.net","subject":"[PATCH 3/3] ref-filter: simplify return type of match_points_at","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-02T22:38:29Z","receivedAt":"2023-07-02T22:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We return the oid that matched, but the sole caller only cares whether\nwe matched anything at all. This is mostly academic, since there's only\none caller, but the lifetime of the returned pointer is not immediately\nclear. Sometimes it points to an oid in a tag struct, which should live\nforever. And sometimes to the oid passed in, which only lives as long as\nthe each_ref_fn callback we're called from.\n\nSimplify this to a boolean return which is more direct and obvious. As a\nbonus, this lets us avoid the weird pattern of overwriting our \"oid\"\nparameter in the loop (since we now only refer to the tagged oid one\ntime, and can just inline the call to get it).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f5ac486430..0629435f08 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2209,20 +2209,22 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,\n /*\n  * Given a ref (oid, refname), check if the ref belongs to the array\n  * of oids. If the given ref is a tag, check if the given tag points\n- * at one of the oids in the given oid array.\n+ * at one of the oids in the given oid array. Returns non-zero if a\n+ * match is found.\n+ *\n  * NEEDSWORK:\n  * As the refs are cached we might know what refname peels to without\n  * the need to parse the object via parse_object(). peel_ref() might be a\n  * more efficient alternative to obtain the pointee.\n  */\n-static const struct object_id *match_points_at(struct oid_array *points_at,\n-\t\t\t\t\t       const struct object_id *oid,\n-\t\t\t\t\t       const char *refname)\n+static int match_points_at(struct oid_array *points_at,\n+\t\t\t   const struct object_id *oid,\n+\t\t\t   const char *refname)\n {\n \tstruct object *obj;\n \n \tif (oid_array_lookup(points_at, oid) >= 0)\n-\t\treturn oid;\n+\t\treturn 1;\n \tobj = parse_object_with_flags(the_repository, oid,\n \t\t\t\t      PARSE_OBJECT_SKIP_HASH_CHECK);\n \twhile (obj && obj->type == OBJ_TAG) {\n@@ -2233,15 +2235,14 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n \t\t\tbreak;\n \t\t}\n \n-\t\toid = get_tagged_oid(tag);\n-\t\tif (oid_array_lookup(points_at, oid) >= 0)\n-\t\t\treturn oid;\n+\t\tif (oid_array_lookup(points_at, get_tagged_oid(tag)) >= 0)\n+\t\t\treturn 1;\n \n \t\tobj = tag->tagged;\n \t}\n \tif (!obj)\n \t\tdie(_(\"malformed object at '%s'\"), refname);\n-\treturn NULL;\n+\treturn 0;\n }\n \n /*\n-- \n2.41.0.586.g3c0cc15bc7\n"},{"id":"479164","messageId":"ZKMun/sl9rS/2gQC@thinkpad.fritz.box","threadId":"59941","inReplyTo":"20230702220243.GA1534980@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jan Klötzke","fromEmail":"jan@kloetzke.net","sentAt":"2023-07-03T20:25:03Z","receivedAt":"2023-07-03T20:25:17Z","isPatch":true,"sender":{"key":"jan@kloetzke.net","avatar":"https://avatars.githubusercontent.com/u/234300?v=4"},"body":"Am Sun, Jul 02, 2023 at 06:02:43PM -0400 schrieb Jeff King:\n> On Sun, Jul 02, 2023 at 08:56:11AM -0400, Jeff King wrote:\n> \n> > My biggest question would be whether this introduces any performance\n> > penalty for the more common cases (lightweight tags and single-level\n> > annotated tags). The answer is \"no\", I think; we are already paying the\n> > cost to parse every object to find out if it's a tag, and your new loop\n> > only does an extra parse if we see a tag-of-tag. Good.\n> \n> Reading more carefully, I think this does actually change the\n> performance a bit, because we end up parsing the pointed-to commits, as\n> well. So here's before and after your patch running \"git for-each-ref\n> --points-at=HEAD\" on linux.git (785 refs, all but 3 are tags):\n> \n>   Benchmark 1: ./git.old for-each-ref --points-at=HEAD\n>     Time (mean ± σ):      11.4 ms ±   0.2 ms    [User: 6.5 ms, System: 4.9 ms]\n>     Range (min … max):    11.0 ms …  12.3 ms    239 runs\n>   \n>   Benchmark 2: ./git.new for-each-ref --points-at=HEAD\n>     Time (mean ± σ):      20.6 ms ±   0.5 ms    [User: 10.4 ms, System: 10.2 ms]\n>     Range (min … max):    19.8 ms …  22.7 ms    133 runs\n>   \n>   Summary\n>     './git.old for-each-ref --points-at=HEAD' ran\n>       1.80 ± 0.06 times faster than './git.new for-each-ref --points-at=HEAD'\n> \n> The absolute numbers are pretty small, but the percent change isn't\n> great. I'll send some patches in a minute that can be applied on top to\n> improve this case, as well as fix the other issues I pointed out in the\n> existing code.\n\nI have to admit I was not entirely sure about the performance\nimplications. The relative performance drop is indeed substantial.\nThanks for the thorough review and swiftly taking care of these!\n\n-- Jan\n"},{"id":"479191","messageId":"xmqqmt0a3n2w.fsf@gitster.g","threadId":"59941","inReplyTo":"20230702125611.GA1036686@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T06:10:47Z","receivedAt":"2023-07-05T06:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> That seems reasonable to me. It is changing the definition of\n> --points-at slightly, but I think in a way that should be less\n> surprising to users (i.e., we can consider the old behavior a bug).\n\nOK.\n\n> The\n> existing documentation is sufficiently vague about \"points\" that I don't\n> think it needs to be updated (though arguably we could improve that\n> here, too).\n\nTrue, too.\n\nHaving said that, as we lack a primitive exposed to the user script\nto only peel one level of a tag, it makes me wonder how much of this\nis a practical issue.  Is a tag that points at another tag mere\ncuriosity and subject of mental gymnastics, or is it a useful\nconstruct with real world use case?  If the latter, isn't it more\nlike \"a tag to a tag _could_ be made useful if we also supported X\nand Y and Z\" where X could be \"There should be a syntax like\nA^{commit} that lets us peel only one level of tag\"?\n\n> Note that most other tag-peeling in Git (like the peeled values returned\n> by upload-pack) errs in the opposite direction: they peel completely,\n> and don't show the intermediate values.\n\nExactly.  While allowing to feed the intermediate level as the\nargument to \"points at\" does sound like a good idea on surface, I am\nskeptical about the practical value it brings---if we cannot do this\nwithout any additional overhead (of course if there are tags to tags\nin the repository, I am perfectly fine to pay the cost of\ndereferencing the chain, but I am more worried about regressing the\nperformance in repositories without tags to tags), I am not sure if\nit is worth it.\n\n> My biggest question would be whether this introduces any performance\n> penalty for the more common cases (lightweight tags and single-level\n> annotated tags). The answer is \"no\", I think; we are already paying the\n> cost to parse every object to find out if it's a tag, and your new loop\n> only does an extra parse if we see a tag-of-tag. Good.\n\nOK.\n\n>   Let me go off on a tangent here, since I'm looking at the performance\n>   of this function. The current code is already rather pessimal here, as\n>   we could probably avoid parsing non-tags entirely. Some strategies\n>   there are:\n>\n>     1. We could check the object type via oid_object_info() before\n>        parsing. This carries a small penalty (two lookups) for tags but\n>        a big win (avoiding loading the object contents) for non-tags.\n>\n>        An easy way to do this is to replace the parse_object() with\n>        parse_object_with_flags(PARSE_OBJECT_SKIP_HASH_CHECK), which\n>        tries to avoid loading object contents (especially using the\n>        commit-graph for commits, which presumably covers most non-tag\n>        refs).\n>\n>     2. We could be using the peel_iterated_oid() interface (this is the\n>        peel_ref() thing mentioned in the comment you touched, but it has\n>        since been renamed). But it does the \"peel all the way\" thing\n>        mentioned above (both because of its interface, but also because\n>        that's what the packed-refs peel lines store).\n>\n>        So to do that we'd either have to enhance the packed-refs store\n>        (which would not be too hard to do in a backwards-compatible\n>        way), or switch --points-at to only match either the direct ref\n>        value or the fully-peeled value.\n>\n>   I don't think either of those is something your patch needs to deal\n>   with. It is not making these kinds of optimizations any harder (it is\n>   the existing \"peel only once\" behavior that does so). I mostly wanted\n>   to get it written down while we are all looking at this function.\n\nMy preference would be to see these optimization done first and then\nadd this new loop on top of it.  That way, we can measure more\neasily what kind of additional overhead, if any, we are paying by\nadding the loop.\n\nThanks.\n"},{"id":"479197","messageId":"20230705124107.GA3932508@coredump.intra.peff.net","threadId":"59941","inReplyTo":"xmqqmt0a3n2w.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-07-05T12:41:07Z","receivedAt":"2023-07-05T12:41:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 04, 2023 at 11:10:47PM -0700, Junio C Hamano wrote:\n\n> My preference would be to see these optimization done first and then\n> add this new loop on top of it.  That way, we can measure more\n> easily what kind of additional overhead, if any, we are paying by\n> adding the loop.\n\nI ended up doing them on top, rather than before, but I think the size\nof the impact can easily be seen.\n\nThe one thing that would actually make us a lot faster (by using the\npacked-refs peels) is to make full peels the only option, and do not\nbother letting --points-at match \"B\" in an A->B->C peel. But that would\nbe removing something that is currently matched (even before the patch\nin this thread), so I stopped short of it in my optimizations. But even\nif we decide to do that, Jan's patch is not making anything worse there\n(in fact, it is making it better, because it is matching \"C\" which we do\nnot currently match).\n\nSo I'd be inclined to proceed with the patches I sent earlier, and then\nif we choose to later refactor again to drop \"B\", we can.\n\nOf course if somebody wants to do that refactor _now_ instead of the\npatches already posted, I'm OK with that. It just won't be me due to\ntime constraints.\n\n-Peff\n"},{"id":"479208","messageId":"xmqqedlm2s9q.fsf@gitster.g","threadId":"59941","inReplyTo":"20230705124107.GA3932508@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T17:16:17Z","receivedAt":"2023-07-05T17:16:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 04, 2023 at 11:10:47PM -0700, Junio C Hamano wrote:\n>\n>> My preference would be to see these optimization done first and then\n>> add this new loop on top of it.  That way, we can measure more\n>> easily what kind of additional overhead, if any, we are paying by\n>> adding the loop.\n>\n> I ended up doing them on top, rather than before, but I think the size\n> of the impact can easily be seen.\n\nAh, I should have first read (or at least skimmed) all messages\nbefore responding.  Thanks for following through.\n\n> The one thing that would actually make us a lot faster (by using the\n> packed-refs peels) is to make full peels the only option, and do not\n> bother letting --points-at match \"B\" in an A->B->C peel. But that would\n> be removing something that is currently matched (even before the patch\n> in this thread), so I stopped short of it in my optimizations.\n\nInteresting.  Right now, if I create a 'direct' tag that points\ndirectly at HEAD, and then create an 'indirect' tag that points at\n'direct', i.e.\n\n    $ git tag -a -m 'a direct tag to HEAD' direct HEAD\n    $ git tag -a -m 'an indirect tag' indirect direct\n\nI would get a piece of advice message that encourages to correct the\nmistake with \"git tag -f indirect direct^{}\".  Then I ask for tags\nthat point at HEAD, I see only 'direct' and not 'indirect'.  Your\noptimization would start showing both 'direct' and 'indirect' if\nthey are packed.  But you are correct to worry about the opposite\ncase.  If I ask for tags that point at 'direct', I currently see\n'indirect', but of course 'indirect' will not appear as the peeled\nvalue of any ref, and the optimized version will stop saying that\n'indirect' is a ref that points at 'direct'.  That sounds like a\nregression.\n\n> But even\n> if we decide to do that, Jan's patch is not making anything worse there\n> (in fact, it is making it better, because it is matching \"C\" which we do\n> not currently match).\n> ...\n> So I'd be inclined to proceed with the patches I sent earlier, and then\n> if we choose to later refactor again to drop \"B\", we can.\n\nWe generally avoid taking away anything once we give it to users;\nonce the patch under discussion goes in, there is no taking it back,\ni.e. the new _behaviour_ closes the door to certain optimizations.\n\nI do not at all mind to see us decide and declare that it is a good\nthing to say that not just 'direct' but also 'indirect' points at\nHEAD, and that 'indirect' points at 'direct' and the patch under\ndiscussion makes the world a etter place, and we will not regret\nthat decision.  But the time to make such a decision is now, before\nwe give a go-ahead to the patch.\n\nThanks.\n"},{"id":"479228","messageId":"ZKW7bjVHeEOnza62@thinkpad.fritz.box","threadId":"59941","inReplyTo":"xmqqedlm2s9q.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Jan Klötzke","fromEmail":"jan@kloetzke.net","sentAt":"2023-07-05T18:50:22Z","receivedAt":"2023-07-05T18:50:31Z","isPatch":true,"sender":{"key":"jan@kloetzke.net","avatar":"https://avatars.githubusercontent.com/u/234300?v=4"},"body":"Am Wed, Jul 05, 2023 at 10:16:17AM -0700 schrieb Junio C Hamano:\n> > The one thing that would actually make us a lot faster (by using the\n> > packed-refs peels) is to make full peels the only option, and do not\n> > bother letting --points-at match \"B\" in an A->B->C peel. But that would\n> > be removing something that is currently matched (even before the patch\n> > in this thread), so I stopped short of it in my optimizations.\n> \n> Interesting.  Right now, if I create a 'direct' tag that points\n> directly at HEAD, and then create an 'indirect' tag that points at\n> 'direct', i.e.\n> \n>     $ git tag -a -m 'a direct tag to HEAD' direct HEAD\n>     $ git tag -a -m 'an indirect tag' indirect direct\n> \n> I would get a piece of advice message that encourages to correct the\n> mistake with \"git tag -f indirect direct^{}\".  Then I ask for tags\n> that point at HEAD, I see only 'direct' and not 'indirect'.  Your\n> optimization would start showing both 'direct' and 'indirect' if\n> they are packed.  But you are correct to worry about the opposite\n> case.  If I ask for tags that point at 'direct', I currently see\n> 'indirect', but of course 'indirect' will not appear as the peeled\n> value of any ref, and the optimized version will stop saying that\n> 'indirect' is a ref that points at 'direct'.  That sounds like a\n> regression.\n\nI agree. I purposely let the loop run through all the tags in the chain\nin my inital patch to keep the existing behaviour. Unless there is a\ncompelling reason I would suggest do keep it this way. I wouldn't rule\nout that someone relies on being able to query for indirect tags by\nsupplying a tag to the --points-at option.\n\n> > But even\n> > if we decide to do that, Jan's patch is not making anything worse there\n> > (in fact, it is making it better, because it is matching \"C\" which we do\n> > not currently match).\n> > ...\n> > So I'd be inclined to proceed with the patches I sent earlier, and then\n> > if we choose to later refactor again to drop \"B\", we can.\n> \n> We generally avoid taking away anything once we give it to users;\n> once the patch under discussion goes in, there is no taking it back,\n> i.e. the new _behaviour_ closes the door to certain optimizations.\n> \n> I do not at all mind to see us decide and declare that it is a good\n> thing to say that not just 'direct' but also 'indirect' points at\n> HEAD, and that 'indirect' points at 'direct' and the patch under\n> discussion makes the world a etter place, and we will not regret\n> that decision.  But the time to make such a decision is now, before\n> we give a go-ahead to the patch.\n\nThe reasoning why I came up with the patch in the first place was an odd\nbehaviour that was reported to me in [1]. The user had a recipe that\nchecked out a nested tag for a package but the Bob Build Tool thought\nthe user messed with the working copy. Behind the scenes Bob checked out\nthe indirect tag (foobar-3.13.1) successfully. But when the user\nexamined the project state (\"bob status\"), Bob ran\n\n  git tag --points-at HEAD\n\nin the working copy to get the list of tags for the current HEAD. That\ndid not return the expected tag in the list, so the workspace state was\nflagged. OTOH, the tag _is_ visible when manually executing \"git log\" in\nthe workspace. Clearly this is confusing and so was I when I tried to\nfind the root cause. To sum up:\n\n $ git checkout nested-tag   --> works\n $ git log                   --> shows nested tag in decoration\n $ git tag --points-at HEAD  --> does *not* show nested tag\n\nAFAICT the nested tag in the mentioned bug report was not created on\npurpose. I haven't come across any sensible use case for them either.\nBut as most git commands can handle nested tags they should better be\nsupported consistently IMHO.\n\n-- Jan\n\n[1] https://github.com/BobBuildTool/bob/issues/520\n"},{"id":"479242","messageId":"xmqq8rbuyv14.fsf@gitster.g","threadId":"59941","inReplyTo":"ZKW7bjVHeEOnza62@thinkpad.fritz.box","subject":"Re: [PATCH] ref-filter: handle nested tags in --points-at option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T20:15:35Z","receivedAt":"2023-07-05T20:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Klötzke <jan@kloetzke.net> writes:\n\n> Am Wed, Jul 05, 2023 at 10:16:17AM -0700 schrieb Junio C Hamano:\n>> Interesting.  Right now, if I create a 'direct' tag that points\n>> directly at HEAD, and then create an 'indirect' tag that points at\n>> 'direct', i.e.\n>> \n>>     $ git tag -a -m 'a direct tag to HEAD' direct HEAD\n>>     $ git tag -a -m 'an indirect tag' indirect direct\n>> \n>> I would get a piece of advice message that encourages to correct the\n>> mistake with \"git tag -f indirect direct^{}\".\n> ...\n> But as most git commands can handle nested tags they should better be\n> supported consistently IMHO.\n\nDo they?  Most git commands handle nested tags in only one way: by\nfully peeling.  \"git checkout --detach indirect\" in the above\nscenario would handle nested tag \"indirect\" well, but it is done by\nmaking \"direct\" tag inaccessible when the only thing you have is the\n\"indirect\" tag.  For example, you cannot create another \"indirect\"\ntag that points at \"direct\" tag with \"git tag\", with\n\n    $ git tag -a -m 'another indirect' indirect-2 indirect^{}\n\nThe resulting tag will be another direct tag to the underlying\ncommit, and not a tag of the \"direct\" tag.\n\nIn that sense, --points-at we currently have that only peels once is\ninconsistent with the others, but --points-at that peels repeatedly\nand allows the intermediate steps to match is also behaving\ninconsistently relative to most git commands.\n\nCombined with the fact that we seem to discourage such an indirect\ntag, we should either:\n\n (1) declare that indirect tags are not useful, turn the warning\n     advice.nestedTag into a stronger error, devise appropriate\n     transition plan to get rid of nested tag (e.g. eventually\n     making it impossible to use \"git tag\" to create such a tag and\n     let \"git fsck\" complain about them), and perhaps change\n     \"--points-at\" to take only the fully peeled object into account\n     so that optimization based on packed-refs becomes possible.  Or\n\n (2) declare that indirect tags are useful thing to support, tone\n     down the advice.nestedTag message, and enhance the support of\n     indirect tags, starting with this \"--points-at\" enhancement.\n\nI am inclined to support (2), but then a consistent support would\nneed to eventually include a \"peel only a single level\" primitive as\nwell.  That would be the first step to allow \"most git commands\" to\nsupport nested tags well, as they currently do not.\n\nThanks for working on this.  Let's queue it, together will Peff's\npatches (which I haven't studied fully yet).\n\n"}]}