[PATCH 0/8] fetch: refactor code that prints reference updates

[PATCH 0/8] fetch: refactor code that prints reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]

Hi,

at GitLab, we want to gain more control over fetches to achieve two
different things:

    1. We want to take control of the reference updates so that we can
       atomically update all or a subset of references that git-fetch
       would have updated.

    2. We want to be able to quarantine objects in a fetch so that we
       can e.g. perform consistency checks for them before they land in
       the main repository.

To do this, we aim to use git-fetch(1)'s `--dry-run` mode with a
manually set up quarantine directory. One issue we currently face though
is that git-fetch(1), to the best of my knowledge, has no mode in which
it would print all reference updates in a machine-parseable format.

I thus set out to implement a "porcelain"-style mode for git-fetch(1)
that surfaces this information:

    - The reference that would be updated.

    - The remote reference this is coming from.

    - The old and new object IDs of the reference.

    - Whether there's any error, like a D/F conflict.

I had a hard time understanding the current implementation of how ref
updates are printed though. So as a first step towards such a porcelain
mode this patch series refactors said code. It sets out to achieve two
major goals:

    - There should be as few global state as possible. This is to reduce
      confusion and having to repeat the same incantations in multiple
      different locations.

    - The logic should be as self-contained as possible. This is so that
      it can easily be changed in a subsequent patch series.

This patch series does exactly that, but does not yet introduce the new
machine-parsebale porcelain mode.

Patrick

Patrick Steinhardt (8):
  fetch: rename `display` buffer to avoid name conflict
  fetch: move reference width calculation into `display_state`
  fetch: move output format into `display_state`
  fetch: pass the full local reference name to `format_display`
  fetch: deduplicate handling of per-reference format
  fetch: deduplicate logic to print remote URL
  fetch: fix inconsistent summary width for pruned and updated refs
  fetch: centralize printing of reference updates

 builtin/fetch.c | 270 ++++++++++++++++++++++++------------------------
 1 file changed, 134 insertions(+), 136 deletions(-)

-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/8] fetch: rename `display` buffer to avoid name conflict

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6122 bytes --]

Rename the `display` buffer parameter we use to print reference updates
to standard error to `display_buffer`. This is done in order to avoid a
name conflict with the new `display_state` structure we're about to
introduce.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a09606b472..087e5cd6f4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -807,10 +807,10 @@ static void prepare_format_display(struct ref *ref_map)
 	}
 }
 
