git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: [PATCH v2 0/6] fetch: refactor code that prints reference updates
Date: Mon, 20 Mar 2023 13:35:16 +0100	[thread overview]
Message-ID: <cover.1679315383.git.ps@pks.im> (raw)
In-Reply-To: <cover.1678878623.git.ps@pks.im>

[-- 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 --]

  parent reply	other threads:[~2023-03-20 12:35 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-15 11:21 [PATCH 0/8] fetch: refactor code that prints reference updates Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 1/8] fetch: rename `display` buffer to avoid name conflict Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 2/8] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-15 20:59   ` Junio C Hamano
2023-03-16 15:05     ` Patrick Steinhardt
2023-03-16 16:18       ` Junio C Hamano
2023-03-17 10:03         ` Patrick Steinhardt
2023-03-16 16:19       ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 3/8] fetch: move output format " Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 4/8] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-15 22:18   ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 5/8] fetch: deduplicate handling of per-reference format Patrick Steinhardt
2023-03-15 22:45   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt
2023-03-16 16:50       ` Junio C Hamano
2023-03-17  9:51         ` Patrick Steinhardt
2023-03-17 15:41           ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 6/8] fetch: deduplicate logic to print remote URL Patrick Steinhardt
2023-03-15 23:02   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs Patrick Steinhardt
2023-03-15 23:12   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt
2023-03-16 16:30       ` Junio C Hamano
2023-03-17  9:55         ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 8/8] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-17 20:24 ` [PATCH 0/8] fetch: refactor code that prints " Jonathan Tan
2023-03-20  6:57   ` Patrick Steinhardt
2023-03-20 12:26   ` Patrick Steinhardt
2023-03-20 12:35 ` Patrick Steinhardt [this message]
2023-03-20 12:35   ` [PATCH v2 1/6] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 2/6] fetch: move output format " Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 3/6] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 4/6] fetch: centralize handling of per-reference format Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 5/6] fetch: centralize logic to print remote URL Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 6/6] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-20 22:57     ` Jonathan Tan
2023-03-22  9:04       ` Patrick Steinhardt
2023-03-29 18:45       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1679315383.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).