{"thread":{"id":"56441","subject":"[hacky PATCH 0/2] speeding up trivial for-each-ref invocations","startedAt":"2021-09-04T12:40:38Z","lastAt":"2021-09-20T07:42:39Z","messageCount":26,"participants":["Jeff King","ZheNing Hu","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"434690","messageId":"YTNpQ7Od1U/5i0R7@coredump.intra.peff.net","threadId":"56441","inReplyTo":null,"subject":"[hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-04T12:40:35Z","receivedAt":"2021-09-04T12:40:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Somebody complained to me off-list recently that for-each-ref was slow\nwhen doing a trivial listing (like refname + objectname) of a large\nnumber of refs, even though you could get mostly the same output by\njust dumping the packed-refs file.\n\nSo I was nerd-sniped into making this horrible series, which does speed\nup some cases. It was a combination of \"how fast can we easily get it\",\nplus I was curious _which_ parts of for-each-ref were actually slow.\n\nIn this version there are 2 patches, tested against 'git for-each-ref\n--format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\nrefs:\n\n - directly outputting items rather than building up a ref_array yields\n about a 1.5x speedup\n\n - avoiding the whole atom-formatting system yields a 2.5x speedup on\n top of that\n\nI had originally written it against a slightly older version of Git,\nbefore show_ref_array_item() was inlined. In that version, there was a\nmiddle ground where we still created a ref_array_item for each ref, and\nthen fed it to the \"quick\" formatter (so we could see the cost of\nhaving to malloc/free a bunch of ref_array_item structs). The numbers\nthere wre:\n\n - streaming out items gave a 1.5x speedup\n\n - using the \"quick\" formatter gave a 1.8x speedup\n\n - avoiding the extra malloc/free for each item gave a 1.4x\n\nwhich makes sense; 1.8 * 1.4 is ~2.5, so it's the same speedup just\nbroken down further.\n\nI'm not sure I'm really advocating that we should do something like\nthis, though it is tempting because it does make some common queries\nmuch faster. But I wanted to share it here to give some insight on where\nthe time may be going in ref-filter / for-each-ref.\n\n [1/2]: ref-filter: hacky \"streaming\" mode\n [2/2]: ref-filter: implement \"quick\" formats\n\n builtin/for-each-ref.c | 7 +++\n ref-filter.c | 113 +++++++++++++++++++++++++++++++++++++----\n ref-filter.h | 21 ++++++++\n 3 files changed, 132 insertions(+), 9 deletions(-)\n\n-Peff\n"},{"id":"434691","messageId":"YTNpeH+jO0zQgAVT@coredump.intra.peff.net","threadId":"56441","inReplyTo":"YTNpQ7Od1U/5i0R7@coredump.intra.peff.net","subject":"[PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-04T12:41:28Z","receivedAt":"2021-09-04T12:41:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ref-filter code is very keen to collect all of the refs in an array\nbefore writing any output. This makes things slower than necessary when\nusing the default sort order (which is already sorted by refname when we\ncall for_each_ref()), and when no filtering options require it.\n\nThis commit adds a mildly-ugly interface to detect this case and stream\ndirectly from filter_refs(). The caller just needs to call the\n\"maybe_stream\" function. Either way, they can call the usual sort/print\nfunctions; they'll just be noops if we did stream instead of collecting.\n\nHere are some timings on a fully-packed 500k-ref repo:\n\n Benchmark #1: ./git.orig for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 340.2 ms ± 5.3 ms [User: 300.5 ms, System: 39.6 ms]\n Range (min … max): 332.9 ms … 347.0 ms 10 runs\n\n Benchmark #2: ./git.stream for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 224.0 ms ± 3.4 ms [User: 222.7 ms, System: 1.3 ms]\n Range (min … max): 218.1 ms … 228.5 ms 13 runs\n\n Summary\n './git.stream for-each-ref --format='%(objectname) %(refname)'' ran\n 1.52 ± 0.03 times faster than './git.orig for-each-ref --format='%(objectname) %(refname)''\n\nSo we definitely shave off some time, though we're still _much_ slower\nthan a simple `wc -l \n---\n builtin/for-each-ref.c | 7 ++++++\n ref-filter.c | 50 ++++++++++++++++++++++++++++++++++--------\n ref-filter.h | 8 +++++++\n 3 files changed, 56 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 89cb6307d4..fe0b92443f 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -70,6 +70,13 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n+\t/*\n+\t * try streaming, but only without maxcount; in theory the ref-filter\n+\t * code could learn about maxcount, but for now it is our own thing\n+\t */\n+\tif (!maxcount)\n+\t\tref_filter_maybe_stream(&filter, sorting, icase, &format);\n+\n \tif (!sorting)\n \t\tsorting = ref_default_sorting();\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93ce2a6ef2..17b78b1d30 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2283,15 +2283,19 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\t\treturn 0;\n \t}\n \n-\t/*\n-\t * We do not open the object yet; sort may only need refname\n-\t * to do its job and the resulting list may yet to be pruned\n-\t * by maxcount logic.\n-\t */\n-\tref = ref_array_push(ref_cbdata->array, refname, oid);\n-\tref->commit = commit;\n-\tref->flag = flag;\n-\tref->kind = kind;\n+\tif (ref_cbdata->filter->streaming_format) {\n+\t\tpretty_print_ref(refname, oid, ref_cbdata->filter->streaming_format);\n+\t} else {\n+\t\t/*\n+\t\t * We do not open the object yet; sort may only need refname\n+\t\t * to do its job and the resulting list may yet to be pruned\n+\t\t * by maxcount logic.\n+\t\t */\n+\t\tref = ref_array_push(ref_cbdata->array, refname, oid);\n+\t\tref->commit = commit;\n+\t\tref->flag = flag;\n+\t\tref->kind = kind;\n+\t}\n \n \treturn 0;\n }\n@@ -2563,6 +2567,34 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \tQSORT_S(array->items, array->nr, compare_refs, sorting);\n }\n \n+void ref_filter_maybe_stream(struct ref_filter *filter,\n+\t\t\t const struct ref_sorting *sort, int icase,\n+\t\t\t struct ref_format *format)\n+{\n+\t/* streaming only works with default for_each_ref sort order */\n+\tif (sort || icase)\n+\t\treturn;\n+\n+\t/* these filters want to see all candidates up front */\n+\tif (filter->reachable_from || filter->unreachable_from)\n+\t\treturn;\n+\n+\t/*\n+\t * the %(symref) placeholder is broken with pretty_print_ref(),\n+\t * which our streaming code uses. I suspect this is a sign of breakage\n+\t * in other callers like verify_tag(), which should be fixed. But for\n+\t * now just disable streaming.\n+\t *\n+\t * Note that this implies we've parsed the format already with\n+\t * verify_ref_format().\n+\t */\n+\tif (need_symref)\n+\t\treturn;\n+\n+\t/* OK to stream */\n+\tfilter->streaming_format = format;\n+}\n+\n static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n \tstruct strbuf *s = &state->stack->output;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex c15dee8d6b..ecea1837a2 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -69,6 +69,9 @@ struct ref_filter {\n \t\tlines;\n \tint abbrev,\n \t\tverbose;\n+\n+\t/* if non-NULL, streaming output during filter_refs() is enabled */\n+\tstruct ref_format *streaming_format;\n };\n \n struct ref_format {\n@@ -135,6 +138,11 @@ char *get_head_description(void);\n /* Set up translated strings in the output. */\n void setup_ref_filter_porcelain_msg(void);\n \n+/* enable streaming during filter_refs() if options allow it */\n+void ref_filter_maybe_stream(struct ref_filter *filter,\n+\t\t\t const struct ref_sorting *sort, int icase,\n+\t\t\t struct ref_format *format);\n+\n /*\n * Print a single ref, outside of any ref-filter. Note that the\n * name must be a fully qualified refname.\n-- \n2.33.0.618.g5b11852304\n\n"},{"id":"434692","messageId":"YTNps0YBOaRNvPzk@coredump.intra.peff.net","threadId":"56441","inReplyTo":"YTNpQ7Od1U/5i0R7@coredump.intra.peff.net","subject":"[PATCH 2/2] ref-filter: implement \"quick\" formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-04T12:42:27Z","receivedAt":"2021-09-04T12:42:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some commonly-used formats can be output _much_ faster than going\nthrough the usual atom-formatting code. E.g., \"%(objectname) %(refname)\"\ncan just be a simple printf. This commit detects a few easy cases and\nuses a hard-coded output function instead.\n\nNote two things about the implementation:\n\n - we could probably go further here. E.g., %(refname:lstrip) should be\n easy-ish to optimize, too. Likewise, mixed-text like \"delete\n %(refname)\" would be nice to have.\n\n - the code is repetitive and enumerates all the cases, and it feels\n like we ought to be able to generalize it more. But that's exactly\n what the current formatting tries to do!\n\nSo this whole thing is pretty horrible, and is a hack around the\nslowness of the whole used_atom system. It _should_ be possible to\nrefactor that system to have roughly the same cost, but this will serve\nin the meantime.\n\nHere are some numbers (\"stream\" is Git with the streaming optimization\nfrom the previous commit, and \"quick\" is this commit):\n\n Benchmark #1: ./git.stream for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 229.2 ms ± 6.6 ms [User: 228.3 ms, System: 0.9 ms]\n Range (min … max): 220.4 ms … 242.6 ms 13 runs\n\n Benchmark #2: ./git.quick for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 94.8 ms ± 2.2 ms [User: 93.5 ms, System: 1.4 ms]\n Range (min … max): 90.8 ms … 98.2 ms 32 runs\n\n Summary\n './git.quick for-each-ref --format='%(objectname) %(refname)'' ran\n 2.42 ± 0.09 times faster than './git.stream for-each-ref --format='%(objectname) %(refname)''\n\nSigned-off-by: Jeff King \n---\n ref-filter.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n ref-filter.h | 13 +++++++++++\n 2 files changed, 76 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 17b78b1d30..1efa3aadc8 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1009,6 +1009,37 @@ static int reject_atom(enum atom_type atom_type)\n \treturn atom_type == ATOM_REST;\n }\n \n+static void set_up_quick_format(struct ref_format *format)\n+{\n+\t/* quick formats don't handle any special quoting */\n+\tif (format->quote_style)\n+\t\treturn;\n+\n+\t/*\n+\t * no atoms at all; this should be uncommon in real life, but it may be\n+\t * interesting for benchmarking\n+\t */\n+\tif (!used_atom_cnt) {\n+\t\tformat->quick = REF_FORMAT_QUICK_VERBATIM;\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * It's tempting to look at used_atom here, but it's wrong to do so: we\n+\t * need not only to be sure of the needed atoms, but also their order\n+\t * and any verbatim parts of the format. So instead, let's just\n+\t * hard-code some specific formats.\n+\t */\n+\tif (!strcmp(format->format, \"%(refname)\"))\n+\t\tformat->quick = REF_FORMAT_QUICK_REFNAME;\n+\telse if (!strcmp(format->format, \"%(objectname)\"))\n+\t\tformat->quick = REF_FORMAT_QUICK_OBJECTNAME;\n+\telse if (!strcmp(format->format, \"%(refname) %(objectname)\"))\n+\t\tformat->quick = REF_FORMAT_QUICK_REFNAME_OBJECTNAME;\n+\telse if (!strcmp(format->format, \"%(objectname) %(refname)\"))\n+\t\tformat->quick = REF_FORMAT_QUICK_OBJECTNAME_REFNAME;\n+}\n+\n /*\n * Make sure the format string is well formed, and parse out\n * the used atoms.\n@@ -1047,6 +1078,9 @@ int verify_ref_format(struct ref_format *format)\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n+\n+\tset_up_quick_format(format);\n+\n \treturn 0;\n }\n \n@@ -2617,6 +2651,32 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n+static int quick_ref_format(const struct ref_format *format,\n+\t\t\t const char *refname,\n+\t\t\t const struct object_id *oid)\n+{\n+\tswitch(format->quick) {\n+\tcase REF_FORMAT_QUICK_NONE:\n+\t\treturn -1;\n+\tcase REF_FORMAT_QUICK_VERBATIM:\n+\t\tprintf(\"%s\\n\", format->format);\n+\t\treturn 0;\n+\tcase REF_FORMAT_QUICK_REFNAME:\n+\t\tprintf(\"%s\\n\", refname);\n+\t\treturn 0;\n+\tcase REF_FORMAT_QUICK_OBJECTNAME:\n+\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n+\t\treturn 0;\n+\tcase REF_FORMAT_QUICK_REFNAME_OBJECTNAME:\n+\t\tprintf(\"%s %s\\n\", refname, oid_to_hex(oid));\n+\t\treturn 0;\n+\tcase REF_FORMAT_QUICK_OBJECTNAME_REFNAME:\n+\t\tprintf(\"%s %s\\n\", oid_to_hex(oid), refname);\n+\t\treturn 0;\n+\t}\n+\tBUG(\"unknown ref_format_quick value: %d\", format->quick);\n+}\n+\n int format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct ref_format *format,\n \t\t\t struct strbuf *final_buf,\n@@ -2670,6 +2730,9 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n \tstruct strbuf output = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \n+\tif (!quick_ref_format(format, name, oid))\n+\t\treturn;\n+\n \tref_item = new_ref_array_item(name, oid);\n \tref_item->kind = ref_kind_from_refname(name);\n \tif (format_ref_array_item(ref_item, format, &output, &err))\ndiff --git a/ref-filter.h b/ref-filter.h\nindex ecea1837a2..fde5c3a1cb 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -87,6 +87,19 @@ struct ref_format {\n \n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n+\n+\t/*\n+\t * Set by verify_ref_format(); if not NONE, we can skip the usual\n+\t * formatting and use an optimized routine.\n+\t */\n+\tenum ref_format_quick {\n+\t\tREF_FORMAT_QUICK_NONE = 0,\n+\t\tREF_FORMAT_QUICK_VERBATIM,\n+\t\tREF_FORMAT_QUICK_REFNAME,\n+\t\tREF_FORMAT_QUICK_OBJECTNAME,\n+\t\tREF_FORMAT_QUICK_REFNAME_OBJECTNAME,\n+\t\tREF_FORMAT_QUICK_OBJECTNAME_REFNAME,\n+\t} quick;\n };\n \n #define REF_FORMAT_INIT { .use_color = -1 }\n-- \n2.33.0.618.g5b11852304\n"},{"id":"434719","messageId":"CAOLTT8RzHb5zWUm1psZRTP=iST-jP4DBmG2Zh1UPdo6z+QyHcg@mail.gmail.com","threadId":"56441","inReplyTo":"YTNpQ7Od1U/5i0R7@coredump.intra.peff.net","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-05T08:19:53Z","receivedAt":"2021-09-05T08:20:07Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月4日周六 下午8:40写道：\n>\n> Somebody complained to me off-list recently that for-each-ref was slow\n> when doing a trivial listing (like refname + objectname) of a large\n> number of refs, even though you could get mostly the same output by\n> just dumping the packed-refs file.\n>\n> So I was nerd-sniped into making this horrible series, which does speed\n> up some cases. It was a combination of \"how fast can we easily get it\",\n> plus I was curious _which_ parts of for-each-ref were actually slow.\n>\n> In this version there are 2 patches, tested against 'git for-each-ref\n> --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> refs:\n>\n\nRegarding this 500k refs, is there any way I can reproduce it?\n\n> - directly outputting items rather than building up a ref_array yields\n> about a 1.5x speedup\n>\n> - avoiding the whole atom-formatting system yields a 2.5x speedup on\n> top of that\n>\n> I had originally written it against a slightly older version of Git,\n> before show_ref_array_item() was inlined. In that version, there was a\n> middle ground where we still created a ref_array_item for each ref, and\n> then fed it to the \"quick\" formatter (so we could see the cost of\n> having to malloc/free a bunch of ref_array_item structs). The numbers\n> there wre:\n>\n> - streaming out items gave a 1.5x speedup\n>\n> - using the \"quick\" formatter gave a 1.8x speedup\n>\n> - avoiding the extra malloc/free for each item gave a 1.4x\n>\n> which makes sense; 1.8 * 1.4 is ~2.5, so it's the same speedup just\n> broken down further.\n>\n> I'm not sure I'm really advocating that we should do something like\n> this, though it is tempting because it does make some common queries\n> much faster. But I wanted to share it here to give some insight on where\n> the time may be going in ref-filter / for-each-ref.\n>\n\nWell, this acceleration looks gratifying. But so far I haven't seen\nits effect on\ngit cat-file --batch | --batch-check.\n\n\n> [1/2]: ref-filter: hacky \"streaming\" mode\n> [2/2]: ref-filter: implement \"quick\" formats\n>\n> builtin/for-each-ref.c | 7 +++\n> ref-filter.c | 113 +++++++++++++++++++++++++++++++++++++----\n> ref-filter.h | 21 ++++++++\n> 3 files changed, 132 insertions(+), 9 deletions(-)\n>\n> -Peff\n"},{"id":"434720","messageId":"CAOLTT8Tka0nxHb3G9yb-fs8ue7RaPCUVSKi5PM+GY+rMjFRnog@mail.gmail.com","threadId":"56441","inReplyTo":"YTNpeH+jO0zQgAVT@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-05T08:20:02Z","receivedAt":"2021-09-05T08:20:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月4日周六 下午8:41写道：\n>\n> The ref-filter code is very keen to collect all of the refs in an array\n> before writing any output. This makes things slower than necessary when\n> using the default sort order (which is already sorted by refname when we\n> call for_each_ref()), and when no filtering options require it.\n>\n> This commit adds a mildly-ugly interface to detect this case and stream\n> directly from filter_refs(). The caller just needs to call the\n> \"maybe_stream\" function. Either way, they can call the usual sort/print\n> functions; they'll just be noops if we did stream instead of collecting.\n>\n> Here are some timings on a fully-packed 500k-ref repo:\n>\n> Benchmark #1: ./git.orig for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 340.2 ms ± 5.3 ms [User: 300.5 ms, System: 39.6 ms]\n> Range (min … max): 332.9 ms … 347.0 ms 10 runs\n>\n> Benchmark #2: ./git.stream for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 224.0 ms ± 3.4 ms [User: 222.7 ms, System: 1.3 ms]\n> Range (min … max): 218.1 ms … 228.5 ms 13 runs\n>\n> Summary\n> './git.stream for-each-ref --format='%(objectname) %(refname)'' ran\n> 1.52 ± 0.03 times faster than './git.orig for-each-ref --format='%(objectname) %(refname)''\n>\n> So we definitely shave off some time, though we're still _much_ slower\n> than a simple `wc -l course it isn't actually doing as much work).\n>\n> The point here is to reduce overall effort, though of course the time to\n> first output is much improved in the streaming version, too (about 250ms\n> versus 4ms).\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/for-each-ref.c | 7 ++++++\n> ref-filter.c | 50 ++++++++++++++++++++++++++++++++++--------\n> ref-filter.h | 8 +++++++\n> 3 files changed, 56 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 89cb6307d4..fe0b92443f 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -70,6 +70,13 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> if (verify_ref_format(&format))\n> usage_with_options(for_each_ref_usage, opts);\n>\n> + /*\n> + * try streaming, but only without maxcount; in theory the ref-filter\n> + * code could learn about maxcount, but for now it is our own thing\n> + */\n> + if (!maxcount)\n> + ref_filter_maybe_stream(&filter, sorting, icase, &format);\n> +\n\nYes, I think this maxcount is easy to support.\n\n> if (!sorting)\n> sorting = ref_default_sorting();\n> ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 93ce2a6ef2..17b78b1d30 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2283,15 +2283,19 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> return 0;\n> }\n>\n> - /*\n> - * We do not open the object yet; sort may only need refname\n> - * to do its job and the resulting list may yet to be pruned\n> - * by maxcount logic.\n> - */\n> - ref = ref_array_push(ref_cbdata->array, refname, oid);\n> - ref->commit = commit;\n> - ref->flag = flag;\n> - ref->kind = kind;\n> + if (ref_cbdata->filter->streaming_format) {\n> + pretty_print_ref(refname, oid, ref_cbdata->filter->streaming_format);\n\nSo we directly use pretty_print_ref() in streaming mode, OK.\n\n> + } else {\n> + /*\n> + * We do not open the object yet; sort may only need refname\n> + * to do its job and the resulting list may yet to be pruned\n> + * by maxcount logic.\n> + */\n> + ref = ref_array_push(ref_cbdata->array, refname, oid);\n> + ref->commit = commit;\n> + ref->flag = flag;\n> + ref->kind = kind;\n> + }\n>\n> return 0;\n> }\n\nTherefore, in streaming mode, there is no need to push ref to\nref_array, which can\nreduce the overhead of malloc(), free(), which makes sense.\n\nBut here is a terrible fact: we did not use ref_array_sort() for sorting here.\nSo in fact, for_each_fullref_in() does the sorting work () for us by\ndefault sort (%(refname)),\nThis may be due to the file system or some implementation of ref_iterator.\nBut this limit the application of this optimization when we use other\natoms to sort.\n\n> @@ -2563,6 +2567,34 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n> QSORT_S(array->items, array->nr, compare_refs, sorting);\n> }\n>\n> +void ref_filter_maybe_stream(struct ref_filter *filter,\n> + const struct ref_sorting *sort, int icase,\n> + struct ref_format *format)\n> +{\n> + /* streaming only works with default for_each_ref sort order */\n> + if (sort || icase)\n> + return;\n> +\n\nYes, this really can only be optimized on the default sort.\n\n> + /* these filters want to see all candidates up front */\n> + if (filter->reachable_from || filter->unreachable_from)\n> + return;\n> +\n\nMake Sence.\n\n> + /*\n> + * the %(symref) placeholder is broken with pretty_print_ref(),\n> + * which our streaming code uses. I suspect this is a sign of breakage\n> + * in other callers like verify_tag(), which should be fixed. But for\n> + * now just disable streaming.\n> + *\n> + * Note that this implies we've parsed the format already with\n> + * verify_ref_format().\n> + */\n> + if (need_symref)\n> + return;\n> +\n\nI haven’t taken a closer look at why pretty_print_ref() does not\nsupport %(symref),\nwe can skip it first.\n\n> + /* OK to stream */\n> + filter->streaming_format = format;\n> +}\n> +\n> static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n> {\n> struct strbuf *s = &state->stack->output;\n> diff --git a/ref-filter.h b/ref-filter.h\n> index c15dee8d6b..ecea1837a2 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -69,6 +69,9 @@ struct ref_filter {\n> lines;\n> int abbrev,\n> verbose;\n> +\n> + /* if non-NULL, streaming output during filter_refs() is enabled */\n> + struct ref_format *streaming_format;\n> };\n>\n> struct ref_format {\n> @@ -135,6 +138,11 @@ char *get_head_description(void);\n> /* Set up translated strings in the output. */\n> void setup_ref_filter_porcelain_msg(void);\n>\n> +/* enable streaming during filter_refs() if options allow it */\n> +void ref_filter_maybe_stream(struct ref_filter *filter,\n> + const struct ref_sorting *sort, int icase,\n> + struct ref_format *format);\n> +\n> /*\n> * Print a single ref, outside of any ref-filter. Note that the\n> * name must be a fully qualified refname.\n> --\n> 2.33.0.618.g5b11852304\n>\n\nUnfortunately, this optimization may not be helpful for git cat-file --batch,\nsee [1], batch_object_write() directly constructs a ref_array_item and call\nformat_ref_array_item() to grab data, It does not use ref_array. So it also\ndoes not have this malloc() | free() overhead.\n\n[1]: https://lore.kernel.org/git/9c5fddf6885875ccd3ce3f047bb938c77d9bbca2.1628842990.git.gitgitgadget@gmail.com/\n\n--\nZheNing Hu\n"},{"id":"434721","messageId":"CAOLTT8QYe3PBPxSH8CYY+FatSfT7C5m6nccR2xMZ1yxSDFh5OQ@mail.gmail.com","threadId":"56441","inReplyTo":"YTNps0YBOaRNvPzk@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ref-filter: implement \"quick\" formats","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-05T08:20:07Z","receivedAt":"2021-09-05T08:20:21Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月4日周六 下午8:42写道：\n>\n> Some commonly-used formats can be output _much_ faster than going\n> through the usual atom-formatting code. E.g., \"%(objectname) %(refname)\"\n> can just be a simple printf. This commit detects a few easy cases and\n> uses a hard-coded output function instead.\n>\n> Note two things about the implementation:\n>\n> - we could probably go further here. E.g., %(refname:lstrip) should be\n> easy-ish to optimize, too. Likewise, mixed-text like \"delete\n> %(refname)\" would be nice to have.\n>\n> - the code is repetitive and enumerates all the cases, and it feels\n> like we ought to be able to generalize it more. But that's exactly\n> what the current formatting tries to do!\n>\n> So this whole thing is pretty horrible, and is a hack around the\n> slowness of the whole used_atom system. It _should_ be possible to\n> refactor that system to have roughly the same cost, but this will serve\n> in the meantime.\n>\n> Here are some numbers (\"stream\" is Git with the streaming optimization\n> from the previous commit, and \"quick\" is this commit):\n>\n> Benchmark #1: ./git.stream for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 229.2 ms ± 6.6 ms [User: 228.3 ms, System: 0.9 ms]\n> Range (min … max): 220.4 ms … 242.6 ms 13 runs\n>\n> Benchmark #2: ./git.quick for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 94.8 ms ± 2.2 ms [User: 93.5 ms, System: 1.4 ms]\n> Range (min … max): 90.8 ms … 98.2 ms 32 runs\n>\n> Summary\n> './git.quick for-each-ref --format='%(objectname) %(refname)'' ran\n> 2.42 ± 0.09 times faster than './git.stream for-each-ref --format='%(objectname) %(refname)''\n>\n> Signed-off-by: Jeff King \n> ---\n> ref-filter.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n> ref-filter.h | 13 +++++++++++\n> 2 files changed, 76 insertions(+)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 17b78b1d30..1efa3aadc8 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1009,6 +1009,37 @@ static int reject_atom(enum atom_type atom_type)\n> return atom_type == ATOM_REST;\n> }\n>\n> +static void set_up_quick_format(struct ref_format *format)\n> +{\n> + /* quick formats don't handle any special quoting */\n> + if (format->quote_style)\n> + return;\n> +\n> + /*\n> + * no atoms at all; this should be uncommon in real life, but it may be\n> + * interesting for benchmarking\n> + */\n> + if (!used_atom_cnt) {\n> + format->quick = REF_FORMAT_QUICK_VERBATIM;\n> + return;\n> + }\n> +\n> + /*\n> + * It's tempting to look at used_atom here, but it's wrong to do so: we\n> + * need not only to be sure of the needed atoms, but also their order\n> + * and any verbatim parts of the format. So instead, let's just\n> + * hard-code some specific formats.\n> + */\n> + if (!strcmp(format->format, \"%(refname)\"))\n> + format->quick = REF_FORMAT_QUICK_REFNAME;\n> + else if (!strcmp(format->format, \"%(objectname)\"))\n> + format->quick = REF_FORMAT_QUICK_OBJECTNAME;\n> + else if (!strcmp(format->format, \"%(refname) %(objectname)\"))\n> + format->quick = REF_FORMAT_QUICK_REFNAME_OBJECTNAME;\n> + else if (!strcmp(format->format, \"%(objectname) %(refname)\"))\n> + format->quick = REF_FORMAT_QUICK_OBJECTNAME_REFNAME;\n> +}\n> +\n> /*\n> * Make sure the format string is well formed, and parse out\n> * the used atoms.\n> @@ -1047,6 +1078,9 @@ int verify_ref_format(struct ref_format *format)\n> }\n> if (format->need_color_reset_at_eol && !want_color(format->use_color))\n> format->need_color_reset_at_eol = 0;\n> +\n> + set_up_quick_format(format);\n> +\n> return 0;\n> }\n>\n> @@ -2617,6 +2651,32 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n> }\n> }\n>\n> +static int quick_ref_format(const struct ref_format *format,\n> + const char *refname,\n> + const struct object_id *oid)\n> +{\n> + switch(format->quick) {\n> + case REF_FORMAT_QUICK_NONE:\n> + return -1;\n> + case REF_FORMAT_QUICK_VERBATIM:\n> + printf(\"%s\\n\", format->format);\n> + return 0;\n> + case REF_FORMAT_QUICK_REFNAME:\n> + printf(\"%s\\n\", refname);\n> + return 0;\n> + case REF_FORMAT_QUICK_OBJECTNAME:\n> + printf(\"%s\\n\", oid_to_hex(oid));\n> + return 0;\n> + case REF_FORMAT_QUICK_REFNAME_OBJECTNAME:\n> + printf(\"%s %s\\n\", refname, oid_to_hex(oid));\n> + return 0;\n> + case REF_FORMAT_QUICK_OBJECTNAME_REFNAME:\n> + printf(\"%s %s\\n\", oid_to_hex(oid), refname);\n> + return 0;\n> + }\n> + BUG(\"unknown ref_format_quick value: %d\", format->quick);\n> +}\n> +\n\nSo as a fast path, we actually avoided format_ref_array_item() when we are using\n%(objectname) and %(refname). But the problem is that it’s not very elegant\n(using string compare), and it is no optimization for other atoms that\nrequire in-depth\nparsing. I remember the \"fast path\" used by Ævar last time, and it\nseems that Junio doesn't\nlike them. [1][2]\n\n> int format_ref_array_item(struct ref_array_item *info,\n> struct ref_format *format,\n> struct strbuf *final_buf,\n> @@ -2670,6 +2730,9 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n> struct strbuf output = STRBUF_INIT;\n> struct strbuf err = STRBUF_INIT;\n>\n> + if (!quick_ref_format(format, name, oid))\n> + return;\n> +\n> ref_item = new_ref_array_item(name, oid);\n> ref_item->kind = ref_kind_from_refname(name);\n> if (format_ref_array_item(ref_item, format, &output, &err))\n> diff --git a/ref-filter.h b/ref-filter.h\n> index ecea1837a2..fde5c3a1cb 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -87,6 +87,19 @@ struct ref_format {\n>\n> /* Internal state to ref-filter */\n> int need_color_reset_at_eol;\n> +\n> + /*\n> + * Set by verify_ref_format(); if not NONE, we can skip the usual\n> + * formatting and use an optimized routine.\n> + */\n> + enum ref_format_quick {\n> + REF_FORMAT_QUICK_NONE = 0,\n> + REF_FORMAT_QUICK_VERBATIM,\n> + REF_FORMAT_QUICK_REFNAME,\n> + REF_FORMAT_QUICK_OBJECTNAME,\n> + REF_FORMAT_QUICK_REFNAME_OBJECTNAME,\n> + REF_FORMAT_QUICK_OBJECTNAME_REFNAME,\n> + } quick;\n> };\n>\n> #define REF_FORMAT_INIT { .use_color = -1 }\n> --\n> 2.33.0.618.g5b11852304\n\n[1]: https://lore.kernel.org/git/5903d02324f3275b3aa442bb3ca2602564c543dc.1626363626.git.gitgitgadget@gmail.com/\n[2]: https://lore.kernel.org/git/87eecf8ork.fsf@evledraar.gmail.com/\n"},{"id":"434726","messageId":"YTS8yiYwR+eeZf4+@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8RzHb5zWUm1psZRTP=iST-jP4DBmG2Zh1UPdo6z+QyHcg@mail.gmail.com","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-05T12:49:14Z","receivedAt":"2021-09-05T12:49:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 05, 2021 at 04:19:53PM +0800, ZheNing Hu wrote:\n\n> > In this version there are 2 patches, tested against 'git for-each-ref\n> > --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> > refs:\n> >\n> \n> Regarding this 500k refs, is there any way I can reproduce it?\n\nTry this in a clone of linux.git (or any other repo):\n\n git rev-list HEAD |\n head -500000 |\n perl -lne 'print \"create refs/foo/$. $_\"' |\n git update-ref --stdin\n\n git pack-refs --all --prune\n\nThough I actually think for these tests that it is not important that\neach ref point to a unique commit (we are not opening up the objects at\nall, and just treating the oids as strings).\n\n-Peff\n"},{"id":"434727","messageId":"YTTARcEvpXWSDfYW@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8Tka0nxHb3G9yb-fs8ue7RaPCUVSKi5PM+GY+rMjFRnog@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-05T13:04:05Z","receivedAt":"2021-09-05T13:04:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 05, 2021 at 04:20:02PM +0800, ZheNing Hu wrote:\n\n> But here is a terrible fact: we did not use ref_array_sort() for sorting here.\n> So in fact, for_each_fullref_in() does the sorting work () for us by\n> default sort (%(refname)),\n> This may be due to the file system or some implementation of ref_iterator.\n> But this limit the application of this optimization when we use other\n> atoms to sort.\n\nRight. This technique does not help at all if you use --sort. I do think\nit's reasonable to rely on the sorted output of the for_each_ref()\nfunctions; other parts of Git likely do as well, so I think we'd try\npretty hard to maintain that (and no, it's not a filesystem thing; we do\nmake sure to sort it ourselves; see the calls to sort_ref_dir()).\n\n> > + /*\n> > + * the %(symref) placeholder is broken with pretty_print_ref(),\n> > + * which our streaming code uses. I suspect this is a sign of breakage\n> > + * in other callers like verify_tag(), which should be fixed. But for\n> > + * now just disable streaming.\n> > + *\n> > + * Note that this implies we've parsed the format already with\n> > + * verify_ref_format().\n> > + */\n> > + if (need_symref)\n> > + return;\n> > +\n> \n> I haven’t taken a closer look at why pretty_print_ref() does not\n> support %(symref),\n> we can skip it first.\n\nI think it's just because pretty_print_ref() does not take a \"flag\"\nparameter for the caller. So it never sees that REF_ISSYMREF is set.\n\nThere aren't many callers of that function, so I think nobody ever\nreally noticed. But you can see the bug like this:\n\n git init repo\n cd repo\n \n git commit --allow-empty -m foo\n git tag -s -m bar annotated &&\n git symbolic-ref refs/tags/symref refs/tags/annotated\n \n # this is ok; ref-filter handles the flags\n git tag --list --format='list: %(refname) %(symref)'\n \n # not ok; we do not tell the formatter about the flags, so it does\n # not notice that \"symref\" is a symref\n git tag --verify --format='verify: %(refname) %(symref)' annotated symref\n\nI notice that the --verify output also shows the short refname, which\nmakes me wonder if %(symref) would have other bugs there (because it\nhas to re-resolve the ref to come up with the symref destination).\n\n> Unfortunately, this optimization may not be helpful for git cat-file --batch,\n> see [1], batch_object_write() directly constructs a ref_array_item and call\n> format_ref_array_item() to grab data, It does not use ref_array. So it also\n> does not have this malloc() | free() overhead.\n\nRight. It would only be helped by the \"quick\" formats in the second\npatch (and by skipping the malloc/free of the individual\nref_array_items).\n\n-Peff\n"},{"id":"434728","messageId":"YTTBCWlsC1cBZMio@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8QYe3PBPxSH8CYY+FatSfT7C5m6nccR2xMZ1yxSDFh5OQ@mail.gmail.com","subject":"Re: [PATCH 2/2] ref-filter: implement \"quick\" formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-05T13:07:21Z","receivedAt":"2021-09-05T13:07:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 05, 2021 at 04:20:07PM +0800, ZheNing Hu wrote:\n\n> > + case REF_FORMAT_QUICK_OBJECTNAME_REFNAME:\n> > + printf(\"%s %s\\n\", oid_to_hex(oid), refname);\n> > + return 0;\n> > + }\n> > + BUG(\"unknown ref_format_quick value: %d\", format->quick);\n> > +}\n> > +\n> \n> So as a fast path, we actually avoided format_ref_array_item() when we are using\n> %(objectname) and %(refname). But the problem is that it’s not very elegant\n> (using string compare), and it is no optimization for other atoms that\n> require in-depth\n> parsing. I remember the \"fast path\" used by Ævar last time, and it\n> seems that Junio doesn't\n> like them. [1][2]\n\nYes, I did say it was \"pretty horrible\". :)\n\nIt was mostly meant as a proof-of-concept to see where the time was\ngoing, and what was possible. It _could_ be used as a stop-gap while\nimproving the general performance, but it's gross enough that it's\nprobably not a good idea (it's increased maintenance, but also it\ndis-incentivizes fixing the real problems).\n\n-Peff\n"},{"id":"434729","messageId":"YTTC2IUO1ZmTOEoR@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8Tka0nxHb3G9yb-fs8ue7RaPCUVSKi5PM+GY+rMjFRnog@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-05T13:15:04Z","receivedAt":"2021-09-05T13:15:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 05, 2021 at 04:20:02PM +0800, ZheNing Hu wrote:\n\n> > + if (ref_cbdata->filter->streaming_format) {\n> > + pretty_print_ref(refname, oid, ref_cbdata->filter->streaming_format);\n> \n> So we directly use pretty_print_ref() in streaming mode, OK.\n> \n> > + } else {\n> > + /*\n> > + * We do not open the object yet; sort may only need refname\n> > + * to do its job and the resulting list may yet to be pruned\n> > + * by maxcount logic.\n> > + */\n> > + ref = ref_array_push(ref_cbdata->array, refname, oid);\n> > + ref->commit = commit;\n> > + ref->flag = flag;\n> > + ref->kind = kind;\n> > + }\n> >\n> > return 0;\n> > }\n> \n> Therefore, in streaming mode, there is no need to push ref to\n> ref_array, which can\n> reduce the overhead of malloc(), free(), which makes sense.\n\nBy the way, one thing I wondered here: how much of the benefit is from\navoiding the ref_array, and how much is from skipping the sort entirely.\n\nIt turns out that most of it is from the latter. If I do this:\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 89cb6307d4..037d5db814 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -78,7 +78,11 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tfilter.name_patterns = argv;\n \tfilter.match_as_path = 1;\n \tfilter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);\n-\tref_array_sort(sorting, &array);\n+\t/*\n+\t * we should skip this only when we are using the default refname\n+\t * sorting, but as an experimental hack, we'll just comment it out.\n+\t */\n+\t// ref_array_sort(sorting, &array);\n \n \tif (!maxcount || array.nr < maxcount)\n \t\tmaxcount = array.nr;\n\nthen the timings I get are:\n\n Benchmark #1: ./git.old for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 341.4 ms ± 7.4 ms [User: 299.8 ms, System: 41.6 ms]\n Range (min … max): 333.5 ms … 355.1 ms 10 runs\n \n Benchmark #2: ./git.new for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 249.1 ms ± 5.7 ms [User: 211.8 ms, System: 37.2 ms]\n Range (min … max): 245.9 ms … 267.0 ms 12 runs\n \n Summary\n './git.new for-each-ref --format='%(objectname) %(refname)'' ran\n 1.37 ± 0.04 times faster than './git.old for-each-ref --format='%(objectname) %(refname)''\n\nSo of the 1.5x improvement that the original patch showed, 1.37x is from\nskipping the sort of the already-sorted data. I suspect that has less to\ndo with sorting at all, and more to do with the fact that even just\nformatting \"%(refname)\" for each entry takes a non-trivial amount of\ntime.\n\n-Peff\n"},{"id":"434765","messageId":"YTW7JV0sCq2rqvRT@ncase","threadId":"56441","inReplyTo":"YTNpQ7Od1U/5i0R7@coredump.intra.peff.net","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-06T06:54:29Z","receivedAt":"2021-09-06T06:54:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Sep 04, 2021 at 08:40:35AM -0400, Jeff King wrote:\n> Somebody complained to me off-list recently that for-each-ref was slow\n> when doing a trivial listing (like refname + objectname) of a large\n> number of refs, even though you could get mostly the same output by\n> just dumping the packed-refs file.\n> \n> So I was nerd-sniped into making this horrible series, which does speed\n> up some cases. It was a combination of \"how fast can we easily get it\",\n> plus I was curious _which_ parts of for-each-ref were actually slow.\n> \n> In this version there are 2 patches, tested against 'git for-each-ref\n> --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> refs:\n> \n> - directly outputting items rather than building up a ref_array yields\n> about a 1.5x speedup\n> \n> - avoiding the whole atom-formatting system yields a 2.5x speedup on\n> top of that\n\nInteresting, thanks for the PoC. Your 500k refs naturally triggered me\ngiven that I have recently been trying to optimize such repos with lots\nof refs, too. So I ran your series on my gitlab-org/gitlab repository\nwith 2.3M ref and saw similar speedups:\n\n Benchmark #1: jk-for-each-ref-speedup~2: git-for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 2.756 s ± 0.013 s [User: 2.416 s, System: 0.339 s]\n Range (min … max): 2.740 s … 2.774 s 5 runs\n\n Benchmark #2: jk-for-each-ref-speedup~: git-for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 2.009 s ± 0.015 s [User: 1.974 s, System: 0.035 s]\n Range (min … max): 1.984 s … 2.020 s 5 runs\n\n Benchmark #3: jk-for-each-ref-speedup: git-for-each-ref --format='%(objectname) %(refname)'\n Time (mean ± σ): 701.8 ms ± 4.2 ms [User: 661.3 ms, System: 40.4 ms]\n Range (min … max): 696.1 ms … 706.9 ms 5 runs\n\n Summary\n 'jk-for-each-ref-speedup: git-for-each-ref --format='%(objectname) %(refname)'' ran\n 2.86 ± 0.03 times faster than 'jk-for-each-ref-speedup~: git-for-each-ref --format='%(objectname) %(refname)''\n 3.93 ± 0.03 times faster than 'jk-for-each-ref-speedup~2: git-for-each-ref --format='%(objectname) %(refname)''\n\n[snip]\n> I'm not sure I'm really advocating that we should do something like\n> this, though it is tempting because it does make some common queries\n> much faster. But I wanted to share it here to give some insight on where\n> the time may be going in ref-filter / for-each-ref.\n\nWell, avoiding the sort like you do in #1 feels like the right thing to\ndo to me. I saw similar wins when optimizing the connectivity checks,\nand it only makes sense that sorting becomes increasingly expensive as\nyour number of refs grows. And if we do skip the sort, then decreasing\nthe initial latency feels like the logical next step because we know we\ncan stream out things directly.\n\nThe quick-formatting is a different story altogether, as you rightly\npoint out. Having to explain why '%(objectname) %(refname)' is 2.5x\nfaster than '%(refname) %(objectname)' is probably nothing we'd want to\ndo. But in any case, your patch does point out that the current\nformatting code could use some tweaking.\n\nIn any case, this is only my 2c. Thanks for this series!\n\nPatrick\n"},{"id":"434789","messageId":"CAOLTT8SYx4f8x-0zjXLJVOpW4-Mj34mjsfwr6tWo0PbZAV9B0w@mail.gmail.com","threadId":"56441","inReplyTo":"YTS8yiYwR+eeZf4+@coredump.intra.peff.net","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-06T13:30:45Z","receivedAt":"2021-09-06T13:30:58Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月5日周日 下午8:49写道：\n>\n> On Sun, Sep 05, 2021 at 04:19:53PM +0800, ZheNing Hu wrote:\n>\n> > > In this version there are 2 patches, tested against 'git for-each-ref\n> > > --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> > > refs:\n> > >\n> >\n> > Regarding this 500k refs, is there any way I can reproduce it?\n>\n> Try this in a clone of linux.git (or any other repo):\n>\n> git rev-list HEAD |\n> head -500000 |\n> perl -lne 'print \"create refs/foo/$. $_\"' |\n> git update-ref --stdin\n>\n> git pack-refs --all --prune\n>\n\nSorry, It seems that the above command is difficult to complete on my\nmachine (it took more than ten minutes). It may be stuck on git update-ref.\nSo I tried to reproduce it in a repo which containing 76K refs:\n\nBenchmark #1: jk-for-each-ref-speedup~2: git for-each-ref\n--format='%(refname) %(objectname)'\n Time (mean ± σ): 108.0 ms ± 1.9 ms [User: 55.2 ms, System: 52.1 ms]\n Range (min … max): 105.7 ms … 112.4 ms 26 runs\n\nBenchmark #2: jk-for-each-ref-speedup~1: git for-each-ref\n--format='%(refname) %(objectname)'\n Time (mean ± σ): 88.2 ms ± 1.7 ms [User: 44.8 ms, System: 43.1 ms]\n Range (min … max): 85.8 ms … 93.2 ms 32 runs\n\nBenchmark #3:jk-for-each-ref-speedup: git for-each-ref\n--format='%(refname) %(objectname)'\n Time (mean ± σ): 69.0 ms ± 2.0 ms [User: 22.7 ms, System: 46.1 ms]\n Range (min … max): 66.2 ms … 74.1 ms 41 runs\n\nFor %(refname) and %(objectname), this performance optimization is\nindeed amazing.\n\n> Though I actually think for these tests that it is not important that\n> each ref point to a unique commit (we are not opening up the objects at\n> all, and just treating the oids as strings).\n>\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"434790","messageId":"CAOLTT8QqLXdvL5Y8OkEzuctGgnDHA1DQu8ZaxQGnfHPBqNVm4g@mail.gmail.com","threadId":"56441","inReplyTo":"YTTBCWlsC1cBZMio@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ref-filter: implement \"quick\" formats","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-06T13:34:56Z","receivedAt":"2021-09-06T13:35:09Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月5日周日 下午9:07写道：\n>\n> On Sun, Sep 05, 2021 at 04:20:07PM +0800, ZheNing Hu wrote:\n>\n> > > + case REF_FORMAT_QUICK_OBJECTNAME_REFNAME:\n> > > + printf(\"%s %s\\n\", oid_to_hex(oid), refname);\n> > > + return 0;\n> > > + }\n> > > + BUG(\"unknown ref_format_quick value: %d\", format->quick);\n> > > +}\n> > > +\n> >\n> > So as a fast path, we actually avoided format_ref_array_item() when we are using\n> > %(objectname) and %(refname). But the problem is that it’s not very elegant\n> > (using string compare), and it is no optimization for other atoms that\n> > require in-depth\n> > parsing. I remember the \"fast path\" used by Ævar last time, and it\n> > seems that Junio doesn't\n> > like them. [1][2]\n>\n> Yes, I did say it was \"pretty horrible\". :)\n>\n> It was mostly meant as a proof-of-concept to see where the time was\n> going, and what was possible. It _could_ be used as a stop-gap while\n> improving the general performance, but it's gross enough that it's\n> probably not a good idea (it's increased maintenance, but also it\n> dis-incentivizes fixing the real problems).\n>\n\nAgree. Like you said, these performance gaps are caused by the used_atom\nsystem.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"434815","messageId":"CAOLTT8QbdNBSY95bCa+UNJBqsJEEHbnaKfZLzvN2Qzd-Np8Lqg@mail.gmail.com","threadId":"56441","inReplyTo":"YTTARcEvpXWSDfYW@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-07T05:28:33Z","receivedAt":"2021-09-07T05:28:50Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月5日周日 下午9:04写道：\n>\n> On Sun, Sep 05, 2021 at 04:20:02PM +0800, ZheNing Hu wrote:\n>\n> > But here is a terrible fact: we did not use ref_array_sort() for sorting here.\n> > So in fact, for_each_fullref_in() does the sorting work () for us by\n> > default sort (%(refname)),\n> > This may be due to the file system or some implementation of ref_iterator.\n> > But this limit the application of this optimization when we use other\n> > atoms to sort.\n>\n> Right. This technique does not help at all if you use --sort. I do think\n> it's reasonable to rely on the sorted output of the for_each_ref()\n> functions; other parts of Git likely do as well, so I think we'd try\n> pretty hard to maintain that (and no, it's not a filesystem thing; we do\n> make sure to sort it ourselves; see the calls to sort_ref_dir()).\n>\n> > > + /*\n> > > + * the %(symref) placeholder is broken with pretty_print_ref(),\n> > > + * which our streaming code uses. I suspect this is a sign of breakage\n> > > + * in other callers like verify_tag(), which should be fixed. But for\n> > > + * now just disable streaming.\n> > > + *\n> > > + * Note that this implies we've parsed the format already with\n> > > + * verify_ref_format().\n> > > + */\n> > > + if (need_symref)\n> > > + return;\n> > > +\n> >\n> > I haven’t taken a closer look at why pretty_print_ref() does not\n> > support %(symref),\n> > we can skip it first.\n>\n> I think it's just because pretty_print_ref() does not take a \"flag\"\n> parameter for the caller. So it never sees that REF_ISSYMREF is set.\n>\n\nyeah, pretty_print_ref() does not set the flag, this is a defect of\npretty_print_ref(), maybe we need to find a way to set this flag.\nI also deliberately avoided %(symref) when refactoring git cat-file --batch,\nperhaps the improvements here can also be applied to git cat-file --batch\n\n> There aren't many callers of that function, so I think nobody ever\n> really noticed. But you can see the bug like this:\n>\n> git init repo\n> cd repo\n>\n> git commit --allow-empty -m foo\n> git tag -s -m bar annotated &&\n> git symbolic-ref refs/tags/symref refs/tags/annotated\n>\n> # this is ok; ref-filter handles the flags\n> git tag --list --format='list: %(refname) %(symref)'\n>\n> # not ok; we do not tell the formatter about the flags, so it does\n> # not notice that \"symref\" is a symref\n> git tag --verify --format='verify: %(refname) %(symref)' annotated symref\n>\n> I notice that the --verify output also shows the short refname, which\n> makes me wonder if %(symref) would have other bugs there (because it\n> has to re-resolve the ref to come up with the symref destination).\n>\n\nThis may be easy to fix:\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 452558ec95..4be5d36366 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -152,11 +152,11 @@ static int verify_tag(const char *name, const char *ref,\n if (format->format)\n flags = GPG_VERIFY_OMIT_STATUS;\n\n- if (gpg_verify_tag(oid, name, flags))\n+ if (gpg_verify_tag(oid, ref, flags))\n return -1;\n\n if (format->format)\n- pretty_print_ref(name, oid, format);\n+ pretty_print_ref(ref, oid, format);\n\n return 0;\n }\n\n> > Unfortunately, this optimization may not be helpful for git cat-file --batch,\n> > see [1], batch_object_write() directly constructs a ref_array_item and call\n> > format_ref_array_item() to grab data, It does not use ref_array. So it also\n> > does not have this malloc() | free() overhead.\n>\n> Right. It would only be helped by the \"quick\" formats in the second\n> patch (and by skipping the malloc/free of the individual\n> ref_array_items).\n>\n\nYes, your test is indeed worth thinking about: how to avoid\nintermediate data to reduce overhead.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"434816","messageId":"CAOLTT8QzNt8dv5-Q4nV9XU9Tq7KZVSjaYcM3T4F=F=xZ9A3UUA@mail.gmail.com","threadId":"56441","inReplyTo":"YTTC2IUO1ZmTOEoR@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-07T05:42:56Z","receivedAt":"2021-09-07T05:43:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月5日周日 下午9:15写道：\n>\n> On Sun, Sep 05, 2021 at 04:20:02PM +0800, ZheNing Hu wrote:\n>\n> > > + if (ref_cbdata->filter->streaming_format) {\n> > > + pretty_print_ref(refname, oid, ref_cbdata->filter->streaming_format);\n> >\n> > So we directly use pretty_print_ref() in streaming mode, OK.\n> >\n> > > + } else {\n> > > + /*\n> > > + * We do not open the object yet; sort may only need refname\n> > > + * to do its job and the resulting list may yet to be pruned\n> > > + * by maxcount logic.\n> > > + */\n> > > + ref = ref_array_push(ref_cbdata->array, refname, oid);\n> > > + ref->commit = commit;\n> > > + ref->flag = flag;\n> > > + ref->kind = kind;\n> > > + }\n> > >\n> > > return 0;\n> > > }\n> >\n> > Therefore, in streaming mode, there is no need to push ref to\n> > ref_array, which can\n> > reduce the overhead of malloc(), free(), which makes sense.\n>\n> By the way, one thing I wondered here: how much of the benefit is from\n> avoiding the ref_array, and how much is from skipping the sort entirely.\n>\n> It turns out that most of it is from the latter. If I do this:\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 89cb6307d4..037d5db814 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -78,7 +78,11 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n> filter.name_patterns = argv;\n> filter.match_as_path = 1;\n> filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);\n> - ref_array_sort(sorting, &array);\n> + /*\n> + * we should skip this only when we are using the default refname\n> + * sorting, but as an experimental hack, we'll just comment it out.\n> + */\n> + // ref_array_sort(sorting, &array);\n>\n> if (!maxcount || array.nr < maxcount)\n> maxcount = array.nr;\n>\n> then the timings I get are:\n>\n> Benchmark #1: ./git.old for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 341.4 ms ± 7.4 ms [User: 299.8 ms, System: 41.6 ms]\n> Range (min … max): 333.5 ms … 355.1 ms 10 runs\n>\n> Benchmark #2: ./git.new for-each-ref --format='%(objectname) %(refname)'\n> Time (mean ± σ): 249.1 ms ± 5.7 ms [User: 211.8 ms, System: 37.2 ms]\n> Range (min … max): 245.9 ms … 267.0 ms 12 runs\n>\n> Summary\n> './git.new for-each-ref --format='%(objectname) %(refname)'' ran\n> 1.37 ± 0.04 times faster than './git.old for-each-ref --format='%(objectname) %(refname)''\n>\n> So of the 1.5x improvement that the original patch showed, 1.37x is from\n> skipping the sort of the already-sorted data. I suspect that has less to\n> do with sorting at all, and more to do with the fact that even just\n> formatting \"%(refname)\" for each entry takes a non-trivial amount of\n> time.\n>\n\nYes, I think this overhead may come from get_ref_atom_value() instead\nof QSORT_S().\n\n> -Peff\n\nThanks.\n--\nZheNing\n"},{"id":"434899","messageId":"YTehP58NfcnDSWEc@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8SYx4f8x-0zjXLJVOpW4-Mj34mjsfwr6tWo0PbZAV9B0w@mail.gmail.com","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-07T17:28:31Z","receivedAt":"2021-09-07T17:28:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 06, 2021 at 09:30:45PM +0800, ZheNing Hu wrote:\n\n> Jeff King 于2021年9月5日周日 下午8:49写道：\n> >\n> > On Sun, Sep 05, 2021 at 04:19:53PM +0800, ZheNing Hu wrote:\n> >\n> > > > In this version there are 2 patches, tested against 'git for-each-ref\n> > > > --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> > > > refs:\n> > > >\n> > >\n> > > Regarding this 500k refs, is there any way I can reproduce it?\n> >\n> > Try this in a clone of linux.git (or any other repo):\n> >\n> > git rev-list HEAD |\n> > head -500000 |\n> > perl -lne 'print \"create refs/foo/$. $_\"' |\n> > git update-ref --stdin\n> >\n> > git pack-refs --all --prune\n> >\n> \n> Sorry, It seems that the above command is difficult to complete on my\n> machine (it took more than ten minutes). It may be stuck on git update-ref.\n> So I tried to reproduce it in a repo which containing 76K refs:\n\nMine didn't take nearly that wrong, but it does depend on filesystem and\ndisk performance. It's going to create 500k lock files in refs/foo. :)\n\nYou can cheat a bit like this:\n\n {\n # grab existing packed refs; don't worry about peel lines or the\n # header comment, we're producing a lowest-common denominator\n # version of the file\n grep '^[0-9a-f]' packed-refs\n\n # now make our new fake refs\n git rev-list HEAD |\n head -500000 |\n perl -lne 'print \"$_ refs/foo/$.\"'\n } >packed-refs.tmp\n mv packed-refs.tmp packed-refs\n\n # and now ask Git to repack to get everything sorted, etc\n git pack-refs --all --prune\n\nIt sounds like you were able to come up with a smaller version to play\nwith anyway, but I enjoy coming up with such hacks. :)\n\n-Peff\n"},{"id":"434903","messageId":"YTeo/dCFfpAIfo3K@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8QbdNBSY95bCa+UNJBqsJEEHbnaKfZLzvN2Qzd-Np8Lqg@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-07T18:01:33Z","receivedAt":"2021-09-07T18:01:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 07, 2021 at 01:28:33PM +0800, ZheNing Hu wrote:\n\n> > I think it's just because pretty_print_ref() does not take a \"flag\"\n> > parameter for the caller. So it never sees that REF_ISSYMREF is set.\n> >\n> \n> yeah, pretty_print_ref() does not set the flag, this is a defect of\n> pretty_print_ref(), maybe we need to find a way to set this flag.\n\nIn general, I think it could take a flag parameter from its caller. But\nits caller comes indirectly from for_each_tag_name(), which isn't a\nregular ref-iterator. It would probably need to switch to using\nread_ref_full() to get the flags.\n\n> > I notice that the --verify output also shows the short refname, which\n> > makes me wonder if %(symref) would have other bugs there (because it\n> > has to re-resolve the ref to come up with the symref destination).\n> >\n> \n> This may be easy to fix:\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 452558ec95..4be5d36366 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -152,11 +152,11 @@ static int verify_tag(const char *name, const char *ref,\n> if (format->format)\n> flags = GPG_VERIFY_OMIT_STATUS;\n> \n> - if (gpg_verify_tag(oid, name, flags))\n> + if (gpg_verify_tag(oid, ref, flags))\n> return -1;\n> \n> if (format->format)\n> - pretty_print_ref(name, oid, format);\n> + pretty_print_ref(ref, oid, format);\n> \n> return 0;\n> }\n\nYeah, I think that would work, although:\n\n - there's another caller in cmd_verify_tag() which seems to just pass\n whatever was on the command line. It doesn't even resolve the ref\n itself!\n\n - I suspect people may be relying on the current behavior. The\n original was added to be able to compare the internal tagname to the\n refname. I.e., that:\n\n git tag -v --format='%(refname) %(tag)' foo\n\n would show \"foo foo\". Now that _should_ be \"%(refname:strip=2)\", I\n think, but we'd probably be breaking scripts. OTOH, it really feels\n like _not_ handing over a real, fully-qualified refname to the\n ref-filter code will mean other things are broken (e.g.,\n ATOM_UPSTREAM is assuming we have a fully-qualified ref).\n\n I think a backwards-compatible way of fixing it would be to have\n this call hand over the full refname to the ref-filter code, but\n tell it that %(refname) should default to strip=2. And then anybody\n who wants the full name can use %(refname:strip=0).\n\n It makes the behavior confusing and quirky, but we can't avoid that\n without breaking compatibility.\n\n-Peff\n"},{"id":"434926","messageId":"xmqq35qgb166.fsf@gitster.g","threadId":"56441","inReplyTo":"YTTBCWlsC1cBZMio@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] ref-filter: implement \"quick\" formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T20:06:41Z","receivedAt":"2021-09-07T20:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It was mostly meant as a proof-of-concept to see where the time was\n> going, and what was possible. It _could_ be used as a stop-gap while\n> improving the general performance, but it's gross enough that it's\n> probably not a good idea (it's increased maintenance, but also it\n> dis-incentivizes fixing the real problems).\n\nThanks. I have to agree with the assessment.\n"},{"id":"435262","messageId":"CAOLTT8TSNtVqZrdYpRpUp-JfvHUpDAMEo5k4SkiLJUjMmefcLA@mail.gmail.com","threadId":"56441","inReplyTo":"YTehP58NfcnDSWEc@coredump.intra.peff.net","subject":"Re: [hacky PATCH 0/2] speeding up trivial for-each-ref invocations","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-09T13:20:56Z","receivedAt":"2021-09-09T14:29:56Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月8日周三 上午1:28写道：\n>\n> On Mon, Sep 06, 2021 at 09:30:45PM +0800, ZheNing Hu wrote:\n>\n> > Jeff King 于2021年9月5日周日 下午8:49写道：\n> > >\n> > > On Sun, Sep 05, 2021 at 04:19:53PM +0800, ZheNing Hu wrote:\n> > >\n> > > > > In this version there are 2 patches, tested against 'git for-each-ref\n> > > > > --format=\"%(objectname) %(refname)\"' on a fully packed repo with 500k\n> > > > > refs:\n> > > > >\n> > > >\n> > > > Regarding this 500k refs, is there any way I can reproduce it?\n> > >\n> > > Try this in a clone of linux.git (or any other repo):\n> > >\n> > > git rev-list HEAD |\n> > > head -500000 |\n> > > perl -lne 'print \"create refs/foo/$. $_\"' |\n> > > git update-ref --stdin\n> > >\n> > > git pack-refs --all --prune\n> > >\n> >\n> > Sorry, It seems that the above command is difficult to complete on my\n> > machine (it took more than ten minutes). It may be stuck on git update-ref.\n> > So I tried to reproduce it in a repo which containing 76K refs:\n>\n> Mine didn't take nearly that wrong, but it does depend on filesystem and\n> disk performance. It's going to create 500k lock files in refs/foo. :)\n>\n> You can cheat a bit like this:\n>\n> {\n> # grab existing packed refs; don't worry about peel lines or the\n> # header comment, we're producing a lowest-common denominator\n> # version of the file\n> grep '^[0-9a-f]' packed-refs\n>\n> # now make our new fake refs\n> git rev-list HEAD |\n> head -500000 |\n> perl -lne 'print \"$_ refs/foo/$.\"'\n> } >packed-refs.tmp\n> mv packed-refs.tmp packed-refs\n>\n> # and now ask Git to repack to get everything sorted, etc\n> git pack-refs --all --prune\n>\n> It sounds like you were able to come up with a smaller version to play\n> with anyway, but I enjoy coming up with such hacks. :)\n>\n\nThanks, this method really works. :-)\n\n> -Peff\n\n--\nZheNing Hu\n"},{"id":"435270","messageId":"CAOLTT8Ru-Zhmo5j=jNjWexrahT0qAO5zEMW09XT00-TCca-SkA@mail.gmail.com","threadId":"56441","inReplyTo":"YTeo/dCFfpAIfo3K@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-09T14:45:15Z","receivedAt":"2021-09-09T14:48:47Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月8日周三 上午2:01写道：\n>\n> On Tue, Sep 07, 2021 at 01:28:33PM +0800, ZheNing Hu wrote:\n>\n> > > I think it's just because pretty_print_ref() does not take a \"flag\"\n> > > parameter for the caller. So it never sees that REF_ISSYMREF is set.\n> > >\n> >\n> > yeah, pretty_print_ref() does not set the flag, this is a defect of\n> > pretty_print_ref(), maybe we need to find a way to set this flag.\n>\n> In general, I think it could take a flag parameter from its caller. But\n> its caller comes indirectly from for_each_tag_name(), which isn't a\n> regular ref-iterator. It would probably need to switch to using\n> read_ref_full() to get the flags.\n>\n\nYeah, I think using read_ref_full() with verify_tag() changes can\nsolve this BUG:\n\n$ git.fix tag --verify --format='verify: %(refname) %(symref)' annotated symref\nverify: refs/tags/annotated\nverify: refs/tags/symref refs/tags/annotated\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1efa3aadc8..71b1d7da4f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2613,18 +2613,6 @@ void ref_filter_maybe_stream(struct ref_filter *filter,\n if (filter->reachable_from || filter->unreachable_from)\n return;\n\n- /*\n- * the %(symref) placeholder is broken with pretty_print_ref(),\n- * which our streaming code uses. I suspect this is a sign of breakage\n- * in other callers like verify_tag(), which should be fixed. But for\n- * now just disable streaming.\n- *\n- * Note that this implies we've parsed the format already with\n- * verify_ref_format().\n- */\n- if (need_symref)\n- return;\n-\n /* OK to stream */\n filter->streaming_format = format;\n }\n@@ -2735,6 +2723,7 @@ void pretty_print_ref(const char *name, const\nstruct object_id *oid,\n\n ref_item = new_ref_array_item(name, oid);\n ref_item->kind = ref_kind_from_refname(name);\n+ read_ref_full(name, 0, NULL, &ref_item->flag);\n if (format_ref_array_item(ref_item, format, &output, &err))\n die(\"%s\", err.buf);\n fwrite(output.buf, 1, output.len, stdout);\n\n> > > I notice that the --verify output also shows the short refname, which\n> > > makes me wonder if %(symref) would have other bugs there (because it\n> > > has to re-resolve the ref to come up with the symref destination).\n> > >\n> >\n> > This may be easy to fix:\n> >\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index 452558ec95..4be5d36366 100644\n> > --- a/builtin/tag.c\n> > +++ b/builtin/tag.c\n> > @@ -152,11 +152,11 @@ static int verify_tag(const char *name, const char *ref,\n> > if (format->format)\n> > flags = GPG_VERIFY_OMIT_STATUS;\n> >\n> > - if (gpg_verify_tag(oid, name, flags))\n> > + if (gpg_verify_tag(oid, ref, flags))\n> > return -1;\n> >\n> > if (format->format)\n> > - pretty_print_ref(name, oid, format);\n> > + pretty_print_ref(ref, oid, format);\n> >\n> > return 0;\n> > }\n>\n> Yeah, I think that would work, although:\n>\n> - there's another caller in cmd_verify_tag() which seems to just pass\n> whatever was on the command line. It doesn't even resolve the ref\n> itself!\n>\n\nWe can modify get_oid() --> read_ref_full() in cmd_verify_tag()...\nYes, the inconsistency between cmd_verify_tag() and verify_tag() makes\nme feel very strange.\n\n> - I suspect people may be relying on the current behavior. The\n> original was added to be able to compare the internal tagname to the\n> refname. I.e., that:\n>\n> git tag -v --format='%(refname) %(tag)' foo\n>\n> would show \"foo foo\". Now that _should_ be \"%(refname:strip=2)\", I\n> think, but we'd probably be breaking scripts. OTOH, it really feels\n> like _not_ handing over a real, fully-qualified refname to the\n> ref-filter code will mean other things are broken (e.g.,\n> ATOM_UPSTREAM is assuming we have a fully-qualified ref).\n>\n\nThis is indeed a sad thing: A bug becomes a feature.\n\n> I think a backwards-compatible way of fixing it would be to have\n> this call hand over the full refname to the ref-filter code, but\n> tell it that %(refname) should default to strip=2. And then anybody\n> who wants the full name can use %(refname:strip=0).\n>\n\nDoesn't this make things more complicated? Those callers of git for-each-ref,\nwouldn't our changes like this destroy them?\n\n\n> It makes the behavior confusing and quirky, but we can't avoid that\n> without breaking compatibility.\n>\n\nEh, I think we may need other solutions.\n\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"435415","messageId":"YTtrF8C0mmT6kBJT@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8Ru-Zhmo5j=jNjWexrahT0qAO5zEMW09XT00-TCca-SkA@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-10T14:26:31Z","receivedAt":"2021-09-10T14:26:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 09, 2021 at 10:45:15PM +0800, ZheNing Hu wrote:\n\n> @@ -2735,6 +2723,7 @@ void pretty_print_ref(const char *name, const\n> struct object_id *oid,\n> \n> ref_item = new_ref_array_item(name, oid);\n> ref_item->kind = ref_kind_from_refname(name);\n> + read_ref_full(name, 0, NULL, &ref_item->flag);\n> if (format_ref_array_item(ref_item, format, &output, &err))\n> die(\"%s\", err.buf);\n> fwrite(output.buf, 1, output.len, stdout);\n\nIMHO this is the wrong place to do it, since the caller may already have\nthe flags (and looking up the ref again is a non-trivial cost).\n\nThe caller in builtin/tag.c should switch to using read_ref_full() and\npass in the flags.\n\nThe caller in builtin/verify-tag.c _probably_ should resolve the ref in\nthe same way and pass in that full refname and flags. I do worry that\nthis may be a compatibility problem, but the current behavior seems so\nbroken to me.\n\n> > - I suspect people may be relying on the current behavior. The\n> > original was added to be able to compare the internal tagname to the\n> > refname. I.e., that:\n> >\n> > git tag -v --format='%(refname) %(tag)' foo\n> >\n> > would show \"foo foo\". Now that _should_ be \"%(refname:strip=2)\", I\n> > think, but we'd probably be breaking scripts. OTOH, it really feels\n> > like _not_ handing over a real, fully-qualified refname to the\n> > ref-filter code will mean other things are broken (e.g.,\n> > ATOM_UPSTREAM is assuming we have a fully-qualified ref).\n> >\n> \n> This is indeed a sad thing: A bug becomes a feature.\n> \n> > I think a backwards-compatible way of fixing it would be to have\n> > this call hand over the full refname to the ref-filter code, but\n> > tell it that %(refname) should default to strip=2. And then anybody\n> > who wants the full name can use %(refname:strip=0).\n> >\n> \n> Doesn't this make things more complicated? Those callers of git for-each-ref,\n> wouldn't our changes like this destroy them?\n\nMy proposal was that we'd have a specific flag in ref-filter to say\n\"default refname:strip to this value\". And then _only_ \"tag --verify\"\nwould set that (to \"2\"), leaving for-each-ref, etc unaffected.\n\nSo yes, it's complicated. And it must be explained to the user that\n\"%(refname)\" behaves slightly differently with \"git tag --verify\", but\nthat is unavoidable if we do not want to break scripts (it _already_\nbehaves slightly differently, and we just never told anyone).\n\nThe other option is to declare the current behavior a bug and fix it. I\nam quite tempted by that route, given the inconsistency with other\nformatters, including even \"git tag --list --format=%(refname)\"!\n\n-Peff\n"},{"id":"435978","messageId":"CAOLTT8RPzutEQxbr9cu=ze7rgPKvG6Ghu4b2Bi47eStY1TqGzQ@mail.gmail.com","threadId":"56441","inReplyTo":"YTtrF8C0mmT6kBJT@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-15T12:27:15Z","receivedAt":"2021-09-15T12:27:31Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月10日周五 下午10:26写道：\n>\n> On Thu, Sep 09, 2021 at 10:45:15PM +0800, ZheNing Hu wrote:\n>\n> > @@ -2735,6 +2723,7 @@ void pretty_print_ref(const char *name, const\n> > struct object_id *oid,\n> >\n> > ref_item = new_ref_array_item(name, oid);\n> > ref_item->kind = ref_kind_from_refname(name);\n> > + read_ref_full(name, 0, NULL, &ref_item->flag);\n> > if (format_ref_array_item(ref_item, format, &output, &err))\n> > die(\"%s\", err.buf);\n> > fwrite(output.buf, 1, output.len, stdout);\n>\n> IMHO this is the wrong place to do it, since the caller may already have\n> the flags (and looking up the ref again is a non-trivial cost).\n>\n\nWell, but not doing this means that we have to pass the flag from the\npretty_print_ref() call stack.\n\n> The caller in builtin/tag.c should switch to using read_ref_full() and\n> pass in the flags.\n>\n\nAgree.\n\n> The caller in builtin/verify-tag.c _probably_ should resolve the ref in\n> the same way and pass in that full refname and flags. I do worry that\n> this may be a compatibility problem, but the current behavior seems so\n> broken to me.\n>\n\nYeah.\n\n> > > - I suspect people may be relying on the current behavior. The\n> > > original was added to be able to compare the internal tagname to the\n> > > refname. I.e., that:\n> > >\n> > > git tag -v --format='%(refname) %(tag)' foo\n> > >\n> > > would show \"foo foo\". Now that _should_ be \"%(refname:strip=2)\", I\n> > > think, but we'd probably be breaking scripts. OTOH, it really feels\n> > > like _not_ handing over a real, fully-qualified refname to the\n> > > ref-filter code will mean other things are broken (e.g.,\n> > > ATOM_UPSTREAM is assuming we have a fully-qualified ref).\n> > >\n> >\n> > This is indeed a sad thing: A bug becomes a feature.\n> >\n> > > I think a backwards-compatible way of fixing it would be to have\n> > > this call hand over the full refname to the ref-filter code, but\n> > > tell it that %(refname) should default to strip=2. And then anybody\n> > > who wants the full name can use %(refname:strip=0).\n> > >\n> >\n> > Doesn't this make things more complicated? Those callers of git for-each-ref,\n> > wouldn't our changes like this destroy them?\n>\n> My proposal was that we'd have a specific flag in ref-filter to say\n> \"default refname:strip to this value\". And then _only_ \"tag --verify\"\n> would set that (to \"2\"), leaving for-each-ref, etc unaffected.\n>\n\nIndeed this may be a feasible solution. I will try to do this first.\n\n> So yes, it's complicated. And it must be explained to the user that\n> \"%(refname)\" behaves slightly differently with \"git tag --verify\", but\n> that is unavoidable if we do not want to break scripts (it _already_\n> behaves slightly differently, and we just never told anyone).\n>\n> The other option is to declare the current behavior a bug and fix it. I\n> am quite tempted by that route, given the inconsistency with other\n> formatters, including even \"git tag --list --format=%(refname)\"!\n\nI don't know, I think both fix methods are okay.\n\n>\n> -Peff\n\nThanks.\n--\nZheNing Hu\n"},{"id":"435980","messageId":"CAOLTT8SYxUbfG7YvAs03nwRdee8JfNPUYCCpKcFoAgBjB2oqLw@mail.gmail.com","threadId":"56441","inReplyTo":"CAOLTT8RPzutEQxbr9cu=ze7rgPKvG6Ghu4b2Bi47eStY1TqGzQ@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-15T14:23:43Z","receivedAt":"2021-09-15T14:23:57Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu 于2021年9月15日周三 下午8:27写道：\n>\n> > So yes, it's complicated. And it must be explained to the user that\n> > \"%(refname)\" behaves slightly differently with \"git tag --verify\", but\n> > that is unavoidable if we do not want to break scripts (it _already_\n> > behaves slightly differently, and we just never told anyone).\n> >\n\n$ git tag --verify --format='verify: %(refname) %(symref)' annotated symref\nverify: annotated\nverify: symref\n$ git tag --verify --format='verify: %(refname) %(symref)'\nrefs/tags/annotated refs/tags/symref\nerror: tag 'refs/tags/annotated' not found.\nerror: tag 'refs/tags/symref' not found.\n\n$ git verify-tag --format='verify: %(refname) %(symref)' annotated\nsymref\nverify: annotated\nverify: symref\n$ git verify-tag --format='verify: %(refname) %(symref)'\nrefs/tags/annotated refs/tags/symref\nverify: refs/tags/annotated\nverify: refs/tags/symref\n\nAs we can see, there is a slight difference between git tag --verify and\ngit verify-tag: git tag --verify can not handle refs' fullname refs/tags/*\n(because read_ref_full() | read_ref() can't handle them). So, as a standard,\nwhich characteristics should we keep?\n\n> > The other option is to declare the current behavior a bug and fix it. I\n> > am quite tempted by that route, given the inconsistency with other\n> > formatters, including even \"git tag --list --format=%(refname)\"!\n>\n> I don't know, I think both fix methods are okay.\n>\n> >\n> > -Peff\n>\n> Thanks.\n> --\n> ZheNing Hu\n"},{"id":"436166","messageId":"YUO3wXG3a/fW7AME@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8RPzutEQxbr9cu=ze7rgPKvG6Ghu4b2Bi47eStY1TqGzQ@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T21:31:45Z","receivedAt":"2021-09-16T21:31:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 08:27:15PM +0800, ZheNing Hu wrote:\n\n> > > @@ -2735,6 +2723,7 @@ void pretty_print_ref(const char *name, const\n> > > struct object_id *oid,\n> > >\n> > > ref_item = new_ref_array_item(name, oid);\n> > > ref_item->kind = ref_kind_from_refname(name);\n> > > + read_ref_full(name, 0, NULL, &ref_item->flag);\n> > > if (format_ref_array_item(ref_item, format, &output, &err))\n> > > die(\"%s\", err.buf);\n> > > fwrite(output.buf, 1, output.len, stdout);\n> >\n> > IMHO this is the wrong place to do it, since the caller may already have\n> > the flags (and looking up the ref again is a non-trivial cost).\n> >\n> \n> Well, but not doing this means that we have to pass the flag from the\n> pretty_print_ref() call stack.\n\nRight. But there are only two callers of that function, so I don't think\nit's a big imposition to require it.\n\n> > My proposal was that we'd have a specific flag in ref-filter to say\n> > \"default refname:strip to this value\". And then _only_ \"tag --verify\"\n> > would set that (to \"2\"), leaving for-each-ref, etc unaffected.\n> >\n> \n> Indeed this may be a feasible solution. I will try to do this first.\n\nGreat, thanks for taking a look at it.\n\n-Peff\n"},{"id":"436169","messageId":"YUO63qy2/5wibY4/@coredump.intra.peff.net","threadId":"56441","inReplyTo":"CAOLTT8SYxUbfG7YvAs03nwRdee8JfNPUYCCpKcFoAgBjB2oqLw@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T21:45:02Z","receivedAt":"2021-09-16T21:45:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 15, 2021 at 10:23:43PM +0800, ZheNing Hu wrote:\n\n> ZheNing Hu 于2021年9月15日周三 下午8:27写道：\n> >\n> > > So yes, it's complicated. And it must be explained to the user that\n> > > \"%(refname)\" behaves slightly differently with \"git tag --verify\", but\n> > > that is unavoidable if we do not want to break scripts (it _already_\n> > > behaves slightly differently, and we just never told anyone).\n> > >\n> \n> $ git tag --verify --format='verify: %(refname) %(symref)' annotated symref\n> verify: annotated\n> verify: symref\n> $ git tag --verify --format='verify: %(refname) %(symref)'\n> refs/tags/annotated refs/tags/symref\n> error: tag 'refs/tags/annotated' not found.\n> error: tag 'refs/tags/symref' not found.\n\nThis is expected. When you provide a tag name on the command line of\n\"git tag\" it is assumed to be a non-qualified name in refs/tags/ (and\nditto for git-branch and refs/heads/). It is tempting to try to be\nfriendly and accept fully-qualified refs there, but it would create\nambiguities (e.g., you could really have refs/tags/refs/tags/foo as a\nref).\n\nI think we can ignore that for our purposes here, though. It's a\nquestion of input from the command-line, and we focus on just the output\nthat we produce.\n\n> $ git verify-tag --format='verify: %(refname) %(symref)' annotated\n> symref\n> verify: annotated\n> verify: symref\n> $ git verify-tag --format='verify: %(refname) %(symref)'\n> refs/tags/annotated refs/tags/symref\n> verify: refs/tags/annotated\n> verify: refs/tags/symref\n> \n> As we can see, there is a slight difference between git tag --verify and\n> git verify-tag: git tag --verify can not handle refs' fullname refs/tags/*\n> (because read_ref_full() | read_ref() can't handle them). So, as a standard,\n> which characteristics should we keep?\n\nWhereas are you notice here, verify-tag takes any name (which could be\nfully qualified), and uses it as-is. In fact, it might not even be a ref\nat all! You can say \"git verify-tag c06b72d02\" if you want to. And as a\nplumbing tool, we should make sure this continues to work. For example,\ncareful scripts may resolve a ref into an object, and want to continue\ntalking about that object without worrying about the ref being changed\nsimultaneously.\n\nBut it also creates a weirdness for \"git verify-tag --format\". We do not\nnecessarily even have a ref to show. So IMHO the feature is somewhat\nmis-designed in the first place. But we should probably continue to\nsupport it as best we can.\n\nThe best I can come up with is:\n\n - when we resolve the name, if it was a ref, we should record that.\n I think this is hard to do now. It would probably require\n get_oid_with_context() learning to report on the results it got from\n dwim_ref().\n\n - if we have a refname, then feed it to pretty_print_ref() as a\n fully-qualified name. And pass whatever \"default lstrip=2\" magic we\n come up with for \"git tag --verify\". That would mean that \"git\n verify-tag --format=%(refname) v2.33.0\" would behave the same before\n and after.\n\n - if we didn't get a refname, then...I guess continue to pass the name\n the user gave us into pretty_print_ref()? That would keep \"git\n verify-tag --format=%(refname) c06b72d02\" working as it does today.\n\nThe alternative is to do none of those things, and just document that\n\"verify-tag\" is weird:\n\n - its %(refname) reports whatever you gave it, whether it is a ref or\n not\n\n - some advanced format placeholders like %(symref) may not work if you\n don't pass a fully-qualified ref\n\n-Peff\n"},{"id":"436402","messageId":"CAOLTT8QxZ3rMwrgD3LkqAUb98gdyVC28=b4SQ5xKwGteFvwOFA@mail.gmail.com","threadId":"56441","inReplyTo":"YUO63qy2/5wibY4/@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: hacky \"streaming\" mode","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-09-20T07:42:17Z","receivedAt":"2021-09-20T07:42:39Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King 于2021年9月17日周五 上午5:45写道：\n>\n> On Wed, Sep 15, 2021 at 10:23:43PM +0800, ZheNing Hu wrote:\n>\n> > ZheNing Hu 于2021年9月15日周三 下午8:27写道：\n> > >\n> > > > So yes, it's complicated. And it must be explained to the user that\n> > > > \"%(refname)\" behaves slightly differently with \"git tag --verify\", but\n> > > > that is unavoidable if we do not want to break scripts (it _already_\n> > > > behaves slightly differently, and we just never told anyone).\n> > > >\n> >\n> > $ git tag --verify --format='verify: %(refname) %(symref)' annotated symref\n> > verify: annotated\n> > verify: symref\n> > $ git tag --verify --format='verify: %(refname) %(symref)'\n> > refs/tags/annotated refs/tags/symref\n> > error: tag 'refs/tags/annotated' not found.\n> > error: tag 'refs/tags/symref' not found.\n>\n> This is expected. When you provide a tag name on the command line of\n> \"git tag\" it is assumed to be a non-qualified name in refs/tags/ (and\n> ditto for git-branch and refs/heads/). It is tempting to try to be\n> friendly and accept fully-qualified refs there, but it would create\n> ambiguities (e.g., you could really have refs/tags/refs/tags/foo as a\n> ref).\n>\n\nYeah, maybe you are right, for git tag --verify, there may have ambiguities.\nBut for git verify-tag, if we have tags like \"refs/tags/refs/tags/foo\" and\n\"refs/tags/foo\":\n\n$ git verify-tag --format='verify: %(refname) %(symref)' refs/tags/foo\nwarning: refname 'refs/tags/foo' is ambiguous.\n\nWe see the ambiguities too here.\n\n> I think we can ignore that for our purposes here, though. It's a\n> question of input from the command-line, and we focus on just the output\n> that we produce.\n>\n\nYeah, but using different functions (read_ref_full(), get_oid()) will\naffect what\nkind of input we can provide.\n\n> > $ git verify-tag --format='verify: %(refname) %(symref)' annotated\n> > symref\n> > verify: annotated\n> > verify: symref\n> > $ git verify-tag --format='verify: %(refname) %(symref)'\n> > refs/tags/annotated refs/tags/symref\n> > verify: refs/tags/annotated\n> > verify: refs/tags/symref\n> >\n> > As we can see, there is a slight difference between git tag --verify and\n> > git verify-tag: git tag --verify can not handle refs' fullname refs/tags/*\n> > (because read_ref_full() | read_ref() can't handle them). So, as a standard,\n> > which characteristics should we keep?\n>\n> Whereas are you notice here, verify-tag takes any name (which could be\n> fully qualified), and uses it as-is. In fact, it might not even be a ref\n> at all! You can say \"git verify-tag c06b72d02\" if you want to. And as a\n> plumbing tool, we should make sure this continues to work. For example,\n> careful scripts may resolve a ref into an object, and want to continue\n> talking about that object without worrying about the ref being changed\n> simultaneously.\n>\n\nYes, this feature is very bad. %(refname) seems to do the %(objectname)\nwork.\n\n\n> But it also creates a weirdness for \"git verify-tag --format\". We do not\n> necessarily even have a ref to show. So IMHO the feature is somewhat\n> mis-designed in the first place. But we should probably continue to\n> support it as best we can.\n>\n> The best I can come up with is:\n>\n> - when we resolve the name, if it was a ref, we should record that.\n> I think this is hard to do now. It would probably require\n> get_oid_with_context() learning to report on the results it got from\n> dwim_ref().\n>\n> - if we have a refname, then feed it to pretty_print_ref() as a\n> fully-qualified name. And pass whatever \"default lstrip=2\" magic we\n> come up with for \"git tag --verify\". That would mean that \"git\n> verify-tag --format=%(refname) v2.33.0\" would behave the same before\n> and after.\n>\n> - if we didn't get a refname, then...I guess continue to pass the name\n> the user gave us into pretty_print_ref()? That would keep \"git\n> verify-tag --format=%(refname) c06b72d02\" working as it does today.\n>\n> The alternative is to do none of those things, and just document that\n> \"verify-tag\" is weird:\n>\n> - its %(refname) reports whatever you gave it, whether it is a ref or\n> not\n>\n> - some advanced format placeholders like %(symref) may not work if you\n> don't pass a fully-qualified ref\n>\n> -Peff\n\nThis is my solution according to your above suggection:\nhttps://lore.kernel.org/git/pull.1042.git.1632123476.gitgitgadget@gmail.com/\n\nThanks.\n--\nZheNing Hu\n"}]}