-static void print_remote_to_local(struct strbuf *display,
+static void print_remote_to_local(struct strbuf *display_buffer,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);
+	strbuf_addf(display_buffer, "%-*s -> %s", refcol_width, remote, local);
 }
 
 static int find_and_replace(struct strbuf *haystack,
@@ -840,14 +840,14 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct strbuf *display,
+static void print_compact(struct strbuf *display_buffer,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;
 	struct strbuf l = STRBUF_INIT;
 
 	if (!strcmp(remote, local)) {
-		strbuf_addf(display, "%-*s -> *", refcol_width, remote);
+		strbuf_addf(display_buffer, "%-*s -> *", refcol_width, remote);
 		return;
 	}
 
@@ -856,13 +856,13 @@ static void print_compact(struct strbuf *display,
 
 	if (!find_and_replace(&r, local, "*"))
 		find_and_replace(&l, remote, "*");
-	print_remote_to_local(display, r.buf, l.buf);
+	print_remote_to_local(display_buffer, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct strbuf *display, char code,
+static void format_display(struct strbuf *display_buffer, char code,
 			   const char *summary, const char *error,
 			   const char *remote, const char *local,
 			   int summary_width)
@@ -874,19 +874,19 @@ static void format_display(struct strbuf *display, char code,
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display, "%c %-*s ", code, width, summary);
+	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
 	if (!compact_format)
-		print_remote_to_local(display, remote, local);
+		print_remote_to_local(display_buffer, remote, local);
 	else
-		print_compact(display, remote, local);
+		print_compact(display_buffer, remote, local);
 	if (error)
-		strbuf_addf(display, "  (%s)", error);
+		strbuf_addf(display_buffer, "  (%s)", error);
 }
 
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    const char *remote, const struct ref *remote_ref,
-			    struct strbuf *display, int summary_width)
+			    struct strbuf *display_buffer, int summary_width)
 {
 	struct commit *current = NULL, *updated;
 	const char *pretty_ref = prettify_refname(ref->name);
@@ -897,7 +897,7 @@ static int update_local_ref(struct ref *ref,
 
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
-			format_display(display, '=', _("[up to date]"), NULL,
+			format_display(display_buffer, '=', _("[up to date]"), NULL,
 				       remote, pretty_ref, summary_width);
 		return 0;
 	}
@@ -909,7 +909,7 @@ static int update_local_ref(struct ref *ref,
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
-		format_display(display, '!', _("[rejected]"),
+		format_display(display_buffer, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
 			       remote, pretty_ref, summary_width);
 		return 1;
@@ -920,12 +920,12 @@ static int update_local_ref(struct ref *ref,
 		if (force || ref->force) {
 			int r;
 			r = s_update_ref("updating tag", ref, transaction, 0);
-			format_display(display, r ? '!' : 't', _("[tag update]"),
+			format_display(display_buffer, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
 				       remote, pretty_ref, summary_width);
 			return r;
 		} else {
-			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+			format_display(display_buffer, '!', _("[rejected]"), _("would clobber existing tag"),
 				       remote, pretty_ref, summary_width);
 			return 1;
 		}
@@ -957,7 +957,7 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display, r ? '!' : '*', what,
+		format_display(display_buffer, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		return r;
@@ -979,7 +979,7 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "..");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("fast-forward", ref, transaction, 1);
-		format_display(display, r ? '!' : ' ', quickref.buf,
+		format_display(display_buffer, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
@@ -991,13 +991,13 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "...");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("forced-update", ref, transaction, 1);
-		format_display(display, r ? '!' : '+', quickref.buf,
+		format_display(display_buffer, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
-		format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
+		format_display(display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/8] fetch: move reference width calculation into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 14327 bytes --]

In order to print references in proper columns we need to calculate the
width of the reference column before starting to print the references.
This is done with the help of a global variable `refcol_width`.

Refactor the code to instead use a new structure `display_state` that
contains the computed width and plumb it through the stack as required.
This is only the first step towards de-globalizing the state required to
print references.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 107 ++++++++++++++++++++++++++++--------------------
 1 file changed, 63 insertions(+), 44 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 087e5cd6f4..3695299177 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -47,6 +47,10 @@ enum {
 	TAGS_SET = 2
 };
 
+struct display_state {
+	int refcol_width;
+};
+
 static int fetch_prune_config = -1; /* unspecified */
 static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
@@ -741,16 +745,15 @@ static int s_update_ref(const char *action,
 	return ret;
 }
 
-static int refcol_width = 10;
 static int compact_format;
 
-static void adjust_refcol_width(const struct ref *ref)
+static int refcol_width(const struct ref *ref)
 {
 	int max, rlen, llen, len;
 
 	/* uptodate lines are only shown on high verbosity level */
 	if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
-		return;
+		return 0;
 
 	max    = term_columns();
 	rlen   = utf8_strwidth(prettify_refname(ref->name));
@@ -769,22 +772,18 @@ static void adjust_refcol_width(const struct ref *ref)
 	}
 	len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
 	if (len >= max)
-		return;
+		return 0;
 
-	/*
-	 * Not precise calculation for compact mode because '*' can
-	 * appear on the left hand side of '->' and shrink the column
-	 * back.
-	 */
-	if (refcol_width < rlen)
-		refcol_width = rlen;
+	return rlen;
 }
 
-static void prepare_format_display(struct ref *ref_map)
+static void display_state_init(struct display_state *display, struct ref *ref_map)
 {
 	struct ref *rm;
 	const char *format = "full";
 
+	memset(display, 0, sizeof(*display));
+
 	if (verbosity < 0)
 		return;
 
@@ -797,20 +796,32 @@ static void prepare_format_display(struct ref *ref_map)
 		die(_("invalid value for '%s': '%s'"),
 		    "fetch.output", format);
 
+	display->refcol_width = 10;
 	for (rm = ref_map; rm; rm = rm->next) {
+		int width;
+
 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
 		    !rm->peer_ref ||
 		    !strcmp(rm->name, "HEAD"))
 			continue;
 
-		adjust_refcol_width(rm);
+		width = refcol_width(rm);
+
+		/*
+		 * Not precise calculation for compact mode because '*' can
+		 * appear on the left hand side of '->' and shrink the column
+		 * back.
+		 */
+		if (display->refcol_width < width)
+			display->refcol_width = width;
 	}
 }
 
-static void print_remote_to_local(struct strbuf *display_buffer,
+static void print_remote_to_local(struct display_state *display,
+				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display_buffer, "%-*s -> %s", refcol_width, remote, local);
+	strbuf_addf(display_buffer, "%-*s -> %s", display->refcol_width, remote, local);
 }
 
 static int find_and_replace(struct strbuf *haystack,
@@ -840,14 +851,14 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct strbuf *display_buffer,
+static void print_compact(struct display_state *display, struct strbuf *display_buffer,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;
 	struct strbuf l = STRBUF_INIT;
 
 	if (!strcmp(remote, local)) {
-		strbuf_addf(display_buffer, "%-*s -> *", refcol_width, remote);
+		strbuf_addf(display_buffer, "%-*s -> *", display->refcol_width, remote);
 		return;
 	}
 
@@ -856,13 +867,14 @@ static void print_compact(struct strbuf *display_buffer,
 
 	if (!find_and_replace(&r, local, "*"))
 		find_and_replace(&l, remote, "*");
-	print_remote_to_local(display_buffer, r.buf, l.buf);
+	print_remote_to_local(display, display_buffer, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct strbuf *display_buffer, char code,
+static void format_display(struct display_state *display,
+			   struct strbuf *display_buffer, char code,
 			   const char *summary, const char *error,
 			   const char *remote, const char *local,
 			   int summary_width)
@@ -876,15 +888,16 @@ static void format_display(struct strbuf *display_buffer, char code,
 
 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
 	if (!compact_format)
-		print_remote_to_local(display_buffer, remote, local);
+		print_remote_to_local(display, display_buffer, remote, local);
 	else
-		print_compact(display_buffer, remote, local);
+		print_compact(display, display_buffer, remote, local);
 	if (error)
 		strbuf_addf(display_buffer, "  (%s)", error);
 }
 
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
+			    struct display_state *display,
 			    const char *remote, const struct ref *remote_ref,
 			    struct strbuf *display_buffer, int summary_width)
 {
@@ -897,7 +910,7 @@ static int update_local_ref(struct ref *ref,
 
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
-			format_display(display_buffer, '=', _("[up to date]"), NULL,
+			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
 				       remote, pretty_ref, summary_width);
 		return 0;
 	}
@@ -909,7 +922,7 @@ static int update_local_ref(struct ref *ref,
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
-		format_display(display_buffer, '!', _("[rejected]"),
+		format_display(display, display_buffer, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
 			       remote, pretty_ref, summary_width);
 		return 1;
@@ -920,12 +933,13 @@ static int update_local_ref(struct ref *ref,
 		if (force || ref->force) {
 			int r;
 			r = s_update_ref("updating tag", ref, transaction, 0);
-			format_display(display_buffer, r ? '!' : 't', _("[tag update]"),
+			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
 				       remote, pretty_ref, summary_width);
 			return r;
 		} else {
-			format_display(display_buffer, '!', _("[rejected]"), _("would clobber existing tag"),
+			format_display(display, display_buffer, '!', _("[rejected]"),
+				       _("would clobber existing tag"),
 				       remote, pretty_ref, summary_width);
 			return 1;
 		}
@@ -957,7 +971,7 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display_buffer, r ? '!' : '*', what,
+		format_display(display, display_buffer, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		return r;
@@ -979,7 +993,7 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "..");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("fast-forward", ref, transaction, 1);
-		format_display(display_buffer, r ? '!' : ' ', quickref.buf,
+		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
@@ -991,13 +1005,13 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "...");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("forced-update", ref, transaction, 1);
-		format_display(display_buffer, r ? '!' : '+', quickref.buf,
+		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
-		format_display(display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
+		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1108,7 +1122,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n"
    "to avoid this check\n");
 
-static int store_updated_refs(const char *raw_url, const char *remote_name,
+static int store_updated_refs(struct display_state *display,
+			      const char *raw_url, const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head)
@@ -1139,8 +1154,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	prepare_format_display(ref_map);
-
 	/*
 	 * We do a pass for each fetch_head_status type in their enum order, so
 	 * merged entries are written before not-for-merge. That lets readers
@@ -1240,7 +1253,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 			strbuf_reset(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, transaction, what,
+				rc |= update_local_ref(ref, transaction, display, what,
 						       rm, &note, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
@@ -1249,7 +1262,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				 * would be written to FETCH_HEAD, if --dry-run
 				 * is set).
 				 */
-				format_display(&note, '*',
+				format_display(display, &note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
@@ -1328,7 +1341,8 @@ static int check_exist_and_connected(struct ref *ref_map)
 	return check_connected(iterate_ref_map, &rm, &opt);
 }
 
-static int fetch_and_consume_refs(struct transport *transport,
+static int fetch_and_consume_refs(struct display_state *display,
+				  struct transport *transport,
 				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
 				  struct fetch_head *fetch_head)
@@ -1352,7 +1366,7 @@ static int fetch_and_consume_refs(struct transport *transport,
 	}
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
-	ret = store_updated_refs(transport->url, transport->remote->name,
+	ret = store_updated_refs(display, transport->url, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
 				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
@@ -1362,7 +1376,8 @@ static int fetch_and_consume_refs(struct transport *transport,
 	return ret;
 }
 
-static int prune_refs(struct refspec *rs,
+static int prune_refs(struct display_state *display,
+		      struct refspec *rs,
 		      struct ref_transaction *transaction,
 		      struct ref *ref_map,
 		      const char *raw_url)
@@ -1416,7 +1431,7 @@ static int prune_refs(struct refspec *rs,
 				fprintf(stderr, _("From %.*s\n"), url_len, url);
 				shown_url = 1;
 			}
-			format_display(&sb, '-', _("[deleted]"), NULL,
+			format_display(display, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), prettify_refname(ref->name),
 				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);
@@ -1542,7 +1557,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	return transport;
 }
 
-static int backfill_tags(struct transport *transport,
+static int backfill_tags(struct display_state *display,
+			 struct transport *transport,
 			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
 			 struct fetch_head *fetch_head)
@@ -1566,7 +1582,7 @@ static int backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);
+	retcode = fetch_and_consume_refs(display, transport, transaction, ref_map, fetch_head);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1581,6 +1597,7 @@ static int do_fetch(struct transport *transport,
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
+	struct display_state display;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1662,6 +1679,8 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
+	display_state_init(&display, ref_map);
+
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
 		if (!transaction) {
@@ -1679,9 +1698,9 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(rs, transaction, ref_map, transport->url);
+			retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
 		} else {
-			retcode = prune_refs(&transport->remote->fetch,
+			retcode = prune_refs(&display, &transport->remote->fetch,
 					     transaction, ref_map,
 					     transport->url);
 		}
@@ -1689,7 +1708,7 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {
+	if (fetch_and_consume_refs(&display, transport, transaction, ref_map, &fetch_head)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1711,7 +1730,7 @@ static int do_fetch(struct transport *transport,
 			 * when `--atomic` is passed: in that case we'll abort
 			 * the transaction and don't commit anything.
 			 */
-			if (backfill_tags(transport, transaction, tags_ref_map,
+			if (backfill_tags(&display, transport, transaction, tags_ref_map,
 					  &fetch_head))
 				retcode = 1;
 		}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> In order to print references in proper columns we need to calculate the
> width of the reference column before starting to print the references.
> This is done with the help of a global variable `refcol_width`.
>
> Refactor the code to instead use a new structure `display_state` that
> contains the computed width and plumb it through the stack as required.
> This is only the first step towards de-globalizing the state required to
> print references.

Nice.

Given that in the previous step, what used to be called display got
renamed to display_buffer (I think "buffer" ought to be sufficient
in this context, though), the variable of "struct display_state"
type should NOT be named "display", as it would be confusing when
two things are related to "display" and only one of them is called
as such.  Either "display_state" or "state" would be fine.

Thanks.

Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Wed, Mar 15, 2023 at 01:59:07PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In order to print references in proper columns we need to calculate the
> > width of the reference column before starting to print the references.
> > This is done with the help of a global variable `refcol_width`.
> >
> > Refactor the code to instead use a new structure `display_state` that
> > contains the computed width and plumb it through the stack as required.
> > This is only the first step towards de-globalizing the state required to
> > print references.
> 
> Nice.
> 
> Given that in the previous step, what used to be called display got
> renamed to display_buffer (I think "buffer" ought to be sufficient
> in this context, though), the variable of "struct display_state"
> type should NOT be named "display", as it would be confusing when
> two things are related to "display" and only one of them is called
> as such.  Either "display_state" or "state" would be fine.

Fair enough. In that case I may just as well drop the first patch.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Given that in the previous step, what used to be called display got
>> renamed to display_buffer (I think "buffer" ought to be sufficient
>> in this context, though), the variable of "struct display_state"
>> type should NOT be named "display", as it would be confusing when
>> two things are related to "display" and only one of them is called
>> as such.  Either "display_state" or "state" would be fine.
>
> Fair enough. In that case I may just as well drop the first patch.

If you plan to get rid of an independent "display_buffer" in the
endgame by moving it into the bigger struct as its .buffer member,
then I think the naming is fine as there will remain only one thing
that is "display".  The fact that I didn't see that plan through
when I read only the first two patches would probably mean that the
route this iteration of the series took was somewhat roundabout, and
there may be a more transparent and possibly a more direct way to
get to that goal?

I am not entirely sure if the buffer should go inside the
display_state structure in the endgame.  An alternative may be to
make it a on-stack variable of format_display() (which will later be
modified to do everything up to and including writing out the
result) and pass it through the callchain below to its helpers, just
like the current code already does.  And in such an approach, you'd
still need to name that variable passed to the helper functions
called by format_display()---"buffer" would be a good name for that.

Just thinking aloud.

Thanks.

Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

On Thu, Mar 16, 2023 at 09:18:09AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Given that in the previous step, what used to be called display got
> >> renamed to display_buffer (I think "buffer" ought to be sufficient
> >> in this context, though), the variable of "struct display_state"
> >> type should NOT be named "display", as it would be confusing when
> >> two things are related to "display" and only one of them is called
> >> as such.  Either "display_state" or "state" would be fine.
> >
> > Fair enough. In that case I may just as well drop the first patch.
> 
> If you plan to get rid of an independent "display_buffer" in the
> endgame by moving it into the bigger struct as its .buffer member,
> then I think the naming is fine as there will remain only one thing
> that is "display".  The fact that I didn't see that plan through
> when I read only the first two patches would probably mean that the
> route this iteration of the series took was somewhat roundabout, and
> there may be a more transparent and possibly a more direct way to
> get to that goal?
> 
> I am not entirely sure if the buffer should go inside the
> display_state structure in the endgame.  An alternative may be to
> make it a on-stack variable of format_display() (which will later be
> modified to do everything up to and including writing out the
> result) and pass it through the callchain below to its helpers, just
> like the current code already does.  And in such an approach, you'd
> still need to name that variable passed to the helper functions
> called by format_display()---"buffer" would be a good name for that.

Well, we could make it an on-stack variable just fine. But I suspect
that the only reason that this buffer exists is to optimize memory
allocations: a git-fetch(1) can easily end up printing thousands or even
hundreds of thousands of updated references, and reallocating that
buffer for each of them is quite wasteful.

Another alternative would be to make the buffer static and local to the
function. But you now have shared state again, and furthermore you have
no easy way to unleak its contents.

In the end, I think that having the buffer as a member of the display
state is the most straight-forward approach. It's self-contained and
allows us to reduce the number of allocations to a minimum. That being
said, I'm obviously biased here.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Given that in the previous step, what used to be called display got
>> renamed to display_buffer (I think "buffer" ought to be sufficient
>> in this context, though), the variable of "struct display_state"
>> type should NOT be named "display", as it would be confusing when
>> two things are related to "display" and only one of them is called
>> as such.  Either "display_state" or "state" would be fine.
>
> Fair enough. In that case I may just as well drop the first patch.

If you plan to get rid of an independent "display_buffer" in the
endgame by moving it into the bigger struct as its .buffer member,
then I think the naming is fine as there will remain only one thing
that is "display".  The fact that I didn't see that plan through
when I read only the first two patches would probably mean that the
route this iteration of the series took was somewhat roundabout, and
there may be a more transparent and possibly a more direct way to
get to that goal?

I am not entirely sure if the buffer should go inside the
display_state structure in the endgame.  An alternative may be to
make it a on-stack variable of format_display() (which will later be
modified to do everything up to and including writing out the
result) and pass it through the callchain below to its helpers, just
like the current code already does.  And in such an approach, you'd
still need to name that variable passed to the helper functions
called by format_display()---"buffer" would be a good name for that.

Just thinking aloud.

Thanks.

[PATCH 3/8] fetch: move output format into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

The git-fetch(1) command supports printing references either in "full"
or "compact" format depending on the `fetch.ouput` config key. The
format that is to be used is tracked in a global variable.

De-globalize the variable by moving it into the `display_state`
structure.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3695299177..f9ed9dac32 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -49,6 +49,7 @@ enum {
 
 struct display_state {
 	int refcol_width;
+	int compact_format;
 };
 
 static int fetch_prune_config = -1; /* unspecified */
@@ -745,9 +746,7 @@ static int s_update_ref(const char *action,
 	return ret;
 }
 
-static int compact_format;
-
-static int refcol_width(const struct ref *ref)
+static int refcol_width(const struct ref *ref, int compact_format)
 {
 	int max, rlen, llen, len;
 
@@ -789,9 +788,9 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 
 	git_config_get_string_tmp("fetch.output", &format);
 	if (!strcasecmp(format, "full"))
-		compact_format = 0;
+		display->compact_format = 0;
 	else if (!strcasecmp(format, "compact"))
-		compact_format = 1;
+		display->compact_format = 1;
 	else
 		die(_("invalid value for '%s': '%s'"),
 		    "fetch.output", format);
@@ -805,7 +804,7 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 		    !strcmp(rm->name, "HEAD"))
 			continue;
 
-		width = refcol_width(rm);
+		width = refcol_width(rm, display->compact_format);
 
 		/*
 		 * Not precise calculation for compact mode because '*' can
@@ -887,7 +886,7 @@ static void format_display(struct display_state *display,
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
-	if (!compact_format)
+	if (!display->compact_format)
 		print_remote_to_local(display, display_buffer, remote, local);
 	else
 		print_compact(display, display_buffer, remote, local);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/8] fetch: pass the full local reference name to `format_display`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4919 bytes --]

Before printing the name of the local references that would be updated
by a fetch we first prettify the reference name. This is done at the
calling side so that `format_display()` never sees the full name of the
local reference. This restricts our ability to introduce new output
formats that might want to print the full reference name.

Right now, all callsites except one are prettifying the reference name
anyway. And the only callsite that doesn't passes `FETCH_HEAD` as the
hardcoded reference name to `format_display()`, which would never be
changed by a call to `prettify_refname()` anyway. So let's refactor the
code to pass in the full local reference name and then prettify it in
the formatting code.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f9ed9dac32..bf2f01245a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -887,9 +887,9 @@ static void format_display(struct display_state *display,
 
 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
 	if (!display->compact_format)
-		print_remote_to_local(display, display_buffer, remote, local);
+		print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
 	else
-		print_compact(display, display_buffer, remote, local);
+		print_compact(display, display_buffer, remote, prettify_refname(local));
 	if (error)
 		strbuf_addf(display_buffer, "  (%s)", error);
 }
@@ -901,7 +901,6 @@ static int update_local_ref(struct ref *ref,
 			    struct strbuf *display_buffer, int summary_width)
 {
 	struct commit *current = NULL, *updated;
-	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
 	if (!repo_has_object_file(the_repository, &ref->new_oid))
@@ -910,7 +909,7 @@ static int update_local_ref(struct ref *ref,
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
 			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 		return 0;
 	}
 
@@ -923,7 +922,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		format_display(display, display_buffer, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return 1;
 	}
 
@@ -934,12 +933,12 @@ static int update_local_ref(struct ref *ref,
 			r = s_update_ref("updating tag", ref, transaction, 0);
 			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 			return r;
 		} else {
 			format_display(display, display_buffer, '!', _("[rejected]"),
 				       _("would clobber existing tag"),
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 			return 1;
 		}
 	}
@@ -972,7 +971,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, transaction, 0);
 		format_display(display, display_buffer, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return r;
 	}
 
@@ -994,7 +993,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, transaction, 1);
 		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1006,12 +1005,12 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, transaction, 1);
 		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
 		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return 1;
 	}
 }
@@ -1431,7 +1430,7 @@ static int prune_refs(struct display_state *display,
 				shown_url = 1;
 			}
 			format_display(display, &sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), prettify_refname(ref->name),
+				       _("(none)"), ref->name,
 				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);
 			strbuf_release(&sb);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] fetch: pass the full local reference name to `format_display`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Before printing the name of the local references that would be updated
> by a fetch we first prettify the reference name. This is done at the
> calling side so that `format_display()` never sees the full name of the
> local reference. This restricts our ability to introduce new output
> formats that might want to print the full reference name.

As long as the prettify at the caller is *not* done to avoid doing
the same prettifying multiple times, this change makes perfect
sense.

> So let's refactor the
> code to pass in the full local reference name and then prettify it in
> the formatting code.

Good thinking.

[PATCH 5/8] fetch: deduplicate handling of per-reference format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

Both callsites that call `format_display()` and then print the result to
standard error use the same formatting directive " %s\n" to print the
reference to disk, thus duplicating a small part of the logic.

Refactor the code to handle this in `format_display()` itself. This
paves the way for handling the printing logic in that function
completely.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index bf2f01245a..6fc2fd0d46 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -885,13 +885,14 @@ static void format_display(struct display_state *display,
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
+	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
 	if (!display->compact_format)
 		print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
 	else
 		print_compact(display, display_buffer, remote, prettify_refname(local));
 	if (error)
 		strbuf_addf(display_buffer, "  (%s)", error);
+	strbuf_addch(display_buffer, '\n');
 }
 
 static int update_local_ref(struct ref *ref,
@@ -1271,7 +1272,7 @@ static int store_updated_refs(struct display_state *display,
 							url_len, url);
 					shown_url = 1;
 				}
-				fprintf(stderr, " %s\n", note.buf);
+				fputs(note.buf, stderr);
 			}
 		}
 	}
@@ -1432,7 +1433,7 @@ static int prune_refs(struct display_state *display,
 			format_display(display, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), ref->name,
 				       summary_width);
-			fprintf(stderr, " %s\n",sb.buf);
+			fputs(sb.buf, stderr);
 			strbuf_release(&sb);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Both callsites that call `format_display()` and then print the result to
> standard error use the same formatting directive " %s\n" to print the
> reference to disk, thus duplicating a small part of the logic.

Hmph.

If format_display() were a function whose role was to prepare the
contents on a single line, it can be argued that it is caller's job
to give a leading indent that is appropriate for the line in the
context of the display it is producing.  "store-updated-refs" and
"prune-refs" may be showing a list of refs that were affected under
different heading, together with different kind of information, and
depending on the way each of these callers organize its output, the
appropriate indentation level for the line might be different.  So I
think the current product format_display() gives its callers is
perfectly defensible in that sense.

On the other hand, if format_display() is only about showing a
single line in the tightly limited context (in other words, both of
its callers promise that they will forever be happy with the
function showing exactly the same output), then this refactoring
would be OK.  In addition, it may even make more sense, if that were
the role of this callee, to do the actual printing, not just
preparing a line of string into a strbuf, in this callee, by moving
the fputs() from caller to callee.

So, I dunno.  The result of applying this patch leaves it in an
in-between state, where the division of labor between the caller and
the callee smells somewhat iffy.

Thanks.

Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On Wed, Mar 15, 2023 at 03:45:28PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Both callsites that call `format_display()` and then print the result to
> > standard error use the same formatting directive " %s\n" to print the
> > reference to disk, thus duplicating a small part of the logic.
> 
> Hmph.
> 
> If format_display() were a function whose role was to prepare the
> contents on a single line, it can be argued that it is caller's job
> to give a leading indent that is appropriate for the line in the
> context of the display it is producing.  "store-updated-refs" and
> "prune-refs" may be showing a list of refs that were affected under
> different heading, together with different kind of information, and
> depending on the way each of these callers organize its output, the
> appropriate indentation level for the line might be different.  So I
> think the current product format_display() gives its callers is
> perfectly defensible in that sense.
> 
> On the other hand, if format_display() is only about showing a
> single line in the tightly limited context (in other words, both of
> its callers promise that they will forever be happy with the
> function showing exactly the same output), then this refactoring
> would be OK.  In addition, it may even make more sense, if that were
> the role of this callee, to do the actual printing, not just
> preparing a line of string into a strbuf, in this callee, by moving
> the fputs() from caller to callee.
> 
> So, I dunno.  The result of applying this patch leaves it in an
> in-between state, where the division of labor between the caller and
> the callee smells somewhat iffy.
> 
> Thanks.

I totally agree with you here. From my point of view this "division of
labor" is getting fixed in the final patch that then also moves the
printing logic into `format_display()`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> So, I dunno.  The result of applying this patch leaves it in an
>> in-between state, where the division of labor between the caller and
>> the callee smells somewhat iffy.
>> 
>> Thanks.
>
> I totally agree with you here. From my point of view this "division of
> labor" is getting fixed in the final patch that then also moves the
> printing logic into `format_display()`.

Yes, again I smell the same "isn't this series going a bit too
roundabout route to its goal?" which I expressed in my response to
an earlier step.  The endgame of the series, even though I may not
agree with it 100%, is self consistent and does not leave the "this
ends at an in-between state" aftertaste in my mouth.

Thanks.

Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Thu, Mar 16, 2023 at 09:50:38AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> So, I dunno.  The result of applying this patch leaves it in an
> >> in-between state, where the division of labor between the caller and
> >> the callee smells somewhat iffy.
> >> 
> >> Thanks.
> >
> > I totally agree with you here. From my point of view this "division of
> > labor" is getting fixed in the final patch that then also moves the
> > printing logic into `format_display()`.
> 
> Yes, again I smell the same "isn't this series going a bit too
> roundabout route to its goal?" which I expressed in my response to
> an earlier step.  The endgame of the series, even though I may not
> agree with it 100%, is self consistent and does not leave the "this
> ends at an in-between state" aftertaste in my mouth.
> 
> Thanks.

Yeah. I did have some problems to lay out this series in a sensible way.
In an earlier iteration I tried moving the printing logic in one of the
initial patches, but from my point of view that resulted in an even more
awkward in-between state where the formatting and printing logic was
kind of all over the place. And another try with "big-bang-refactoring"
was barely reviewable, either.

Maybe the solution is to keep the order, but document intent better in
each of the patches leading towards the unified printing logic.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Maybe the solution is to keep the order, but document intent better in
> each of the patches leading towards the unified printing logic.

;-).

[PATCH 6/8] fetch: deduplicate logic to print remote URL

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8962 bytes --]

When fetching from a remote, we not only print the actual references
that have changed, but will also print the URL from which we have
fetched them to standard output. The logic to handle this is duplicated
across two different callsites with some non-trivial logic to compute
the anonymized URL. Furthermore, we're using global state to track
whether we have already shown the URL to the user or not.

Refactor the code by moving it into `format_display()`. Like this, we
can convert the global variable into a member of `display_state`. And
second, we can deduplicate the logic to compute the anonymized URL.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 96 +++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 55 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6fc2fd0d46..4e18c3902d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -50,6 +50,9 @@ enum {
 struct display_state {
 	int refcol_width;
 	int compact_format;
+
+	char *url;
+	int url_len, shown_url;
 };
 
 static int fetch_prune_config = -1; /* unspecified */
@@ -84,7 +87,6 @@ static const char *submodule_prefix = "";
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
-static int shown_url = 0;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
@@ -776,13 +778,27 @@ static int refcol_width(const struct ref *ref, int compact_format)
 	return rlen;
 }
 
-static void display_state_init(struct display_state *display, struct ref *ref_map)
+static void display_state_init(struct display_state *display, struct ref *ref_map,
+			       const char *raw_url)
 {
 	struct ref *rm;
 	const char *format = "full";
+	int i;
 
 	memset(display, 0, sizeof(*display));
 
+	if (raw_url)
+		display->url = transport_anonymize_url(raw_url);
+	else
+		display->url = xstrdup("foreign");
+
+	display->url_len = strlen(display->url);
+	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
+		;
+	display->url_len = i + 1;
+	if (4 < i && !strncmp(".git", display->url + i - 3, 4))
+		display->url_len = i - 3;
+
 	if (verbosity < 0)
 		return;
 
@@ -816,6 +832,11 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 	}
 }
 
+static void display_state_release(struct display_state *display)
+{
+	free(display->url);
+}
+
 static void print_remote_to_local(struct display_state *display,
 				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
@@ -883,6 +904,11 @@ static void format_display(struct display_state *display,
 	if (verbosity < 0)
 		return;
 
+	if (!display->shown_url) {
+		strbuf_addf(display_buffer, _("From %.*s\n"), display->url_len, display->url);
+		display->shown_url = 1;
+	}
+
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
@@ -1122,33 +1148,27 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "to avoid this check\n");
 
 static int store_updated_refs(struct display_state *display,
-			      const char *raw_url, const char *remote_name,
+			      const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head)
 {
-	int url_len, i, rc = 0;
+	int rc = 0;
 	struct strbuf note = STRBUF_INIT;
 	const char *what, *kind;
 	struct ref *rm;
-	char *url;
 	int want_status;
 	int summary_width = 0;
 
 	if (verbosity >= 0)
 		summary_width = transport_summary_width(ref_map);
 
-	if (raw_url)
-		url = transport_anonymize_url(raw_url);
-	else
-		url = xstrdup("foreign");
-
 	if (!connectivity_checked) {
 		struct check_connected_options opt = CHECK_CONNECTED_INIT;
 
 		rm = ref_map;
 		if (check_connected(iterate_ref_map, &rm, &opt)) {
-			rc = error(_("%s did not send all necessary objects\n"), url);
+			rc = error(_("%s did not send all necessary objects\n"), display->url);
 			goto abort;
 		}
 	}
@@ -1232,13 +1252,6 @@ static int store_updated_refs(struct display_state *display,
 				what = rm->name;
 			}
 
-			url_len = strlen(url);
-			for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-				;
-			url_len = i + 1;
-			if (4 < i && !strncmp(".git", url + i - 3, 4))
-				url_len = i - 3;
-
 			strbuf_reset(&note);
 			if (*what) {
 				if (*kind)
@@ -1248,7 +1261,7 @@ static int store_updated_refs(struct display_state *display,
 
 			append_fetch_head(fetch_head, &rm->old_oid,
 					  rm->fetch_head_status,
-					  note.buf, url, url_len);
+					  note.buf, display->url, display->url_len);
 
 			strbuf_reset(&note);
 			if (ref) {
@@ -1266,14 +1279,8 @@ static int store_updated_refs(struct display_state *display,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
 			}
-			if (note.len) {
-				if (!shown_url) {
-					fprintf(stderr, _("From %.*s\n"),
-							url_len, url);
-					shown_url = 1;
-				}
+			if (note.len)
 				fputs(note.buf, stderr);
-			}
 		}
 	}
 
@@ -1293,7 +1300,6 @@ static int store_updated_refs(struct display_state *display,
 
  abort:
 	strbuf_release(&note);
-	free(url);
 	return rc;
 }
 
@@ -1365,7 +1371,7 @@ static int fetch_and_consume_refs(struct display_state *display,
 	}
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
-	ret = store_updated_refs(display, transport->url, transport->remote->name,
+	ret = store_updated_refs(display, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
 				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
@@ -1378,30 +1384,15 @@ static int fetch_and_consume_refs(struct display_state *display,
 static int prune_refs(struct display_state *display,
 		      struct refspec *rs,
 		      struct ref_transaction *transaction,
-		      struct ref *ref_map,
-		      const char *raw_url)
+		      struct ref *ref_map)
 {
-	int url_len, i, result = 0;
+	int result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
 	struct strbuf err = STRBUF_INIT;
-	char *url;
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)")
 		: _("   (%s has become dangling)");
 
-	if (raw_url)
-		url = transport_anonymize_url(raw_url);
-	else
-		url = xstrdup("foreign");
-
-	url_len = strlen(url);
-	for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-		;
-
-	url_len = i + 1;
-	if (4 < i && !strncmp(".git", url + i - 3, 4))
-		url_len = i - 3;
-
 	if (!dry_run) {
 		if (transaction) {
 			for (ref = stale_refs; ref; ref = ref->next) {
@@ -1426,10 +1417,6 @@ static int prune_refs(struct display_state *display,
 
 		for (ref = stale_refs; ref; ref = ref->next) {
 			struct strbuf sb = STRBUF_INIT;
-			if (!shown_url) {
-				fprintf(stderr, _("From %.*s\n"), url_len, url);
-				shown_url = 1;
-			}
 			format_display(display, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), ref->name,
 				       summary_width);
@@ -1441,7 +1428,6 @@ static int prune_refs(struct display_state *display,
 
 cleanup:
 	strbuf_release(&err);
-	free(url);
 	free_refs(stale_refs);
 	return result;
 }
@@ -1596,7 +1582,7 @@ static int do_fetch(struct transport *transport,
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
-	struct display_state display;
+	struct display_state display = { 0 };
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1678,7 +1664,7 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	display_state_init(&display, ref_map);
+	display_state_init(&display, ref_map, transport->url);
 
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
@@ -1697,11 +1683,10 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
+			retcode = prune_refs(&display, rs, transaction, ref_map);
 		} else {
 			retcode = prune_refs(&display, &transport->remote->fetch,
-					     transaction, ref_map,
-					     transport->url);
+					     transaction, ref_map);
 		}
 		if (retcode != 0)
 			retcode = 1;
@@ -1812,6 +1797,7 @@ static int do_fetch(struct transport *transport,
 		error("%s", err.buf);
 	}
 
+	display_state_release(&display);
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
 	free_refs(ref_map);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/8] fetch: deduplicate logic to print remote URL

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When fetching from a remote, we not only print the actual references
> that have changed, but will also print the URL from which we have
> fetched them to standard output. The logic to handle this is duplicated
> across two different callsites with some non-trivial logic to compute
> the anonymized URL. Furthermore, we're using global state to track
> whether we have already shown the URL to the user or not.

If we are certain that store_updated_refs() is called only once for
the entire process, then storing the preprocessed url in the display
state and passing it around does sound like a good optimization and
clean-up.  What do we do when fetching from multiple remotes?

> +	display->url_len = strlen(display->url);
> +	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
> +		;
> +	display->url_len = i + 1;

This loop is inherited from the original, but we may want to use
strrchr() or rindex() as a post clean-up after this series settles.

Re: [PATCH 6/8] fetch: deduplicate logic to print remote URL

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

On Wed, Mar 15, 2023 at 04:02:18PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When fetching from a remote, we not only print the actual references
> > that have changed, but will also print the URL from which we have
> > fetched them to standard output. The logic to handle this is duplicated
> > across two different callsites with some non-trivial logic to compute
> > the anonymized URL. Furthermore, we're using global state to track
> > whether we have already shown the URL to the user or not.
> 
> If we are certain that store_updated_refs() is called only once for
> the entire process, then storing the preprocessed url in the display
> state and passing it around does sound like a good optimization and
> clean-up.  What do we do when fetching from multiple remotes?

We execute separate git-fetch(1) processes when fetching from multiple
remotes or when fetching submodules, so we should be fine here.

> > +	display->url_len = strlen(display->url);
> > +	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
> > +		;
> > +	display->url_len = i + 1;
> 
> This loop is inherited from the original, but we may want to use
> strrchr() or rindex() as a post clean-up after this series settles.

Yeah, that'd make sense.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 7067 bytes --]

When printing references that are updated during a fetch, we also print
a short summary of what is happening. This summary may either be a word
like "[up to date]" or "[rejected]", or the abbreviated old and new hash
on a reference update.

As the abbreviated hashes may have different lengths in order to be
unique we thus need to precompute the width of the summary's column by
iterating through all the objects. This is done in two locations: once
to compute the width for references that are to be pruned, and once for
all the other references. Consequentially, it can happen that the width
as calculated for these sets of references is different.

This isn't really a huge issue, but it is a bit confusing when reading
through the code. Furthermore, the current way of passing through the
summary width is rather verbose.

Refactor the code to compute the summary width once when initializing
the display state. Like this, the width will be the same and we have to
pass less variables around.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4e18c3902d..31724e9aaf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -49,6 +49,7 @@ enum {
 
 struct display_state {
 	int refcol_width;
+	int summary_width;
 	int compact_format;
 
 	char *url;
@@ -830,6 +831,8 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 		if (display->refcol_width < width)
 			display->refcol_width = width;
 	}
+
+	display->summary_width = transport_summary_width(ref_map);
 }
 
 static void display_state_release(struct display_state *display)
@@ -896,8 +899,7 @@ static void print_compact(struct display_state *display, struct strbuf *display_
 static void format_display(struct display_state *display,
 			   struct strbuf *display_buffer, char code,
 			   const char *summary, const char *error,
-			   const char *remote, const char *local,
-			   int summary_width)
+			   const char *remote, const char *local)
 {
 	int width;
 
@@ -909,7 +911,7 @@ static void format_display(struct display_state *display,
 		display->shown_url = 1;
 	}
 
-	width = (summary_width + strlen(summary) - gettext_width(summary));
+	width = (display->summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
 	if (!display->compact_format)
@@ -925,7 +927,7 @@ static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    struct display_state *display,
 			    const char *remote, const struct ref *remote_ref,
-			    struct strbuf *display_buffer, int summary_width)
+			    struct strbuf *display_buffer)
 {
 	struct commit *current = NULL, *updated;
 	int fast_forward = 0;
@@ -936,7 +938,7 @@ static int update_local_ref(struct ref *ref,
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
 			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
-				       remote, ref->name, summary_width);
+				       remote, ref->name);
 		return 0;
 	}
 
@@ -949,7 +951,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		format_display(display, display_buffer, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
-			       remote, ref->name, summary_width);
+			       remote, ref->name);
 		return 1;
 	}
 
@@ -960,12 +962,12 @@ static int update_local_ref(struct ref *ref,
 			r = s_update_ref("updating tag", ref, transaction, 0);
 			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
-				       remote, ref->name, summary_width);
+				       remote, ref->name);
 			return r;
 		} else {
 			format_display(display, display_buffer, '!', _("[rejected]"),
 				       _("would clobber existing tag"),
-				       remote, ref->name, summary_width);
+				       remote, ref->name);
 			return 1;
 		}
 	}
@@ -998,7 +1000,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, transaction, 0);
 		format_display(display, display_buffer, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name, summary_width);
+			       remote, ref->name);
 		return r;
 	}
 
@@ -1020,7 +1022,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, transaction, 1);
 		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name, summary_width);
+			       remote, ref->name);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1032,12 +1034,12 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, transaction, 1);
 		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, ref->name, summary_width);
+			       remote, ref->name);
 		strbuf_release(&quickref);
 		return r;
 	} else {
 		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, ref->name, summary_width);
+			       remote, ref->name);
 		return 1;
 	}
 }
@@ -1158,10 +1160,6 @@ static int store_updated_refs(struct display_state *display,
 	const char *what, *kind;
 	struct ref *rm;
 	int want_status;
-	int summary_width = 0;
-
-	if (verbosity >= 0)
-		summary_width = transport_summary_width(ref_map);
 
 	if (!connectivity_checked) {
 		struct check_connected_options opt = CHECK_CONNECTED_INIT;
@@ -1266,7 +1264,7 @@ static int store_updated_refs(struct display_state *display,
 			strbuf_reset(&note);
 			if (ref) {
 				rc |= update_local_ref(ref, transaction, display, what,
-						       rm, &note, summary_width);
+						       rm, &note);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1277,7 +1275,7 @@ static int store_updated_refs(struct display_state *display,
 				format_display(display, &note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
-					       "FETCH_HEAD", summary_width);
+					       "FETCH_HEAD");
 			}
 			if (note.len)
 				fputs(note.buf, stderr);
@@ -1413,13 +1411,10 @@ static int prune_refs(struct display_state *display,
 	}
 
 	if (verbosity >= 0) {
-		int summary_width = transport_summary_width(stale_refs);
-
 		for (ref = stale_refs; ref; ref = ref->next) {
 			struct strbuf sb = STRBUF_INIT;
 			format_display(display, &sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), ref->name,
-				       summary_width);
+				       _("(none)"), ref->name);
 			fputs(sb.buf, stderr);
 			strbuf_release(&sb);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> As the abbreviated hashes may have different lengths in order to be
> unique we thus need to precompute the width of the summary's column by
> iterating through all the objects. This is done in two locations: once
> to compute the width for references that are to be pruned, and once for
> all the other references. Consequentially, it can happen that the width
> as calculated for these sets of references is different.

Hmph.  Use of ref_map vs stale_refs as the parameter to call
transport_summary_width() is to come up with an appropriate width
for showing the list of stored refs vs the list of pruned refs, so
from that point of view, an appropriate width for each list is
calculated to a different number may even be a feature, no?

I do not mind either way all that much, but a change like this to
update the presentation may want to be protected with a test from
future breakages.

Thanks.

Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Wed, Mar 15, 2023 at 04:12:59PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > As the abbreviated hashes may have different lengths in order to be
> > unique we thus need to precompute the width of the summary's column by
> > iterating through all the objects. This is done in two locations: once
> > to compute the width for references that are to be pruned, and once for
> > all the other references. Consequentially, it can happen that the width
> > as calculated for these sets of references is different.
> 
> Hmph.  Use of ref_map vs stale_refs as the parameter to call
> transport_summary_width() is to come up with an appropriate width
> for showing the list of stored refs vs the list of pruned refs, so
> from that point of view, an appropriate width for each list is
> calculated to a different number may even be a feature, no?

I'd say it's not. Look at the following output generated by a `git fetch
--prune --no-progress` with a deleted and an updated reference:

    From /tmp/repo
     - [deleted]         (none)     -> origin/to-be-deleted
       82307bb..107b50a  main       -> origin/main

Before my change, the width of the deletion and the reference update are
calculated separately. Given that:

    - we don't even display the object IDs for deleted references

    - the width of the deleted reference's column is static anyway.

I'd argue that it's not a feature that the widths are computed
separately. If it was, you could just skip calculating the width of
deleted references and just print them with a static column width.

The current implementation tends to work in most cases as the column
width is based on the minimum length where all abbreviated object IDs
become unique. And I assume that it's the same for both sets of refs in
the majority of cases. And in the other cases I guess that nobody cares
much anyway.

Practically speaking we could go even further than the current version,
as I now compute the width across _all_ reference updates, even those
which are deletions. But theoretically speaking, we could just skip over
any deletions completely as they won't ever contribute to the column
width anyway.

> I do not mind either way all that much, but a change like this to
> update the presentation may want to be protected with a test from
> future breakages.

Fair, having a test for this would be great. But what kept me from
adding one here is that the column width depends on the length of the
longest shared prefix of two object IDs that are about to be updated.
And I just have no clue how to generate those without brute forcing them
for both SHA1 and SHA256.

Do we have any mechanism for this?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Practically speaking we could go even further than the current version,
> as I now compute the width across _all_ reference updates, even those
> which are deletions. But theoretically speaking, we could just skip over
> any deletions completely as they won't ever contribute to the column
> width anyway.

OK.  It's good to see that you have thought it through.

> Fair, having a test for this would be great. But what kept me from
> adding one here is that the column width depends on the length of the
> longest shared prefix of two object IDs that are about to be updated.

You do not have to prepare "this is the correct expected output",
when you need to make sure that two kinds of lines use the same
width settings, no?  Extract such lines from the two camps, measure
them and see if they are of the same length, or something?

Thanks.

Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

On Thu, Mar 16, 2023 at 09:30:46AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Practically speaking we could go even further than the current version,
> > as I now compute the width across _all_ reference updates, even those
> > which are deletions. But theoretically speaking, we could just skip over
> > any deletions completely as they won't ever contribute to the column
> > width anyway.
> 
> OK.  It's good to see that you have thought it through.
> 
> > Fair, having a test for this would be great. But what kept me from
> > adding one here is that the column width depends on the length of the
> > longest shared prefix of two object IDs that are about to be updated.
> 
> You do not have to prepare "this is the correct expected output",
> when you need to make sure that two kinds of lines use the same
> width settings, no?  Extract such lines from the two camps, measure
> them and see if they are of the same length, or something?

Well, comparing widths of these two lines is the easy version of the
test, agreed. But in order to test for the bug I'm fixing I'd need to
generate two sets of object IDs, where the first set of object IDs has a
longer shared prefix length than the second. Because otherwise, the bug
wouldn't be guaranteed to even surface.

Anyway -- I'll have a look at adding the easy version.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 8/8] fetch: centralize printing of reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 9914 bytes --]

In order to print updated references during a fetch, the two different
call sites that do this will first call `format_display()` followed by a
call to `fputs()`. This is needlessly roundabout now that we have the
`display_state` structure that encapsulates all of the printing logic
for references.

Move displaying the reference updates into `format_display()` and rename
it to `display_ref_update()` to better match its new purpose. We have
now centralized the logic to print reference updates into a single
function, which enables us to more readily introduce new formats that
may even be printing to a different file descriptor than `stderr`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 105 ++++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 53 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 31724e9aaf..6e864f8457 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -48,6 +48,8 @@ enum {
 };
 
 struct display_state {
+	struct strbuf buf;
+
 	int refcol_width;
 	int summary_width;
 	int compact_format;
@@ -788,6 +790,8 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 
 	memset(display, 0, sizeof(*display));
 
+	strbuf_init(&display->buf, 0);
+
 	if (raw_url)
 		display->url = transport_anonymize_url(raw_url);
 	else
@@ -837,14 +841,14 @@ static void display_state_init(struct display_state *display, struct ref *ref_ma
 
 static void display_state_release(struct display_state *display)
 {
+	strbuf_release(&display->buf);
 	free(display->url);
 }
 
 static void print_remote_to_local(struct display_state *display,
-				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display_buffer, "%-*s -> %s", display->refcol_width, remote, local);
+	strbuf_addf(&display->buf, "%-*s -> %s", display->refcol_width, remote, local);
 }
 
 static int find_and_replace(struct strbuf *haystack,
@@ -874,14 +878,14 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct display_state *display, struct strbuf *display_buffer,
+static void print_compact(struct display_state *display,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;
 	struct strbuf l = STRBUF_INIT;
 
 	if (!strcmp(remote, local)) {
-		strbuf_addf(display_buffer, "%-*s -> *", display->refcol_width, remote);
+		strbuf_addf(&display->buf, "%-*s -> *", display->refcol_width, remote);
 		return;
 	}
 
@@ -890,44 +894,46 @@ static void print_compact(struct display_state *display, struct strbuf *display_
 
 	if (!find_and_replace(&r, local, "*"))
 		find_and_replace(&l, remote, "*");
-	print_remote_to_local(display, display_buffer, r.buf, l.buf);
+	print_remote_to_local(display, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct display_state *display,
-			   struct strbuf *display_buffer, char code,
-			   const char *summary, const char *error,
-			   const char *remote, const char *local)
+static void display_ref_update(struct display_state *display, char code,
+			       const char *summary, const char *error,
+			       const char *remote, const char *local)
 {
 	int width;
 
 	if (verbosity < 0)
 		return;
 
+	strbuf_reset(&display->buf);
+
 	if (!display->shown_url) {
-		strbuf_addf(display_buffer, _("From %.*s\n"), display->url_len, display->url);
+		strbuf_addf(&display->buf, _("From %.*s\n"), display->url_len, display->url);
 		display->shown_url = 1;
 	}
 
 	width = (display->summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
+	strbuf_addf(&display->buf, " %c %-*s ", code, width, summary);
 	if (!display->compact_format)
-		print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
+		print_remote_to_local(display, remote, prettify_refname(local));
 	else
-		print_compact(display, display_buffer, remote, prettify_refname(local));
+		print_compact(display, remote, prettify_refname(local));
 	if (error)
-		strbuf_addf(display_buffer, "  (%s)", error);
-	strbuf_addch(display_buffer, '\n');
+		strbuf_addf(&display->buf, "  (%s)", error);
+	strbuf_addch(&display->buf, '\n');
+
+	fputs(display->buf.buf, stderr);
 }
 
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    struct display_state *display,
-			    const char *remote, const struct ref *remote_ref,
-			    struct strbuf *display_buffer)
+			    const char *remote, const struct ref *remote_ref)
 {
 	struct commit *current = NULL, *updated;
 	int fast_forward = 0;
@@ -937,8 +943,8 @@ static int update_local_ref(struct ref *ref,
 
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
-			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
-				       remote, ref->name);
+			display_ref_update(display, '=', _("[up to date]"), NULL,
+					   remote, ref->name);
 		return 0;
 	}
 
@@ -949,9 +955,9 @@ static int update_local_ref(struct ref *ref,
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
-		format_display(display, display_buffer, '!', _("[rejected]"),
-			       _("can't fetch into checked-out branch"),
-			       remote, ref->name);
+		display_ref_update(display, '!', _("[rejected]"),
+				   _("can't fetch into checked-out branch"),
+				   remote, ref->name);
 		return 1;
 	}
 
@@ -960,14 +966,14 @@ static int update_local_ref(struct ref *ref,
 		if (force || ref->force) {
 			int r;
 			r = s_update_ref("updating tag", ref, transaction, 0);
-			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
-				       r ? _("unable to update local ref") : NULL,
-				       remote, ref->name);
+			display_ref_update(display, r ? '!' : 't', _("[tag update]"),
+					   r ? _("unable to update local ref") : NULL,
+					   remote, ref->name);
 			return r;
 		} else {
-			format_display(display, display_buffer, '!', _("[rejected]"),
-				       _("would clobber existing tag"),
-				       remote, ref->name);
+			display_ref_update(display, '!', _("[rejected]"),
+					   _("would clobber existing tag"),
+					   remote, ref->name);
 			return 1;
 		}
 	}
@@ -998,9 +1004,9 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display, display_buffer, r ? '!' : '*', what,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name);
+		display_ref_update(display, r ? '!' : '*', what,
+				   r ? _("unable to update local ref") : NULL,
+				   remote, ref->name);
 		return r;
 	}
 
@@ -1020,9 +1026,9 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "..");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("fast-forward", ref, transaction, 1);
-		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name);
+		display_ref_update(display, r ? '!' : ' ', quickref.buf,
+				   r ? _("unable to update local ref") : NULL,
+				   remote, ref->name);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1032,14 +1038,14 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "...");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("forced-update", ref, transaction, 1);
-		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
-			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, ref->name);
+		display_ref_update(display, r ? '!' : '+', quickref.buf,
+				   r ? _("unable to update local ref") : _("forced update"),
+				   remote, ref->name);
 		strbuf_release(&quickref);
 		return r;
 	} else {
-		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, ref->name);
+		display_ref_update(display, '!', _("[rejected]"), _("non-fast-forward"),
+				   remote, ref->name);
 		return 1;
 	}
 }
@@ -1261,10 +1267,8 @@ static int store_updated_refs(struct display_state *display,
 					  rm->fetch_head_status,
 					  note.buf, display->url, display->url_len);
 
-			strbuf_reset(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, transaction, display, what,
-						       rm, &note);
+				rc |= update_local_ref(ref, transaction, display, what, rm);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1272,13 +1276,11 @@ static int store_updated_refs(struct display_state *display,
 				 * would be written to FETCH_HEAD, if --dry-run
 				 * is set).
 				 */
-				format_display(display, &note, '*',
-					       *kind ? kind : "branch", NULL,
-					       *what ? what : "HEAD",
-					       "FETCH_HEAD");
+				display_ref_update(display, '*',
+						   *kind ? kind : "branch", NULL,
+						   *what ? what : "HEAD",
+						   "FETCH_HEAD");
 			}
-			if (note.len)
-				fputs(note.buf, stderr);
 		}
 	}
 
@@ -1412,11 +1414,8 @@ static int prune_refs(struct display_state *display,
 
 	if (verbosity >= 0) {
 		for (ref = stale_refs; ref; ref = ref->next) {
-			struct strbuf sb = STRBUF_INIT;
-			format_display(display, &sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), ref->name);
-			fputs(sb.buf, stderr);
-			strbuf_release(&sb);
+			display_ref_update(display, '-', _("[deleted]"), NULL,
+					   _("(none)"), ref->name);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
 	}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/8] fetch: refactor code that prints reference updates

[Jonathan Tan]

Patrick Steinhardt <ps@pks.im> writes:
>     1. We want to take control of the reference updates so that we can
>        atomically update all or a subset of references that git-fetch
>        would have updated.
> 
>     2. We want to be able to quarantine objects in a fetch so that we
>        can e.g. perform consistency checks for them before they land in
>        the main repository.

If you want to do this, something that might be possible is to change
the RHS of the refspecs to put the refs in a namespace of your choice
(e.g. ...:refs/<UUID>/...) and then you can look at what's generated and
process them as you wish.

>     - There should be as few global state as possible. This is to reduce
>       confusion and having to repeat the same incantations in multiple
>       different locations.

Makes sense.

>     - The logic should be as self-contained as possible. This is so that
>       it can easily be changed in a subsequent patch series.

Also makes sense, but I think that some of your patches might be
contrary to this goal (more details below).

I've read all the patches, but will just summarize my thoughts here.

> Patrick Steinhardt (8):
>   fetch: rename `display` buffer to avoid name conflict

One other way, as others have discussed, is to just name the new
variable display_state. (I would prefer that, at the very least so
that in case someone else has a patch that contains the identifier
"display", problems would be more easily noticed. This is very unlikely
to happen but I think it's a good general direction for the Git project
to follow.)

>   fetch: move reference width calculation into `display_state`
>   fetch: move output format into `display_state`
>   fetch: pass the full local reference name to `format_display`

All these are good changes that I would be happy to see merged.

>   fetch: deduplicate handling of per-reference format

I'm not so sure that this is the correct abstraction. I think that this
and the last patch might be counterproductive to your stated goal of
having one more mode of printing the refs, in fact, since when we have
that new mode, the format would be different but the printing would
remain (so we should split the format and printing).

>   fetch: deduplicate logic to print remote URL

Makes sense, although I would need to consider only storing the
raw URL in the struct display_state and processing it when it needs
to be emitted (haven't checked if this is feasible, though).

>   fetch: fix inconsistent summary width for pruned and updated refs

This changes the behavior in that the summary width, even when printing
the summary of pruned refs, is computed based only on the updated refs.
The summary width might need to remain out of the struct display_state
for now.

>   fetch: centralize printing of reference updates

Same as "fetch: deduplicate handling of per-reference format".

Re: [PATCH 0/8] fetch: refactor code that prints reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4932 bytes --]

On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >     1. We want to take control of the reference updates so that we can
> >        atomically update all or a subset of references that git-fetch
> >        would have updated.
> > 
> >     2. We want to be able to quarantine objects in a fetch so that we
> >        can e.g. perform consistency checks for them before they land in
> >        the main repository.
> 
> If you want to do this, something that might be possible is to change
> the RHS of the refspecs to put the refs in a namespace of your choice
> (e.g. ...:refs/<UUID>/...) and then you can look at what's generated and
> process them as you wish.

There's two major problems with this, unfortunately:

- We want to use the machine-parseable format in our repository
  mirroring functionality, where you can easily end up fetching
  thousands or even hundreds of thousands of references. If you need to
  write all of them anew in a first step then you'll end up slower than
  before.

- Repository mirroring is comparatively flexible in what it allows. Most
  importantly, it gives you the opiton to say that divergent references
  should not be updated at all, which translates into an unforced fetch.
  It's even possible to have fetches with mixed forced and unforced
  reference updates. So if we fetched into a separate namespace first,
  we'd now have to reimplement checks for forced updates in Gitaly so
  that we correctly update only those refs that would have been updated
  by Git. We'd also need to manually figure out deleted references.

  This would be quite a risky change and would duplicate a lot of
  knowledge. Furthermore, merging the two sets of references would
  likely be quite expensive performance-wise.

Also, even if we did have a different RHS, it still wouldn't fix the
issue that objects are written into the main object database directly.
Ideally, we'd really only accept changes into the repository once we
have fully verified all of them. Right now it can happen that we refuse
a fetch, but the objects would continue to exist in the repository.

A second motivation for the quarantine directory is so that we can
enumerate all objects that are indeed new. This will eventually be used
to implement more efficient replication of the repository, where we can
theoretically just take all of the fetched objects in the quarantine
object directory and copy it to the replicas of that repository.

[snip]
> >   fetch: deduplicate handling of per-reference format
> 
> I'm not so sure that this is the correct abstraction. I think that this
> and the last patch might be counterproductive to your stated goal of
> having one more mode of printing the refs, in fact, since when we have
> that new mode, the format would be different but the printing would
> remain (so we should split the format and printing).

I already have the full implementation of the new machine-parseable
format available locally, but didn't want to send it as part of this
patch series yet to avoid it becoming overly large. But I can say that
this change really did make the end goal easier to achieve, due to two
reasons:

- If we continued to handle the per-reference format at the different
  callsites, I'd have to also amend each of the callers when introducing
  the new format as we're going to use a different format there. But
  when doing this in `format_display()`, we really only need to have a
  single switch at the beginning to check whether to use the machine
  parseable format or the other one.

- Currently, all reference updates are printed to stderr. As stderr is
  also used to display errors and the progress bar, this really makes it
  not a good fit for the machine-parseable format. Instead, I decided
  that it would make more sense to print the new format to stdout. And
  by having the printing-logic self-contained we again only have a
  single location we need to change.

I realize though that all of this isn't as well-documented in the commit
messages as it should be, which is also something that Junio complained
about. I'll hopefully do a better job in v2 of this patch series.

> >   fetch: deduplicate logic to print remote URL
> 
> Makes sense, although I would need to consider only storing the
> raw URL in the struct display_state and processing it when it needs
> to be emitted (haven't checked if this is feasible, though).
> 
> >   fetch: fix inconsistent summary width for pruned and updated refs
> 
> This changes the behavior in that the summary width, even when printing
> the summary of pruned refs, is computed based only on the updated refs.
> The summary width might need to remain out of the struct display_state
> for now.

Fair, that's a case I didn't yet consider. I'll have another look.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/8] fetch: refactor code that prints reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Fri, Mar 17, 2023 at 01:24:49PM -0700, Jonathan Tan wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> >   fetch: deduplicate logic to print remote URL
> 
> Makes sense, although I would need to consider only storing the
> raw URL in the struct display_state and processing it when it needs
> to be emitted (haven't checked if this is feasible, though).

We likely could, but right now the benefit isn't all that high. If the
URL was only used in `display_ref_update()` then this would be easy
enough to do. But we also access the sanitized URL when the connectivity
check fails or when printing to FETCH_HEAD.

If we provided an accessor function thet returns the URL it would be
trivial to do, but what do we really gain here? In the best case we save
an allocation for the URL and two loops ranging over it. That doesn't
quite feel worth it to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/6] fetch: refactor code that prints reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 41718 bytes --]

Hi,

This is the second version of my patch series to refactor the code that
prints reference updates during git-fetch(1). Its aim is it to make the
code more self-contained so that it can be easily amended for a new
machine-parseable format.

Changes compared to v1:

    - I've dropped the first patch to rename the preexisting `display`
      buffer variable. Instead, I'm now just picking the longer
      `display_state` variable name for the newly introduced structure
      in order to avoid naming conflicts.

    - I've touched up the commit messages of patches 4-6 to hopefully
      clarify their intent a little bit better.

    - I've dropped patch 7/8 that unified the logic to calculate the
      summary width. Even though it fixes a bug in one case, Jonathan
      correctly pointed out that it's weird in the case where there are
      only reference deletions without any reference updates. I may have
      another go in a separate patch series after the dust has settled.

Thanks for the feedback so far!

Patrick

Patrick Steinhardt (6):
  fetch: move reference width calculation into `display_state`
  fetch: move output format into `display_state`
  fetch: pass the full local reference name to `format_display`
  fetch: centralize handling of per-reference format
  fetch: centralize logic to print remote URL
  fetch: centralize printing of reference updates

 builtin/fetch.c | 267 +++++++++++++++++++++++++-----------------------
 1 file changed, 138 insertions(+), 129 deletions(-)

Range-diff against v1:
1:  692206f7ff < -:  ---------- fetch: rename `display` buffer to avoid name conflict
2:  aa792b12a4 ! 1:  ce2c4b61ae fetch: move reference width calculation into `display_state`
    @@ builtin/fetch.c: static void adjust_refcol_width(const struct ref *ref)
      }
      
     -static void prepare_format_display(struct ref *ref_map)
    -+static void display_state_init(struct display_state *display, struct ref *ref_map)
    ++static void display_state_init(struct display_state *display_state, struct ref *ref_map)
      {
      	struct ref *rm;
      	const char *format = "full";
      
    -+	memset(display, 0, sizeof(*display));
    ++	memset(display_state, 0, sizeof(*display_state));
     +
      	if (verbosity < 0)
      		return;
    @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
      		die(_("invalid value for '%s': '%s'"),
      		    "fetch.output", format);
      
    -+	display->refcol_width = 10;
    ++	display_state->refcol_width = 10;
      	for (rm = ref_map; rm; rm = rm->next) {
     +		int width;
     +
    @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
     +		 * appear on the left hand side of '->' and shrink the column
     +		 * back.
     +		 */
    -+		if (display->refcol_width < width)
    -+			display->refcol_width = width;
    ++		if (display_state->refcol_width < width)
    ++			display_state->refcol_width = width;
      	}
      }
      
    --static void print_remote_to_local(struct strbuf *display_buffer,
    -+static void print_remote_to_local(struct display_state *display,
    +-static void print_remote_to_local(struct strbuf *display,
    ++static void print_remote_to_local(struct display_state *display_state,
     +				  struct strbuf *display_buffer,
      				  const char *remote, const char *local)
      {
    --	strbuf_addf(display_buffer, "%-*s -> %s", refcol_width, remote, local);
    -+	strbuf_addf(display_buffer, "%-*s -> %s", display->refcol_width, remote, local);
    +-	strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);
    ++	strbuf_addf(display_buffer, "%-*s -> %s", display_state->refcol_width, remote, local);
      }
      
      static int find_and_replace(struct strbuf *haystack,
    @@ builtin/fetch.c: static int find_and_replace(struct strbuf *haystack,
      	return 1;
      }
      
    --static void print_compact(struct strbuf *display_buffer,
    -+static void print_compact(struct display_state *display, struct strbuf *display_buffer,
    +-static void print_compact(struct strbuf *display,
    ++static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,
      			  const char *remote, const char *local)
      {
      	struct strbuf r = STRBUF_INIT;
      	struct strbuf l = STRBUF_INIT;
      
      	if (!strcmp(remote, local)) {
    --		strbuf_addf(display_buffer, "%-*s -> *", refcol_width, remote);
    -+		strbuf_addf(display_buffer, "%-*s -> *", display->refcol_width, remote);
    +-		strbuf_addf(display, "%-*s -> *", refcol_width, remote);
    ++		strbuf_addf(display_buffer, "%-*s -> *", display_state->refcol_width, remote);
      		return;
      	}
      
    -@@ builtin/fetch.c: static void print_compact(struct strbuf *display_buffer,
    +@@ builtin/fetch.c: static void print_compact(struct strbuf *display,
      
      	if (!find_and_replace(&r, local, "*"))
      		find_and_replace(&l, remote, "*");
    --	print_remote_to_local(display_buffer, r.buf, l.buf);
    -+	print_remote_to_local(display, display_buffer, r.buf, l.buf);
    +-	print_remote_to_local(display, r.buf, l.buf);
    ++	print_remote_to_local(display_state, display_buffer, r.buf, l.buf);
      
      	strbuf_release(&r);
      	strbuf_release(&l);
      }
      
    --static void format_display(struct strbuf *display_buffer, char code,
    -+static void format_display(struct display_state *display,
    +-static void format_display(struct strbuf *display, char code,
    ++static void format_display(struct display_state *display_state,
     +			   struct strbuf *display_buffer, char code,
      			   const char *summary, const char *error,
      			   const char *remote, const char *local,
      			   int summary_width)
    -@@ builtin/fetch.c: static void format_display(struct strbuf *display_buffer, char code,
    +@@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,
      
    - 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
    + 	width = (summary_width + strlen(summary) - gettext_width(summary));
    + 
    +-	strbuf_addf(display, "%c %-*s ", code, width, summary);
    ++	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
      	if (!compact_format)
    --		print_remote_to_local(display_buffer, remote, local);
    -+		print_remote_to_local(display, display_buffer, remote, local);
    +-		print_remote_to_local(display, remote, local);
    ++		print_remote_to_local(display_state, display_buffer, remote, local);
      	else
    --		print_compact(display_buffer, remote, local);
    -+		print_compact(display, display_buffer, remote, local);
    +-		print_compact(display, remote, local);
    ++		print_compact(display_state, display_buffer, remote, local);
      	if (error)
    - 		strbuf_addf(display_buffer, "  (%s)", error);
    +-		strbuf_addf(display, "  (%s)", error);
    ++		strbuf_addf(display_buffer, "  (%s)", error);
      }
      
      static int update_local_ref(struct ref *ref,
      			    struct ref_transaction *transaction,
    -+			    struct display_state *display,
    ++			    struct display_state *display_state,
      			    const char *remote, const struct ref *remote_ref,
    - 			    struct strbuf *display_buffer, int summary_width)
    + 			    struct strbuf *display, int summary_width)
      {
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      
      	if (oideq(&ref->old_oid, &ref->new_oid)) {
      		if (verbosity > 0)
    --			format_display(display_buffer, '=', _("[up to date]"), NULL,
    -+			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
    +-			format_display(display, '=', _("[up to date]"), NULL,
    ++			format_display(display_state, display, '=', _("[up to date]"), NULL,
      				       remote, pretty_ref, summary_width);
      		return 0;
      	}
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		 * If this is the head, and it's not okay to update
      		 * the head, and the old value of the head isn't empty...
      		 */
    --		format_display(display_buffer, '!', _("[rejected]"),
    -+		format_display(display, display_buffer, '!', _("[rejected]"),
    +-		format_display(display, '!', _("[rejected]"),
    ++		format_display(display_state, display, '!', _("[rejected]"),
      			       _("can't fetch into checked-out branch"),
      			       remote, pretty_ref, summary_width);
      		return 1;
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		if (force || ref->force) {
      			int r;
      			r = s_update_ref("updating tag", ref, transaction, 0);
    --			format_display(display_buffer, r ? '!' : 't', _("[tag update]"),
    -+			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
    +-			format_display(display, r ? '!' : 't', _("[tag update]"),
    ++			format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
      				       r ? _("unable to update local ref") : NULL,
      				       remote, pretty_ref, summary_width);
      			return r;
      		} else {
    --			format_display(display_buffer, '!', _("[rejected]"), _("would clobber existing tag"),
    -+			format_display(display, display_buffer, '!', _("[rejected]"),
    +-			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
    ++			format_display(display_state, display, '!', _("[rejected]"),
     +				       _("would clobber existing tag"),
      				       remote, pretty_ref, summary_width);
      			return 1;
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		}
      
      		r = s_update_ref(msg, ref, transaction, 0);
    --		format_display(display_buffer, r ? '!' : '*', what,
    -+		format_display(display, display_buffer, r ? '!' : '*', what,
    +-		format_display(display, r ? '!' : '*', what,
    ++		format_display(display_state, display, r ? '!' : '*', what,
      			       r ? _("unable to update local ref") : NULL,
      			       remote, pretty_ref, summary_width);
      		return r;
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		strbuf_addstr(&quickref, "..");
      		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
      		r = s_update_ref("fast-forward", ref, transaction, 1);
    --		format_display(display_buffer, r ? '!' : ' ', quickref.buf,
    -+		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
    +-		format_display(display, r ? '!' : ' ', quickref.buf,
    ++		format_display(display_state, display, r ? '!' : ' ', quickref.buf,
      			       r ? _("unable to update local ref") : NULL,
      			       remote, pretty_ref, summary_width);
      		strbuf_release(&quickref);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		strbuf_addstr(&quickref, "...");
      		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
      		r = s_update_ref("forced-update", ref, transaction, 1);
    --		format_display(display_buffer, r ? '!' : '+', quickref.buf,
    -+		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
    +-		format_display(display, r ? '!' : '+', quickref.buf,
    ++		format_display(display_state, display, r ? '!' : '+', quickref.buf,
      			       r ? _("unable to update local ref") : _("forced update"),
      			       remote, pretty_ref, summary_width);
      		strbuf_release(&quickref);
      		return r;
      	} else {
    --		format_display(display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
    -+		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
    +-		format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
    ++		format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
      			       remote, pretty_ref, summary_width);
      		return 1;
      	}
    @@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n
         "to avoid this check\n");
      
     -static int store_updated_refs(const char *raw_url, const char *remote_name,
    -+static int store_updated_refs(struct display_state *display,
    ++static int store_updated_refs(struct display_state *display_state,
     +			      const char *raw_url, const char *remote_name,
      			      int connectivity_checked,
      			      struct ref_transaction *transaction, struct ref *ref_map,
    @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
      			strbuf_reset(&note);
      			if (ref) {
     -				rc |= update_local_ref(ref, transaction, what,
    -+				rc |= update_local_ref(ref, transaction, display, what,
    ++				rc |= update_local_ref(ref, transaction, display_state, what,
      						       rm, &note, summary_width);
      				free(ref);
      			} else if (write_fetch_head || dry_run) {
    @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
      				 * is set).
      				 */
     -				format_display(&note, '*',
    -+				format_display(display, &note, '*',
    ++				format_display(display_state, &note, '*',
      					       *kind ? kind : "branch", NULL,
      					       *what ? what : "HEAD",
      					       "FETCH_HEAD", summary_width);
    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)
      }
      
     -static int fetch_and_consume_refs(struct transport *transport,
    -+static int fetch_and_consume_refs(struct display_state *display,
    ++static int fetch_and_consume_refs(struct display_state *display_state,
     +				  struct transport *transport,
      				  struct ref_transaction *transaction,
      				  struct ref *ref_map,
    @@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,
      
      	trace2_region_enter("fetch", "consume_refs", the_repository);
     -	ret = store_updated_refs(transport->url, transport->remote->name,
    -+	ret = store_updated_refs(display, transport->url, transport->remote->name,
    ++	ret = store_updated_refs(display_state, transport->url, transport->remote->name,
      				 connectivity_checked, transaction, ref_map,
      				 fetch_head);
      	trace2_region_leave("fetch", "consume_refs", the_repository);
    @@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,
      }
      
     -static int prune_refs(struct refspec *rs,
    -+static int prune_refs(struct display_state *display,
    ++static int prune_refs(struct display_state *display_state,
     +		      struct refspec *rs,
      		      struct ref_transaction *transaction,
      		      struct ref *ref_map,
    @@ builtin/fetch.c: static int prune_refs(struct refspec *rs,
      				shown_url = 1;
      			}
     -			format_display(&sb, '-', _("[deleted]"), NULL,
    -+			format_display(display, &sb, '-', _("[deleted]"), NULL,
    ++			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
      				       _("(none)"), prettify_refname(ref->name),
      				       summary_width);
      			fprintf(stderr, " %s\n",sb.buf);
    @@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
      }
      
     -static int backfill_tags(struct transport *transport,
    -+static int backfill_tags(struct display_state *display,
    ++static int backfill_tags(struct display_state *display_state,
     +			 struct transport *transport,
      			 struct ref_transaction *transaction,
      			 struct ref *ref_map,
    @@ builtin/fetch.c: static int backfill_tags(struct transport *transport,
      	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
      	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
     -	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);
    -+	retcode = fetch_and_consume_refs(display, transport, transaction, ref_map, fetch_head);
    ++	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);
      
      	if (gsecondary) {
      		transport_disconnect(gsecondary);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      {
      	struct ref_transaction *transaction = NULL;
      	struct ref *ref_map = NULL;
    -+	struct display_state display;
    ++	struct display_state display_state;
      	int autotags = (transport->remote->fetch_tags == 1);
      	int retcode = 0;
      	const struct ref *remote_refs;
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      	if (retcode)
      		goto cleanup;
      
    -+	display_state_init(&display, ref_map);
    ++	display_state_init(&display_state, ref_map);
     +
      	if (atomic_fetch) {
      		transaction = ref_transaction_begin(&err);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		 */
      		if (rs->nr) {
     -			retcode = prune_refs(rs, transaction, ref_map, transport->url);
    -+			retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
    ++			retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
      		} else {
     -			retcode = prune_refs(&transport->remote->fetch,
    -+			retcode = prune_refs(&display, &transport->remote->fetch,
    ++			retcode = prune_refs(&display_state, &transport->remote->fetch,
      					     transaction, ref_map,
      					     transport->url);
      		}
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      	}
      
     -	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {
    -+	if (fetch_and_consume_refs(&display, transport, transaction, ref_map, &fetch_head)) {
    ++	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {
      		retcode = 1;
      		goto cleanup;
      	}
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      			 * the transaction and don't commit anything.
      			 */
     -			if (backfill_tags(transport, transaction, tags_ref_map,
    -+			if (backfill_tags(&display, transport, transaction, tags_ref_map,
    ++			if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
      					  &fetch_head))
      				retcode = 1;
      		}
3:  a4fd935c40 ! 2:  34eedb882c fetch: move output format into `display_state`
    @@ builtin/fetch.c: static int s_update_ref(const char *action,
      {
      	int max, rlen, llen, len;
      
    -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
    +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
      
      	git_config_get_string_tmp("fetch.output", &format);
      	if (!strcasecmp(format, "full"))
     -		compact_format = 0;
    -+		display->compact_format = 0;
    ++		display_state->compact_format = 0;
      	else if (!strcasecmp(format, "compact"))
     -		compact_format = 1;
    -+		display->compact_format = 1;
    ++		display_state->compact_format = 1;
      	else
      		die(_("invalid value for '%s': '%s'"),
      		    "fetch.output", format);
    -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
    +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
      		    !strcmp(rm->name, "HEAD"))
      			continue;
      
     -		width = refcol_width(rm);
    -+		width = refcol_width(rm, display->compact_format);
    ++		width = refcol_width(rm, display_state->compact_format);
      
      		/*
      		 * Not precise calculation for compact mode because '*' can
    -@@ builtin/fetch.c: static void format_display(struct display_state *display,
    +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
      	width = (summary_width + strlen(summary) - gettext_width(summary));
      
      	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
     -	if (!compact_format)
    -+	if (!display->compact_format)
    - 		print_remote_to_local(display, display_buffer, remote, local);
    ++	if (!display_state->compact_format)
    + 		print_remote_to_local(display_state, display_buffer, remote, local);
      	else
    - 		print_compact(display, display_buffer, remote, local);
    + 		print_compact(display_state, display_buffer, remote, local);
4:  0998173b57 ! 3:  ec355b8b8d fetch: pass the full local reference name to `format_display`
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/fetch.c ##
    -@@ builtin/fetch.c: static void format_display(struct display_state *display,
    +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
      
      	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
    - 	if (!display->compact_format)
    --		print_remote_to_local(display, display_buffer, remote, local);
    -+		print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
    + 	if (!display_state->compact_format)
    +-		print_remote_to_local(display_state, display_buffer, remote, local);
    ++		print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
      	else
    --		print_compact(display, display_buffer, remote, local);
    -+		print_compact(display, display_buffer, remote, prettify_refname(local));
    +-		print_compact(display_state, display_buffer, remote, local);
    ++		print_compact(display_state, display_buffer, remote, prettify_refname(local));
      	if (error)
      		strbuf_addf(display_buffer, "  (%s)", error);
      }
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
    - 			    struct strbuf *display_buffer, int summary_width)
    + 			    struct strbuf *display, int summary_width)
      {
      	struct commit *current = NULL, *updated;
     -	const char *pretty_ref = prettify_refname(ref->name);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      	if (oideq(&ref->old_oid, &ref->new_oid)) {
      		if (verbosity > 0)
    - 			format_display(display, display_buffer, '=', _("[up to date]"), NULL,
    + 			format_display(display_state, display, '=', _("[up to date]"), NULL,
     -				       remote, pretty_ref, summary_width);
     +				       remote, ref->name, summary_width);
      		return 0;
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		 */
    - 		format_display(display, display_buffer, '!', _("[rejected]"),
    + 		format_display(display_state, display, '!', _("[rejected]"),
      			       _("can't fetch into checked-out branch"),
     -			       remote, pretty_ref, summary_width);
     +			       remote, ref->name, summary_width);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      			r = s_update_ref("updating tag", ref, transaction, 0);
    - 			format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
    + 			format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
      				       r ? _("unable to update local ref") : NULL,
     -				       remote, pretty_ref, summary_width);
     +				       remote, ref->name, summary_width);
      			return r;
      		} else {
    - 			format_display(display, display_buffer, '!', _("[rejected]"),
    + 			format_display(display_state, display, '!', _("[rejected]"),
      				       _("would clobber existing tag"),
     -				       remote, pretty_ref, summary_width);
     +				       remote, ref->name, summary_width);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      	}
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		r = s_update_ref(msg, ref, transaction, 0);
    - 		format_display(display, display_buffer, r ? '!' : '*', what,
    + 		format_display(display_state, display, r ? '!' : '*', what,
      			       r ? _("unable to update local ref") : NULL,
     -			       remote, pretty_ref, summary_width);
     +			       remote, ref->name, summary_width);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		r = s_update_ref("fast-forward", ref, transaction, 1);
    - 		format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
    + 		format_display(display_state, display, r ? '!' : ' ', quickref.buf,
      			       r ? _("unable to update local ref") : NULL,
     -			       remote, pretty_ref, summary_width);
     +			       remote, ref->name, summary_width);
    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      	} else if (force || ref->force) {
     @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
      		r = s_update_ref("forced-update", ref, transaction, 1);
    - 		format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
    + 		format_display(display_state, display, r ? '!' : '+', quickref.buf,
      			       r ? _("unable to update local ref") : _("forced update"),
     -			       remote, pretty_ref, summary_width);
     +			       remote, ref->name, summary_width);
      		strbuf_release(&quickref);
      		return r;
      	} else {
    - 		format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
    + 		format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
     -			       remote, pretty_ref, summary_width);
     +			       remote, ref->name, summary_width);
      		return 1;
      	}
      }
    -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
      				shown_url = 1;
      			}
    - 			format_display(display, &sb, '-', _("[deleted]"), NULL,
    + 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
     -				       _("(none)"), prettify_refname(ref->name),
     +				       _("(none)"), ref->name,
      				       summary_width);
5:  d45ec31eea ! 4:  e4c1ed4ad5 fetch: deduplicate handling of per-reference format
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    fetch: deduplicate handling of per-reference format
    +    fetch: centralize handling of per-reference format
     
    -    Both callsites that call `format_display()` and then print the result to
    -    standard error use the same formatting directive " %s\n" to print the
    -    reference to disk, thus duplicating a small part of the logic.
    +    The function `format_display()` is used to print a single reference
    +    update to a buffer which will then ultimately be printed by the caller.
    +    This architecture causes us to duplicate some logic across the different
    +    callsites of this function. This makes it hard to follow the code as
    +    some parts of the logic are located in one place, while other parts of
    +    the logic are located in a different place. Furthermore, by having the
    +    logic scattered around it becomes quite hard to implement a new output
    +    format for the reference updates.
     
    -    Refactor the code to handle this in `format_display()` itself. This
    -    paves the way for handling the printing logic in that function
    -    completely.
    +    We can make the logic a whole lot easier to understand by making the
    +    `format_display()` function self-contained so that it handles formatting
    +    and printing of the references. This will eventually allow us to easily
    +    implement a completely different output format, but also opens the door
    +    to conditionally print to either stdout or stderr depending on the
    +    output format.
    +
    +    As a first step towards that goal we move the formatting directive used
    +    by both callers to print a single reference update into this function.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/fetch.c ##
    -@@ builtin/fetch.c: static void format_display(struct display_state *display,
    +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
      
      	width = (summary_width + strlen(summary) - gettext_width(summary));
      
     -	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
     +	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
    - 	if (!display->compact_format)
    - 		print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
    + 	if (!display_state->compact_format)
    + 		print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
      	else
    - 		print_compact(display, display_buffer, remote, prettify_refname(local));
    + 		print_compact(display_state, display_buffer, remote, prettify_refname(local));
      	if (error)
      		strbuf_addf(display_buffer, "  (%s)", error);
     +	strbuf_addch(display_buffer, '\n');
      }
      
      static int update_local_ref(struct ref *ref,
    -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
      							url_len, url);
      					shown_url = 1;
      				}
    @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
      			}
      		}
      	}
    -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
    - 			format_display(display, &sb, '-', _("[deleted]"), NULL,
    +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
    + 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
      				       _("(none)"), ref->name,
      				       summary_width);
     -			fprintf(stderr, " %s\n",sb.buf);
6:  2ea3a4e308 ! 5:  98b799af71 fetch: deduplicate logic to print remote URL
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    fetch: deduplicate logic to print remote URL
    +    fetch: centralize logic to print remote URL
     
         When fetching from a remote, we not only print the actual references
         that have changed, but will also print the URL from which we have
    @@ Commit message
         can convert the global variable into a member of `display_state`. And
         second, we can deduplicate the logic to compute the anonymized URL.
     
    +    This also works as expected when fetching from multiple remotes, for
    +    example via a group of remotes, as we do this by forking a standalone
    +    git-fetch(1) process per remote that is to be fetched.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/fetch.c ##
    @@ builtin/fetch.c: static int refcol_width(const struct ref *ref, int compact_form
      	return rlen;
      }
      
    --static void display_state_init(struct display_state *display, struct ref *ref_map)
    -+static void display_state_init(struct display_state *display, struct ref *ref_map,
    +-static void display_state_init(struct display_state *display_state, struct ref *ref_map)
    ++static void display_state_init(struct display_state *display_state, struct ref *ref_map,
     +			       const char *raw_url)
      {
      	struct ref *rm;
      	const char *format = "full";
     +	int i;
      
    - 	memset(display, 0, sizeof(*display));
    + 	memset(display_state, 0, sizeof(*display_state));
      
     +	if (raw_url)
    -+		display->url = transport_anonymize_url(raw_url);
    ++		display_state->url = transport_anonymize_url(raw_url);
     +	else
    -+		display->url = xstrdup("foreign");
    ++		display_state->url = xstrdup("foreign");
     +
    -+	display->url_len = strlen(display->url);
    -+	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
    ++	display_state->url_len = strlen(display_state->url);
    ++	for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
     +		;
    -+	display->url_len = i + 1;
    -+	if (4 < i && !strncmp(".git", display->url + i - 3, 4))
    -+		display->url_len = i - 3;
    ++	display_state->url_len = i + 1;
    ++	if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))
    ++		display_state->url_len = i - 3;
     +
      	if (verbosity < 0)
      		return;
      
    -@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
    +@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
      	}
      }
      
    -+static void display_state_release(struct display_state *display)
    ++static void display_state_release(struct display_state *display_state)
     +{
    -+	free(display->url);
    ++	free(display_state->url);
     +}
     +
    - static void print_remote_to_local(struct display_state *display,
    + static void print_remote_to_local(struct display_state *display_state,
      				  struct strbuf *display_buffer,
      				  const char *remote, const char *local)
    -@@ builtin/fetch.c: static void format_display(struct display_state *display,
    +@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
      	if (verbosity < 0)
      		return;
      
    -+	if (!display->shown_url) {
    -+		strbuf_addf(display_buffer, _("From %.*s\n"), display->url_len, display->url);
    -+		display->shown_url = 1;
    ++	if (!display_state->shown_url) {
    ++		strbuf_addf(display_buffer, _("From %.*s\n"),
    ++			    display_state->url_len, display_state->url);
    ++		display_state->shown_url = 1;
     +	}
     +
      	width = (summary_width + strlen(summary) - gettext_width(summary));
    @@ builtin/fetch.c: static void format_display(struct display_state *display,
     @@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n"
         "to avoid this check\n");
      
    - static int store_updated_refs(struct display_state *display,
    + static int store_updated_refs(struct display_state *display_state,
     -			      const char *raw_url, const char *remote_name,
     +			      const char *remote_name,
      			      int connectivity_checked,
    @@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n
      		rm = ref_map;
      		if (check_connected(iterate_ref_map, &rm, &opt)) {
     -			rc = error(_("%s did not send all necessary objects\n"), url);
    -+			rc = error(_("%s did not send all necessary objects\n"), display->url);
    ++			rc = error(_("%s did not send all necessary objects\n"),
    ++				   display_state->url);
      			goto abort;
      		}
      	}
    -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
      				what = rm->name;
      			}
      
    @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
      			strbuf_reset(&note);
      			if (*what) {
      				if (*kind)
    -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
      
      			append_fetch_head(fetch_head, &rm->old_oid,
      					  rm->fetch_head_status,
     -					  note.buf, url, url_len);
    -+					  note.buf, display->url, display->url_len);
    ++					  note.buf, display_state->url,
    ++					  display_state->url_len);
      
      			strbuf_reset(&note);
      			if (ref) {
    -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
      					       *what ? what : "HEAD",
      					       "FETCH_HEAD", summary_width);
      			}
    @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
      		}
      	}
      
    -@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
      
       abort:
      	strbuf_release(&note);
    @@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
      	return rc;
      }
      
    -@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,
      	}
      
      	trace2_region_enter("fetch", "consume_refs", the_repository);
    --	ret = store_updated_refs(display, transport->url, transport->remote->name,
    -+	ret = store_updated_refs(display, transport->remote->name,
    +-	ret = store_updated_refs(display_state, transport->url, transport->remote->name,
    ++	ret = store_updated_refs(display_state, transport->remote->name,
      				 connectivity_checked, transaction, ref_map,
      				 fetch_head);
      	trace2_region_leave("fetch", "consume_refs", the_repository);
    -@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,
    - static int prune_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,
    + static int prune_refs(struct display_state *display_state,
      		      struct refspec *rs,
      		      struct ref_transaction *transaction,
     -		      struct ref *ref_map,
    @@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display
      	if (!dry_run) {
      		if (transaction) {
      			for (ref = stale_refs; ref; ref = ref->next) {
    -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
      
      		for (ref = stale_refs; ref; ref = ref->next) {
      			struct strbuf sb = STRBUF_INIT;
    @@ builtin/fetch.c: static int prune_refs(struct display_state *display,
     -				fprintf(stderr, _("From %.*s\n"), url_len, url);
     -				shown_url = 1;
     -			}
    - 			format_display(display, &sb, '-', _("[deleted]"), NULL,
    + 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
      				       _("(none)"), ref->name,
      				       summary_width);
    -@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
    +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
      
      cleanup:
      	strbuf_release(&err);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      {
      	struct ref_transaction *transaction = NULL;
      	struct ref *ref_map = NULL;
    --	struct display_state display;
    -+	struct display_state display = { 0 };
    +-	struct display_state display_state;
    ++	struct display_state display_state = { 0 };
      	int autotags = (transport->remote->fetch_tags == 1);
      	int retcode = 0;
      	const struct ref *remote_refs;
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      	if (retcode)
      		goto cleanup;
      
    --	display_state_init(&display, ref_map);
    -+	display_state_init(&display, ref_map, transport->url);
    +-	display_state_init(&display_state, ref_map);
    ++	display_state_init(&display_state, ref_map, transport->url);
      
      	if (atomic_fetch) {
      		transaction = ref_transaction_begin(&err);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		 * don't care whether --tags was specified.
      		 */
      		if (rs->nr) {
    --			retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
    -+			retcode = prune_refs(&display, rs, transaction, ref_map);
    +-			retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
    ++			retcode = prune_refs(&display_state, rs, transaction, ref_map);
      		} else {
    - 			retcode = prune_refs(&display, &transport->remote->fetch,
    + 			retcode = prune_refs(&display_state, &transport->remote->fetch,
     -					     transaction, ref_map,
     -					     transport->url);
     +					     transaction, ref_map);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		error("%s", err.buf);
      	}
      
    -+	display_state_release(&display);
    ++	display_state_release(&display_state);
      	close_fetch_head(&fetch_head);
      	strbuf_release(&err);
      	free_refs(ref_map);
7:  f67f9640a8 < -:  ---------- fetch: fix inconsistent summary width for pruned and updated refs
8:  9667301711 < -:  ---------- fetch: centralize printing of reference updates
-:  ---------- > 6:  fe7e2e85eb fetch: centralize printing of reference updates
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/6] fetch: move reference width calculation into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 14542 bytes --]

In order to print references in proper columns we need to calculate the
width of the reference column before starting to print the references.
This is done with the help of a global variable `refcol_width`.

Refactor the code to instead use a new structure `display_state` that
contains the computed width and plumb it through the stack as required.
This is only the first step towards de-globalizing the state required to
print references.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 111 ++++++++++++++++++++++++++++--------------------
 1 file changed, 65 insertions(+), 46 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a09606b472..391959ad3d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -47,6 +47,10 @@ enum {
 	TAGS_SET = 2
 };
 
+struct display_state {
+	int refcol_width;
+};
+
 static int fetch_prune_config = -1; /* unspecified */
 static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
@@ -741,16 +745,15 @@ static int s_update_ref(const char *action,
 	return ret;
 }
 
-static int refcol_width = 10;
 static int compact_format;
 
-static void adjust_refcol_width(const struct ref *ref)
+static int refcol_width(const struct ref *ref)
 {
 	int max, rlen, llen, len;
 
 	/* uptodate lines are only shown on high verbosity level */
 	if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
-		return;
+		return 0;
 
 	max    = term_columns();
 	rlen   = utf8_strwidth(prettify_refname(ref->name));
@@ -769,22 +772,18 @@ static void adjust_refcol_width(const struct ref *ref)
 	}
 	len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
 	if (len >= max)
-		return;
+		return 0;
 
-	/*
-	 * Not precise calculation for compact mode because '*' can
-	 * appear on the left hand side of '->' and shrink the column
-	 * back.
-	 */
-	if (refcol_width < rlen)
-		refcol_width = rlen;
+	return rlen;
 }
 
-static void prepare_format_display(struct ref *ref_map)
+static void display_state_init(struct display_state *display_state, struct ref *ref_map)
 {
 	struct ref *rm;
 	const char *format = "full";
 
+	memset(display_state, 0, sizeof(*display_state));
+
 	if (verbosity < 0)
 		return;
 
@@ -797,20 +796,32 @@ static void prepare_format_display(struct ref *ref_map)
 		die(_("invalid value for '%s': '%s'"),
 		    "fetch.output", format);
 
+	display_state->refcol_width = 10;
 	for (rm = ref_map; rm; rm = rm->next) {
+		int width;
+
 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
 		    !rm->peer_ref ||
 		    !strcmp(rm->name, "HEAD"))
 			continue;
 
-		adjust_refcol_width(rm);
+		width = refcol_width(rm);
+
+		/*
+		 * Not precise calculation for compact mode because '*' can
+		 * appear on the left hand side of '->' and shrink the column
+		 * back.
+		 */
+		if (display_state->refcol_width < width)
+			display_state->refcol_width = width;
 	}
 }
 
-static void print_remote_to_local(struct strbuf *display,
+static void print_remote_to_local(struct display_state *display_state,
+				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);
+	strbuf_addf(display_buffer, "%-*s -> %s", display_state->refcol_width, remote, local);
 }
 
 static int find_and_replace(struct strbuf *haystack,
@@ -840,14 +851,14 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct strbuf *display,
+static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;
 	struct strbuf l = STRBUF_INIT;
 
 	if (!strcmp(remote, local)) {
-		strbuf_addf(display, "%-*s -> *", refcol_width, remote);
+		strbuf_addf(display_buffer, "%-*s -> *", display_state->refcol_width, remote);
 		return;
 	}
 
@@ -856,13 +867,14 @@ static void print_compact(struct strbuf *display,
 
 	if (!find_and_replace(&r, local, "*"))
 		find_and_replace(&l, remote, "*");
-	print_remote_to_local(display, r.buf, l.buf);
+	print_remote_to_local(display_state, display_buffer, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct strbuf *display, char code,
+static void format_display(struct display_state *display_state,
+			   struct strbuf *display_buffer, char code,
 			   const char *summary, const char *error,
 			   const char *remote, const char *local,
 			   int summary_width)
@@ -874,17 +886,18 @@ static void format_display(struct strbuf *display, char code,
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display, "%c %-*s ", code, width, summary);
+	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
 	if (!compact_format)
-		print_remote_to_local(display, remote, local);
+		print_remote_to_local(display_state, display_buffer, remote, local);
 	else
-		print_compact(display, remote, local);
+		print_compact(display_state, display_buffer, remote, local);
 	if (error)
-		strbuf_addf(display, "  (%s)", error);
+		strbuf_addf(display_buffer, "  (%s)", error);
 }
 
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
+			    struct display_state *display_state,
 			    const char *remote, const struct ref *remote_ref,
 			    struct strbuf *display, int summary_width)
 {
@@ -897,7 +910,7 @@ static int update_local_ref(struct ref *ref,
 
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
-			format_display(display, '=', _("[up to date]"), NULL,
+			format_display(display_state, display, '=', _("[up to date]"), NULL,
 				       remote, pretty_ref, summary_width);
 		return 0;
 	}
@@ -909,7 +922,7 @@ static int update_local_ref(struct ref *ref,
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
-		format_display(display, '!', _("[rejected]"),
+		format_display(display_state, display, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
 			       remote, pretty_ref, summary_width);
 		return 1;
@@ -920,12 +933,13 @@ static int update_local_ref(struct ref *ref,
 		if (force || ref->force) {
 			int r;
 			r = s_update_ref("updating tag", ref, transaction, 0);
-			format_display(display, r ? '!' : 't', _("[tag update]"),
+			format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
 				       remote, pretty_ref, summary_width);
 			return r;
 		} else {
-			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+			format_display(display_state, display, '!', _("[rejected]"),
+				       _("would clobber existing tag"),
 				       remote, pretty_ref, summary_width);
 			return 1;
 		}
@@ -957,7 +971,7 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display, r ? '!' : '*', what,
+		format_display(display_state, display, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		return r;
@@ -979,7 +993,7 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "..");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("fast-forward", ref, transaction, 1);
-		format_display(display, r ? '!' : ' ', quickref.buf,
+		format_display(display_state, display, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
@@ -991,13 +1005,13 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "...");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("forced-update", ref, transaction, 1);
-		format_display(display, r ? '!' : '+', quickref.buf,
+		format_display(display_state, display, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
-		format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
+		format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1108,7 +1122,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n"
    "to avoid this check\n");
 
-static int store_updated_refs(const char *raw_url, const char *remote_name,
+static int store_updated_refs(struct display_state *display_state,
+			      const char *raw_url, const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head)
@@ -1139,8 +1154,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	prepare_format_display(ref_map);
-
 	/*
 	 * We do a pass for each fetch_head_status type in their enum order, so
 	 * merged entries are written before not-for-merge. That lets readers
@@ -1240,7 +1253,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 			strbuf_reset(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, transaction, what,
+				rc |= update_local_ref(ref, transaction, display_state, what,
 						       rm, &note, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
@@ -1249,7 +1262,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				 * would be written to FETCH_HEAD, if --dry-run
 				 * is set).
 				 */
-				format_display(&note, '*',
+				format_display(display_state, &note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
@@ -1328,7 +1341,8 @@ static int check_exist_and_connected(struct ref *ref_map)
 	return check_connected(iterate_ref_map, &rm, &opt);
 }
 
-static int fetch_and_consume_refs(struct transport *transport,
+static int fetch_and_consume_refs(struct display_state *display_state,
+				  struct transport *transport,
 				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
 				  struct fetch_head *fetch_head)
@@ -1352,7 +1366,7 @@ static int fetch_and_consume_refs(struct transport *transport,
 	}
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
-	ret = store_updated_refs(transport->url, transport->remote->name,
+	ret = store_updated_refs(display_state, transport->url, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
 				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
@@ -1362,7 +1376,8 @@ static int fetch_and_consume_refs(struct transport *transport,
 	return ret;
 }
 
-static int prune_refs(struct refspec *rs,
+static int prune_refs(struct display_state *display_state,
+		      struct refspec *rs,
 		      struct ref_transaction *transaction,
 		      struct ref *ref_map,
 		      const char *raw_url)
@@ -1416,7 +1431,7 @@ static int prune_refs(struct refspec *rs,
 				fprintf(stderr, _("From %.*s\n"), url_len, url);
 				shown_url = 1;
 			}
-			format_display(&sb, '-', _("[deleted]"), NULL,
+			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), prettify_refname(ref->name),
 				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);
@@ -1542,7 +1557,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	return transport;
 }
 
-static int backfill_tags(struct transport *transport,
+static int backfill_tags(struct display_state *display_state,
+			 struct transport *transport,
 			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
 			 struct fetch_head *fetch_head)
@@ -1566,7 +1582,7 @@ static int backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);
+	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1581,6 +1597,7 @@ static int do_fetch(struct transport *transport,
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
+	struct display_state display_state;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1662,6 +1679,8 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
+	display_state_init(&display_state, ref_map);
+
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
 		if (!transaction) {
@@ -1679,9 +1698,9 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(rs, transaction, ref_map, transport->url);
+			retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
 		} else {
-			retcode = prune_refs(&transport->remote->fetch,
+			retcode = prune_refs(&display_state, &transport->remote->fetch,
 					     transaction, ref_map,
 					     transport->url);
 		}
@@ -1689,7 +1708,7 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {
+	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1711,7 +1730,7 @@ static int do_fetch(struct transport *transport,
 			 * when `--atomic` is passed: in that case we'll abort
 			 * the transaction and don't commit anything.
 			 */
-			if (backfill_tags(transport, transaction, tags_ref_map,
+			if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
 					  &fetch_head))
 				retcode = 1;
 		}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/6] fetch: move output format into `display_state`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

The git-fetch(1) command supports printing references either in "full"
or "compact" format depending on the `fetch.ouput` config key. The
format that is to be used is tracked in a global variable.

De-globalize the variable by moving it into the `display_state`
structure.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 391959ad3d..6d6146b0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -49,6 +49,7 @@ enum {
 
 struct display_state {
 	int refcol_width;
+	int compact_format;
 };
 
 static int fetch_prune_config = -1; /* unspecified */
@@ -745,9 +746,7 @@ static int s_update_ref(const char *action,
 	return ret;
 }
 
-static int compact_format;
-
-static int refcol_width(const struct ref *ref)
+static int refcol_width(const struct ref *ref, int compact_format)
 {
 	int max, rlen, llen, len;
 
@@ -789,9 +788,9 @@ static void display_state_init(struct display_state *display_state, struct ref *
 
 	git_config_get_string_tmp("fetch.output", &format);
 	if (!strcasecmp(format, "full"))
-		compact_format = 0;
+		display_state->compact_format = 0;
 	else if (!strcasecmp(format, "compact"))
-		compact_format = 1;
+		display_state->compact_format = 1;
 	else
 		die(_("invalid value for '%s': '%s'"),
 		    "fetch.output", format);
@@ -805,7 +804,7 @@ static void display_state_init(struct display_state *display_state, struct ref *
 		    !strcmp(rm->name, "HEAD"))
 			continue;
 
-		width = refcol_width(rm);
+		width = refcol_width(rm, display_state->compact_format);
 
 		/*
 		 * Not precise calculation for compact mode because '*' can
@@ -887,7 +886,7 @@ static void format_display(struct display_state *display_state,
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
-	if (!compact_format)
+	if (!display_state->compact_format)
 		print_remote_to_local(display_state, display_buffer, remote, local);
 	else
 		print_compact(display_state, display_buffer, remote, local);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/6] fetch: pass the full local reference name to `format_display`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4952 bytes --]

Before printing the name of the local references that would be updated
by a fetch we first prettify the reference name. This is done at the
calling side so that `format_display()` never sees the full name of the
local reference. This restricts our ability to introduce new output
formats that might want to print the full reference name.

Right now, all callsites except one are prettifying the reference name
anyway. And the only callsite that doesn't passes `FETCH_HEAD` as the
hardcoded reference name to `format_display()`, which would never be
changed by a call to `prettify_refname()` anyway. So let's refactor the
code to pass in the full local reference name and then prettify it in
the formatting code.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6d6146b0f0..81ba3900cb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -887,9 +887,9 @@ static void format_display(struct display_state *display_state,
 
 	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
 	if (!display_state->compact_format)
-		print_remote_to_local(display_state, display_buffer, remote, local);
+		print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
 	else
-		print_compact(display_state, display_buffer, remote, local);
+		print_compact(display_state, display_buffer, remote, prettify_refname(local));
 	if (error)
 		strbuf_addf(display_buffer, "  (%s)", error);
 }
@@ -901,7 +901,6 @@ static int update_local_ref(struct ref *ref,
 			    struct strbuf *display, int summary_width)
 {
 	struct commit *current = NULL, *updated;
-	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
 	if (!repo_has_object_file(the_repository, &ref->new_oid))
@@ -910,7 +909,7 @@ static int update_local_ref(struct ref *ref,
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
 			format_display(display_state, display, '=', _("[up to date]"), NULL,
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 		return 0;
 	}
 
@@ -923,7 +922,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		format_display(display_state, display, '!', _("[rejected]"),
 			       _("can't fetch into checked-out branch"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return 1;
 	}
 
@@ -934,12 +933,12 @@ static int update_local_ref(struct ref *ref,
 			r = s_update_ref("updating tag", ref, transaction, 0);
 			format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
 				       r ? _("unable to update local ref") : NULL,
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 			return r;
 		} else {
 			format_display(display_state, display, '!', _("[rejected]"),
 				       _("would clobber existing tag"),
-				       remote, pretty_ref, summary_width);
+				       remote, ref->name, summary_width);
 			return 1;
 		}
 	}
@@ -972,7 +971,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, transaction, 0);
 		format_display(display_state, display, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return r;
 	}
 
@@ -994,7 +993,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, transaction, 1);
 		format_display(display_state, display, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1006,12 +1005,12 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, transaction, 1);
 		format_display(display_state, display, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
 		format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, pretty_ref, summary_width);
+			       remote, ref->name, summary_width);
 		return 1;
 	}
 }
@@ -1431,7 +1430,7 @@ static int prune_refs(struct display_state *display_state,
 				shown_url = 1;
 			}
 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), prettify_refname(ref->name),
+				       _("(none)"), ref->name,
 				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);
 			strbuf_release(&sb);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/6] fetch: centralize handling of per-reference format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

The function `format_display()` is used to print a single reference
update to a buffer which will then ultimately be printed by the caller.
This architecture causes us to duplicate some logic across the different
callsites of this function. This makes it hard to follow the code as
some parts of the logic are located in one place, while other parts of
the logic are located in a different place. Furthermore, by having the
logic scattered around it becomes quite hard to implement a new output
format for the reference updates.

We can make the logic a whole lot easier to understand by making the
`format_display()` function self-contained so that it handles formatting
and printing of the references. This will eventually allow us to easily
implement a completely different output format, but also opens the door
to conditionally print to either stdout or stderr depending on the
output format.

As a first step towards that goal we move the formatting directive used
by both callers to print a single reference update into this function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 81ba3900cb..a66428dfd8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -885,13 +885,14 @@ static void format_display(struct display_state *display_state,
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
+	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
 	if (!display_state->compact_format)
 		print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
 	else
 		print_compact(display_state, display_buffer, remote, prettify_refname(local));
 	if (error)
 		strbuf_addf(display_buffer, "  (%s)", error);
+	strbuf_addch(display_buffer, '\n');
 }
 
 static int update_local_ref(struct ref *ref,
@@ -1271,7 +1272,7 @@ static int store_updated_refs(struct display_state *display_state,
 							url_len, url);
 					shown_url = 1;
 				}
-				fprintf(stderr, " %s\n", note.buf);
+				fputs(note.buf, stderr);
 			}
 		}
 	}
@@ -1432,7 +1433,7 @@ static int prune_refs(struct display_state *display_state,
 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), ref->name,
 				       summary_width);
-			fprintf(stderr, " %s\n",sb.buf);
+			fputs(sb.buf, stderr);
 			strbuf_release(&sb);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/6] fetch: centralize logic to print remote URL

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 9458 bytes --]

When fetching from a remote, we not only print the actual references
that have changed, but will also print the URL from which we have
fetched them to standard output. The logic to handle this is duplicated
across two different callsites with some non-trivial logic to compute
the anonymized URL. Furthermore, we're using global state to track
whether we have already shown the URL to the user or not.

Refactor the code by moving it into `format_display()`. Like this, we
can convert the global variable into a member of `display_state`. And
second, we can deduplicate the logic to compute the anonymized URL.

This also works as expected when fetching from multiple remotes, for
example via a group of remotes, as we do this by forking a standalone
git-fetch(1) process per remote that is to be fetched.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 99 ++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 55 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a66428dfd8..1e3599cb74 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -50,6 +50,9 @@ enum {
 struct display_state {
 	int refcol_width;
 	int compact_format;
+
+	char *url;
+	int url_len, shown_url;
 };
 
 static int fetch_prune_config = -1; /* unspecified */
@@ -84,7 +87,6 @@ static const char *submodule_prefix = "";
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
-static int shown_url = 0;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
@@ -776,13 +778,27 @@ static int refcol_width(const struct ref *ref, int compact_format)
 	return rlen;
 }
 
-static void display_state_init(struct display_state *display_state, struct ref *ref_map)
+static void display_state_init(struct display_state *display_state, struct ref *ref_map,
+			       const char *raw_url)
 {
 	struct ref *rm;
 	const char *format = "full";
+	int i;
 
 	memset(display_state, 0, sizeof(*display_state));
 
+	if (raw_url)
+		display_state->url = transport_anonymize_url(raw_url);
+	else
+		display_state->url = xstrdup("foreign");
+
+	display_state->url_len = strlen(display_state->url);
+	for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
+		;
+	display_state->url_len = i + 1;
+	if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))
+		display_state->url_len = i - 3;
+
 	if (verbosity < 0)
 		return;
 
@@ -816,6 +832,11 @@ static void display_state_init(struct display_state *display_state, struct ref *
 	}
 }
 
+static void display_state_release(struct display_state *display_state)
+{
+	free(display_state->url);
+}
+
 static void print_remote_to_local(struct display_state *display_state,
 				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
@@ -883,6 +904,12 @@ static void format_display(struct display_state *display_state,
 	if (verbosity < 0)
 		return;
 
+	if (!display_state->shown_url) {
+		strbuf_addf(display_buffer, _("From %.*s\n"),
+			    display_state->url_len, display_state->url);
+		display_state->shown_url = 1;
+	}
+
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
@@ -1122,33 +1149,28 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "to avoid this check\n");
 
 static int store_updated_refs(struct display_state *display_state,
-			      const char *raw_url, const char *remote_name,
+			      const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head)
 {
-	int url_len, i, rc = 0;
+	int rc = 0;
 	struct strbuf note = STRBUF_INIT;
 	const char *what, *kind;
 	struct ref *rm;
-	char *url;
 	int want_status;
 	int summary_width = 0;
 
 	if (verbosity >= 0)
 		summary_width = transport_summary_width(ref_map);
 
-	if (raw_url)
-		url = transport_anonymize_url(raw_url);
-	else
-		url = xstrdup("foreign");
-
 	if (!connectivity_checked) {
 		struct check_connected_options opt = CHECK_CONNECTED_INIT;
 
 		rm = ref_map;
 		if (check_connected(iterate_ref_map, &rm, &opt)) {
-			rc = error(_("%s did not send all necessary objects\n"), url);
+			rc = error(_("%s did not send all necessary objects\n"),
+				   display_state->url);
 			goto abort;
 		}
 	}
@@ -1232,13 +1254,6 @@ static int store_updated_refs(struct display_state *display_state,
 				what = rm->name;
 			}
 
-			url_len = strlen(url);
-			for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-				;
-			url_len = i + 1;
-			if (4 < i && !strncmp(".git", url + i - 3, 4))
-				url_len = i - 3;
-
 			strbuf_reset(&note);
 			if (*what) {
 				if (*kind)
@@ -1248,7 +1263,8 @@ static int store_updated_refs(struct display_state *display_state,
 
 			append_fetch_head(fetch_head, &rm->old_oid,
 					  rm->fetch_head_status,
-					  note.buf, url, url_len);
+					  note.buf, display_state->url,
+					  display_state->url_len);
 
 			strbuf_reset(&note);
 			if (ref) {
@@ -1266,14 +1282,8 @@ static int store_updated_refs(struct display_state *display_state,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
 			}
-			if (note.len) {
-				if (!shown_url) {
-					fprintf(stderr, _("From %.*s\n"),
-							url_len, url);
-					shown_url = 1;
-				}
+			if (note.len)
 				fputs(note.buf, stderr);
-			}
 		}
 	}
 
@@ -1293,7 +1303,6 @@ static int store_updated_refs(struct display_state *display_state,
 
  abort:
 	strbuf_release(&note);
-	free(url);
 	return rc;
 }
 
@@ -1365,7 +1374,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,
 	}
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
-	ret = store_updated_refs(display_state, transport->url, transport->remote->name,
+	ret = store_updated_refs(display_state, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
 				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
@@ -1378,30 +1387,15 @@ static int fetch_and_consume_refs(struct display_state *display_state,
 static int prune_refs(struct display_state *display_state,
 		      struct refspec *rs,
 		      struct ref_transaction *transaction,
-		      struct ref *ref_map,
-		      const char *raw_url)
+		      struct ref *ref_map)
 {
-	int url_len, i, result = 0;
+	int result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
 	struct strbuf err = STRBUF_INIT;
-	char *url;
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)")
 		: _("   (%s has become dangling)");
 
-	if (raw_url)
-		url = transport_anonymize_url(raw_url);
-	else
-		url = xstrdup("foreign");
-
-	url_len = strlen(url);
-	for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-		;
-
-	url_len = i + 1;
-	if (4 < i && !strncmp(".git", url + i - 3, 4))
-		url_len = i - 3;
-
 	if (!dry_run) {
 		if (transaction) {
 			for (ref = stale_refs; ref; ref = ref->next) {
@@ -1426,10 +1420,6 @@ static int prune_refs(struct display_state *display_state,
 
 		for (ref = stale_refs; ref; ref = ref->next) {
 			struct strbuf sb = STRBUF_INIT;
-			if (!shown_url) {
-				fprintf(stderr, _("From %.*s\n"), url_len, url);
-				shown_url = 1;
-			}
 			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), ref->name,
 				       summary_width);
@@ -1441,7 +1431,6 @@ static int prune_refs(struct display_state *display_state,
 
 cleanup:
 	strbuf_release(&err);
-	free(url);
 	free_refs(stale_refs);
 	return result;
 }
@@ -1596,7 +1585,7 @@ static int do_fetch(struct transport *transport,
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
-	struct display_state display_state;
+	struct display_state display_state = { 0 };
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1678,7 +1667,7 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	display_state_init(&display_state, ref_map);
+	display_state_init(&display_state, ref_map, transport->url);
 
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
@@ -1697,11 +1686,10 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
+			retcode = prune_refs(&display_state, rs, transaction, ref_map);
 		} else {
 			retcode = prune_refs(&display_state, &transport->remote->fetch,
-					     transaction, ref_map,
-					     transport->url);
+					     transaction, ref_map);
 		}
 		if (retcode != 0)
 			retcode = 1;
@@ -1812,6 +1800,7 @@ static int do_fetch(struct transport *transport,
 		error("%s", err.buf);
 	}
 
+	display_state_release(&display_state);
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
 	free_refs(ref_map);
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/6] fetch: centralize printing of reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 10555 bytes --]

In order to print updated references during a fetch, the two different
call sites that do this will first call `format_display()` followed by a
call to `fputs()`. This is needlessly roundabout now that we have the
`display_state` structure that encapsulates all of the printing logic
for references.

Move displaying the reference updates into `format_display()` and rename
it to `display_ref_update()` to better match its new purpose, which
finalizes the conversion to make both the formatting and printing logic
of reference updates self-contained. This will make it easier to add new
output formats and printing to a different file descriptor than stderr.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 108 ++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 53 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1e3599cb74..c202c18fb4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -48,6 +48,8 @@ enum {
 };
 
 struct display_state {
+	struct strbuf buf;
+
 	int refcol_width;
 	int compact_format;
 
@@ -787,6 +789,8 @@ static void display_state_init(struct display_state *display_state, struct ref *
 
 	memset(display_state, 0, sizeof(*display_state));
 
+	strbuf_init(&display_state->buf, 0);
+
 	if (raw_url)
 		display_state->url = transport_anonymize_url(raw_url);
 	else
@@ -834,14 +838,15 @@ static void display_state_init(struct display_state *display_state, struct ref *
 
 static void display_state_release(struct display_state *display_state)
 {
+	strbuf_release(&display_state->buf);
 	free(display_state->url);
 }
 
 static void print_remote_to_local(struct display_state *display_state,
-				  struct strbuf *display_buffer,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display_buffer, "%-*s -> %s", display_state->refcol_width, remote, local);
+	strbuf_addf(&display_state->buf, "%-*s -> %s",
+		    display_state->refcol_width, remote, local);
 }
 
 static int find_and_replace(struct strbuf *haystack,
@@ -871,14 +876,14 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,
+static void print_compact(struct display_state *display_state,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;
 	struct strbuf l = STRBUF_INIT;
 
 	if (!strcmp(remote, local)) {
-		strbuf_addf(display_buffer, "%-*s -> *", display_state->refcol_width, remote);
+		strbuf_addf(&display_state->buf, "%-*s -> *", display_state->refcol_width, remote);
 		return;
 	}
 
@@ -887,46 +892,49 @@ static void print_compact(struct display_state *display_state, struct strbuf *di
 
 	if (!find_and_replace(&r, local, "*"))
 		find_and_replace(&l, remote, "*");
-	print_remote_to_local(display_state, display_buffer, r.buf, l.buf);
+	print_remote_to_local(display_state, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct display_state *display_state,
-			   struct strbuf *display_buffer, char code,
-			   const char *summary, const char *error,
-			   const char *remote, const char *local,
-			   int summary_width)
+static void display_ref_update(struct display_state *display_state, char code,
+			       const char *summary, const char *error,
+			       const char *remote, const char *local,
+			       int summary_width)
 {
 	int width;
 
 	if (verbosity < 0)
 		return;
 
+	strbuf_reset(&display_state->buf);
+
 	if (!display_state->shown_url) {
-		strbuf_addf(display_buffer, _("From %.*s\n"),
+		strbuf_addf(&display_state->buf, _("From %.*s\n"),
 			    display_state->url_len, display_state->url);
 		display_state->shown_url = 1;
 	}
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
 
-	strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
+	strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary);
 	if (!display_state->compact_format)
-		print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
+		print_remote_to_local(display_state, remote, prettify_refname(local));
 	else
-		print_compact(display_state, display_buffer, remote, prettify_refname(local));
+		print_compact(display_state, remote, prettify_refname(local));
 	if (error)
-		strbuf_addf(display_buffer, "  (%s)", error);
-	strbuf_addch(display_buffer, '\n');
+		strbuf_addf(&display_state->buf, "  (%s)", error);
+	strbuf_addch(&display_state->buf, '\n');
+
+	fputs(display_state->buf.buf, stderr);
 }
 
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    struct display_state *display_state,
 			    const char *remote, const struct ref *remote_ref,
-			    struct strbuf *display, int summary_width)
+			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
 	int fast_forward = 0;
@@ -936,8 +944,8 @@ static int update_local_ref(struct ref *ref,
 
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
-			format_display(display_state, display, '=', _("[up to date]"), NULL,
-				       remote, ref->name, summary_width);
+			display_ref_update(display_state, '=', _("[up to date]"), NULL,
+					   remote, ref->name, summary_width);
 		return 0;
 	}
 
@@ -948,9 +956,9 @@ static int update_local_ref(struct ref *ref,
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
-		format_display(display_state, display, '!', _("[rejected]"),
-			       _("can't fetch into checked-out branch"),
-			       remote, ref->name, summary_width);
+		display_ref_update(display_state, '!', _("[rejected]"),
+				   _("can't fetch into checked-out branch"),
+				   remote, ref->name, summary_width);
 		return 1;
 	}
 
@@ -959,14 +967,14 @@ static int update_local_ref(struct ref *ref,
 		if (force || ref->force) {
 			int r;
 			r = s_update_ref("updating tag", ref, transaction, 0);
-			format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
-				       r ? _("unable to update local ref") : NULL,
-				       remote, ref->name, summary_width);
+			display_ref_update(display_state, r ? '!' : 't', _("[tag update]"),
+					   r ? _("unable to update local ref") : NULL,
+					   remote, ref->name, summary_width);
 			return r;
 		} else {
-			format_display(display_state, display, '!', _("[rejected]"),
-				       _("would clobber existing tag"),
-				       remote, ref->name, summary_width);
+			display_ref_update(display_state, '!', _("[rejected]"),
+					   _("would clobber existing tag"),
+					   remote, ref->name, summary_width);
 			return 1;
 		}
 	}
@@ -997,9 +1005,9 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display_state, display, r ? '!' : '*', what,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name, summary_width);
+		display_ref_update(display_state, r ? '!' : '*', what,
+				   r ? _("unable to update local ref") : NULL,
+				   remote, ref->name, summary_width);
 		return r;
 	}
 
@@ -1019,9 +1027,9 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "..");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("fast-forward", ref, transaction, 1);
-		format_display(display_state, display, r ? '!' : ' ', quickref.buf,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, ref->name, summary_width);
+		display_ref_update(display_state, r ? '!' : ' ', quickref.buf,
+				   r ? _("unable to update local ref") : NULL,
+				   remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1031,14 +1039,14 @@ static int update_local_ref(struct ref *ref,
 		strbuf_addstr(&quickref, "...");
 		strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 		r = s_update_ref("forced-update", ref, transaction, 1);
-		format_display(display_state, display, r ? '!' : '+', quickref.buf,
-			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, ref->name, summary_width);
+		display_ref_update(display_state, r ? '!' : '+', quickref.buf,
+				   r ? _("unable to update local ref") : _("forced update"),
+				   remote, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
-		format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, ref->name, summary_width);
+		display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"),
+				   remote, ref->name, summary_width);
 		return 1;
 	}
 }
@@ -1266,10 +1274,9 @@ static int store_updated_refs(struct display_state *display_state,
 					  note.buf, display_state->url,
 					  display_state->url_len);
 
-			strbuf_reset(&note);
 			if (ref) {
 				rc |= update_local_ref(ref, transaction, display_state, what,
-						       rm, &note, summary_width);
+						       rm, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1277,13 +1284,11 @@ static int store_updated_refs(struct display_state *display_state,
 				 * would be written to FETCH_HEAD, if --dry-run
 				 * is set).
 				 */
-				format_display(display_state, &note, '*',
-					       *kind ? kind : "branch", NULL,
-					       *what ? what : "HEAD",
-					       "FETCH_HEAD", summary_width);
+				display_ref_update(display_state, '*',
+						   *kind ? kind : "branch", NULL,
+						   *what ? what : "HEAD",
+						   "FETCH_HEAD", summary_width);
 			}
-			if (note.len)
-				fputs(note.buf, stderr);
 		}
 	}
 
@@ -1419,12 +1424,9 @@ static int prune_refs(struct display_state *display_state,
 		int summary_width = transport_summary_width(stale_refs);
 
 		for (ref = stale_refs; ref; ref = ref->next) {
-			struct strbuf sb = STRBUF_INIT;
-			format_display(display_state, &sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), ref->name,
-				       summary_width);
-			fputs(sb.buf, stderr);
-			strbuf_release(&sb);
+			display_ref_update(display_state, '-', _("[deleted]"), NULL,
+					   _("(none)"), ref->name,
+					   summary_width);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
 	}
-- 
2.40.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] fetch: centralize printing of reference updates

[Jonathan Tan]

Patrick Steinhardt <ps@pks.im> writes:
> In order to print updated references during a fetch, the two different
> call sites that do this will first call `format_display()` followed by a
> call to `fputs()`. This is needlessly roundabout now that we have the
> `display_state` structure that encapsulates all of the printing logic
> for references.
> 
> Move displaying the reference updates into `format_display()` and rename
> it to `display_ref_update()` to better match its new purpose, which
> finalizes the conversion to make both the formatting and printing logic
> of reference updates self-contained. This will make it easier to add new
> output formats and printing to a different file descriptor than stderr.

Thanks. Patches 1-5 look good to me. As for this patch, I'm still not
convinced (I thought that the new mode would still print to stderr), but
if other reviewers are OK with it then that's fine. Alternatively, patch
6 could go with the next set of patches that implement the new mode of
printing ref updates.
 

Re: [PATCH v2 6/6] fetch: centralize printing of reference updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Mon, Mar 20, 2023 at 03:57:02PM -0700, Jonathan Tan wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > In order to print updated references during a fetch, the two different
> > call sites that do this will first call `format_display()` followed by a
> > call to `fputs()`. This is needlessly roundabout now that we have the
> > `display_state` structure that encapsulates all of the printing logic
> > for references.
> > 
> > Move displaying the reference updates into `format_display()` and rename
> > it to `display_ref_update()` to better match its new purpose, which
> > finalizes the conversion to make both the formatting and printing logic
> > of reference updates self-contained. This will make it easier to add new
> > output formats and printing to a different file descriptor than stderr.
> 
> Thanks. Patches 1-5 look good to me. As for this patch, I'm still not
> convinced (I thought that the new mode would still print to stderr),

The reason why I decided against printing to stderr is that it's already
used by other things. The progress meter is one of these, runtime errors
are another one. I also think it's weird to have a parseable format that
should be parsed via stderr.

Anyway, let's discuss this once I'm posting the second patch series to
the mailing list.

> but
> if other reviewers are OK with it then that's fine. Alternatively, patch
> 6 could go with the next set of patches that implement the new mode of
> printing ref updates.

I'd be fine either way. Thanks for your review!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] fetch: centralize printing of reference updates

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Thanks. Patches 1-5 look good to me. As for this patch, I'm still not
> convinced (I thought that the new mode would still print to stderr), but
> if other reviewers are OK with it then that's fine. Alternatively, patch
> 6 could go with the next set of patches that implement the new mode of
> printing ref updates.

When further changes to the display_ref_update() starts doing
something iffy, we may still want to block them at that time, but
unless I am grossly misreading this patch, I do not think this one
changes the behaviour to start showing the ref update notification
to anywhere other than the standard error stream at this stage, and
the changes in this step can be evaluated for its "clean-up" value
alone.  

So I am inclined to say that we should take it as-is.

Thanks.