Merge branch 'ps/fetch-ref-update-reporting'

Clean-up of the code path that reports what "git fetch" did to each
ref.

* ps/fetch-ref-update-reporting:
  fetch: centralize printing of reference updates
  fetch: centralize logic to print remote URL
  fetch: centralize handling of per-reference format
  fetch: pass the full local reference name to `format_display`
  fetch: move output format into `display_state`
  fetch: move reference width calculation into `display_state`

1 files changed, 138 insertions, 129 deletions

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7221e57f35..c310d89878 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -48,6 +48,16 @@ enum {
 	TAGS_SET = 2
 };
 
+struct display_state {
+	struct strbuf buf;
+
+	int refcol_width;
+	int compact_format;
+
+	char *url;
+	int url_len, shown_url;
+};
+
 static int fetch_prune_config = -1; /* unspecified */
 static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
@@ -80,7 +90,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;
@@ -742,16 +751,13 @@ out:
 	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 compact_format)
 {
 	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));
@@ -770,48 +776,78 @@ 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,
+			       const char *raw_url)
 {
 	struct ref *rm;
 	const char *format = "full";
+	int i;
+
+	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
+		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;
 
 	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);
 
+	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, display_state->compact_format);
+
+		/*
+		 * 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 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,
 				  const char *remote, const char *local)
 {
-	strbuf_addf(display, "%-*s -> %s", 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,
@@ -841,14 +877,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,
 			  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_state->buf, "%-*s -> *", display_state->refcol_width, remote);
 		return;
 	}
 
@@ -857,40 +893,51 @@ 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, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct strbuf *display, 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_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, "%c %-*s ", code, width, summary);
-	if (!compact_format)
-		print_remote_to_local(display, remote, local);
+	strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary);
+	if (!display_state->compact_format)
+		print_remote_to_local(display_state, remote, prettify_refname(local));
 	else
-		print_compact(display, remote, local);
+		print_compact(display_state, remote, prettify_refname(local));
 	if (error)
-		strbuf_addf(display, "  (%s)", error);
+		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;
-	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
 	if (!repo_has_object_file(the_repository, &ref->new_oid))
@@ -898,8 +945,8 @@ 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,
-				       remote, pretty_ref, summary_width);
+			display_ref_update(display_state, '=', _("[up to date]"), NULL,
+					   remote, ref->name, summary_width);
 		return 0;
 	}
 
@@ -910,9 +957,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, '!', _("[rejected]"),
-			       _("can't fetch into checked-out branch"),
-			       remote, pretty_ref, summary_width);
+		display_ref_update(display_state, '!', _("[rejected]"),
+				   _("can't fetch into checked-out branch"),
+				   remote, ref->name, summary_width);
 		return 1;
 	}
 
@@ -921,13 +968,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, r ? '!' : 't', _("[tag update]"),
-				       r ? _("unable to update local ref") : NULL,
-				       remote, pretty_ref, 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, '!', _("[rejected]"), _("would clobber existing tag"),
-				       remote, pretty_ref, summary_width);
+			display_ref_update(display_state, '!', _("[rejected]"),
+					   _("would clobber existing tag"),
+					   remote, ref->name, summary_width);
 			return 1;
 		}
 	}
@@ -958,9 +1006,9 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display, r ? '!' : '*', what,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, summary_width);
+		display_ref_update(display_state, r ? '!' : '*', what,
+				   r ? _("unable to update local ref") : NULL,
+				   remote, ref->name, summary_width);
 		return r;
 	}
 
@@ -980,9 +1028,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, r ? '!' : ' ', quickref.buf,
-			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref, 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) {
@@ -992,14 +1040,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, r ? '!' : '+', quickref.buf,
-			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, pretty_ref, 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, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, pretty_ref, summary_width);
+		display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"),
+				   remote, ref->name, summary_width);
 		return 1;
 	}
 }
@@ -1109,40 +1157,34 @@ 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 *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;
 
 		opt.exclude_hidden_refs_section = "fetch";
 		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;
 		}
 	}
 
-	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
@@ -1222,13 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				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)
@@ -1238,12 +1273,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 			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) {
-				rc |= update_local_ref(ref, transaction, what,
-						       rm, &note, summary_width);
+				rc |= update_local_ref(ref, transaction, display_state, what,
+						       rm, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1251,18 +1286,10 @@ 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, '*',
-					       *kind ? kind : "branch", NULL,
-					       *what ? what : "HEAD",
-					       "FETCH_HEAD", summary_width);
-			}
-			if (note.len) {
-				if (!shown_url) {
-					fprintf(stderr, _("From %.*s\n"),
-							url_len, url);
-					shown_url = 1;
-				}
-				fprintf(stderr, " %s\n", note.buf);
+				display_ref_update(display_state, '*',
+						   *kind ? kind : "branch", NULL,
+						   *what ? what : "HEAD",
+						   "FETCH_HEAD", summary_width);
 			}
 		}
 	}
@@ -1283,7 +1310,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
  abort:
 	strbuf_release(&note);
-	free(url);
 	return rc;
 }
 
@@ -1331,7 +1357,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)
@@ -1355,7 +1382,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->remote->name,
 				 connectivity_checked, transaction, ref_map,
 				 fetch_head);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
@@ -1365,32 +1392,18 @@ out:
 	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)
+		      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) {
@@ -1414,23 +1427,15 @@ static int prune_refs(struct refspec *rs,
 		int summary_width = transport_summary_width(stale_refs);
 
 		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(&sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), prettify_refname(ref->name),
-				       summary_width);
-			fprintf(stderr, " %s\n",sb.buf);
-			strbuf_release(&sb);
+			display_ref_update(display_state, '-', _("[deleted]"), NULL,
+					   _("(none)"), ref->name,
+					   summary_width);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
 	}
 
 cleanup:
 	strbuf_release(&err);
-	free(url);
 	free_refs(stale_refs);
 	return result;
 }
@@ -1545,7 +1550,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)
@@ -1569,7 +1575,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);
@@ -1584,6 +1590,7 @@ static int do_fetch(struct transport *transport,
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
+	struct display_state display_state = { 0 };
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1665,6 +1672,8 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
+	display_state_init(&display_state, ref_map, transport->url);
+
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
 		if (!transaction) {
@@ -1682,17 +1691,16 @@ 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);
 		} else {
-			retcode = prune_refs(&transport->remote->fetch,
-					     transaction, ref_map,
-					     transport->url);
+			retcode = prune_refs(&display_state, &transport->remote->fetch,
+					     transaction, ref_map);
 		}
 		if (retcode != 0)
 			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;
 	}
@@ -1714,7 +1722,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;
 		}
@@ -1797,6 +1805,7 @@ cleanup:
 		error("%s", err.buf);
 	}
 
+	display_state_release(&display_state);
 	close_fetch_head(&fetch_head);
 	strbuf_release(&err);
 	free_refs(ref_map);