{"thread":{"id":"59399","subject":"[PATCH 0/8] fetch: refactor code that prints reference updates","startedAt":"2023-03-15T11:23:00Z","lastAt":"2023-03-29T18:45:20Z","messageCount":39,"participants":["Patrick Steinhardt","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"473513","messageId":"cover.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":null,"subject":"[PATCH 0/8] fetch: refactor code that prints reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:01Z","receivedAt":"2023-03-15T11:23:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nat GitLab, we want to gain more control over fetches to achieve two\ndifferent things:\n\n 1. We want to take control of the reference updates so that we can\n atomically update all or a subset of references that git-fetch\n would have updated.\n\n 2. We want to be able to quarantine objects in a fetch so that we\n can e.g. perform consistency checks for them before they land in\n the main repository.\n\nTo do this, we aim to use git-fetch(1)'s `--dry-run` mode with a\nmanually set up quarantine directory. One issue we currently face though\nis that git-fetch(1), to the best of my knowledge, has no mode in which\nit would print all reference updates in a machine-parseable format.\n\nI thus set out to implement a \"porcelain\"-style mode for git-fetch(1)\nthat surfaces this information:\n\n - The reference that would be updated.\n\n - The remote reference this is coming from.\n\n - The old and new object IDs of the reference.\n\n - Whether there's any error, like a D/F conflict.\n\nI had a hard time understanding the current implementation of how ref\nupdates are printed though. So as a first step towards such a porcelain\nmode this patch series refactors said code. It sets out to achieve two\nmajor goals:\n\n - There should be as few global state as possible. This is to reduce\n confusion and having to repeat the same incantations in multiple\n different locations.\n\n - The logic should be as self-contained as possible. This is so that\n it can easily be changed in a subsequent patch series.\n\nThis patch series does exactly that, but does not yet introduce the new\nmachine-parsebale porcelain mode.\n\nPatrick\n\nPatrick Steinhardt (8):\n fetch: rename `display` buffer to avoid name conflict\n fetch: move reference width calculation into `display_state`\n fetch: move output format into `display_state`\n fetch: pass the full local reference name to `format_display`\n fetch: deduplicate handling of per-reference format\n fetch: deduplicate logic to print remote URL\n fetch: fix inconsistent summary width for pruned and updated refs\n fetch: centralize printing of reference updates\n\n builtin/fetch.c | 270 ++++++++++++++++++++++++------------------------\n 1 file changed, 134 insertions(+), 136 deletions(-)\n\n-- \n2.40.0\n\n"},{"id":"473514","messageId":"692206f7ff81132c47405a3120b0e0cb2d79ad90.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 1/8] fetch: rename `display` buffer to avoid name conflict","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:05Z","receivedAt":"2023-03-15T11:23:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Rename the `display` buffer parameter we use to print reference updates\nto standard error to `display_buffer`. This is done in order to avoid a\nname conflict with the new `display_state` structure we're about to\nintroduce.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a09606b472..087e5cd6f4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -807,10 +807,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 strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n-\tstrbuf_addf(display, \"%-*s -> %s\", refcol_width, remote, local);\n+\tstrbuf_addf(display_buffer, \"%-*s -> %s\", refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n@@ -840,14 +840,14 @@ 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 strbuf *display_buffer,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n-\t\tstrbuf_addf(display, \"%-*s -> *\", refcol_width, remote);\n+\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", refcol_width, remote);\n \t\treturn;\n \t}\n \n@@ -856,13 +856,13 @@ 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(display_buffer, 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 strbuf *display_buffer, 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@@ -874,19 +874,19 @@ static void format_display(struct strbuf *display, char code,\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n+\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n-\t\tprint_remote_to_local(display, remote, local);\n+\t\tprint_remote_to_local(display_buffer, remote, local);\n \telse\n-\t\tprint_compact(display, remote, local);\n+\t\tprint_compact(display_buffer, remote, local);\n \tif (error)\n-\t\tstrbuf_addf(display, \" (%s)\", error);\n+\t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t const char *remote, const struct ref *remote_ref,\n-\t\t\t struct strbuf *display, int summary_width)\n+\t\t\t struct strbuf *display_buffer, int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n \tconst char *pretty_ref = prettify_refname(ref->name);\n@@ -897,7 +897,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(display_buffer, '=', _(\"[up to date]\"), NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n@@ -909,7 +909,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(display_buffer, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n@@ -920,12 +920,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(display_buffer, 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(display_buffer, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\t\treturn 1;\n \t\t}\n@@ -957,7 +957,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(display_buffer, 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@@ -979,7 +979,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(display_buffer, 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@@ -991,13 +991,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(display_buffer, 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(display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n-- \n2.40.0\n\n"},{"id":"473515","messageId":"aa792b12a468263d05e8615d4b3691ed8fe823ff.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:09Z","receivedAt":"2023-03-15T11:23:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to print references in proper columns we need to calculate the\nwidth of the reference column before starting to print the references.\nThis is done with the help of a global variable `refcol_width`.\n\nRefactor the code to instead use a new structure `display_state` that\ncontains the computed width and plumb it through the stack as required.\nThis is only the first step towards de-globalizing the state required to\nprint references.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 107 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 63 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 087e5cd6f4..3695299177 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -47,6 +47,10 @@ enum {\n \tTAGS_SET = 2\n };\n \n+struct display_state {\n+\tint refcol_width;\n+};\n+\n static int fetch_prune_config = -1; /* unspecified */\n static int fetch_show_forced_updates = 1;\n static uint64_t forced_updates_ms = 0;\n@@ -741,16 +745,15 @@ static int s_update_ref(const char *action,\n \treturn ret;\n }\n \n-static int refcol_width = 10;\n static int compact_format;\n \n-static void adjust_refcol_width(const struct ref *ref)\n+static int refcol_width(const struct ref *ref)\n {\n \tint max, rlen, llen, len;\n \n \t/* uptodate lines are only shown on high verbosity level */\n \tif (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n-\t\treturn;\n+\t\treturn 0;\n \n \tmax = term_columns();\n \trlen = utf8_strwidth(prettify_refname(ref->name));\n@@ -769,22 +772,18 @@ static void adjust_refcol_width(const struct ref *ref)\n \t}\n \tlen = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;\n \tif (len >= max)\n-\t\treturn;\n+\t\treturn 0;\n \n-\t/*\n-\t * Not precise calculation for compact mode because '*' can\n-\t * appear on the left hand side of '->' and shrink the column\n-\t * back.\n-\t */\n-\tif (refcol_width < rlen)\n-\t\trefcol_width = rlen;\n+\treturn rlen;\n }\n \n-static void prepare_format_display(struct ref *ref_map)\n+static void display_state_init(struct display_state *display, struct ref *ref_map)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n+\tmemset(display, 0, sizeof(*display));\n+\n \tif (verbosity < 0)\n \t\treturn;\n \n@@ -797,20 +796,32 @@ static void prepare_format_display(struct ref *ref_map)\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n \n+\tdisplay->refcol_width = 10;\n \tfor (rm = ref_map; rm; rm = rm->next) {\n+\t\tint width;\n+\n \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n \t\t !rm->peer_ref ||\n \t\t !strcmp(rm->name, \"HEAD\"))\n \t\t\tcontinue;\n \n-\t\tadjust_refcol_width(rm);\n+\t\twidth = refcol_width(rm);\n+\n+\t\t/*\n+\t\t * Not precise calculation for compact mode because '*' can\n+\t\t * appear on the left hand side of '->' and shrink the column\n+\t\t * back.\n+\t\t */\n+\t\tif (display->refcol_width < width)\n+\t\t\tdisplay->refcol_width = width;\n \t}\n }\n \n-static void print_remote_to_local(struct strbuf *display_buffer,\n+static void print_remote_to_local(struct display_state *display,\n+\t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n-\tstrbuf_addf(display_buffer, \"%-*s -> %s\", refcol_width, remote, local);\n+\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display->refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n@@ -840,14 +851,14 @@ static int find_and_replace(struct strbuf *haystack,\n \treturn 1;\n }\n \n-static void print_compact(struct strbuf *display_buffer,\n+static void print_compact(struct display_state *display, struct strbuf *display_buffer,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n-\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", refcol_width, remote);\n+\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display->refcol_width, remote);\n \t\treturn;\n \t}\n \n@@ -856,13 +867,14 @@ static void print_compact(struct strbuf *display_buffer,\n \n \tif (!find_and_replace(&r, local, \"*\"))\n \t\tfind_and_replace(&l, remote, \"*\");\n-\tprint_remote_to_local(display_buffer, r.buf, l.buf);\n+\tprint_remote_to_local(display, display_buffer, r.buf, l.buf);\n \n \tstrbuf_release(&r);\n \tstrbuf_release(&l);\n }\n \n-static void format_display(struct strbuf *display_buffer, char code,\n+static void format_display(struct display_state *display,\n+\t\t\t struct strbuf *display_buffer, 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@@ -876,15 +888,16 @@ static void format_display(struct strbuf *display_buffer, char code,\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n-\t\tprint_remote_to_local(display_buffer, remote, local);\n+\t\tprint_remote_to_local(display, display_buffer, remote, local);\n \telse\n-\t\tprint_compact(display_buffer, remote, local);\n+\t\tprint_compact(display, display_buffer, remote, local);\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n+\t\t\t struct display_state *display,\n \t\t\t const char *remote, const struct ref *remote_ref,\n \t\t\t struct strbuf *display_buffer, int summary_width)\n {\n@@ -897,7 +910,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_buffer, '=', _(\"[up to date]\"), NULL,\n+\t\t\tformat_display(display, display_buffer, '=', _(\"[up to date]\"), NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n@@ -909,7 +922,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_buffer, '!', _(\"[rejected]\"),\n+\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n@@ -920,12 +933,13 @@ 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_buffer, r ? '!' : 't', _(\"[tag update]\"),\n+\t\t\tformat_display(display, display_buffer, 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_buffer, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n+\t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n+\t\t\t\t _(\"would clobber existing tag\"),\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\t\treturn 1;\n \t\t}\n@@ -957,7 +971,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_buffer, r ? '!' : '*', what,\n+\t\tformat_display(display, display_buffer, 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@@ -979,7 +993,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_buffer, r ? '!' : ' ', quickref.buf,\n+\t\tformat_display(display, display_buffer, 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@@ -991,13 +1005,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_buffer, r ? '!' : '+', quickref.buf,\n+\t\tformat_display(display, display_buffer, 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_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n+\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1108,7 +1122,8 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\\n\"\n \"to avoid this check\\n\");\n \n-static int store_updated_refs(const char *raw_url, const char *remote_name,\n+static int store_updated_refs(struct display_state *display,\n+\t\t\t const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n@@ -1139,8 +1154,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@@ -1240,7 +1253,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, transaction, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, display, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1249,7 +1262,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(¬e, '*',\n+\t\t\t\tformat_display(display, ¬e, '*',\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@@ -1328,7 +1341,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_and_consume_refs(struct transport *transport,\n+static int fetch_and_consume_refs(struct display_state *display,\n+\t\t\t\t struct transport *transport,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct ref *ref_map,\n \t\t\t\t struct fetch_head *fetch_head)\n@@ -1352,7 +1366,7 @@ static int fetch_and_consume_refs(struct transport *transport,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(transport->url, transport->remote->name,\n+\tret = store_updated_refs(display, transport->url, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n@@ -1362,7 +1376,8 @@ static int fetch_and_consume_refs(struct transport *transport,\n \treturn ret;\n }\n \n-static int prune_refs(struct refspec *rs,\n+static int prune_refs(struct display_state *display,\n+\t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n \t\t struct ref *ref_map,\n \t\t const char *raw_url)\n@@ -1416,7 +1431,7 @@ static int prune_refs(struct refspec *rs,\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(display, &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@@ -1542,7 +1557,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \treturn transport;\n }\n \n-static int backfill_tags(struct transport *transport,\n+static int backfill_tags(struct display_state *display,\n+\t\t\t struct transport *transport,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n@@ -1566,7 +1582,7 @@ static int backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);\n+\tretcode = fetch_and_consume_refs(display, transport, transaction, ref_map, fetch_head);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1581,6 +1597,7 @@ static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n+\tstruct display_state display;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n@@ -1662,6 +1679,8 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n+\tdisplay_state_init(&display, ref_map);\n+\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction) {\n@@ -1679,9 +1698,9 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tretcode = prune_refs(rs, transaction, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map, transport->url);\n \t\t} else {\n-\t\t\tretcode = prune_refs(&transport->remote->fetch,\n+\t\t\tretcode = prune_refs(&display, &transport->remote->fetch,\n \t\t\t\t\t transaction, ref_map,\n \t\t\t\t\t transport->url);\n \t\t}\n@@ -1689,7 +1708,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n-\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {\n+\tif (fetch_and_consume_refs(&display, transport, transaction, ref_map, &fetch_head)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1711,7 +1730,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t * when `--atomic` is passed: in that case we'll abort\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n-\t\t\tif (backfill_tags(transport, transaction, tags_ref_map,\n+\t\t\tif (backfill_tags(&display, transport, transaction, tags_ref_map,\n \t\t\t\t\t &fetch_head))\n \t\t\t\tretcode = 1;\n \t\t}\n-- \n2.40.0\n\n"},{"id":"473516","messageId":"a4fd935c40575503c4d708bd19c60f5134fb073b.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 3/8] fetch: move output format into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:15Z","receivedAt":"2023-03-15T11:23:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-fetch(1) command supports printing references either in \"full\"\nor \"compact\" format depending on the `fetch.ouput` config key. The\nformat that is to be used is tracked in a global variable.\n\nDe-globalize the variable by moving it into the `display_state`\nstructure.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3695299177..f9ed9dac32 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -49,6 +49,7 @@ enum {\n \n struct display_state {\n \tint refcol_width;\n+\tint compact_format;\n };\n \n static int fetch_prune_config = -1; /* unspecified */\n@@ -745,9 +746,7 @@ static int s_update_ref(const char *action,\n \treturn ret;\n }\n \n-static int compact_format;\n-\n-static int refcol_width(const struct ref *ref)\n+static int refcol_width(const struct ref *ref, int compact_format)\n {\n \tint max, rlen, llen, len;\n \n@@ -789,9 +788,9 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \n \tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n-\t\tcompact_format = 0;\n+\t\tdisplay->compact_format = 0;\n \telse if (!strcasecmp(format, \"compact\"))\n-\t\tcompact_format = 1;\n+\t\tdisplay->compact_format = 1;\n \telse\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n@@ -805,7 +804,7 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \t\t !strcmp(rm->name, \"HEAD\"))\n \t\t\tcontinue;\n \n-\t\twidth = refcol_width(rm);\n+\t\twidth = refcol_width(rm, display->compact_format);\n \n \t\t/*\n \t\t * Not precise calculation for compact mode because '*' can\n@@ -887,7 +886,7 @@ static void format_display(struct display_state *display,\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n-\tif (!compact_format)\n+\tif (!display->compact_format)\n \t\tprint_remote_to_local(display, display_buffer, remote, local);\n \telse\n \t\tprint_compact(display, display_buffer, remote, local);\n-- \n2.40.0\n\n"},{"id":"473517","messageId":"0998173b57e8d7aea7a77673fa8e54dc54538a9b.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 4/8] fetch: pass the full local reference name to `format_display`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:19Z","receivedAt":"2023-03-15T11:23:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Before printing the name of the local references that would be updated\nby a fetch we first prettify the reference name. This is done at the\ncalling side so that `format_display()` never sees the full name of the\nlocal reference. This restricts our ability to introduce new output\nformats that might want to print the full reference name.\n\nRight now, all callsites except one are prettifying the reference name\nanyway. And the only callsite that doesn't passes `FETCH_HEAD` as the\nhardcoded reference name to `format_display()`, which would never be\nchanged by a call to `prettify_refname()` anyway. So let's refactor the\ncode to pass in the full local reference name and then prettify it in\nthe formatting code.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f9ed9dac32..bf2f01245a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -887,9 +887,9 @@ static void format_display(struct display_state *display,\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!display->compact_format)\n-\t\tprint_remote_to_local(display, display_buffer, remote, local);\n+\t\tprint_remote_to_local(display, display_buffer, remote, prettify_refname(local));\n \telse\n-\t\tprint_compact(display, display_buffer, remote, local);\n+\t\tprint_compact(display, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n@@ -901,7 +901,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct strbuf *display_buffer, int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n \tif (!repo_has_object_file(the_repository, &ref->new_oid))\n@@ -910,7 +909,7 @@ static int update_local_ref(struct ref *ref,\n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n \t\t\tformat_display(display, display_buffer, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 0;\n \t}\n \n@@ -923,7 +922,7 @@ static int update_local_ref(struct ref *ref,\n \t\t */\n \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n-\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n \n@@ -934,12 +933,12 @@ static int update_local_ref(struct ref *ref,\n \t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, display_buffer, 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\t\t remote, ref->name, summary_width);\n \t\t\treturn r;\n \t\t} else {\n \t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n \t\t\t\t _(\"would clobber existing tag\"),\n-\t\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t\t remote, ref->name, summary_width);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -972,7 +971,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, display_buffer, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t remote, ref->name, summary_width);\n \t\treturn r;\n \t}\n \n@@ -994,7 +993,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, display_buffer, 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\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -1006,12 +1005,12 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, display_buffer, 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\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n }\n@@ -1431,7 +1430,7 @@ static int prune_refs(struct display_state *display,\n \t\t\t\tshown_url = 1;\n \t\t\t}\n \t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t _(\"(none)\"), prettify_refname(ref->name),\n+\t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n \t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n \t\t\tstrbuf_release(&sb);\n-- \n2.40.0\n\n"},{"id":"473518","messageId":"d45ec31eeaf42ee042bad04efd69668144df3138.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:23Z","receivedAt":"2023-03-15T11:24:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both callsites that call `format_display()` and then print the result to\nstandard error use the same formatting directive \" %s\\n\" to print the\nreference to disk, thus duplicating a small part of the logic.\n\nRefactor the code to handle this in `format_display()` itself. This\npaves the way for handling the printing logic in that function\ncompletely.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bf2f01245a..6fc2fd0d46 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -885,13 +885,14 @@ static void format_display(struct display_state *display,\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n+\tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n \tif (!display->compact_format)\n \t\tprint_remote_to_local(display, display_buffer, remote, prettify_refname(local));\n \telse\n \t\tprint_compact(display, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n+\tstrbuf_addch(display_buffer, '\\n');\n }\n \n static int update_local_ref(struct ref *ref,\n@@ -1271,7 +1272,7 @@ static int store_updated_refs(struct display_state *display,\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\tfprintf(stderr, \" %s\\n\", note.buf);\n+\t\t\t\tfputs(note.buf, stderr);\n \t\t\t}\n \t\t}\n \t}\n@@ -1432,7 +1433,7 @@ static int prune_refs(struct display_state *display,\n \t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n-\t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n+\t\t\tfputs(sb.buf, stderr);\n \t\t\tstrbuf_release(&sb);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n \t\t}\n-- \n2.40.0\n\n"},{"id":"473519","messageId":"f67f9640a853b605dd1bc4be25e9988c4f059684.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:32Z","receivedAt":"2023-03-15T11:24:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When printing references that are updated during a fetch, we also print\na short summary of what is happening. This summary may either be a word\nlike \"[up to date]\" or \"[rejected]\", or the abbreviated old and new hash\non a reference update.\n\nAs the abbreviated hashes may have different lengths in order to be\nunique we thus need to precompute the width of the summary's column by\niterating through all the objects. This is done in two locations: once\nto compute the width for references that are to be pruned, and once for\nall the other references. Consequentially, it can happen that the width\nas calculated for these sets of references is different.\n\nThis isn't really a huge issue, but it is a bit confusing when reading\nthrough the code. Furthermore, the current way of passing through the\nsummary width is rather verbose.\n\nRefactor the code to compute the summary width once when initializing\nthe display state. Like this, the width will be the same and we have to\npass less variables around.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 39 +++++++++++++++++----------------------\n 1 file changed, 17 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4e18c3902d..31724e9aaf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -49,6 +49,7 @@ enum {\n \n struct display_state {\n \tint refcol_width;\n+\tint summary_width;\n \tint compact_format;\n \n \tchar *url;\n@@ -830,6 +831,8 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \t\tif (display->refcol_width < width)\n \t\t\tdisplay->refcol_width = width;\n \t}\n+\n+\tdisplay->summary_width = transport_summary_width(ref_map);\n }\n \n static void display_state_release(struct display_state *display)\n@@ -896,8 +899,7 @@ static void print_compact(struct display_state *display, struct strbuf *display_\n static void format_display(struct display_state *display,\n \t\t\t struct strbuf *display_buffer, 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+\t\t\t const char *remote, const char *local)\n {\n \tint width;\n \n@@ -909,7 +911,7 @@ static void format_display(struct display_state *display,\n \t\tdisplay->shown_url = 1;\n \t}\n \n-\twidth = (summary_width + strlen(summary) - gettext_width(summary));\n+\twidth = (display->summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n \tif (!display->compact_format)\n@@ -925,7 +927,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct display_state *display,\n \t\t\t const char *remote, const struct ref *remote_ref,\n-\t\t\t struct strbuf *display_buffer, int summary_width)\n+\t\t\t struct strbuf *display_buffer)\n {\n \tstruct commit *current = NULL, *updated;\n \tint fast_forward = 0;\n@@ -936,7 +938,7 @@ static int update_local_ref(struct ref *ref,\n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n \t\t\tformat_display(display, display_buffer, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\t\t remote, ref->name);\n \t\treturn 0;\n \t}\n \n@@ -949,7 +951,7 @@ static int update_local_ref(struct ref *ref,\n \t\t */\n \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\t\t remote, ref->name);\n \t\treturn 1;\n \t}\n \n@@ -960,12 +962,12 @@ static int update_local_ref(struct ref *ref,\n \t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, display_buffer, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\t\t remote, ref->name);\n \t\t\treturn r;\n \t\t} else {\n \t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n \t\t\t\t _(\"would clobber existing tag\"),\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\t\t remote, ref->name);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -998,7 +1000,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, display_buffer, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name, summary_width);\n+\t\t\t remote, ref->name);\n \t\treturn r;\n \t}\n \n@@ -1020,7 +1022,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, display_buffer, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name, summary_width);\n+\t\t\t remote, ref->name);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -1032,12 +1034,12 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, display_buffer, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\t\t remote, ref->name);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\t\t remote, ref->name);\n \t\treturn 1;\n \t}\n }\n@@ -1158,10 +1160,6 @@ static int store_updated_refs(struct display_state *display,\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tint want_status;\n-\tint summary_width = 0;\n-\n-\tif (verbosity >= 0)\n-\t\tsummary_width = transport_summary_width(ref_map);\n \n \tif (!connectivity_checked) {\n \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n@@ -1266,7 +1264,7 @@ static int store_updated_refs(struct display_state *display,\n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n \t\t\t\trc |= update_local_ref(ref, transaction, display, what,\n-\t\t\t\t\t\t rm, ¬e, summary_width);\n+\t\t\t\t\t\t rm, ¬e);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n@@ -1277,7 +1275,7 @@ static int store_updated_refs(struct display_state *display,\n \t\t\t\tformat_display(display, ¬e, '*',\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+\t\t\t\t\t \"FETCH_HEAD\");\n \t\t\t}\n \t\t\tif (note.len)\n \t\t\t\tfputs(note.buf, stderr);\n@@ -1413,13 +1411,10 @@ static int prune_refs(struct display_state *display,\n \t}\n \n \tif (verbosity >= 0) {\n-\t\tint summary_width = transport_summary_width(stale_refs);\n-\n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t _(\"(none)\"), ref->name,\n-\t\t\t\t summary_width);\n+\t\t\t\t _(\"(none)\"), ref->name);\n \t\t\tfputs(sb.buf, stderr);\n \t\t\tstrbuf_release(&sb);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n-- \n2.40.0\n\n"},{"id":"473520","messageId":"2ea3a4e308d2fb89f24d0cd2efc9d3867983a4b0.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 6/8] fetch: deduplicate logic to print remote URL","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:28Z","receivedAt":"2023-03-15T11:24:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching from a remote, we not only print the actual references\nthat have changed, but will also print the URL from which we have\nfetched them to standard output. The logic to handle this is duplicated\nacross two different callsites with some non-trivial logic to compute\nthe anonymized URL. Furthermore, we're using global state to track\nwhether we have already shown the URL to the user or not.\n\nRefactor the code by moving it into `format_display()`. Like this, we\ncan convert the global variable into a member of `display_state`. And\nsecond, we can deduplicate the logic to compute the anonymized URL.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 96 +++++++++++++++++++++----------------------------\n 1 file changed, 41 insertions(+), 55 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6fc2fd0d46..4e18c3902d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -50,6 +50,9 @@ enum {\n struct display_state {\n \tint refcol_width;\n \tint compact_format;\n+\n+\tchar *url;\n+\tint url_len, shown_url;\n };\n \n static int fetch_prune_config = -1; /* unspecified */\n@@ -84,7 +87,6 @@ static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n-static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n@@ -776,13 +778,27 @@ static int refcol_width(const struct ref *ref, int compact_format)\n \treturn rlen;\n }\n \n-static void display_state_init(struct display_state *display, struct ref *ref_map)\n+static void display_state_init(struct display_state *display, struct ref *ref_map,\n+\t\t\t const char *raw_url)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n+\tint i;\n \n \tmemset(display, 0, sizeof(*display));\n \n+\tif (raw_url)\n+\t\tdisplay->url = transport_anonymize_url(raw_url);\n+\telse\n+\t\tdisplay->url = xstrdup(\"foreign\");\n+\n+\tdisplay->url_len = strlen(display->url);\n+\tfor (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)\n+\t\t;\n+\tdisplay->url_len = i + 1;\n+\tif (4 < i && !strncmp(\".git\", display->url + i - 3, 4))\n+\t\tdisplay->url_len = i - 3;\n+\n \tif (verbosity < 0)\n \t\treturn;\n \n@@ -816,6 +832,11 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \t}\n }\n \n+static void display_state_release(struct display_state *display)\n+{\n+\tfree(display->url);\n+}\n+\n static void print_remote_to_local(struct display_state *display,\n \t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n@@ -883,6 +904,11 @@ static void format_display(struct display_state *display,\n \tif (verbosity < 0)\n \t\treturn;\n \n+\tif (!display->shown_url) {\n+\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"), display->url_len, display->url);\n+\t\tdisplay->shown_url = 1;\n+\t}\n+\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n@@ -1122,33 +1148,27 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\n \n static int store_updated_refs(struct display_state *display,\n-\t\t\t const char *raw_url, const char *remote_name,\n+\t\t\t const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n {\n-\tint url_len, i, rc = 0;\n+\tint rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n-\tchar *url;\n \tint want_status;\n \tint summary_width = 0;\n \n \tif (verbosity >= 0)\n \t\tsummary_width = transport_summary_width(ref_map);\n \n-\tif (raw_url)\n-\t\turl = transport_anonymize_url(raw_url);\n-\telse\n-\t\turl = xstrdup(\"foreign\");\n-\n \tif (!connectivity_checked) {\n \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n-\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), display->url);\n \t\t\tgoto abort;\n \t\t}\n \t}\n@@ -1232,13 +1252,6 @@ static int store_updated_refs(struct display_state *display,\n \t\t\t\twhat = rm->name;\n \t\t\t}\n \n-\t\t\turl_len = strlen(url);\n-\t\t\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t\t\t;\n-\t\t\turl_len = i + 1;\n-\t\t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\t\t\turl_len = i - 3;\n-\n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (*what) {\n \t\t\t\tif (*kind)\n@@ -1248,7 +1261,7 @@ static int store_updated_refs(struct display_state *display,\n \n \t\t\tappend_fetch_head(fetch_head, &rm->old_oid,\n \t\t\t\t\t rm->fetch_head_status,\n-\t\t\t\t\t note.buf, url, url_len);\n+\t\t\t\t\t note.buf, display->url, display->url_len);\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n@@ -1266,14 +1279,8 @@ static int store_updated_refs(struct display_state *display,\n \t\t\t\t\t *what ? what : \"HEAD\",\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 (!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\tif (note.len)\n \t\t\t\tfputs(note.buf, stderr);\n-\t\t\t}\n \t\t}\n \t}\n \n@@ -1293,7 +1300,6 @@ static int store_updated_refs(struct display_state *display,\n \n abort:\n \tstrbuf_release(¬e);\n-\tfree(url);\n \treturn rc;\n }\n \n@@ -1365,7 +1371,7 @@ static int fetch_and_consume_refs(struct display_state *display,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(display, transport->url, transport->remote->name,\n+\tret = store_updated_refs(display, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n@@ -1378,30 +1384,15 @@ static int fetch_and_consume_refs(struct display_state *display,\n static int prune_refs(struct display_state *display,\n \t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n-\t\t struct ref *ref_map,\n-\t\t const char *raw_url)\n+\t\t struct ref *ref_map)\n {\n-\tint url_len, i, result = 0;\n+\tint result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);\n \tstruct strbuf err = STRBUF_INIT;\n-\tchar *url;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" (%s will become dangling)\")\n \t\t: _(\" (%s has become dangling)\");\n \n-\tif (raw_url)\n-\t\turl = transport_anonymize_url(raw_url);\n-\telse\n-\t\turl = xstrdup(\"foreign\");\n-\n-\turl_len = strlen(url);\n-\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t;\n-\n-\turl_len = i + 1;\n-\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\turl_len = i - 3;\n-\n \tif (!dry_run) {\n \t\tif (transaction) {\n \t\t\tfor (ref = stale_refs; ref; ref = ref->next) {\n@@ -1426,10 +1417,6 @@ static int prune_refs(struct display_state *display,\n \n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tif (!shown_url) {\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(display, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n@@ -1441,7 +1428,6 @@ static int prune_refs(struct display_state *display,\n \n cleanup:\n \tstrbuf_release(&err);\n-\tfree(url);\n \tfree_refs(stale_refs);\n \treturn result;\n }\n@@ -1596,7 +1582,7 @@ static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n-\tstruct display_state display;\n+\tstruct display_state display = { 0 };\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n@@ -1678,7 +1664,7 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tdisplay_state_init(&display, ref_map);\n+\tdisplay_state_init(&display, ref_map, transport->url);\n \n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n@@ -1697,11 +1683,10 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map);\n \t\t} else {\n \t\t\tretcode = prune_refs(&display, &transport->remote->fetch,\n-\t\t\t\t\t transaction, ref_map,\n-\t\t\t\t\t transport->url);\n+\t\t\t\t\t transaction, ref_map);\n \t\t}\n \t\tif (retcode != 0)\n \t\t\tretcode = 1;\n@@ -1812,6 +1797,7 @@ static int do_fetch(struct transport *transport,\n \t\terror(\"%s\", err.buf);\n \t}\n \n+\tdisplay_state_release(&display);\n \tclose_fetch_head(&fetch_head);\n \tstrbuf_release(&err);\n \tfree_refs(ref_map);\n-- \n2.40.0\n\n"},{"id":"473521","messageId":"966730171106703cc378351f16f8b47d3ecb5a2c.1678878623.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH 8/8] fetch: centralize printing of reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-15T11:21:36Z","receivedAt":"2023-03-15T11:25:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to print updated references during a fetch, the two different\ncall sites that do this will first call `format_display()` followed by a\ncall to `fputs()`. This is needlessly roundabout now that we have the\n`display_state` structure that encapsulates all of the printing logic\nfor references.\n\nMove displaying the reference updates into `format_display()` and rename\nit to `display_ref_update()` to better match its new purpose. We have\nnow centralized the logic to print reference updates into a single\nfunction, which enables us to more readily introduce new formats that\nmay even be printing to a different file descriptor than `stderr`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 105 ++++++++++++++++++++++++------------------------\n 1 file changed, 52 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 31724e9aaf..6e864f8457 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -48,6 +48,8 @@ enum {\n };\n \n struct display_state {\n+\tstruct strbuf buf;\n+\n \tint refcol_width;\n \tint summary_width;\n \tint compact_format;\n@@ -788,6 +790,8 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \n \tmemset(display, 0, sizeof(*display));\n \n+\tstrbuf_init(&display->buf, 0);\n+\n \tif (raw_url)\n \t\tdisplay->url = transport_anonymize_url(raw_url);\n \telse\n@@ -837,14 +841,14 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma\n \n static void display_state_release(struct display_state *display)\n {\n+\tstrbuf_release(&display->buf);\n \tfree(display->url);\n }\n \n static void print_remote_to_local(struct display_state *display,\n-\t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n-\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display->refcol_width, remote, local);\n+\tstrbuf_addf(&display->buf, \"%-*s -> %s\", display->refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n@@ -874,14 +878,14 @@ static int find_and_replace(struct strbuf *haystack,\n \treturn 1;\n }\n \n-static void print_compact(struct display_state *display, struct strbuf *display_buffer,\n+static void print_compact(struct display_state *display,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n-\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display->refcol_width, remote);\n+\t\tstrbuf_addf(&display->buf, \"%-*s -> *\", display->refcol_width, remote);\n \t\treturn;\n \t}\n \n@@ -890,44 +894,46 @@ static void print_compact(struct display_state *display, struct strbuf *display_\n \n \tif (!find_and_replace(&r, local, \"*\"))\n \t\tfind_and_replace(&l, remote, \"*\");\n-\tprint_remote_to_local(display, display_buffer, r.buf, l.buf);\n+\tprint_remote_to_local(display, r.buf, l.buf);\n \n \tstrbuf_release(&r);\n \tstrbuf_release(&l);\n }\n \n-static void format_display(struct display_state *display,\n-\t\t\t struct strbuf *display_buffer, char code,\n-\t\t\t const char *summary, const char *error,\n-\t\t\t const char *remote, const char *local)\n+static void display_ref_update(struct display_state *display, char code,\n+\t\t\t const char *summary, const char *error,\n+\t\t\t const char *remote, const char *local)\n {\n \tint width;\n \n \tif (verbosity < 0)\n \t\treturn;\n \n+\tstrbuf_reset(&display->buf);\n+\n \tif (!display->shown_url) {\n-\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"), display->url_len, display->url);\n+\t\tstrbuf_addf(&display->buf, _(\"From %.*s\\n\"), display->url_len, display->url);\n \t\tdisplay->shown_url = 1;\n \t}\n \n \twidth = (display->summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n+\tstrbuf_addf(&display->buf, \" %c %-*s \", code, width, summary);\n \tif (!display->compact_format)\n-\t\tprint_remote_to_local(display, display_buffer, remote, prettify_refname(local));\n+\t\tprint_remote_to_local(display, remote, prettify_refname(local));\n \telse\n-\t\tprint_compact(display, display_buffer, remote, prettify_refname(local));\n+\t\tprint_compact(display, remote, prettify_refname(local));\n \tif (error)\n-\t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n-\tstrbuf_addch(display_buffer, '\\n');\n+\t\tstrbuf_addf(&display->buf, \" (%s)\", error);\n+\tstrbuf_addch(&display->buf, '\\n');\n+\n+\tfputs(display->buf.buf, stderr);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct display_state *display,\n-\t\t\t const char *remote, const struct ref *remote_ref,\n-\t\t\t struct strbuf *display_buffer)\n+\t\t\t const char *remote, const struct ref *remote_ref)\n {\n \tstruct commit *current = NULL, *updated;\n \tint fast_forward = 0;\n@@ -937,8 +943,8 @@ 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, display_buffer, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t remote, ref->name);\n+\t\t\tdisplay_ref_update(display, '=', _(\"[up to date]\"), NULL,\n+\t\t\t\t\t remote, ref->name);\n \t\treturn 0;\n \t}\n \n@@ -949,9 +955,9 @@ 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, display_buffer, '!', _(\"[rejected]\"),\n-\t\t\t _(\"can't fetch into checked-out branch\"),\n-\t\t\t remote, ref->name);\n+\t\tdisplay_ref_update(display, '!', _(\"[rejected]\"),\n+\t\t\t\t _(\"can't fetch into checked-out branch\"),\n+\t\t\t\t remote, ref->name);\n \t\treturn 1;\n \t}\n \n@@ -960,14 +966,14 @@ 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, display_buffer, r ? '!' : 't', _(\"[tag update]\"),\n-\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t\t remote, ref->name);\n+\t\t\tdisplay_ref_update(display, r ? '!' : 't', _(\"[tag update]\"),\n+\t\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t\t remote, ref->name);\n \t\t\treturn r;\n \t\t} else {\n-\t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n-\t\t\t\t _(\"would clobber existing tag\"),\n-\t\t\t\t remote, ref->name);\n+\t\t\tdisplay_ref_update(display, '!', _(\"[rejected]\"),\n+\t\t\t\t\t _(\"would clobber existing tag\"),\n+\t\t\t\t\t remote, ref->name);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -998,9 +1004,9 @@ 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, display_buffer, r ? '!' : '*', what,\n-\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name);\n+\t\tdisplay_ref_update(display, r ? '!' : '*', what,\n+\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t remote, ref->name);\n \t\treturn r;\n \t}\n \n@@ -1020,9 +1026,9 @@ 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, display_buffer, r ? '!' : ' ', quickref.buf,\n-\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name);\n+\t\tdisplay_ref_update(display, r ? '!' : ' ', quickref.buf,\n+\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t remote, ref->name);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -1032,14 +1038,14 @@ 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, display_buffer, r ? '!' : '+', quickref.buf,\n-\t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n-\t\t\t remote, ref->name);\n+\t\tdisplay_ref_update(display, r ? '!' : '+', quickref.buf,\n+\t\t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n+\t\t\t\t remote, ref->name);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n-\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t remote, ref->name);\n+\t\tdisplay_ref_update(display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n+\t\t\t\t remote, ref->name);\n \t\treturn 1;\n \t}\n }\n@@ -1261,10 +1267,8 @@ static int store_updated_refs(struct display_state *display,\n \t\t\t\t\t rm->fetch_head_status,\n \t\t\t\t\t note.buf, display->url, display->url_len);\n \n-\t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, transaction, display, what,\n-\t\t\t\t\t\t rm, ¬e);\n+\t\t\t\trc |= update_local_ref(ref, transaction, display, what, rm);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n@@ -1272,13 +1276,11 @@ static int store_updated_refs(struct display_state *display,\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(display, ¬e, '*',\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\");\n+\t\t\t\tdisplay_ref_update(display, '*',\n+\t\t\t\t\t\t *kind ? kind : \"branch\", NULL,\n+\t\t\t\t\t\t *what ? what : \"HEAD\",\n+\t\t\t\t\t\t \"FETCH_HEAD\");\n \t\t\t}\n-\t\t\tif (note.len)\n-\t\t\t\tfputs(note.buf, stderr);\n \t\t}\n \t}\n \n@@ -1412,11 +1414,8 @@ static int prune_refs(struct display_state *display,\n \n \tif (verbosity >= 0) {\n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t _(\"(none)\"), ref->name);\n-\t\t\tfputs(sb.buf, stderr);\n-\t\t\tstrbuf_release(&sb);\n+\t\t\tdisplay_ref_update(display, '-', _(\"[deleted]\"), NULL,\n+\t\t\t\t\t _(\"(none)\"), ref->name);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n \t\t}\n \t}\n-- \n2.40.0\n\n"},{"id":"473567","messageId":"xmqqedppohno.fsf@gitster.g","threadId":"59399","inReplyTo":"aa792b12a468263d05e8615d4b3691ed8fe823ff.1678878623.git.ps@pks.im","subject":"Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T20:59:07Z","receivedAt":"2023-03-15T20:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In order to print references in proper columns we need to calculate the\n> width of the reference column before starting to print the references.\n> This is done with the help of a global variable `refcol_width`.\n>\n> Refactor the code to instead use a new structure `display_state` that\n> contains the computed width and plumb it through the stack as required.\n> This is only the first step towards de-globalizing the state required to\n> print references.\n\nNice.\n\nGiven that in the previous step, what used to be called display got\nrenamed to display_buffer (I think \"buffer\" ought to be sufficient\nin this context, though), the variable of \"struct display_state\"\ntype should NOT be named \"display\", as it would be confusing when\ntwo things are related to \"display\" and only one of them is called\nas such. Either \"display_state\" or \"state\" would be fine.\n\nThanks.\n\n\n"},{"id":"473568","messageId":"xmqqa60dodze.fsf@gitster.g","threadId":"59399","inReplyTo":"0998173b57e8d7aea7a77673fa8e54dc54538a9b.1678878623.git.ps@pks.im","subject":"Re: [PATCH 4/8] fetch: pass the full local reference name to `format_display`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T22:18:29Z","receivedAt":"2023-03-15T22:19:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Before printing the name of the local references that would be updated\n> by a fetch we first prettify the reference name. This is done at the\n> calling side so that `format_display()` never sees the full name of the\n> local reference. This restricts our ability to introduce new output\n> formats that might want to print the full reference name.\n\nAs long as the prettify at the caller is *not* done to avoid doing\nthe same prettifying multiple times, this change makes perfect\nsense.\n\n> So let's refactor the\n> code to pass in the full local reference name and then prettify it in\n> the formatting code.\n\nGood thinking.\n"},{"id":"473570","messageId":"xmqq4jqlocqf.fsf@gitster.g","threadId":"59399","inReplyTo":"d45ec31eeaf42ee042bad04efd69668144df3138.1678878623.git.ps@pks.im","subject":"Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T22:45:28Z","receivedAt":"2023-03-15T22:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Both callsites that call `format_display()` and then print the result to\n> standard error use the same formatting directive \" %s\\n\" to print the\n> reference to disk, thus duplicating a small part of the logic.\n\nHmph.\n\nIf format_display() were a function whose role was to prepare the\ncontents on a single line, it can be argued that it is caller's job\nto give a leading indent that is appropriate for the line in the\ncontext of the display it is producing. \"store-updated-refs\" and\n\"prune-refs\" may be showing a list of refs that were affected under\ndifferent heading, together with different kind of information, and\ndepending on the way each of these callers organize its output, the\nappropriate indentation level for the line might be different. So I\nthink the current product format_display() gives its callers is\nperfectly defensible in that sense.\n\nOn the other hand, if format_display() is only about showing a\nsingle line in the tightly limited context (in other words, both of\nits callers promise that they will forever be happy with the\nfunction showing exactly the same output), then this refactoring\nwould be OK. In addition, it may even make more sense, if that were\nthe role of this callee, to do the actual printing, not just\npreparing a line of string into a strbuf, in this callee, by moving\nthe fputs() from caller to callee.\n\nSo, I dunno. The result of applying this patch leaves it in an\nin-between state, where the division of labor between the caller and\nthe callee smells somewhat iffy.\n\nThanks.\n"},{"id":"473573","messageId":"xmqqy1nxmxdx.fsf@gitster.g","threadId":"59399","inReplyTo":"2ea3a4e308d2fb89f24d0cd2efc9d3867983a4b0.1678878623.git.ps@pks.im","subject":"Re: [PATCH 6/8] fetch: deduplicate logic to print remote URL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T23:02:18Z","receivedAt":"2023-03-15T23:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When fetching from a remote, we not only print the actual references\n> that have changed, but will also print the URL from which we have\n> fetched them to standard output. The logic to handle this is duplicated\n> across two different callsites with some non-trivial logic to compute\n> the anonymized URL. Furthermore, we're using global state to track\n> whether we have already shown the URL to the user or not.\n\nIf we are certain that store_updated_refs() is called only once for\nthe entire process, then storing the preprocessed url in the display\nstate and passing it around does sound like a good optimization and\nclean-up. What do we do when fetching from multiple remotes?\n\n> +\tdisplay->url_len = strlen(display->url);\n> +\tfor (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)\n> +\t\t;\n> +\tdisplay->url_len = i + 1;\n\nThis loop is inherited from the original, but we may want to use\nstrrchr() or rindex() as a post clean-up after this series settles.\n"},{"id":"473574","messageId":"xmqqttylmww4.fsf@gitster.g","threadId":"59399","inReplyTo":"f67f9640a853b605dd1bc4be25e9988c4f059684.1678878623.git.ps@pks.im","subject":"Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-15T23:12:59Z","receivedAt":"2023-03-15T23:13:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> As the abbreviated hashes may have different lengths in order to be\n> unique we thus need to precompute the width of the summary's column by\n> iterating through all the objects. This is done in two locations: once\n> to compute the width for references that are to be pruned, and once for\n> all the other references. Consequentially, it can happen that the width\n> as calculated for these sets of references is different.\n\nHmph. Use of ref_map vs stale_refs as the parameter to call\ntransport_summary_width() is to come up with an appropriate width\nfor showing the list of stored refs vs the list of pruned refs, so\nfrom that point of view, an appropriate width for each list is\ncalculated to a different number may even be a feature, no?\n\nI do not mind either way all that much, but a change like this to\nupdate the presentation may want to be protected with a test from\nfuture breakages.\n\nThanks.\n"},{"id":"473588","messageId":"ZBMwUmBhqEd74YAx@ncase","threadId":"59399","inReplyTo":"xmqqedppohno.fsf@gitster.g","subject":"Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-16T15:05:54Z","receivedAt":"2023-03-16T15:06:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 15, 2023 at 01:59:07PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > In order to print references in proper columns we need to calculate the\n> > width of the reference column before starting to print the references.\n> > This is done with the help of a global variable `refcol_width`.\n> >\n> > Refactor the code to instead use a new structure `display_state` that\n> > contains the computed width and plumb it through the stack as required.\n> > This is only the first step towards de-globalizing the state required to\n> > print references.\n> \n> Nice.\n> \n> Given that in the previous step, what used to be called display got\n> renamed to display_buffer (I think \"buffer\" ought to be sufficient\n> in this context, though), the variable of \"struct display_state\"\n> type should NOT be named \"display\", as it would be confusing when\n> two things are related to \"display\" and only one of them is called\n> as such. Either \"display_state\" or \"state\" would be fine.\n\nFair enough. In that case I may just as well drop the first patch.\n\nPatrick\n"},{"id":"473589","messageId":"ZBMwXAnEnD5QjsFE@ncase","threadId":"59399","inReplyTo":"xmqq4jqlocqf.fsf@gitster.g","subject":"Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-16T15:06:04Z","receivedAt":"2023-03-16T15:06:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 15, 2023 at 03:45:28PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Both callsites that call `format_display()` and then print the result to\n> > standard error use the same formatting directive \" %s\\n\" to print the\n> > reference to disk, thus duplicating a small part of the logic.\n> \n> Hmph.\n> \n> If format_display() were a function whose role was to prepare the\n> contents on a single line, it can be argued that it is caller's job\n> to give a leading indent that is appropriate for the line in the\n> context of the display it is producing. \"store-updated-refs\" and\n> \"prune-refs\" may be showing a list of refs that were affected under\n> different heading, together with different kind of information, and\n> depending on the way each of these callers organize its output, the\n> appropriate indentation level for the line might be different. So I\n> think the current product format_display() gives its callers is\n> perfectly defensible in that sense.\n> \n> On the other hand, if format_display() is only about showing a\n> single line in the tightly limited context (in other words, both of\n> its callers promise that they will forever be happy with the\n> function showing exactly the same output), then this refactoring\n> would be OK. In addition, it may even make more sense, if that were\n> the role of this callee, to do the actual printing, not just\n> preparing a line of string into a strbuf, in this callee, by moving\n> the fputs() from caller to callee.\n> \n> So, I dunno. The result of applying this patch leaves it in an\n> in-between state, where the division of labor between the caller and\n> the callee smells somewhat iffy.\n> \n> Thanks.\n\nI totally agree with you here. From my point of view this \"division of\nlabor\" is getting fixed in the final patch that then also moves the\nprinting logic into `format_display()`.\n\nPatrick\n"},{"id":"473590","messageId":"ZBMwZvQOooFiIl6S@ncase","threadId":"59399","inReplyTo":"xmqqy1nxmxdx.fsf@gitster.g","subject":"Re: [PATCH 6/8] fetch: deduplicate logic to print remote URL","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-16T15:06:14Z","receivedAt":"2023-03-16T15:06:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 15, 2023 at 04:02:18PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > When fetching from a remote, we not only print the actual references\n> > that have changed, but will also print the URL from which we have\n> > fetched them to standard output. The logic to handle this is duplicated\n> > across two different callsites with some non-trivial logic to compute\n> > the anonymized URL. Furthermore, we're using global state to track\n> > whether we have already shown the URL to the user or not.\n> \n> If we are certain that store_updated_refs() is called only once for\n> the entire process, then storing the preprocessed url in the display\n> state and passing it around does sound like a good optimization and\n> clean-up. What do we do when fetching from multiple remotes?\n\nWe execute separate git-fetch(1) processes when fetching from multiple\nremotes or when fetching submodules, so we should be fine here.\n\n> > +\tdisplay->url_len = strlen(display->url);\n> > +\tfor (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)\n> > +\t\t;\n> > +\tdisplay->url_len = i + 1;\n> \n> This loop is inherited from the original, but we may want to use\n> strrchr() or rindex() as a post clean-up after this series settles.\n\nYeah, that'd make sense.\n\nPatrick\n"},{"id":"473591","messageId":"ZBMwhLgGeUhtd5Zb@ncase","threadId":"59399","inReplyTo":"xmqqttylmww4.fsf@gitster.g","subject":"Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-16T15:06:44Z","receivedAt":"2023-03-16T15:07:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 15, 2023 at 04:12:59PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > As the abbreviated hashes may have different lengths in order to be\n> > unique we thus need to precompute the width of the summary's column by\n> > iterating through all the objects. This is done in two locations: once\n> > to compute the width for references that are to be pruned, and once for\n> > all the other references. Consequentially, it can happen that the width\n> > as calculated for these sets of references is different.\n> \n> Hmph. Use of ref_map vs stale_refs as the parameter to call\n> transport_summary_width() is to come up with an appropriate width\n> for showing the list of stored refs vs the list of pruned refs, so\n> from that point of view, an appropriate width for each list is\n> calculated to a different number may even be a feature, no?\n\nI'd say it's not. Look at the following output generated by a `git fetch\n--prune --no-progress` with a deleted and an updated reference:\n\n From /tmp/repo\n - [deleted] (none) -> origin/to-be-deleted\n 82307bb..107b50a main -> origin/main\n\nBefore my change, the width of the deletion and the reference update are\ncalculated separately. Given that:\n\n - we don't even display the object IDs for deleted references\n\n - the width of the deleted reference's column is static anyway.\n\nI'd argue that it's not a feature that the widths are computed\nseparately. If it was, you could just skip calculating the width of\ndeleted references and just print them with a static column width.\n\nThe current implementation tends to work in most cases as the column\nwidth is based on the minimum length where all abbreviated object IDs\nbecome unique. And I assume that it's the same for both sets of refs in\nthe majority of cases. And in the other cases I guess that nobody cares\nmuch anyway.\n\nPractically speaking we could go even further than the current version,\nas I now compute the width across _all_ reference updates, even those\nwhich are deletions. But theoretically speaking, we could just skip over\nany deletions completely as they won't ever contribute to the column\nwidth anyway.\n\n> I do not mind either way all that much, but a change like this to\n> update the presentation may want to be protected with a test from\n> future breakages.\n\nFair, having a test for this would be great. But what kept me from\nadding one here is that the column width depends on the length of the\nlongest shared prefix of two object IDs that are about to be updated.\nAnd I just have no clue how to generate those without brute forcing them\nfor both SHA1 and SHA256.\n\nDo we have any mechanism for this?\n\nPatrick\n"},{"id":"473592","messageId":"xmqqpm98mzzy.fsf@gitster.g","threadId":"59399","inReplyTo":"ZBMwUmBhqEd74YAx@ncase","subject":"Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T16:18:09Z","receivedAt":"2023-03-16T16:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Given that in the previous step, what used to be called display got\n>> renamed to display_buffer (I think \"buffer\" ought to be sufficient\n>> in this context, though), the variable of \"struct display_state\"\n>> type should NOT be named \"display\", as it would be confusing when\n>> two things are related to \"display\" and only one of them is called\n>> as such. Either \"display_state\" or \"state\" would be fine.\n>\n> Fair enough. In that case I may just as well drop the first patch.\n\nIf you plan to get rid of an independent \"display_buffer\" in the\nendgame by moving it into the bigger struct as its .buffer member,\nthen I think the naming is fine as there will remain only one thing\nthat is \"display\". The fact that I didn't see that plan through\nwhen I read only the first two patches would probably mean that the\nroute this iteration of the series took was somewhat roundabout, and\nthere may be a more transparent and possibly a more direct way to\nget to that goal?\n\nI am not entirely sure if the buffer should go inside the\ndisplay_state structure in the endgame. An alternative may be to\nmake it a on-stack variable of format_display() (which will later be\nmodified to do everything up to and including writing out the\nresult) and pass it through the callchain below to its helpers, just\nlike the current code already does. And in such an approach, you'd\nstill need to name that variable passed to the helper functions\ncalled by format_display()---\"buffer\" would be a good name for that.\n\nJust thinking aloud.\n\nThanks.\n\n"},{"id":"473593","messageId":"xmqqmt4cmzy9.fsf@gitster.g","threadId":"59399","inReplyTo":"ZBMwUmBhqEd74YAx@ncase","subject":"Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T16:19:10Z","receivedAt":"2023-03-16T16:24:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Given that in the previous step, what used to be called display got\n>> renamed to display_buffer (I think \"buffer\" ought to be sufficient\n>> in this context, though), the variable of \"struct display_state\"\n>> type should NOT be named \"display\", as it would be confusing when\n>> two things are related to \"display\" and only one of them is called\n>> as such. Either \"display_state\" or \"state\" would be fine.\n>\n> Fair enough. In that case I may just as well drop the first patch.\n\nIf you plan to get rid of an independent \"display_buffer\" in the\nendgame by moving it into the bigger struct as its .buffer member,\nthen I think the naming is fine as there will remain only one thing\nthat is \"display\". The fact that I didn't see that plan through\nwhen I read only the first two patches would probably mean that the\nroute this iteration of the series took was somewhat roundabout, and\nthere may be a more transparent and possibly a more direct way to\nget to that goal?\n\nI am not entirely sure if the buffer should go inside the\ndisplay_state structure in the endgame. An alternative may be to\nmake it a on-stack variable of format_display() (which will later be\nmodified to do everything up to and including writing out the\nresult) and pass it through the callchain below to its helpers, just\nlike the current code already does. And in such an approach, you'd\nstill need to name that variable passed to the helper functions\ncalled by format_display()---\"buffer\" would be a good name for that.\n\nJust thinking aloud.\n\nThanks.\n\n"},{"id":"473594","messageId":"xmqqh6ukmzex.fsf@gitster.g","threadId":"59399","inReplyTo":"ZBMwhLgGeUhtd5Zb@ncase","subject":"Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T16:30:46Z","receivedAt":"2023-03-16T16:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Practically speaking we could go even further than the current version,\n> as I now compute the width across _all_ reference updates, even those\n> which are deletions. But theoretically speaking, we could just skip over\n> any deletions completely as they won't ever contribute to the column\n> width anyway.\n\nOK. It's good to see that you have thought it through.\n\n> Fair, having a test for this would be great. But what kept me from\n> adding one here is that the column width depends on the length of the\n> longest shared prefix of two object IDs that are about to be updated.\n\nYou do not have to prepare \"this is the correct expected output\",\nwhen you need to make sure that two kinds of lines use the same\nwidth settings, no? Extract such lines from the two camps, measure\nthem and see if they are of the same length, or something?\n\nThanks.\n"},{"id":"473597","messageId":"xmqqv8j0ljxd.fsf@gitster.g","threadId":"59399","inReplyTo":"ZBMwXAnEnD5QjsFE@ncase","subject":"Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T16:50:38Z","receivedAt":"2023-03-16T16:50:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> So, I dunno. The result of applying this patch leaves it in an\n>> in-between state, where the division of labor between the caller and\n>> the callee smells somewhat iffy.\n>> \n>> Thanks.\n>\n> I totally agree with you here. From my point of view this \"division of\n> labor\" is getting fixed in the final patch that then also moves the\n> printing logic into `format_display()`.\n\nYes, again I smell the same \"isn't this series going a bit too\nroundabout route to its goal?\" which I expressed in my response to\nan earlier step. The endgame of the series, even though I may not\nagree with it 100%, is self consistent and does not leave the \"this\nends at an in-between state\" aftertaste in my mouth.\n\nThanks.\n\n"},{"id":"473628","messageId":"ZBQ4Cqkq9v2cB77f@ncase","threadId":"59399","inReplyTo":"xmqqv8j0ljxd.fsf@gitster.g","subject":"Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-17T09:51:06Z","receivedAt":"2023-03-17T09:51:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 16, 2023 at 09:50:38AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> So, I dunno. The result of applying this patch leaves it in an\n> >> in-between state, where the division of labor between the caller and\n> >> the callee smells somewhat iffy.\n> >> \n> >> Thanks.\n> >\n> > I totally agree with you here. From my point of view this \"division of\n> > labor\" is getting fixed in the final patch that then also moves the\n> > printing logic into `format_display()`.\n> \n> Yes, again I smell the same \"isn't this series going a bit too\n> roundabout route to its goal?\" which I expressed in my response to\n> an earlier step. The endgame of the series, even though I may not\n> agree with it 100%, is self consistent and does not leave the \"this\n> ends at an in-between state\" aftertaste in my mouth.\n> \n> Thanks.\n\nYeah. I did have some problems to lay out this series in a sensible way.\nIn an earlier iteration I tried moving the printing logic in one of the\ninitial patches, but from my point of view that resulted in an even more\nawkward in-between state where the formatting and printing logic was\nkind of all over the place. And another try with \"big-bang-refactoring\"\nwas barely reviewable, either.\n\nMaybe the solution is to keep the order, but document intent better in\neach of the patches leading towards the unified printing logic.\n\nPatrick\n"},{"id":"473629","messageId":"ZBQ4+f/YU0r6ntfb@ncase","threadId":"59399","inReplyTo":"xmqqh6ukmzex.fsf@gitster.g","subject":"Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-17T09:55:05Z","receivedAt":"2023-03-17T09:55:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 16, 2023 at 09:30:46AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Practically speaking we could go even further than the current version,\n> > as I now compute the width across _all_ reference updates, even those\n> > which are deletions. But theoretically speaking, we could just skip over\n> > any deletions completely as they won't ever contribute to the column\n> > width anyway.\n> \n> OK. It's good to see that you have thought it through.\n> \n> > Fair, having a test for this would be great. But what kept me from\n> > adding one here is that the column width depends on the length of the\n> > longest shared prefix of two object IDs that are about to be updated.\n> \n> You do not have to prepare \"this is the correct expected output\",\n> when you need to make sure that two kinds of lines use the same\n> width settings, no? Extract such lines from the two camps, measure\n> them and see if they are of the same length, or something?\n\nWell, comparing widths of these two lines is the easy version of the\ntest, agreed. But in order to test for the bug I'm fixing I'd need to\ngenerate two sets of object IDs, where the first set of object IDs has a\nlonger shared prefix length than the second. Because otherwise, the bug\nwouldn't be guaranteed to even surface.\n\nAnyway -- I'll have a look at adding the easy version.\n\nPatrick\n"},{"id":"473630","messageId":"ZBQ612KWqYg4ky2j@ncase","threadId":"59399","inReplyTo":"xmqqpm98mzzy.fsf@gitster.g","subject":"Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-17T10:03:03Z","receivedAt":"2023-03-17T10:03:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 16, 2023 at 09:18:09AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> Given that in the previous step, what used to be called display got\n> >> renamed to display_buffer (I think \"buffer\" ought to be sufficient\n> >> in this context, though), the variable of \"struct display_state\"\n> >> type should NOT be named \"display\", as it would be confusing when\n> >> two things are related to \"display\" and only one of them is called\n> >> as such. Either \"display_state\" or \"state\" would be fine.\n> >\n> > Fair enough. In that case I may just as well drop the first patch.\n> \n> If you plan to get rid of an independent \"display_buffer\" in the\n> endgame by moving it into the bigger struct as its .buffer member,\n> then I think the naming is fine as there will remain only one thing\n> that is \"display\". The fact that I didn't see that plan through\n> when I read only the first two patches would probably mean that the\n> route this iteration of the series took was somewhat roundabout, and\n> there may be a more transparent and possibly a more direct way to\n> get to that goal?\n> \n> I am not entirely sure if the buffer should go inside the\n> display_state structure in the endgame. An alternative may be to\n> make it a on-stack variable of format_display() (which will later be\n> modified to do everything up to and including writing out the\n> result) and pass it through the callchain below to its helpers, just\n> like the current code already does. And in such an approach, you'd\n> still need to name that variable passed to the helper functions\n> called by format_display()---\"buffer\" would be a good name for that.\n\nWell, we could make it an on-stack variable just fine. But I suspect\nthat the only reason that this buffer exists is to optimize memory\nallocations: a git-fetch(1) can easily end up printing thousands or even\nhundreds of thousands of updated references, and reallocating that\nbuffer for each of them is quite wasteful.\n\nAnother alternative would be to make the buffer static and local to the\nfunction. But you now have shared state again, and furthermore you have\nno easy way to unleak its contents.\n\nIn the end, I think that having the buffer as a member of the display\nstate is the most straight-forward approach. It's self-contained and\nallows us to reduce the number of allocations to a minimum. That being\nsaid, I'm obviously biased here.\n\nPatrick\n"},{"id":"473651","messageId":"xmqq1qlnidw2.fsf@gitster.g","threadId":"59399","inReplyTo":"ZBQ4Cqkq9v2cB77f@ncase","subject":"Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-17T15:41:33Z","receivedAt":"2023-03-17T15:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Maybe the solution is to keep the order, but document intent better in\n> each of the patches leading towards the unified printing logic.\n\n;-).\n"},{"id":"473669","messageId":"20230317202449.1083635-1-jonathantanmy@google.com","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"Re: [PATCH 0/8] fetch: refactor code that prints reference updates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-17T20:24:49Z","receivedAt":"2023-03-17T20:24:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patrick Steinhardt writes:\n> 1. We want to take control of the reference updates so that we can\n> atomically update all or a subset of references that git-fetch\n> would have updated.\n> \n> 2. We want to be able to quarantine objects in a fetch so that we\n> can e.g. perform consistency checks for them before they land in\n> the main repository.\n\nIf you want to do this, something that might be possible is to change\nthe RHS of the refspecs to put the refs in a namespace of your choice\n(e.g. ...:refs//...) and then you can look at what's generated and\nprocess them as you wish.\n\n> - There should be as few global state as possible. This is to reduce\n> confusion and having to repeat the same incantations in multiple\n> different locations.\n\nMakes sense.\n\n> - The logic should be as self-contained as possible. This is so that\n> it can easily be changed in a subsequent patch series.\n\nAlso makes sense, but I think that some of your patches might be\ncontrary to this goal (more details below).\n\nI've read all the patches, but will just summarize my thoughts here.\n\n> Patrick Steinhardt (8):\n> fetch: rename `display` buffer to avoid name conflict\n\nOne other way, as others have discussed, is to just name the new\nvariable display_state. (I would prefer that, at the very least so\nthat in case someone else has a patch that contains the identifier\n\"display\", problems would be more easily noticed. This is very unlikely\nto happen but I think it's a good general direction for the Git project\nto follow.)\n\n> fetch: move reference width calculation into `display_state`\n> fetch: move output format into `display_state`\n> fetch: pass the full local reference name to `format_display`\n\nAll these are good changes that I would be happy to see merged.\n\n> fetch: deduplicate handling of per-reference format\n\nI'm not so sure that this is the correct abstraction. I think that this\nand the last patch might be counterproductive to your stated goal of\nhaving one more mode of printing the refs, in fact, since when we have\nthat new mode, the format would be different but the printing would\nremain (so we should split the format and printing).\n\n> fetch: deduplicate logic to print remote URL\n\nMakes sense, although I would need to consider only storing the\nraw URL in the struct display_state and processing it when it needs\nto be emitted (haven't checked if this is feasible, though).\n\n> fetch: fix inconsistent summary width for pruned and updated refs\n\nThis changes the behavior in that the summary width, even when printing\nthe summary of pruned refs, is computed based only on the updated refs.\nThe summary width might need to remain out of the struct display_state\nfor now.\n\n> fetch: centralize printing of reference updates\n\nSame as \"fetch: deduplicate handling of per-reference format\".\n"},{"id":"473747","messageId":"ZBgD3Ux34V/U/Lv5@ncase","threadId":"59399","inReplyTo":"20230317202449.1083635-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/8] fetch: refactor code that prints reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T06:57:33Z","receivedAt":"2023-03-20T06:57:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote:\n> Patrick Steinhardt writes:\n> > 1. We want to take control of the reference updates so that we can\n> > atomically update all or a subset of references that git-fetch\n> > would have updated.\n> > \n> > 2. We want to be able to quarantine objects in a fetch so that we\n> > can e.g. perform consistency checks for them before they land in\n> > the main repository.\n> \n> If you want to do this, something that might be possible is to change\n> the RHS of the refspecs to put the refs in a namespace of your choice\n> (e.g. ...:refs//...) and then you can look at what's generated and\n> process them as you wish.\n\nThere's two major problems with this, unfortunately:\n\n- We want to use the machine-parseable format in our repository\n mirroring functionality, where you can easily end up fetching\n thousands or even hundreds of thousands of references. If you need to\n write all of them anew in a first step then you'll end up slower than\n before.\n\n- Repository mirroring is comparatively flexible in what it allows. Most\n importantly, it gives you the opiton to say that divergent references\n should not be updated at all, which translates into an unforced fetch.\n It's even possible to have fetches with mixed forced and unforced\n reference updates. So if we fetched into a separate namespace first,\n we'd now have to reimplement checks for forced updates in Gitaly so\n that we correctly update only those refs that would have been updated\n by Git. We'd also need to manually figure out deleted references.\n\n This would be quite a risky change and would duplicate a lot of\n knowledge. Furthermore, merging the two sets of references would\n likely be quite expensive performance-wise.\n\nAlso, even if we did have a different RHS, it still wouldn't fix the\nissue that objects are written into the main object database directly.\nIdeally, we'd really only accept changes into the repository once we\nhave fully verified all of them. Right now it can happen that we refuse\na fetch, but the objects would continue to exist in the repository.\n\nA second motivation for the quarantine directory is so that we can\nenumerate all objects that are indeed new. This will eventually be used\nto implement more efficient replication of the repository, where we can\ntheoretically just take all of the fetched objects in the quarantine\nobject directory and copy it to the replicas of that repository.\n\n[snip]\n> > fetch: deduplicate handling of per-reference format\n> \n> I'm not so sure that this is the correct abstraction. I think that this\n> and the last patch might be counterproductive to your stated goal of\n> having one more mode of printing the refs, in fact, since when we have\n> that new mode, the format would be different but the printing would\n> remain (so we should split the format and printing).\n\nI already have the full implementation of the new machine-parseable\nformat available locally, but didn't want to send it as part of this\npatch series yet to avoid it becoming overly large. But I can say that\nthis change really did make the end goal easier to achieve, due to two\nreasons:\n\n- If we continued to handle the per-reference format at the different\n callsites, I'd have to also amend each of the callers when introducing\n the new format as we're going to use a different format there. But\n when doing this in `format_display()`, we really only need to have a\n single switch at the beginning to check whether to use the machine\n parseable format or the other one.\n\n- Currently, all reference updates are printed to stderr. As stderr is\n also used to display errors and the progress bar, this really makes it\n not a good fit for the machine-parseable format. Instead, I decided\n that it would make more sense to print the new format to stdout. And\n by having the printing-logic self-contained we again only have a\n single location we need to change.\n\nI realize though that all of this isn't as well-documented in the commit\nmessages as it should be, which is also something that Junio complained\nabout. I'll hopefully do a better job in v2 of this patch series.\n\n> > fetch: deduplicate logic to print remote URL\n> \n> Makes sense, although I would need to consider only storing the\n> raw URL in the struct display_state and processing it when it needs\n> to be emitted (haven't checked if this is feasible, though).\n> \n> > fetch: fix inconsistent summary width for pruned and updated refs\n> \n> This changes the behavior in that the summary width, even when printing\n> the summary of pruned refs, is computed based only on the updated refs.\n> The summary width might need to remain out of the struct display_state\n> for now.\n\nFair, that's a case I didn't yet consider. I'll have another look.\n\nPatrick\n"},{"id":"473763","messageId":"ZBhQ/z7mUxa/ly95@ncase","threadId":"59399","inReplyTo":"20230317202449.1083635-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/8] fetch: refactor code that prints reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:26:39Z","receivedAt":"2023-03-20T12:26:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote:\n> Patrick Steinhardt writes:\n[snip]\n> > fetch: deduplicate logic to print remote URL\n> \n> Makes sense, although I would need to consider only storing the\n> raw URL in the struct display_state and processing it when it needs\n> to be emitted (haven't checked if this is feasible, though).\n\nWe likely could, but right now the benefit isn't all that high. If the\nURL was only used in `display_ref_update()` then this would be easy\nenough to do. But we also access the sanitized URL when the connectivity\ncheck fails or when printing to FETCH_HEAD.\n\nIf we provided an accessor function thet returns the URL it would be\ntrivial to do, but what do we really gain here? In the best case we save\nan allocation for the URL and two loops ranging over it. That doesn't\nquite feel worth it to me.\n\nPatrick\n"},{"id":"473764","messageId":"cover.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1678878623.git.ps@pks.im","subject":"[PATCH v2 0/6] fetch: refactor code that prints reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:16Z","receivedAt":"2023-03-20T12:35:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nThis is the second version of my patch series to refactor the code that\nprints reference updates during git-fetch(1). Its aim is it to make the\ncode more self-contained so that it can be easily amended for a new\nmachine-parseable format.\n\nChanges compared to v1:\n\n - I've dropped the first patch to rename the preexisting `display`\n buffer variable. Instead, I'm now just picking the longer\n `display_state` variable name for the newly introduced structure\n in order to avoid naming conflicts.\n\n - I've touched up the commit messages of patches 4-6 to hopefully\n clarify their intent a little bit better.\n\n - I've dropped patch 7/8 that unified the logic to calculate the\n summary width. Even though it fixes a bug in one case, Jonathan\n correctly pointed out that it's weird in the case where there are\n only reference deletions without any reference updates. I may have\n another go in a separate patch series after the dust has settled.\n\nThanks for the feedback so far!\n\nPatrick\n\nPatrick Steinhardt (6):\n fetch: move reference width calculation into `display_state`\n fetch: move output format into `display_state`\n fetch: pass the full local reference name to `format_display`\n fetch: centralize handling of per-reference format\n fetch: centralize logic to print remote URL\n fetch: centralize printing of reference updates\n\n builtin/fetch.c | 267 +++++++++++++++++++++++++-----------------------\n 1 file changed, 138 insertions(+), 129 deletions(-)\n\nRange-diff against v1:\n1: 692206f7ff < -: ---------- fetch: rename `display` buffer to avoid name conflict\n2: aa792b12a4 ! 1: ce2c4b61ae fetch: move reference width calculation into `display_state`\n @@ builtin/fetch.c: static void adjust_refcol_width(const struct ref *ref)\n }\n \n -static void prepare_format_display(struct ref *ref_map)\n -+static void display_state_init(struct display_state *display, struct ref *ref_map)\n ++static void display_state_init(struct display_state *display_state, struct ref *ref_map)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n -+\tmemset(display, 0, sizeof(*display));\n ++\tmemset(display_state, 0, sizeof(*display_state));\n +\n \tif (verbosity < 0)\n \t\treturn;\n @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n \n -+\tdisplay->refcol_width = 10;\n ++\tdisplay_state->refcol_width = 10;\n \tfor (rm = ref_map; rm; rm = rm->next) {\n +\t\tint width;\n +\n @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)\n +\t\t * appear on the left hand side of '->' and shrink the column\n +\t\t * back.\n +\t\t */\n -+\t\tif (display->refcol_width < width)\n -+\t\t\tdisplay->refcol_width = width;\n ++\t\tif (display_state->refcol_width < width)\n ++\t\t\tdisplay_state->refcol_width = width;\n \t}\n }\n \n --static void print_remote_to_local(struct strbuf *display_buffer,\n -+static void print_remote_to_local(struct display_state *display,\n +-static void print_remote_to_local(struct strbuf *display,\n ++static void print_remote_to_local(struct display_state *display_state,\n +\t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n --\tstrbuf_addf(display_buffer, \"%-*s -> %s\", refcol_width, remote, local);\n -+\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display->refcol_width, remote, local);\n +-\tstrbuf_addf(display, \"%-*s -> %s\", refcol_width, remote, local);\n ++\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display_state->refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n @@ builtin/fetch.c: static int find_and_replace(struct strbuf *haystack,\n \treturn 1;\n }\n \n --static void print_compact(struct strbuf *display_buffer,\n -+static void print_compact(struct display_state *display, struct strbuf *display_buffer,\n +-static void print_compact(struct strbuf *display,\n ++static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n --\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", refcol_width, remote);\n -+\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display->refcol_width, remote);\n +-\t\tstrbuf_addf(display, \"%-*s -> *\", refcol_width, remote);\n ++\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display_state->refcol_width, remote);\n \t\treturn;\n \t}\n \n -@@ builtin/fetch.c: static void print_compact(struct strbuf *display_buffer,\n +@@ builtin/fetch.c: 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_buffer, r.buf, l.buf);\n -+\tprint_remote_to_local(display, display_buffer, r.buf, l.buf);\n +-\tprint_remote_to_local(display, r.buf, l.buf);\n ++\tprint_remote_to_local(display_state, display_buffer, r.buf, l.buf);\n \n \tstrbuf_release(&r);\n \tstrbuf_release(&l);\n }\n \n --static void format_display(struct strbuf *display_buffer, char code,\n -+static void format_display(struct display_state *display,\n +-static void format_display(struct strbuf *display, char code,\n ++static void format_display(struct display_state *display_state,\n +\t\t\t struct strbuf *display_buffer, 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 -@@ builtin/fetch.c: static void format_display(struct strbuf *display_buffer, char code,\n +@@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,\n \n - \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n + \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n + \n +-\tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n ++\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n --\t\tprint_remote_to_local(display_buffer, remote, local);\n -+\t\tprint_remote_to_local(display, display_buffer, remote, local);\n +-\t\tprint_remote_to_local(display, remote, local);\n ++\t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n \telse\n --\t\tprint_compact(display_buffer, remote, local);\n -+\t\tprint_compact(display, display_buffer, remote, local);\n +-\t\tprint_compact(display, remote, local);\n ++\t\tprint_compact(display_state, display_buffer, remote, local);\n \tif (error)\n - \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n +-\t\tstrbuf_addf(display, \" (%s)\", error);\n ++\t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n -+\t\t\t struct display_state *display,\n ++\t\t\t struct display_state *display_state,\n \t\t\t const char *remote, const struct ref *remote_ref,\n - \t\t\t struct strbuf *display_buffer, int summary_width)\n + \t\t\t struct strbuf *display, int summary_width)\n {\n @@ builtin/fetch.c: 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_buffer, '=', _(\"[up to date]\"), NULL,\n -+\t\t\tformat_display(display, display_buffer, '=', _(\"[up to date]\"), NULL,\n +-\t\t\tformat_display(display, '=', _(\"[up to date]\"), NULL,\n ++\t\t\tformat_display(display_state, display, '=', _(\"[up to date]\"), NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n @@ builtin/fetch.c: 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_buffer, '!', _(\"[rejected]\"),\n -+\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n +-\t\tformat_display(display, '!', _(\"[rejected]\"),\n ++\t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n @@ builtin/fetch.c: 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_buffer, r ? '!' : 't', _(\"[tag update]\"),\n -+\t\t\tformat_display(display, display_buffer, r ? '!' : 't', _(\"[tag update]\"),\n +-\t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n ++\t\t\tformat_display(display_state, 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_buffer, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n -+\t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n +-\t\t\tformat_display(display, '!', _(\"[rejected]\"), _(\"would clobber existing tag\"),\n ++\t\t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n +\t\t\t\t _(\"would clobber existing tag\"),\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\t\treturn 1;\n @@ builtin/fetch.c: 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_buffer, r ? '!' : '*', what,\n -+\t\tformat_display(display, display_buffer, r ? '!' : '*', what,\n +-\t\tformat_display(display, r ? '!' : '*', what,\n ++\t\tformat_display(display_state, 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 @@ builtin/fetch.c: 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_buffer, r ? '!' : ' ', quickref.buf,\n -+\t\tformat_display(display, display_buffer, r ? '!' : ' ', quickref.buf,\n +-\t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n ++\t\tformat_display(display_state, 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 @@ builtin/fetch.c: 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_buffer, r ? '!' : '+', quickref.buf,\n -+\t\tformat_display(display, display_buffer, r ? '!' : '+', quickref.buf,\n +-\t\tformat_display(display, r ? '!' : '+', quickref.buf,\n ++\t\tformat_display(display_state, 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_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n -+\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n +-\t\tformat_display(display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n ++\t\tformat_display(display_state, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n @@ builtin/fetch.c: N_(\"it took %.2f seconds to check forced updates; you can use\\n\n \"to avoid this check\\n\");\n \n -static int store_updated_refs(const char *raw_url, const char *remote_name,\n -+static int store_updated_refs(struct display_state *display,\n ++static int store_updated_refs(struct display_state *display_state,\n +\t\t\t const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n -\t\t\t\trc |= update_local_ref(ref, transaction, what,\n -+\t\t\t\trc |= update_local_ref(ref, transaction, display, what,\n ++\t\t\t\trc |= update_local_ref(ref, transaction, display_state, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n \t\t\t\t * is set).\n \t\t\t\t */\n -\t\t\t\tformat_display(¬e, '*',\n -+\t\t\t\tformat_display(display, ¬e, '*',\n ++\t\t\t\tformat_display(display_state, ¬e, '*',\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 @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n }\n \n -static int fetch_and_consume_refs(struct transport *transport,\n -+static int fetch_and_consume_refs(struct display_state *display,\n ++static int fetch_and_consume_refs(struct display_state *display_state,\n +\t\t\t\t struct transport *transport,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct ref *ref_map,\n @@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n -\tret = store_updated_refs(transport->url, transport->remote->name,\n -+\tret = store_updated_refs(display, transport->url, transport->remote->name,\n ++\tret = store_updated_refs(display_state, transport->url, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n @@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,\n }\n \n -static int prune_refs(struct refspec *rs,\n -+static int prune_refs(struct display_state *display,\n ++static int prune_refs(struct display_state *display_state,\n +\t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n \t\t struct ref *ref_map,\n @@ builtin/fetch.c: static int prune_refs(struct refspec *rs,\n \t\t\t\tshown_url = 1;\n \t\t\t}\n -\t\t\tformat_display(&sb, '-', _(\"[deleted]\"), NULL,\n -+\t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n ++\t\t\tformat_display(display_state, &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 @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot\n }\n \n -static int backfill_tags(struct transport *transport,\n -+static int backfill_tags(struct display_state *display,\n ++static int backfill_tags(struct display_state *display_state,\n +\t\t\t struct transport *transport,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n @@ builtin/fetch.c: static int backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n -\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);\n -+\tretcode = fetch_and_consume_refs(display, transport, transaction, ref_map, fetch_head);\n ++\tretcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n -+\tstruct display_state display;\n ++\tstruct display_state display_state;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n -+\tdisplay_state_init(&display, ref_map);\n ++\tdisplay_state_init(&display_state, ref_map);\n +\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\t */\n \t\tif (rs->nr) {\n -\t\t\tretcode = prune_refs(rs, transaction, ref_map, transport->url);\n -+\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map, transport->url);\n ++\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);\n \t\t} else {\n -\t\t\tretcode = prune_refs(&transport->remote->fetch,\n -+\t\t\tretcode = prune_refs(&display, &transport->remote->fetch,\n ++\t\t\tretcode = prune_refs(&display_state, &transport->remote->fetch,\n \t\t\t\t\t transaction, ref_map,\n \t\t\t\t\t transport->url);\n \t\t}\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t}\n \n -\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {\n -+\tif (fetch_and_consume_refs(&display, transport, transaction, ref_map, &fetch_head)) {\n ++\tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n -\t\t\tif (backfill_tags(transport, transaction, tags_ref_map,\n -+\t\t\tif (backfill_tags(&display, transport, transaction, tags_ref_map,\n ++\t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n \t\t\t\t\t &fetch_head))\n \t\t\t\tretcode = 1;\n \t\t}\n3: a4fd935c40 ! 2: 34eedb882c fetch: move output format into `display_state`\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n {\n \tint max, rlen, llen, len;\n \n -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma\n +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *\n \n \tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n -\t\tcompact_format = 0;\n -+\t\tdisplay->compact_format = 0;\n ++\t\tdisplay_state->compact_format = 0;\n \telse if (!strcasecmp(format, \"compact\"))\n -\t\tcompact_format = 1;\n -+\t\tdisplay->compact_format = 1;\n ++\t\tdisplay_state->compact_format = 1;\n \telse\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma\n +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *\n \t\t !strcmp(rm->name, \"HEAD\"))\n \t\t\tcontinue;\n \n -\t\twidth = refcol_width(rm);\n -+\t\twidth = refcol_width(rm, display->compact_format);\n ++\t\twidth = refcol_width(rm, display_state->compact_format);\n \n \t\t/*\n \t\t * Not precise calculation for compact mode because '*' can\n -@@ builtin/fetch.c: static void format_display(struct display_state *display,\n +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n -\tif (!compact_format)\n -+\tif (!display->compact_format)\n - \t\tprint_remote_to_local(display, display_buffer, remote, local);\n ++\tif (!display_state->compact_format)\n + \t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n \telse\n - \t\tprint_compact(display, display_buffer, remote, local);\n + \t\tprint_compact(display_state, display_buffer, remote, local);\n4: 0998173b57 ! 3: ec355b8b8d fetch: pass the full local reference name to `format_display`\n @@ Commit message\n Signed-off-by: Patrick Steinhardt \n \n ## builtin/fetch.c ##\n -@@ builtin/fetch.c: static void format_display(struct display_state *display,\n +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n - \tif (!display->compact_format)\n --\t\tprint_remote_to_local(display, display_buffer, remote, local);\n -+\t\tprint_remote_to_local(display, display_buffer, remote, prettify_refname(local));\n + \tif (!display_state->compact_format)\n +-\t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n ++\t\tprint_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));\n \telse\n --\t\tprint_compact(display, display_buffer, remote, local);\n -+\t\tprint_compact(display, display_buffer, remote, prettify_refname(local));\n +-\t\tprint_compact(display_state, display_buffer, remote, local);\n ++\t\tprint_compact(display_state, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n - \t\t\t struct strbuf *display_buffer, int summary_width)\n + \t\t\t struct strbuf *display, int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n -\tconst char *pretty_ref = prettify_refname(ref->name);\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n - \t\t\tformat_display(display, display_buffer, '=', _(\"[up to date]\"), NULL,\n + \t\t\tformat_display(display_state, display, '=', _(\"[up to date]\"), NULL,\n -\t\t\t\t remote, pretty_ref, summary_width);\n +\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 0;\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t\t */\n - \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n + \t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n -\t\t\t remote, pretty_ref, summary_width);\n +\t\t\t remote, ref->name, summary_width);\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n - \t\t\tformat_display(display, display_buffer, r ? '!' : 't', _(\"[tag update]\"),\n + \t\t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\t\treturn r;\n \t\t} else {\n - \t\t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"),\n + \t\t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t\t _(\"would clobber existing tag\"),\n -\t\t\t\t remote, pretty_ref, summary_width);\n +\t\t\t\t remote, ref->name, summary_width);\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t}\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(msg, ref, transaction, 0);\n - \t\tformat_display(display, display_buffer, r ? '!' : '*', what,\n + \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n - \t\tformat_display(display, display_buffer, r ? '!' : ' ', quickref.buf,\n + \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t} else if (force || ref->force) {\n @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n - \t\tformat_display(display, display_buffer, r ? '!' : '+', quickref.buf,\n + \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n - \t\tformat_display(display, display_buffer, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n + \t\tformat_display(display_state, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n -\t\t\t remote, pretty_ref, summary_width);\n +\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n }\n -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,\n \t\t\t\tshown_url = 1;\n \t\t\t}\n - \t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n + \t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n -\t\t\t\t _(\"(none)\"), prettify_refname(ref->name),\n +\t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n5: d45ec31eea ! 4: e4c1ed4ad5 fetch: deduplicate handling of per-reference format\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - fetch: deduplicate handling of per-reference format\n + fetch: centralize handling of per-reference format\n \n - Both callsites that call `format_display()` and then print the result to\n - standard error use the same formatting directive \" %s\\n\" to print the\n - reference to disk, thus duplicating a small part of the logic.\n + The function `format_display()` is used to print a single reference\n + update to a buffer which will then ultimately be printed by the caller.\n + This architecture causes us to duplicate some logic across the different\n + callsites of this function. This makes it hard to follow the code as\n + some parts of the logic are located in one place, while other parts of\n + the logic are located in a different place. Furthermore, by having the\n + logic scattered around it becomes quite hard to implement a new output\n + format for the reference updates.\n \n - Refactor the code to handle this in `format_display()` itself. This\n - paves the way for handling the printing logic in that function\n - completely.\n + We can make the logic a whole lot easier to understand by making the\n + `format_display()` function self-contained so that it handles formatting\n + and printing of the references. This will eventually allow us to easily\n + implement a completely different output format, but also opens the door\n + to conditionally print to either stdout or stderr depending on the\n + output format.\n +\n + As a first step towards that goal we move the formatting directive used\n + by both callers to print a single reference update into this function.\n \n Signed-off-by: Patrick Steinhardt \n \n ## builtin/fetch.c ##\n -@@ builtin/fetch.c: static void format_display(struct display_state *display,\n +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n -\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n +\tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n - \tif (!display->compact_format)\n - \t\tprint_remote_to_local(display, display_buffer, remote, prettify_refname(local));\n + \tif (!display_state->compact_format)\n + \t\tprint_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));\n \telse\n - \t\tprint_compact(display, display_buffer, remote, prettify_refname(local));\n + \t\tprint_compact(display_state, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n +\tstrbuf_addch(display_buffer, '\\n');\n }\n \n static int update_local_ref(struct ref *ref,\n -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,\n \t\t\t\t\t\t\turl_len, url);\n \t\t\t\t\tshown_url = 1;\n \t\t\t\t}\n @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n \t\t\t}\n \t\t}\n \t}\n -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,\n - \t\t\tformat_display(display, &sb, '-', _(\"[deleted]\"), NULL,\n +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,\n + \t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n -\t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n6: 2ea3a4e308 ! 5: 98b799af71 fetch: deduplicate logic to print remote URL\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - fetch: deduplicate logic to print remote URL\n + fetch: centralize logic to print remote URL\n \n When fetching from a remote, we not only print the actual references\n that have changed, but will also print the URL from which we have\n @@ Commit message\n can convert the global variable into a member of `display_state`. And\n second, we can deduplicate the logic to compute the anonymized URL.\n \n + This also works as expected when fetching from multiple remotes, for\n + example via a group of remotes, as we do this by forking a standalone\n + git-fetch(1) process per remote that is to be fetched.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## builtin/fetch.c ##\n @@ builtin/fetch.c: static int refcol_width(const struct ref *ref, int compact_form\n \treturn rlen;\n }\n \n --static void display_state_init(struct display_state *display, struct ref *ref_map)\n -+static void display_state_init(struct display_state *display, struct ref *ref_map,\n +-static void display_state_init(struct display_state *display_state, struct ref *ref_map)\n ++static void display_state_init(struct display_state *display_state, struct ref *ref_map,\n +\t\t\t const char *raw_url)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n +\tint i;\n \n - \tmemset(display, 0, sizeof(*display));\n + \tmemset(display_state, 0, sizeof(*display_state));\n \n +\tif (raw_url)\n -+\t\tdisplay->url = transport_anonymize_url(raw_url);\n ++\t\tdisplay_state->url = transport_anonymize_url(raw_url);\n +\telse\n -+\t\tdisplay->url = xstrdup(\"foreign\");\n ++\t\tdisplay_state->url = xstrdup(\"foreign\");\n +\n -+\tdisplay->url_len = strlen(display->url);\n -+\tfor (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)\n ++\tdisplay_state->url_len = strlen(display_state->url);\n ++\tfor (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)\n +\t\t;\n -+\tdisplay->url_len = i + 1;\n -+\tif (4 < i && !strncmp(\".git\", display->url + i - 3, 4))\n -+\t\tdisplay->url_len = i - 3;\n ++\tdisplay_state->url_len = i + 1;\n ++\tif (4 < i && !strncmp(\".git\", display_state->url + i - 3, 4))\n ++\t\tdisplay_state->url_len = i - 3;\n +\n \tif (verbosity < 0)\n \t\treturn;\n \n -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma\n +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *\n \t}\n }\n \n -+static void display_state_release(struct display_state *display)\n ++static void display_state_release(struct display_state *display_state)\n +{\n -+\tfree(display->url);\n ++\tfree(display_state->url);\n +}\n +\n - static void print_remote_to_local(struct display_state *display,\n + static void print_remote_to_local(struct display_state *display_state,\n \t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n -@@ builtin/fetch.c: static void format_display(struct display_state *display,\n +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,\n \tif (verbosity < 0)\n \t\treturn;\n \n -+\tif (!display->shown_url) {\n -+\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"), display->url_len, display->url);\n -+\t\tdisplay->shown_url = 1;\n ++\tif (!display_state->shown_url) {\n ++\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"),\n ++\t\t\t display_state->url_len, display_state->url);\n ++\t\tdisplay_state->shown_url = 1;\n +\t}\n +\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n @@ builtin/fetch.c: static void format_display(struct display_state *display,\n @@ builtin/fetch.c: N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\n \n - static int store_updated_refs(struct display_state *display,\n + static int store_updated_refs(struct display_state *display_state,\n -\t\t\t const char *raw_url, const char *remote_name,\n +\t\t\t const char *remote_name,\n \t\t\t int connectivity_checked,\n @@ builtin/fetch.c: N_(\"it took %.2f seconds to check forced updates; you can use\\n\n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n -\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n -+\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), display->url);\n ++\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"),\n ++\t\t\t\t display_state->url);\n \t\t\tgoto abort;\n \t\t}\n \t}\n -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,\n \t\t\t\twhat = rm->name;\n \t\t\t}\n \n @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (*what) {\n \t\t\t\tif (*kind)\n -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,\n \n \t\t\tappend_fetch_head(fetch_head, &rm->old_oid,\n \t\t\t\t\t rm->fetch_head_status,\n -\t\t\t\t\t note.buf, url, url_len);\n -+\t\t\t\t\t note.buf, display->url, display->url_len);\n ++\t\t\t\t\t note.buf, display_state->url,\n ++\t\t\t\t\t display_state->url_len);\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,\n \t\t\t\t\t *what ? what : \"HEAD\",\n \t\t\t\t\t \"FETCH_HEAD\", summary_width);\n \t\t\t}\n @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n \t\t}\n \t}\n \n -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,\n \n abort:\n \tstrbuf_release(¬e);\n @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,\n \treturn rc;\n }\n \n -@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n --\tret = store_updated_refs(display, transport->url, transport->remote->name,\n -+\tret = store_updated_refs(display, transport->remote->name,\n +-\tret = store_updated_refs(display_state, transport->url, transport->remote->name,\n ++\tret = store_updated_refs(display_state, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n -@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,\n - static int prune_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,\n + static int prune_refs(struct display_state *display_state,\n \t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n -\t\t struct ref *ref_map,\n @@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display\n \tif (!dry_run) {\n \t\tif (transaction) {\n \t\t\tfor (ref = stale_refs; ref; ref = ref->next) {\n -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,\n \n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n @@ builtin/fetch.c: static int prune_refs(struct display_state *display,\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(display, &sb, '-', _(\"[deleted]\"), NULL,\n + \t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,\n +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,\n \n cleanup:\n \tstrbuf_release(&err);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n --\tstruct display_state display;\n -+\tstruct display_state display = { 0 };\n +-\tstruct display_state display_state;\n ++\tstruct display_state display_state = { 0 };\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n --\tdisplay_state_init(&display, ref_map);\n -+\tdisplay_state_init(&display, ref_map, transport->url);\n +-\tdisplay_state_init(&display_state, ref_map);\n ++\tdisplay_state_init(&display_state, ref_map, transport->url);\n \n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n --\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map, transport->url);\n -+\t\t\tretcode = prune_refs(&display, rs, transaction, ref_map);\n +-\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);\n ++\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map);\n \t\t} else {\n - \t\t\tretcode = prune_refs(&display, &transport->remote->fetch,\n + \t\t\tretcode = prune_refs(&display_state, &transport->remote->fetch,\n -\t\t\t\t\t transaction, ref_map,\n -\t\t\t\t\t transport->url);\n +\t\t\t\t\t transaction, ref_map);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\terror(\"%s\", err.buf);\n \t}\n \n -+\tdisplay_state_release(&display);\n ++\tdisplay_state_release(&display_state);\n \tclose_fetch_head(&fetch_head);\n \tstrbuf_release(&err);\n \tfree_refs(ref_map);\n7: f67f9640a8 < -: ---------- fetch: fix inconsistent summary width for pruned and updated refs\n8: 9667301711 < -: ---------- fetch: centralize printing of reference updates\n-: ---------- > 6: fe7e2e85eb fetch: centralize printing of reference updates\n-- \n2.40.0\n\n"},{"id":"473765","messageId":"ce2c4b61aee26461b4edb934ceccf57daa62b056.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 1/6] fetch: move reference width calculation into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:20Z","receivedAt":"2023-03-20T12:35:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to print references in proper columns we need to calculate the\nwidth of the reference column before starting to print the references.\nThis is done with the help of a global variable `refcol_width`.\n\nRefactor the code to instead use a new structure `display_state` that\ncontains the computed width and plumb it through the stack as required.\nThis is only the first step towards de-globalizing the state required to\nprint references.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 111 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 65 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a09606b472..391959ad3d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -47,6 +47,10 @@ enum {\n \tTAGS_SET = 2\n };\n \n+struct display_state {\n+\tint refcol_width;\n+};\n+\n static int fetch_prune_config = -1; /* unspecified */\n static int fetch_show_forced_updates = 1;\n static uint64_t forced_updates_ms = 0;\n@@ -741,16 +745,15 @@ static int s_update_ref(const char *action,\n \treturn ret;\n }\n \n-static int refcol_width = 10;\n static int compact_format;\n \n-static void adjust_refcol_width(const struct ref *ref)\n+static int refcol_width(const struct ref *ref)\n {\n \tint max, rlen, llen, len;\n \n \t/* uptodate lines are only shown on high verbosity level */\n \tif (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))\n-\t\treturn;\n+\t\treturn 0;\n \n \tmax = term_columns();\n \trlen = utf8_strwidth(prettify_refname(ref->name));\n@@ -769,22 +772,18 @@ static void adjust_refcol_width(const struct ref *ref)\n \t}\n \tlen = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;\n \tif (len >= max)\n-\t\treturn;\n+\t\treturn 0;\n \n-\t/*\n-\t * Not precise calculation for compact mode because '*' can\n-\t * appear on the left hand side of '->' and shrink the column\n-\t * back.\n-\t */\n-\tif (refcol_width < rlen)\n-\t\trefcol_width = rlen;\n+\treturn rlen;\n }\n \n-static void prepare_format_display(struct ref *ref_map)\n+static void display_state_init(struct display_state *display_state, struct ref *ref_map)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n \n+\tmemset(display_state, 0, sizeof(*display_state));\n+\n \tif (verbosity < 0)\n \t\treturn;\n \n@@ -797,20 +796,32 @@ static void prepare_format_display(struct ref *ref_map)\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n \n+\tdisplay_state->refcol_width = 10;\n \tfor (rm = ref_map; rm; rm = rm->next) {\n+\t\tint width;\n+\n \t\tif (rm->status == REF_STATUS_REJECT_SHALLOW ||\n \t\t !rm->peer_ref ||\n \t\t !strcmp(rm->name, \"HEAD\"))\n \t\t\tcontinue;\n \n-\t\tadjust_refcol_width(rm);\n+\t\twidth = refcol_width(rm);\n+\n+\t\t/*\n+\t\t * Not precise calculation for compact mode because '*' can\n+\t\t * appear on the left hand side of '->' and shrink the column\n+\t\t * back.\n+\t\t */\n+\t\tif (display_state->refcol_width < width)\n+\t\t\tdisplay_state->refcol_width = width;\n \t}\n }\n \n-static void print_remote_to_local(struct strbuf *display,\n+static void print_remote_to_local(struct display_state *display_state,\n+\t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n-\tstrbuf_addf(display, \"%-*s -> %s\", refcol_width, remote, local);\n+\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display_state->refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n@@ -840,14 +851,14 @@ 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 display_state *display_state, struct strbuf *display_buffer,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n-\t\tstrbuf_addf(display, \"%-*s -> *\", refcol_width, remote);\n+\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display_state->refcol_width, remote);\n \t\treturn;\n \t}\n \n@@ -856,13 +867,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(display_state, display_buffer, 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 display_state *display_state,\n+\t\t\t struct strbuf *display_buffer, 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@@ -874,17 +886,18 @@ static void format_display(struct strbuf *display, char code,\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n+\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n-\t\tprint_remote_to_local(display, remote, local);\n+\t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n \telse\n-\t\tprint_compact(display, remote, local);\n+\t\tprint_compact(display_state, display_buffer, remote, local);\n \tif (error)\n-\t\tstrbuf_addf(display, \" (%s)\", error);\n+\t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n+\t\t\t struct display_state *display_state,\n \t\t\t const char *remote, const struct ref *remote_ref,\n \t\t\t struct strbuf *display, int summary_width)\n {\n@@ -897,7 +910,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(display_state, display, '=', _(\"[up to date]\"), NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n@@ -909,7 +922,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(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n@@ -920,12 +933,13 @@ 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(display_state, 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(display_state, display, '!', _(\"[rejected]\"),\n+\t\t\t\t _(\"would clobber existing tag\"),\n \t\t\t\t remote, pretty_ref, summary_width);\n \t\t\treturn 1;\n \t\t}\n@@ -957,7 +971,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(display_state, 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@@ -979,7 +993,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(display_state, 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@@ -991,13 +1005,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(display_state, 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(display_state, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n \t\t\t remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n@@ -1108,7 +1122,8 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\\n\"\n \"to avoid this check\\n\");\n \n-static int store_updated_refs(const char *raw_url, const char *remote_name,\n+static int store_updated_refs(struct display_state *display_state,\n+\t\t\t const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n@@ -1139,8 +1154,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@@ -1240,7 +1253,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, transaction, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, display_state, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1249,7 +1262,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(¬e, '*',\n+\t\t\t\tformat_display(display_state, ¬e, '*',\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@@ -1328,7 +1341,8 @@ static int check_exist_and_connected(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_and_consume_refs(struct transport *transport,\n+static int fetch_and_consume_refs(struct display_state *display_state,\n+\t\t\t\t struct transport *transport,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct ref *ref_map,\n \t\t\t\t struct fetch_head *fetch_head)\n@@ -1352,7 +1366,7 @@ static int fetch_and_consume_refs(struct transport *transport,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(transport->url, transport->remote->name,\n+\tret = store_updated_refs(display_state, transport->url, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n@@ -1362,7 +1376,8 @@ static int fetch_and_consume_refs(struct transport *transport,\n \treturn ret;\n }\n \n-static int prune_refs(struct refspec *rs,\n+static int prune_refs(struct display_state *display_state,\n+\t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n \t\t struct ref *ref_map,\n \t\t const char *raw_url)\n@@ -1416,7 +1431,7 @@ static int prune_refs(struct refspec *rs,\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(display_state, &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@@ -1542,7 +1557,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \treturn transport;\n }\n \n-static int backfill_tags(struct transport *transport,\n+static int backfill_tags(struct display_state *display_state,\n+\t\t\t struct transport *transport,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n@@ -1566,7 +1582,7 @@ static int backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);\n+\tretcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1581,6 +1597,7 @@ static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n+\tstruct display_state display_state;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n@@ -1662,6 +1679,8 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n+\tdisplay_state_init(&display_state, ref_map);\n+\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction) {\n@@ -1679,9 +1698,9 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tretcode = prune_refs(rs, transaction, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);\n \t\t} else {\n-\t\t\tretcode = prune_refs(&transport->remote->fetch,\n+\t\t\tretcode = prune_refs(&display_state, &transport->remote->fetch,\n \t\t\t\t\t transaction, ref_map,\n \t\t\t\t\t transport->url);\n \t\t}\n@@ -1689,7 +1708,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n-\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {\n+\tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1711,7 +1730,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t * when `--atomic` is passed: in that case we'll abort\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n-\t\t\tif (backfill_tags(transport, transaction, tags_ref_map,\n+\t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n \t\t\t\t\t &fetch_head))\n \t\t\t\tretcode = 1;\n \t\t}\n-- \n2.40.0\n\n"},{"id":"473766","messageId":"34eedb882cf06ddc6dba9027d15872527c4500b6.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 2/6] fetch: move output format into `display_state`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:24Z","receivedAt":"2023-03-20T12:35:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-fetch(1) command supports printing references either in \"full\"\nor \"compact\" format depending on the `fetch.ouput` config key. The\nformat that is to be used is tracked in a global variable.\n\nDe-globalize the variable by moving it into the `display_state`\nstructure.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 391959ad3d..6d6146b0f0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -49,6 +49,7 @@ enum {\n \n struct display_state {\n \tint refcol_width;\n+\tint compact_format;\n };\n \n static int fetch_prune_config = -1; /* unspecified */\n@@ -745,9 +746,7 @@ static int s_update_ref(const char *action,\n \treturn ret;\n }\n \n-static int compact_format;\n-\n-static int refcol_width(const struct ref *ref)\n+static int refcol_width(const struct ref *ref, int compact_format)\n {\n \tint max, rlen, llen, len;\n \n@@ -789,9 +788,9 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \n \tgit_config_get_string_tmp(\"fetch.output\", &format);\n \tif (!strcasecmp(format, \"full\"))\n-\t\tcompact_format = 0;\n+\t\tdisplay_state->compact_format = 0;\n \telse if (!strcasecmp(format, \"compact\"))\n-\t\tcompact_format = 1;\n+\t\tdisplay_state->compact_format = 1;\n \telse\n \t\tdie(_(\"invalid value for '%s': '%s'\"),\n \t\t \"fetch.output\", format);\n@@ -805,7 +804,7 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \t\t !strcmp(rm->name, \"HEAD\"))\n \t\t\tcontinue;\n \n-\t\twidth = refcol_width(rm);\n+\t\twidth = refcol_width(rm, display_state->compact_format);\n \n \t\t/*\n \t\t * Not precise calculation for compact mode because '*' can\n@@ -887,7 +886,7 @@ static void format_display(struct display_state *display_state,\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n-\tif (!compact_format)\n+\tif (!display_state->compact_format)\n \t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n \telse\n \t\tprint_compact(display_state, display_buffer, remote, local);\n-- \n2.40.0\n\n"},{"id":"473767","messageId":"ec355b8b8d3de445a69559b75db840b26f4fb3cc.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 3/6] fetch: pass the full local reference name to `format_display`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:28Z","receivedAt":"2023-03-20T12:35:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Before printing the name of the local references that would be updated\nby a fetch we first prettify the reference name. This is done at the\ncalling side so that `format_display()` never sees the full name of the\nlocal reference. This restricts our ability to introduce new output\nformats that might want to print the full reference name.\n\nRight now, all callsites except one are prettifying the reference name\nanyway. And the only callsite that doesn't passes `FETCH_HEAD` as the\nhardcoded reference name to `format_display()`, which would never be\nchanged by a call to `prettify_refname()` anyway. So let's refactor the\ncode to pass in the full local reference name and then prettify it in\nthe formatting code.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 23 +++++++++++------------\n 1 file changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6d6146b0f0..81ba3900cb 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -887,9 +887,9 @@ static void format_display(struct display_state *display_state,\n \n \tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n \tif (!display_state->compact_format)\n-\t\tprint_remote_to_local(display_state, display_buffer, remote, local);\n+\t\tprint_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));\n \telse\n-\t\tprint_compact(display_state, display_buffer, remote, local);\n+\t\tprint_compact(display_state, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n }\n@@ -901,7 +901,6 @@ static int update_local_ref(struct ref *ref,\n \t\t\t struct strbuf *display, int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n \tif (!repo_has_object_file(the_repository, &ref->new_oid))\n@@ -910,7 +909,7 @@ static int update_local_ref(struct ref *ref,\n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n \t\t\tformat_display(display_state, display, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 0;\n \t}\n \n@@ -923,7 +922,7 @@ static int update_local_ref(struct ref *ref,\n \t\t */\n \t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t _(\"can't fetch into checked-out branch\"),\n-\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n \n@@ -934,12 +933,12 @@ static int update_local_ref(struct ref *ref,\n \t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\t\treturn r;\n \t\t} else {\n \t\t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n \t\t\t\t _(\"would clobber existing tag\"),\n-\t\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t\t remote, ref->name, summary_width);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -972,7 +971,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\treturn r;\n \t}\n \n@@ -994,7 +993,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -1006,12 +1005,12 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display_state, 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\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n \t\tformat_display(display_state, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t remote, pretty_ref, summary_width);\n+\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n }\n@@ -1431,7 +1430,7 @@ static int prune_refs(struct display_state *display_state,\n \t\t\t\tshown_url = 1;\n \t\t\t}\n \t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t _(\"(none)\"), prettify_refname(ref->name),\n+\t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n \t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n \t\t\tstrbuf_release(&sb);\n-- \n2.40.0\n\n"},{"id":"473768","messageId":"e4c1ed4ad5c36839babd8910180f070b62a59ab6.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 4/6] fetch: centralize handling of per-reference format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:32Z","receivedAt":"2023-03-20T12:35:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `format_display()` is used to print a single reference\nupdate to a buffer which will then ultimately be printed by the caller.\nThis architecture causes us to duplicate some logic across the different\ncallsites of this function. This makes it hard to follow the code as\nsome parts of the logic are located in one place, while other parts of\nthe logic are located in a different place. Furthermore, by having the\nlogic scattered around it becomes quite hard to implement a new output\nformat for the reference updates.\n\nWe can make the logic a whole lot easier to understand by making the\n`format_display()` function self-contained so that it handles formatting\nand printing of the references. This will eventually allow us to easily\nimplement a completely different output format, but also opens the door\nto conditionally print to either stdout or stderr depending on the\noutput format.\n\nAs a first step towards that goal we move the formatting directive used\nby both callers to print a single reference update into this function.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 81ba3900cb..a66428dfd8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -885,13 +885,14 @@ static void format_display(struct display_state *display_state,\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display_buffer, \"%c %-*s \", code, width, summary);\n+\tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n \tif (!display_state->compact_format)\n \t\tprint_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));\n \telse\n \t\tprint_compact(display_state, display_buffer, remote, prettify_refname(local));\n \tif (error)\n \t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n+\tstrbuf_addch(display_buffer, '\\n');\n }\n \n static int update_local_ref(struct ref *ref,\n@@ -1271,7 +1272,7 @@ static int store_updated_refs(struct display_state *display_state,\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\tfprintf(stderr, \" %s\\n\", note.buf);\n+\t\t\t\tfputs(note.buf, stderr);\n \t\t\t}\n \t\t}\n \t}\n@@ -1432,7 +1433,7 @@ static int prune_refs(struct display_state *display_state,\n \t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n-\t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n+\t\t\tfputs(sb.buf, stderr);\n \t\t\tstrbuf_release(&sb);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n \t\t}\n-- \n2.40.0\n\n"},{"id":"473769","messageId":"98b799af713b2ff22cc79531d5057fda64ddfb2a.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 5/6] fetch: centralize logic to print remote URL","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:36Z","receivedAt":"2023-03-20T12:35:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching from a remote, we not only print the actual references\nthat have changed, but will also print the URL from which we have\nfetched them to standard output. The logic to handle this is duplicated\nacross two different callsites with some non-trivial logic to compute\nthe anonymized URL. Furthermore, we're using global state to track\nwhether we have already shown the URL to the user or not.\n\nRefactor the code by moving it into `format_display()`. Like this, we\ncan convert the global variable into a member of `display_state`. And\nsecond, we can deduplicate the logic to compute the anonymized URL.\n\nThis also works as expected when fetching from multiple remotes, for\nexample via a group of remotes, as we do this by forking a standalone\ngit-fetch(1) process per remote that is to be fetched.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 99 ++++++++++++++++++++++---------------------------\n 1 file changed, 44 insertions(+), 55 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a66428dfd8..1e3599cb74 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -50,6 +50,9 @@ enum {\n struct display_state {\n \tint refcol_width;\n \tint compact_format;\n+\n+\tchar *url;\n+\tint url_len, shown_url;\n };\n \n static int fetch_prune_config = -1; /* unspecified */\n@@ -84,7 +87,6 @@ static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n-static int shown_url = 0;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n@@ -776,13 +778,27 @@ static int refcol_width(const struct ref *ref, int compact_format)\n \treturn rlen;\n }\n \n-static void display_state_init(struct display_state *display_state, struct ref *ref_map)\n+static void display_state_init(struct display_state *display_state, struct ref *ref_map,\n+\t\t\t const char *raw_url)\n {\n \tstruct ref *rm;\n \tconst char *format = \"full\";\n+\tint i;\n \n \tmemset(display_state, 0, sizeof(*display_state));\n \n+\tif (raw_url)\n+\t\tdisplay_state->url = transport_anonymize_url(raw_url);\n+\telse\n+\t\tdisplay_state->url = xstrdup(\"foreign\");\n+\n+\tdisplay_state->url_len = strlen(display_state->url);\n+\tfor (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)\n+\t\t;\n+\tdisplay_state->url_len = i + 1;\n+\tif (4 < i && !strncmp(\".git\", display_state->url + i - 3, 4))\n+\t\tdisplay_state->url_len = i - 3;\n+\n \tif (verbosity < 0)\n \t\treturn;\n \n@@ -816,6 +832,11 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \t}\n }\n \n+static void display_state_release(struct display_state *display_state)\n+{\n+\tfree(display_state->url);\n+}\n+\n static void print_remote_to_local(struct display_state *display_state,\n \t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n@@ -883,6 +904,12 @@ static void format_display(struct display_state *display_state,\n \tif (verbosity < 0)\n \t\treturn;\n \n+\tif (!display_state->shown_url) {\n+\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"),\n+\t\t\t display_state->url_len, display_state->url);\n+\t\tdisplay_state->shown_url = 1;\n+\t}\n+\n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n \tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n@@ -1122,33 +1149,28 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\n \n static int store_updated_refs(struct display_state *display_state,\n-\t\t\t const char *raw_url, const char *remote_name,\n+\t\t\t const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head)\n {\n-\tint url_len, i, rc = 0;\n+\tint rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n-\tchar *url;\n \tint want_status;\n \tint summary_width = 0;\n \n \tif (verbosity >= 0)\n \t\tsummary_width = transport_summary_width(ref_map);\n \n-\tif (raw_url)\n-\t\turl = transport_anonymize_url(raw_url);\n-\telse\n-\t\turl = xstrdup(\"foreign\");\n-\n \tif (!connectivity_checked) {\n \t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n-\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"),\n+\t\t\t\t display_state->url);\n \t\t\tgoto abort;\n \t\t}\n \t}\n@@ -1232,13 +1254,6 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t\t\twhat = rm->name;\n \t\t\t}\n \n-\t\t\turl_len = strlen(url);\n-\t\t\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t\t\t;\n-\t\t\turl_len = i + 1;\n-\t\t\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\t\t\turl_len = i - 3;\n-\n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (*what) {\n \t\t\t\tif (*kind)\n@@ -1248,7 +1263,8 @@ static int store_updated_refs(struct display_state *display_state,\n \n \t\t\tappend_fetch_head(fetch_head, &rm->old_oid,\n \t\t\t\t\t rm->fetch_head_status,\n-\t\t\t\t\t note.buf, url, url_len);\n+\t\t\t\t\t note.buf, display_state->url,\n+\t\t\t\t\t display_state->url_len);\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n@@ -1266,14 +1282,8 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t\t\t\t *what ? what : \"HEAD\",\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 (!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\tif (note.len)\n \t\t\t\tfputs(note.buf, stderr);\n-\t\t\t}\n \t\t}\n \t}\n \n@@ -1293,7 +1303,6 @@ static int store_updated_refs(struct display_state *display_state,\n \n abort:\n \tstrbuf_release(¬e);\n-\tfree(url);\n \treturn rc;\n }\n \n@@ -1365,7 +1374,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(display_state, transport->url, transport->remote->name,\n+\tret = store_updated_refs(display_state, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n \t\t\t\t fetch_head);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n@@ -1378,30 +1387,15 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n static int prune_refs(struct display_state *display_state,\n \t\t struct refspec *rs,\n \t\t struct ref_transaction *transaction,\n-\t\t struct ref *ref_map,\n-\t\t const char *raw_url)\n+\t\t struct ref *ref_map)\n {\n-\tint url_len, i, result = 0;\n+\tint result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);\n \tstruct strbuf err = STRBUF_INIT;\n-\tchar *url;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" (%s will become dangling)\")\n \t\t: _(\" (%s has become dangling)\");\n \n-\tif (raw_url)\n-\t\turl = transport_anonymize_url(raw_url);\n-\telse\n-\t\turl = xstrdup(\"foreign\");\n-\n-\turl_len = strlen(url);\n-\tfor (i = url_len - 1; url[i] == '/' && 0 <= i; i--)\n-\t\t;\n-\n-\turl_len = i + 1;\n-\tif (4 < i && !strncmp(\".git\", url + i - 3, 4))\n-\t\turl_len = i - 3;\n-\n \tif (!dry_run) {\n \t\tif (transaction) {\n \t\t\tfor (ref = stale_refs; ref; ref = ref->next) {\n@@ -1426,10 +1420,6 @@ static int prune_refs(struct display_state *display_state,\n \n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tif (!shown_url) {\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(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n \t\t\t\t _(\"(none)\"), ref->name,\n \t\t\t\t summary_width);\n@@ -1441,7 +1431,6 @@ static int prune_refs(struct display_state *display_state,\n \n cleanup:\n \tstrbuf_release(&err);\n-\tfree(url);\n \tfree_refs(stale_refs);\n \treturn result;\n }\n@@ -1596,7 +1585,7 @@ static int do_fetch(struct transport *transport,\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n-\tstruct display_state display_state;\n+\tstruct display_state display_state = { 0 };\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n@@ -1678,7 +1667,7 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tdisplay_state_init(&display_state, ref_map);\n+\tdisplay_state_init(&display_state, ref_map, transport->url);\n \n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n@@ -1697,11 +1686,10 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(&display_state, rs, transaction, ref_map);\n \t\t} else {\n \t\t\tretcode = prune_refs(&display_state, &transport->remote->fetch,\n-\t\t\t\t\t transaction, ref_map,\n-\t\t\t\t\t transport->url);\n+\t\t\t\t\t transaction, ref_map);\n \t\t}\n \t\tif (retcode != 0)\n \t\t\tretcode = 1;\n@@ -1812,6 +1800,7 @@ static int do_fetch(struct transport *transport,\n \t\terror(\"%s\", err.buf);\n \t}\n \n+\tdisplay_state_release(&display_state);\n \tclose_fetch_head(&fetch_head);\n \tstrbuf_release(&err);\n \tfree_refs(ref_map);\n-- \n2.40.0\n\n"},{"id":"473770","messageId":"fe7e2e85eb37cd4068b5160721663c21a16a8138.1679315383.git.ps@pks.im","threadId":"59399","inReplyTo":"cover.1679315383.git.ps@pks.im","subject":"[PATCH v2 6/6] fetch: centralize printing of reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-20T12:35:40Z","receivedAt":"2023-03-20T12:36:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to print updated references during a fetch, the two different\ncall sites that do this will first call `format_display()` followed by a\ncall to `fputs()`. This is needlessly roundabout now that we have the\n`display_state` structure that encapsulates all of the printing logic\nfor references.\n\nMove displaying the reference updates into `format_display()` and rename\nit to `display_ref_update()` to better match its new purpose, which\nfinalizes the conversion to make both the formatting and printing logic\nof reference updates self-contained. This will make it easier to add new\noutput formats and printing to a different file descriptor than stderr.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 108 ++++++++++++++++++++++++------------------------\n 1 file changed, 55 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 1e3599cb74..c202c18fb4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -48,6 +48,8 @@ enum {\n };\n \n struct display_state {\n+\tstruct strbuf buf;\n+\n \tint refcol_width;\n \tint compact_format;\n \n@@ -787,6 +789,8 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \n \tmemset(display_state, 0, sizeof(*display_state));\n \n+\tstrbuf_init(&display_state->buf, 0);\n+\n \tif (raw_url)\n \t\tdisplay_state->url = transport_anonymize_url(raw_url);\n \telse\n@@ -834,14 +838,15 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \n static void display_state_release(struct display_state *display_state)\n {\n+\tstrbuf_release(&display_state->buf);\n \tfree(display_state->url);\n }\n \n static void print_remote_to_local(struct display_state *display_state,\n-\t\t\t\t struct strbuf *display_buffer,\n \t\t\t\t const char *remote, const char *local)\n {\n-\tstrbuf_addf(display_buffer, \"%-*s -> %s\", display_state->refcol_width, remote, local);\n+\tstrbuf_addf(&display_state->buf, \"%-*s -> %s\",\n+\t\t display_state->refcol_width, remote, local);\n }\n \n static int find_and_replace(struct strbuf *haystack,\n@@ -871,14 +876,14 @@ static int find_and_replace(struct strbuf *haystack,\n \treturn 1;\n }\n \n-static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,\n+static void print_compact(struct display_state *display_state,\n \t\t\t const char *remote, const char *local)\n {\n \tstruct strbuf r = STRBUF_INIT;\n \tstruct strbuf l = STRBUF_INIT;\n \n \tif (!strcmp(remote, local)) {\n-\t\tstrbuf_addf(display_buffer, \"%-*s -> *\", display_state->refcol_width, remote);\n+\t\tstrbuf_addf(&display_state->buf, \"%-*s -> *\", display_state->refcol_width, remote);\n \t\treturn;\n \t}\n \n@@ -887,46 +892,49 @@ static void print_compact(struct display_state *display_state, struct strbuf *di\n \n \tif (!find_and_replace(&r, local, \"*\"))\n \t\tfind_and_replace(&l, remote, \"*\");\n-\tprint_remote_to_local(display_state, display_buffer, r.buf, l.buf);\n+\tprint_remote_to_local(display_state, r.buf, l.buf);\n \n \tstrbuf_release(&r);\n \tstrbuf_release(&l);\n }\n \n-static void format_display(struct display_state *display_state,\n-\t\t\t struct strbuf *display_buffer, 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+static void display_ref_update(struct display_state *display_state, 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 {\n \tint width;\n \n \tif (verbosity < 0)\n \t\treturn;\n \n+\tstrbuf_reset(&display_state->buf);\n+\n \tif (!display_state->shown_url) {\n-\t\tstrbuf_addf(display_buffer, _(\"From %.*s\\n\"),\n+\t\tstrbuf_addf(&display_state->buf, _(\"From %.*s\\n\"),\n \t\t\t display_state->url_len, display_state->url);\n \t\tdisplay_state->shown_url = 1;\n \t}\n \n \twidth = (summary_width + strlen(summary) - gettext_width(summary));\n \n-\tstrbuf_addf(display_buffer, \" %c %-*s \", code, width, summary);\n+\tstrbuf_addf(&display_state->buf, \" %c %-*s \", code, width, summary);\n \tif (!display_state->compact_format)\n-\t\tprint_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));\n+\t\tprint_remote_to_local(display_state, remote, prettify_refname(local));\n \telse\n-\t\tprint_compact(display_state, display_buffer, remote, prettify_refname(local));\n+\t\tprint_compact(display_state, remote, prettify_refname(local));\n \tif (error)\n-\t\tstrbuf_addf(display_buffer, \" (%s)\", error);\n-\tstrbuf_addch(display_buffer, '\\n');\n+\t\tstrbuf_addf(&display_state->buf, \" (%s)\", error);\n+\tstrbuf_addch(&display_state->buf, '\\n');\n+\n+\tfputs(display_state->buf.buf, stderr);\n }\n \n static int update_local_ref(struct ref *ref,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct display_state *display_state,\n \t\t\t const char *remote, const struct ref *remote_ref,\n-\t\t\t struct strbuf *display, int summary_width)\n+\t\t\t int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n \tint fast_forward = 0;\n@@ -936,8 +944,8 @@ 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_state, display, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\tdisplay_ref_update(display_state, '=', _(\"[up to date]\"), NULL,\n+\t\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 0;\n \t}\n \n@@ -948,9 +956,9 @@ 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_state, display, '!', _(\"[rejected]\"),\n-\t\t\t _(\"can't fetch into checked-out branch\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\tdisplay_ref_update(display_state, '!', _(\"[rejected]\"),\n+\t\t\t\t _(\"can't fetch into checked-out branch\"),\n+\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n \n@@ -959,14 +967,14 @@ 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_state, display, r ? '!' : 't', _(\"[tag update]\"),\n-\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\tdisplay_ref_update(display_state, r ? '!' : 't', _(\"[tag update]\"),\n+\t\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t\t remote, ref->name, summary_width);\n \t\t\treturn r;\n \t\t} else {\n-\t\t\tformat_display(display_state, display, '!', _(\"[rejected]\"),\n-\t\t\t\t _(\"would clobber existing tag\"),\n-\t\t\t\t remote, ref->name, summary_width);\n+\t\t\tdisplay_ref_update(display_state, '!', _(\"[rejected]\"),\n+\t\t\t\t\t _(\"would clobber existing tag\"),\n+\t\t\t\t\t remote, ref->name, summary_width);\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -997,9 +1005,9 @@ 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_state, display, r ? '!' : '*', what,\n-\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name, summary_width);\n+\t\tdisplay_ref_update(display_state, r ? '!' : '*', what,\n+\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t remote, ref->name, summary_width);\n \t\treturn r;\n \t}\n \n@@ -1019,9 +1027,9 @@ 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_state, display, r ? '!' : ' ', quickref.buf,\n-\t\t\t r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t remote, ref->name, summary_width);\n+\t\tdisplay_ref_update(display_state, r ? '!' : ' ', quickref.buf,\n+\t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n+\t\t\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -1031,14 +1039,14 @@ 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_state, display, r ? '!' : '+', quickref.buf,\n-\t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\tdisplay_ref_update(display_state, r ? '!' : '+', quickref.buf,\n+\t\t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n+\t\t\t\t remote, ref->name, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n-\t\tformat_display(display_state, display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t remote, ref->name, summary_width);\n+\t\tdisplay_ref_update(display_state, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n+\t\t\t\t remote, ref->name, summary_width);\n \t\treturn 1;\n \t}\n }\n@@ -1266,10 +1274,9 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t\t\t\t note.buf, display_state->url,\n \t\t\t\t\t display_state->url_len);\n \n-\t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n \t\t\t\trc |= update_local_ref(ref, transaction, display_state, what,\n-\t\t\t\t\t\t rm, ¬e, summary_width);\n+\t\t\t\t\t\t rm, 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@@ -1277,13 +1284,11 @@ static int store_updated_refs(struct display_state *display_state,\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(display_state, ¬e, '*',\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+\t\t\t\tdisplay_ref_update(display_state, '*',\n+\t\t\t\t\t\t *kind ? kind : \"branch\", NULL,\n+\t\t\t\t\t\t *what ? what : \"HEAD\",\n+\t\t\t\t\t\t \"FETCH_HEAD\", summary_width);\n \t\t\t}\n-\t\t\tif (note.len)\n-\t\t\t\tfputs(note.buf, stderr);\n \t\t}\n \t}\n \n@@ -1419,12 +1424,9 @@ static int prune_refs(struct display_state *display_state,\n \t\tint summary_width = transport_summary_width(stale_refs);\n \n \t\tfor (ref = stale_refs; ref; ref = ref->next) {\n-\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tformat_display(display_state, &sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t _(\"(none)\"), ref->name,\n-\t\t\t\t summary_width);\n-\t\t\tfputs(sb.buf, stderr);\n-\t\t\tstrbuf_release(&sb);\n+\t\t\tdisplay_ref_update(display_state, '-', _(\"[deleted]\"), NULL,\n+\t\t\t\t\t _(\"(none)\"), ref->name,\n+\t\t\t\t\t summary_width);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n \t\t}\n \t}\n-- \n2.40.0\n\n"},{"id":"473817","messageId":"20230320225702.1471172-1-jonathantanmy@google.com","threadId":"59399","inReplyTo":"fe7e2e85eb37cd4068b5160721663c21a16a8138.1679315383.git.ps@pks.im","subject":"Re: [PATCH v2 6/6] fetch: centralize printing of reference updates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-20T22:57:02Z","receivedAt":"2023-03-20T22:57:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patrick Steinhardt writes:\n> In order to print updated references during a fetch, the two different\n> call sites that do this will first call `format_display()` followed by a\n> call to `fputs()`. This is needlessly roundabout now that we have the\n> `display_state` structure that encapsulates all of the printing logic\n> for references.\n> \n> Move displaying the reference updates into `format_display()` and rename\n> it to `display_ref_update()` to better match its new purpose, which\n> finalizes the conversion to make both the formatting and printing logic\n> of reference updates self-contained. This will make it easier to add new\n> output formats and printing to a different file descriptor than stderr.\n\nThanks. Patches 1-5 look good to me. As for this patch, I'm still not\nconvinced (I thought that the new mode would still print to stderr), but\nif other reviewers are OK with it then that's fine. Alternatively, patch\n6 could go with the next set of patches that implement the new mode of\nprinting ref updates.\n \n"},{"id":"473898","messageId":"ZBrEiKnbaq5N9DOl@ncase","threadId":"59399","inReplyTo":"20230320225702.1471172-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 6/6] fetch: centralize printing of reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-03-22T09:04:08Z","receivedAt":"2023-03-22T09:04:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 20, 2023 at 03:57:02PM -0700, Jonathan Tan wrote:\n> Patrick Steinhardt writes:\n> > In order to print updated references during a fetch, the two different\n> > call sites that do this will first call `format_display()` followed by a\n> > call to `fputs()`. This is needlessly roundabout now that we have the\n> > `display_state` structure that encapsulates all of the printing logic\n> > for references.\n> > \n> > Move displaying the reference updates into `format_display()` and rename\n> > it to `display_ref_update()` to better match its new purpose, which\n> > finalizes the conversion to make both the formatting and printing logic\n> > of reference updates self-contained. This will make it easier to add new\n> > output formats and printing to a different file descriptor than stderr.\n> \n> Thanks. Patches 1-5 look good to me. As for this patch, I'm still not\n> convinced (I thought that the new mode would still print to stderr),\n\nThe reason why I decided against printing to stderr is that it's already\nused by other things. The progress meter is one of these, runtime errors\nare another one. I also think it's weird to have a parseable format that\nshould be parsed via stderr.\n\nAnyway, let's discuss this once I'm posting the second patch series to\nthe mailing list.\n\n> but\n> if other reviewers are OK with it then that's fine. Alternatively, patch\n> 6 could go with the next set of patches that implement the new mode of\n> printing ref updates.\n\nI'd be fine either way. Thanks for your review!\n\nPatrick\n"},{"id":"474417","messageId":"xmqqlejf9z2c.fsf@gitster.g","threadId":"59399","inReplyTo":"20230320225702.1471172-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 6/6] fetch: centralize printing of reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T18:45:15Z","receivedAt":"2023-03-29T18:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks. Patches 1-5 look good to me. As for this patch, I'm still not\n> convinced (I thought that the new mode would still print to stderr), but\n> if other reviewers are OK with it then that's fine. Alternatively, patch\n> 6 could go with the next set of patches that implement the new mode of\n> printing ref updates.\n\nWhen further changes to the display_ref_update() starts doing\nsomething iffy, we may still want to block them at that time, but\nunless I am grossly misreading this patch, I do not think this one\nchanges the behaviour to start showing the ref update notification\nto anywhere other than the standard error stream at this stage, and\nthe changes in this step can be evaluated for its \"clean-up\" value\nalone. \n\nSo I am inclined to say that we should take it as-is.\n\nThanks.\n\n"}]}