{"thread":{"id":"56376","subject":"[PATCH] fetch: skip formatting updated refs with `--quiet`","startedAt":"2021-08-25T15:45:14Z","lastAt":"2021-08-30T17:17:30Z","messageCount":7,"participants":["Patrick Steinhardt","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433747","messageId":"40c385048a023dbd447c5f0b4c95ff32485e1e23.1629906005.git.ps@pks.im","threadId":"56376","inReplyTo":null,"subject":"[PATCH] fetch: skip formatting updated refs with `--quiet`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T15:45:09Z","receivedAt":"2021-08-25T15:45:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching, Git will by default print a list of all updated refs in a\nnicely formatted table. In order to come up with this table, Git needs\nto iterate refs twice: first to determine the maximum column width, and\na second time to actually format these changed refs.\n\nWhile this table will not be printed in case the user passes `--quiet`,\nwe still go out of our way and do all these steps. In fact, we even do\nmore work compared to not passing `--quiet`: without the flag, we will\nskip all references in the column width computation which have not been\nupdated, but if it is set we will now compute widths for all refs.\n\nFix this issue by completely skipping both preparation of the format and\nformatting data for display in case the user passes `--quiet`, improving\nperformance especially with many refs. The following benchmark shows a\nnice speedup for a quiet mirror-fetch in a repository with 2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]\n      Range (min … max):   26.692 s … 27.068 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]\n      Range (min … max):   25.070 s … 25.314 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.07 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbd..d064b66512 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -757,6 +757,9 @@ static void prepare_format_display(struct ref *ref_map)\n \t\tdie(_(\"configuration fetch.output contains invalid value %s\"),\n \t\t    format);\n \n+\tif (verbosity < 0)\n+\t\treturn;\n+\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n \t\t    !rm->peer_ref ||\n@@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,\n \t\t\t   const char *remote, const char *local,\n \t\t\t   int summary_width)\n {\n-\tint width = (summary_width + strlen(summary) - gettext_width(summary));\n+\tint width;\n+\n+\tif (verbosity < 0)\n+\t\treturn;\n+\n+\twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n-- \n2.33.0\n\n"},{"id":"433754","messageId":"87k0k9cv7c.fsf@evledraar.gmail.com","threadId":"56376","inReplyTo":"40c385048a023dbd447c5f0b4c95ff32485e1e23.1629906005.git.ps@pks.im","subject":"Re: [PATCH] fetch: skip formatting updated refs with `--quiet`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-25T16:57:44Z","receivedAt":"2021-08-25T17:04:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 25 2021, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> When fetching, Git will by default print a list of all updated refs in a\n> nicely formatted table. In order to come up with this table, Git needs\n> to iterate refs twice: first to determine the maximum column width, and\n> a second time to actually format these changed refs.\n>\n> While this table will not be printed in case the user passes `--quiet`,\n> we still go out of our way and do all these steps. In fact, we even do\n> more work compared to not passing `--quiet`: without the flag, we will\n> skip all references in the column width computation which have not been\n> updated, but if it is set we will now compute widths for all refs.\n>\n> Fix this issue by completely skipping both preparation of the format and\n> formatting data for display in case the user passes `--quiet`, improving\n> performance especially with many refs. The following benchmark shows a\n> nice speedup for a quiet mirror-fetch in a repository with 2.3M refs:\n>\n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]\n>       Range (min … max):   26.692 s … 27.068 s    5 runs\n>\n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]\n>       Range (min … max):   25.070 s … 25.314 s    5 runs\n>\n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.07 ± 0.01 times faster than 'HEAD~: git-fetch'\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/fetch.c | 10 +++++++++-\n>  1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index e064687dbd..d064b66512 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -757,6 +757,9 @@ static void prepare_format_display(struct ref *ref_map)\n>  \t\tdie(_(\"configuration fetch.output contains invalid value %s\"),\n>  \t\t    format);\n>  \n> +\tif (verbosity < 0)\n> +\t\treturn;\n> +\n>  \tfor (rm = ref_map; rm; rm = rm->next) {\n>  \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n>  \t\t    !rm->peer_ref ||\n> @@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,\n>  \t\t\t   const char *remote, const char *local,\n>  \t\t\t   int summary_width)\n>  {\n> -\tint width = (summary_width + strlen(summary) - gettext_width(summary));\n> +\tint width;\n> +\n> +\tif (verbosity < 0)\n> +\t\treturn;\n> +\n> +\twidth = (summary_width + strlen(summary) - gettext_width(summary));\n>  \n>  \tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n>  \tif (!compact_format)\n\nI wonder what you think of the below, which is a bit more of a verbose\nsearch/replacement change, but I think ultimately cleaner. We just pass\nthe \"ref_map\" down to all the formatting functions, and the first\nfunction that needs to know the \"compact_format\" or \"refcol_width\"\nlazily computes it (stored in a static variable). So we avoid the tricky\naction at a distance of not preparing certain things because we know\nwe're in the quiet mode.\n\nBut if I understand your change correctly we're on the one hand not\npreparing the format change, but then also not printing this at all now\nunder --quiet, correct? I think it would be good to split up that\nproposed functional change from the optimization of the output, it also\nseems like if that were done the git-fetch docs on --quiet need to be\nchanged.\n\nBut I wonder if once we have the change below, whether the small change\non top of it to just add a fetch.outputRefWidth=123 wouldn't also\nlargely solve the optimization problem you have, i.e. adding a config\nvariable to adjust the width, instead of us auto-discovering it by\nlooping over all refs.\n\nOr maybe not, but I think it would be interesting to see how much of the\nslowdown you have is that v.s. that we end up emitting this output to\nstderr. I.e. is it the I/O of the output or the extra loop that's the\nexpensive part?\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbdc..88a8b3660d4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -707,6 +707,24 @@ static int s_update_ref(const char *action,\n static int refcol_width = 10;\n static int compact_format;\n \n+static int prepared_compact_format;\n+static void prepare_compact_format(void)\n+{\n+\tconst char *format = \"full\";\n+\n+\tif (prepared_compact_format++)\n+\t\treturn;\n+\n+\tgit_config_get_string_tmp(\"fetch.output\", &format);\n+\tif (!strcasecmp(format, \"full\"))\n+\t\tcompact_format = 0;\n+\telse if (!strcasecmp(format, \"compact\"))\n+\t\tcompact_format = 1;\n+\telse\n+\t\tdie(_(\"configuration fetch.output contains invalid value %s\"),\n+\t\t    format);\n+}\n+\n static void adjust_refcol_width(const struct ref *ref)\n {\n \tint max, rlen, llen, len;\n@@ -726,6 +744,7 @@ static void adjust_refcol_width(const struct ref *ref)\n \t * anyway because we don't know if the error explanation part\n \t * will be printed in update_local_ref)\n \t */\n+\tprepare_compact_format();\n \tif (compact_format) {\n \t\tllen = 0;\n \t\tmax = max * 2 / 3;\n@@ -743,19 +762,13 @@ static void adjust_refcol_width(const struct ref *ref)\n \t\trefcol_width = rlen;\n }\n \n-static void prepare_format_display(struct ref *ref_map)\n+static int prepared_refcol_width;\n+static void prepare_refcol_width(struct ref *ref_map)\n {\n \tstruct ref *rm;\n-\tconst char *format = \"full\";\n \n-\tgit_config_get_string_tmp(\"fetch.output\", &format);\n-\tif (!strcasecmp(format, \"full\"))\n-\t\tcompact_format = 0;\n-\telse if (!strcasecmp(format, \"compact\"))\n-\t\tcompact_format = 1;\n-\telse\n-\t\tdie(_(\"configuration fetch.output contains invalid value %s\"),\n-\t\t    format);\n+\tif (prepared_refcol_width++)\n+\t\treturn;\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n@@ -767,9 +780,10 @@ static void prepare_format_display(struct ref *ref_map)\n \t}\n }\n \n-static void print_remote_to_local(struct strbuf *display,\n+static void print_remote_to_local(struct ref *ref_map, struct strbuf *display,\n \t\t\t\t  const char *remote, const char *local)\n {\n+\tprepare_refcol_width(ref_map);\n \tstrbuf_addf(display, \"%-*s -> %s\", refcol_width, remote, local);\n }\n \n@@ -800,7 +814,7 @@ static int find_and_replace(struct strbuf *haystack,\n \treturn 1;\n }\n \n-static void print_compact(struct strbuf *display,\n+static void print_compact(struct ref *ref_map, struct strbuf *display,\n \t\t\t  const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n@@ -816,13 +830,14 @@ static void print_compact(struct strbuf *display,\n \n \tif (!find_and_replace(&r, local, \"*\"))\n \t\tfind_and_replace(&l, remote, \"*\");\n-\tprint_remote_to_local(display, r.buf, l.buf);\n+\tprint_remote_to_local(ref_map, display, r.buf, l.buf);\n \n \tstrbuf_release(&r);\n \tstrbuf_release(&l);\n }\n \n-static void format_display(struct strbuf *display, char code,\n+static void format_display(struct ref *ref_map,\n+\t\t\t   struct strbuf *display, char code,\n \t\t\t   const char *summary, const char *error,\n \t\t\t   const char *remote, const char *local,\n \t\t\t   int summary_width)\n@@ -830,15 +845,17 @@ static void format_display(struct strbuf *display, char code,\n \tint width = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n+\tprepare_compact_format();\n \tif (!compact_format)\n-\t\tprint_remote_to_local(display, remote, local);\n+\t\tprint_remote_to_local(ref_map, display, remote, local);\n \telse\n-\t\tprint_compact(display, remote, local);\n+\t\tprint_compact(ref_map, display, remote, local);\n \tif (error)\n \t\tstrbuf_addf(display, \"  (%s)\", error);\n }\n \n-static int update_local_ref(struct ref *ref,\n+static int update_local_ref(struct ref *ref_map,\n+\t\t\t    struct ref *ref,\n \t\t\t    struct ref_transaction *transaction,\n \t\t\t    const char *remote,\n \t\t\t    const struct ref *remote_ref,\n@@ -857,7 +874,7 @@ static int update_local_ref(struct ref *ref,\n \n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n-\t\t\tformat_display(display, '=', _(\"[up to date]\"), NULL,\n+\t\t\tformat_display(ref_map, display, '=', _(\"[up to date]\"), NULL,\n \t\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n@@ -870,7 +887,7 @@ static int update_local_ref(struct ref *ref,\n \t\t * If this is the head, and it's not okay to update\n \t\t * the head, and the old value of the head isn't empty...\n \t\t */\n-\t\tformat_display(display, '!', _(\"[rejected]\"),\n+\t\tformat_display(ref_map, display, '!', _(\"[rejected]\"),\n \t\t\t       _(\"can't fetch in current branch\"),\n \t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 1;\n@@ -881,12 +898,12 @@ static int update_local_ref(struct ref *ref,\n \t\tif (force || ref->force) {\n \t\t\tint r;\n \t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n-\t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n+\t\t\tformat_display(ref_map, display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t       r ? _(\"unable to update local ref\") : NULL,\n \t\t\t\t       remote, pretty_ref, summary_width);\n \t\t\treturn r;\n \t\t} else {\n-\t\t\tformat_display(display, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n+\t\t\tformat_display(ref_map, display, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n \t\t\t\t       remote, pretty_ref, summary_width);\n \t\t\treturn 1;\n \t\t}\n@@ -918,7 +935,7 @@ static int update_local_ref(struct ref *ref,\n \t\t}\n \n \t\tr = s_update_ref(msg, ref, transaction, 0);\n-\t\tformat_display(display, r ? '!' : '*', what,\n+\t\tformat_display(ref_map, display, r ? '!' : '*', what,\n \t\t\t       r ? _(\"unable to update local ref\") : NULL,\n \t\t\t       remote, pretty_ref, summary_width);\n \t\treturn r;\n@@ -940,7 +957,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n \t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n-\t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n+\t\tformat_display(ref_map, display, r ? '!' : ' ', quickref.buf,\n \t\t\t       r ? _(\"unable to update local ref\") : NULL,\n \t\t\t       remote, pretty_ref, summary_width);\n \t\tstrbuf_release(&quickref);\n@@ -952,13 +969,13 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n \t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n-\t\tformat_display(display, r ? '!' : '+', quickref.buf,\n+\t\tformat_display(ref_map, display, r ? '!' : '+', quickref.buf,\n \t\t\t       r ? _(\"unable to update local ref\") : _(\"forced update\"),\n \t\t\t       remote, pretty_ref, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n-\t\tformat_display(display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n+\t\tformat_display(ref_map, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n \t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1111,8 +1128,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n-\tprepare_format_display(ref_map);\n-\n \t/*\n \t * We do a pass for each fetch_head_status type in their enum order, so\n \t * merged entries are written before not-for-merge. That lets readers\n@@ -1187,8 +1202,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(&note);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, transaction, what,\n-\t\t\t\t\t\t       rm, &note, summary_width);\n+\t\t\t\trc |= update_local_ref(ref_map, ref,\n+\t\t\t\t\t\t       transaction, what, rm,\n+\t\t\t\t\t\t       &note, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n@@ -1196,7 +1212,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t * would be written to FETCH_HEAD, if --dry-run\n \t\t\t\t * is set).\n \t\t\t\t */\n-\t\t\t\tformat_display(&note, '*',\n+\t\t\t\tformat_display(ref_map, &note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n@@ -1357,7 +1373,7 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"), url_len, url);\n \t\t\t\tshown_url = 1;\n \t\t\t}\n-\t\t\tformat_display(&sb, '-', _(\"[deleted]\"), NULL,\n+\t\t\tformat_display(ref_map, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t       _(\"(none)\"), prettify_refname(ref->name),\n \t\t\t\t       summary_width);\n \t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n"},{"id":"433757","messageId":"xmqqv93tif9g.fsf@gitster.g","threadId":"56376","inReplyTo":"40c385048a023dbd447c5f0b4c95ff32485e1e23.1629906005.git.ps@pks.im","subject":"Re: [PATCH] fetch: skip formatting updated refs with `--quiet`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T17:51:55Z","receivedAt":"2021-08-25T17:52:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When fetching, Git will by default print a list of all updated refs in a\n> nicely formatted table. In order to come up with this table, Git needs\n> to iterate refs twice: first to determine the maximum column width, and\n> a second time to actually format these changed refs.\n>\n> While this table will not be printed in case the user passes `--quiet`,\n> we still go out of our way and do all these steps. In fact, we even do\n> more work compared to not passing `--quiet`: without the flag, we will\n> skip all references in the column width computation which have not been\n> updated, but if it is set we will now compute widths for all refs.\n\nInteresting.  This line\n\n\t/* uptodate lines are only shown on high verbosity level */\n\tif (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n\t\treturn;\n\nat the beginning of the adjust_refcol_width() function indeed does\nnot skip if verbosity is negative, so the comment is wrong---it is\nnot only computed on high verbosity level.  Why doesn't this patch\ninclude a change like this then?\n\n\tif (verbosity <= 0 || oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n\t\treturn;\n\nAnother thing I notice is this part from store_updated_refs():\n\n\t\t\tif (note.len) {\n\t\t\t\tif (verbosity >= 0 && !shown_url) {\n\t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n\t\t\t\t\t\t\turl_len, url);\n\t\t\t\t\tshown_url = 1;\n\t\t\t\t}\n\t\t\t\tif (verbosity >= 0)\n\t\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n\t\t\t}\n\nWe no longer need to check for verbosity, right?\n\nOther than these two, I like the approach a lot.\n\n"},{"id":"433758","messageId":"YSaF2h67u1WTz1b3@tanuki","threadId":"56376","inReplyTo":"xmqqv93tif9g.fsf@gitster.g","subject":"Re: [PATCH] fetch: skip formatting updated refs with `--quiet`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T18:03:06Z","receivedAt":"2021-08-25T18:00:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 25, 2021 at 10:51:55AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > When fetching, Git will by default print a list of all updated refs in a\n> > nicely formatted table. In order to come up with this table, Git needs\n> > to iterate refs twice: first to determine the maximum column width, and\n> > a second time to actually format these changed refs.\n> >\n> > While this table will not be printed in case the user passes `--quiet`,\n> > we still go out of our way and do all these steps. In fact, we even do\n> > more work compared to not passing `--quiet`: without the flag, we will\n> > skip all references in the column width computation which have not been\n> > updated, but if it is set we will now compute widths for all refs.\n> \n> Interesting.  This line\n> \n> \t/* uptodate lines are only shown on high verbosity level */\n> \tif (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n> \t\treturn;\n> \n> at the beginning of the adjust_refcol_width() function indeed does\n> not skip if verbosity is negative, so the comment is wrong---it is\n> not only computed on high verbosity level.  Why doesn't this patch\n> include a change like this then?\n> \n> \tif (verbosity <= 0 || oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n> \t\treturn;\n\nThis was indeed my first iteration. But if we just fix the condition\nlike you do here, then we still iterate over all refs even though we\nknow that we're not going to do anything with them. So my version just\nskips over the iteration completely.\n\n> Another thing I notice is this part from store_updated_refs():\n> \n> \t\t\tif (note.len) {\n> \t\t\t\tif (verbosity >= 0 && !shown_url) {\n> \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n> \t\t\t\t\t\t\turl_len, url);\n> \t\t\t\t\tshown_url = 1;\n> \t\t\t\t}\n> \t\t\t\tif (verbosity >= 0)\n> \t\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n> \t\t\t}\n> \n> We no longer need to check for verbosity, right?\n\nRight. It would be less obvious though that we indeed never print to the\nbuffer if `verbosity < 0`, which is why I bailed on that refactoring.\n\nI wonder whether we can just refactor this such that the buffer is\ncaller-provided. If `--quiet`, the caller just passes a `NULL` pointer\nso it's explicit that it cannot be written to. This would also address\nÆvar's feedback about the \"tricky action at a distance\", which is valid\ncriticism in my eyes.\n\nPatrick\n"},{"id":"433760","messageId":"xmqqo89lid59.fsf@gitster.g","threadId":"56376","inReplyTo":"YSaF2h67u1WTz1b3@tanuki","subject":"Re: [PATCH] fetch: skip formatting updated refs with `--quiet`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T18:37:38Z","receivedAt":"2021-08-25T18:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Interesting.  This line\n>> \n>> \t/* uptodate lines are only shown on high verbosity level */\n>> \tif (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n>> \t\treturn;\n>> \n>> at the beginning of the adjust_refcol_width() function indeed does\n>> not skip if verbosity is negative, so the comment is wrong---it is\n>> not only computed on high verbosity level.  Why doesn't this patch\n>> include a change like this then?\n>> \n>> \tif (verbosity <= 0 || oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n>> \t\treturn;\n>\n> This was indeed my first iteration. But if we just fix the condition\n> like you do here, then we still iterate over all refs even though we\n> know that we're not going to do anything with them. So my version just\n> skips over the iteration completely.\n\nI didn't ask \"why isn't this patch the above change and nothing else?\"\n\nWith or without the \"don't bother counting columns under --quiet\"\nfix, the condition used in the above if statement is wrong.  With\nthe fix, only because the function is never called with negative\nverbosity, \"if (!verbosity)\" means the same thing as \"only shown on\nhigh verbosity level\", but the reader must have followed the flow of\nlogic to realize it.  If you fix the condition to exclude all\nnon-verbose cases (including --quiet), the readers do not have to do\nso to realize that the code is doing what the comment claims that it\nis doing.\n\n>> Another thing I notice is this part from store_updated_refs():\n>> \n>> \t\t\tif (note.len) {\n>> \t\t\t\tif (verbosity >= 0 && !shown_url) {\n>> \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n>> \t\t\t\t\t\t\turl_len, url);\n>> \t\t\t\t\tshown_url = 1;\n>> \t\t\t\t}\n>> \t\t\t\tif (verbosity >= 0)\n>> \t\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n>> \t\t\t}\n>> \n>> We no longer need to check for verbosity, right?\n>\n> Right. It would be less obvious though that we indeed never print to the\n> buffer if `verbosity < 0`, which is why I bailed on that refactoring.\n\nI actually think that the resulting code will still be obvious, even\nwith the (apparently unrelated) URL display, and actually even be\nbetter, if we drop the condition on verbosity from this code.  \n\nThe code that led to this part would have stuffed bytes in the note\nstrbuf only when we wanted to show something based on the verbosity\nsetting, and we show what is in note.len only when we have something\nto say (the URL thing is to give the header for the message).\nDecision to give what contents to show (or not to show at all) is\nmade elsewhere---it happens to involve verbosity and perhaps in the\nfuture it may use some other input, but this part of the code does\nnot have to know.\n\nThanks.\n"},{"id":"434062","messageId":"e5ffa17753d4aca57d486d500a2d114290361ea7.1630320848.git.ps@pks.im","threadId":"56376","inReplyTo":"40c385048a023dbd447c5f0b4c95ff32485e1e23.1629906005.git.ps@pks.im","subject":"[PATCH v2] fetch: skip formatting updated refs with `--quiet`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-30T10:54:26Z","receivedAt":"2021-08-30T10:54:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching, Git will by default print a list of all updated refs in a\nnicely formatted table. In order to come up with this table, Git needs\nto iterate refs twice: first to determine the maximum column width, and\na second time to actually format these changed refs.\n\nWhile this table will not be printed in case the user passes `--quiet`,\nwe still go out of our way and do all these steps. In fact, we even do\nmore work compared to not passing `--quiet`: without the flag, we will\nskip all references in the column width computation which have not been\nupdated, but if it is set we will now compute widths for all refs.\n\nFix this issue by completely skipping both preparation of the format and\nformatting data for display in case the user passes `--quiet`, improving\nperformance especially with many refs. The following benchmark shows a\nnice speedup for a quiet mirror-fetch in a repository with 2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]\n      Range (min … max):   26.692 s … 27.068 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]\n      Range (min … max):   25.070 s … 25.314 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.07 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nWhile at it, this patch also fixes `adjust_refcol_width()` such that it\nskips unchanged refs in case the user passed `--quiet`, where verbosity\nwill be negative. While this function won't be called anymore if so,\nthis brings the comment in line with actual code. Furthermore, needless\n`verbosity >= 0` checks are now removed in `store_updated_refs()`: we\nnever print to the `note` buffer anymore in case `verbosity < 0`, so we\nwon't end up in that code block anyway.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nRange-diff against v1:\n1:  40c385048a ! 1:  e5ffa17753 fetch: skip formatting updated refs with `--quiet`\n    @@ Commit message\n               'HEAD: git-fetch' ran\n                 1.07 ± 0.01 times faster than 'HEAD~: git-fetch'\n     \n    +    While at it, this patch also fixes `adjust_refcol_width()` such that it\n    +    skips unchanged refs in case the user passed `--quiet`, where verbosity\n    +    will be negative. While this function won't be called anymore if so,\n    +    this brings the comment in line with actual code. Furthermore, needless\n    +    `verbosity >= 0` checks are now removed in `store_updated_refs()`: we\n    +    never print to the `note` buffer anymore in case `verbosity < 0`, so we\n    +    won't end up in that code block anyway.\n    +\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## builtin/fetch.c ##\n    +@@ builtin/fetch.c: static void adjust_refcol_width(const struct ref *ref)\n    + \tint max, rlen, llen, len;\n    + \n    + \t/* uptodate lines are only shown on high verbosity level */\n    +-\tif (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n    ++\tif (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n    + \t\treturn;\n    + \n    + \tmax    = term_columns();\n     @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)\n    - \t\tdie(_(\"configuration fetch.output contains invalid value %s\"),\n    - \t\t    format);\n    + \tstruct ref *rm;\n    + \tconst char *format = \"full\";\n      \n     +\tif (verbosity < 0)\n     +\t\treturn;\n     +\n    - \tfor (rm = ref_map; rm; rm = rm->next) {\n    - \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n    - \t\t    !rm->peer_ref ||\n    + \tgit_config_get_string_tmp(\"fetch.output\", &format);\n    + \tif (!strcasecmp(format, \"full\"))\n    + \t\tcompact_format = 0;\n     @@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,\n      \t\t\t   const char *remote, const char *local,\n      \t\t\t   int summary_width)\n    @@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,\n      \n      \tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n      \tif (!compact_format)\n    +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n    + \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n    + \t\t\t}\n    + \t\t\tif (note.len) {\n    +-\t\t\t\tif (verbosity >= 0 && !shown_url) {\n    ++\t\t\t\tif (!shown_url) {\n    + \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n    + \t\t\t\t\t\t\turl_len, url);\n    + \t\t\t\t\tshown_url = 1;\n    + \t\t\t\t}\n    +-\t\t\t\tif (verbosity >= 0)\n    +-\t\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n    ++\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n    + \t\t\t}\n    + \t\t}\n    + \t}\n\n builtin/fetch.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbd..fc7b6bb84e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -712,7 +712,7 @@ static void adjust_refcol_width(const struct ref *ref)\n \tint max, rlen, llen, len;\n \n \t/* uptodate lines are only shown on high verbosity level */\n-\tif (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n+\tif (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n \t\treturn;\n \n \tmax    = term_columns();\n@@ -748,6 +748,9 @@ static void prepare_format_display(struct ref *ref_map)\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n+\tif (verbosity < 0)\n+\t\treturn;\n+\n \tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n \t\tcompact_format = 0;\n@@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,\n \t\t\t   const char *remote, const char *local,\n \t\t\t   int summary_width)\n {\n-\tint width = (summary_width + strlen(summary) - gettext_width(summary));\n+\tint width;\n+\n+\tif (verbosity < 0)\n+\t\treturn;\n+\n+\twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n@@ -1202,13 +1210,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n \t\t\t}\n \t\t\tif (note.len) {\n-\t\t\t\tif (verbosity >= 0 && !shown_url) {\n+\t\t\t\tif (!shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n \t\t\t\t\t\t\turl_len, url);\n \t\t\t\t\tshown_url = 1;\n \t\t\t\t}\n-\t\t\t\tif (verbosity >= 0)\n-\t\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n+\t\t\t\tfprintf(stderr, \" %s\\n\", note.buf);\n \t\t\t}\n \t\t}\n \t}\n-- \n2.33.0\n\n"},{"id":"434124","messageId":"xmqqo89e4ztk.fsf@gitster.g","threadId":"56376","inReplyTo":"e5ffa17753d4aca57d486d500a2d114290361ea7.1630320848.git.ps@pks.im","subject":"Re: [PATCH v2] fetch: skip formatting updated refs with `--quiet`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T17:17:27Z","receivedAt":"2021-08-30T17:17:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When fetching, Git will by default print a list of all updated refs in a\n> nicely formatted table. In order to come up with this table, Git needs\n> to iterate refs twice: first to determine the maximum column width, and\n> a second time to actually format these changed refs.\n> ...\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nWill queue.  Looks sensible.\n\nThanks.\n"}]}