[PATCH] fetch: skip formatting updated refs with `--quiet`

Subsystems: the rest

STALE1804d

3 messages, 2 authors, 2021-08-30 · open the first message on its own page

[PATCH] fetch: skip formatting updated refs with `--quiet`

From: Patrick Steinhardt <hidden>
Date: 2021-08-25 15:45:14

When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.

While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.

Fix this issue by completely skipping both preparation of the format and
formatting data for display in case the user passes `--quiet`, improving
performance especially with many refs. The following benchmark shows a
nice speedup for a quiet mirror-fetch in a repository with 2.3M refs:

    Benchmark #1: HEAD~: git-fetch
      Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]
      Range (min … max):   26.692 s … 27.068 s    5 runs

    Benchmark #2: HEAD: git-fetch
      Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]
      Range (min … max):   25.070 s … 25.314 s    5 runs

    Summary
      'HEAD: git-fetch' ran
        1.07 ± 0.01 times faster than 'HEAD~: git-fetch'

Signed-off-by: Patrick Steinhardt <redacted>
---
 builtin/fetch.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbd..d064b66512 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -757,6 +757,9 @@ static void prepare_format_display(struct ref *ref_map)
 		die(_("configuration fetch.output contains invalid value %s"),
 		    format);
 
+	if (verbosity < 0)
+		return;
+
 	for (rm = ref_map; rm; rm = rm->next) {
 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
 		    !rm->peer_ref ||

@@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,
 			   const char *remote, const char *local,
 			   int summary_width)
 {
-	int width = (summary_width + strlen(summary) - gettext_width(summary));
+	int width;
+
+	if (verbosity < 0)
+		return;
+
+	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display, "%c %-*s ", code, width, summary);
 	if (!compact_format)

-- 
2.33.0

Re: [PATCH] fetch: skip formatting updated refs with `--quiet`

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-08-25 17:04:12

On Wed, Aug 25 2021, Patrick Steinhardt wrote:

quoted hunk

[[PGP Signed Part:Undecided]]
When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.

While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.

Fix this issue by completely skipping both preparation of the format and
formatting data for display in case the user passes `--quiet`, improving
performance especially with many refs. The following benchmark shows a
nice speedup for a quiet mirror-fetch in a repository with 2.3M refs:

    Benchmark #1: HEAD~: git-fetch
      Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]
      Range (min … max):   26.692 s … 27.068 s    5 runs

    Benchmark #2: HEAD: git-fetch
      Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]
      Range (min … max):   25.070 s … 25.314 s    5 runs

    Summary
      'HEAD: git-fetch' ran
        1.07 ± 0.01 times faster than 'HEAD~: git-fetch'

Signed-off-by: Patrick Steinhardt <redacted>
---
 builtin/fetch.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbd..d064b66512 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -757,6 +757,9 @@ static void prepare_format_display(struct ref *ref_map)
 		die(_("configuration fetch.output contains invalid value %s"),
 		    format);
 
+	if (verbosity < 0)
+		return;
+
 	for (rm = ref_map; rm; rm = rm->next) {
 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
 		    !rm->peer_ref ||

@@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,
 			   const char *remote, const char *local,
 			   int summary_width)
 {
-	int width = (summary_width + strlen(summary) - gettext_width(summary));
+	int width;
+
+	if (verbosity < 0)
+		return;
+
+	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display, "%c %-*s ", code, width, summary);
 	if (!compact_format)

I wonder what you think of the below, which is a bit more of a verbose
search/replacement change, but I think ultimately cleaner. We just pass
the "ref_map" down to all the formatting functions, and the first
function that needs to know the "compact_format" or "refcol_width"
lazily computes it (stored in a static variable). So we avoid the tricky
action at a distance of not preparing certain things because we know
we're in the quiet mode.

But if I understand your change correctly we're on the one hand not
preparing the format change, but then also not printing this at all now
under --quiet, correct? I think it would be good to split up that
proposed functional change from the optimization of the output, it also
seems like if that were done the git-fetch docs on --quiet need to be
changed.

But I wonder if once we have the change below, whether the small change
on top of it to just add a fetch.outputRefWidth=123 wouldn't also
largely solve the optimization problem you have, i.e. adding a config
variable to adjust the width, instead of us auto-discovering it by
looping over all refs.

Or maybe not, but I think it would be interesting to see how much of the
slowdown you have is that v.s. that we end up emitting this output to
stderr. I.e. is it the I/O of the output or the extra loop that's the
expensive part?

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbdc..88a8b3660d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -707,6 +707,24 @@ static int s_update_ref(const char *action,
 static int refcol_width = 10;
 static int compact_format;
 
+static int prepared_compact_format;
+static void prepare_compact_format(void)
+{
+	const char *format = "full";
+
+	if (prepared_compact_format++)
+		return;
+
+	git_config_get_string_tmp("fetch.output", &format);
+	if (!strcasecmp(format, "full"))
+		compact_format = 0;
+	else if (!strcasecmp(format, "compact"))
+		compact_format = 1;
+	else
+		die(_("configuration fetch.output contains invalid value %s"),
+		    format);
+}
+
 static void adjust_refcol_width(const struct ref *ref)
 {
 	int max, rlen, llen, len;

@@ -726,6 +744,7 @@ static void adjust_refcol_width(const struct ref *ref)
 	 * anyway because we don't know if the error explanation part
 	 * will be printed in update_local_ref)
 	 */
+	prepare_compact_format();
 	if (compact_format) {
 		llen = 0;
 		max = max * 2 / 3;

@@ -743,19 +762,13 @@ static void adjust_refcol_width(const struct ref *ref)
 		refcol_width = rlen;
 }
 
-static void prepare_format_display(struct ref *ref_map)
+static int prepared_refcol_width;
+static void prepare_refcol_width(struct ref *ref_map)
 {
 	struct ref *rm;
-	const char *format = "full";
 
-	git_config_get_string_tmp("fetch.output", &format);
-	if (!strcasecmp(format, "full"))
-		compact_format = 0;
-	else if (!strcasecmp(format, "compact"))
-		compact_format = 1;
-	else
-		die(_("configuration fetch.output contains invalid value %s"),
-		    format);
+	if (prepared_refcol_width++)
+		return;
 
 	for (rm = ref_map; rm; rm = rm->next) {
 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||

@@ -767,9 +780,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 ref *ref_map, struct strbuf *display,
 				  const char *remote, const char *local)
 {
+	prepare_refcol_width(ref_map);
 	strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);
 }

@@ -800,7 +814,7 @@ static int find_and_replace(struct strbuf *haystack,
 	return 1;
 }
 
-static void print_compact(struct strbuf *display,
+static void print_compact(struct ref *ref_map, struct strbuf *display,
 			  const char *remote, const char *local)
 {
 	struct strbuf r = STRBUF_INIT;

@@ -816,13 +830,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(ref_map, display, r.buf, l.buf);
 
 	strbuf_release(&r);
 	strbuf_release(&l);
 }
 
-static void format_display(struct strbuf *display, char code,
+static void format_display(struct ref *ref_map,
+			   struct strbuf *display, char code,
 			   const char *summary, const char *error,
 			   const char *remote, const char *local,
 			   int summary_width)

@@ -830,15 +845,17 @@ static void format_display(struct strbuf *display, char code,
 	int width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display, "%c %-*s ", code, width, summary);
+	prepare_compact_format();
 	if (!compact_format)
-		print_remote_to_local(display, remote, local);
+		print_remote_to_local(ref_map, display, remote, local);
 	else
-		print_compact(display, remote, local);
+		print_compact(ref_map, display, remote, local);
 	if (error)
 		strbuf_addf(display, "  (%s)", error);
 }
 
-static int update_local_ref(struct ref *ref,
+static int update_local_ref(struct ref *ref_map,
+			    struct ref *ref,
 			    struct ref_transaction *transaction,
 			    const char *remote,
 			    const struct ref *remote_ref,

@@ -857,7 +874,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(ref_map, display, '=', _("[up to date]"), NULL,
 				       remote, pretty_ref, summary_width);
 		return 0;
 	}

@@ -870,7 +887,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(ref_map, display, '!', _("[rejected]"),
 			       _("can't fetch in current branch"),
 			       remote, pretty_ref, summary_width);
 		return 1;

@@ -881,12 +898,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(ref_map, 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(ref_map, display, '!', _("[rejected]"), _("would clobber existing tag"),
 				       remote, pretty_ref, summary_width);
 			return 1;
 		}

@@ -918,7 +935,7 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, transaction, 0);
-		format_display(display, r ? '!' : '*', what,
+		format_display(ref_map, display, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		return r;

@@ -940,7 +957,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(ref_map, display, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
 			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);

@@ -952,13 +969,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(ref_map, 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(ref_map, display, '!', _("[rejected]"), _("non-fast-forward"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}

@@ -1111,8 +1128,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

@@ -1187,8 +1202,9 @@ 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,
-						       rm, &note, summary_width);
+				rc |= update_local_ref(ref_map, ref,
+						       transaction, what, rm,
+						       &note, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*

@@ -1196,7 +1212,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(ref_map, &note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);

@@ -1357,7 +1373,7 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 				fprintf(stderr, _("From %.*s\n"), url_len, url);
 				shown_url = 1;
 			}
-			format_display(&sb, '-', _("[deleted]"), NULL,
+			format_display(ref_map, &sb, '-', _("[deleted]"), NULL,
 				       _("(none)"), prettify_refname(ref->name),
 				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);

[PATCH v2] fetch: skip formatting updated refs with `--quiet`

From: Patrick Steinhardt <hidden>
Date: 2021-08-30 10:54:36

When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.

While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.

Fix this issue by completely skipping both preparation of the format and
formatting data for display in case the user passes `--quiet`, improving
performance especially with many refs. The following benchmark shows a
nice speedup for a quiet mirror-fetch in a repository with 2.3M refs:

    Benchmark #1: HEAD~: git-fetch
      Time (mean ± σ):     26.929 s ±  0.145 s    [User: 24.194 s, System: 4.656 s]
      Range (min … max):   26.692 s … 27.068 s    5 runs

    Benchmark #2: HEAD: git-fetch
      Time (mean ± σ):     25.189 s ±  0.094 s    [User: 22.556 s, System: 4.606 s]
      Range (min … max):   25.070 s … 25.314 s    5 runs

    Summary
      'HEAD: git-fetch' ran
        1.07 ± 0.01 times faster than 'HEAD~: git-fetch'

While at it, this patch also fixes `adjust_refcol_width()` such that it
skips unchanged refs in case the user passed `--quiet`, where verbosity
will be negative. While this function won't be called anymore if so,
this brings the comment in line with actual code. Furthermore, needless
`verbosity >= 0` checks are now removed in `store_updated_refs()`: we
never print to the `note` buffer anymore in case `verbosity < 0`, so we
won't end up in that code block anyway.

Signed-off-by: Patrick Steinhardt <redacted>
---
Range-diff against v1:
1:  40c385048a ! 1:  e5ffa17753 fetch: skip formatting updated refs with `--quiet`
    @@ Commit message
               'HEAD: git-fetch' ran
                 1.07 ± 0.01 times faster than 'HEAD~: git-fetch'
     
    +    While at it, this patch also fixes `adjust_refcol_width()` such that it
    +    skips unchanged refs in case the user passed `--quiet`, where verbosity
    +    will be negative. While this function won't be called anymore if so,
    +    this brings the comment in line with actual code. Furthermore, needless
    +    `verbosity >= 0` checks are now removed in `store_updated_refs()`: we
    +    never print to the `note` buffer anymore in case `verbosity < 0`, so we
    +    won't end up in that code block anyway.
    +
         Signed-off-by: Patrick Steinhardt [off-list ref]
     
      ## builtin/fetch.c ##
    +@@ builtin/fetch.c: static void adjust_refcol_width(const struct ref *ref)
    + 	int max, rlen, llen, len;
    + 
    + 	/* uptodate lines are only shown on high verbosity level */
    +-	if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
    ++	if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
    + 		return;
    + 
    + 	max    = term_columns();
     @@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
    - 		die(_("configuration fetch.output contains invalid value %s"),
    - 		    format);
    + 	struct ref *rm;
    + 	const char *format = "full";
      
     +	if (verbosity < 0)
     +		return;
     +
    - 	for (rm = ref_map; rm; rm = rm->next) {
    - 		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
    - 		    !rm->peer_ref ||
    + 	git_config_get_string_tmp("fetch.output", &format);
    + 	if (!strcasecmp(format, "full"))
    + 		compact_format = 0;
     @@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,
      			   const char *remote, const char *local,
      			   int summary_width)
    @@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,
      
      	strbuf_addf(display, "%c %-*s ", code, width, summary);
      	if (!compact_format)
    +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,
    + 					       "FETCH_HEAD", summary_width);
    + 			}
    + 			if (note.len) {
    +-				if (verbosity >= 0 && !shown_url) {
    ++				if (!shown_url) {
    + 					fprintf(stderr, _("From %.*s\n"),
    + 							url_len, url);
    + 					shown_url = 1;
    + 				}
    +-				if (verbosity >= 0)
    +-					fprintf(stderr, " %s\n", note.buf);
    ++				fprintf(stderr, " %s\n", note.buf);
    + 			}
    + 		}
    + 	}

 builtin/fetch.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbd..fc7b6bb84e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -712,7 +712,7 @@ static void adjust_refcol_width(const struct ref *ref)
 	int max, rlen, llen, len;
 
 	/* uptodate lines are only shown on high verbosity level */
-	if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
+	if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
 		return;
 
 	max    = term_columns();

@@ -748,6 +748,9 @@ static void prepare_format_display(struct ref *ref_map)
 	struct ref *rm;
 	const char *format = "full";
 
+	if (verbosity < 0)
+		return;
+
 	git_config_get_string_tmp("fetch.output", &format);
 	if (!strcasecmp(format, "full"))
 		compact_format = 0;

@@ -827,7 +830,12 @@ static void format_display(struct strbuf *display, char code,
 			   const char *remote, const char *local,
 			   int summary_width)
 {
-	int width = (summary_width + strlen(summary) - gettext_width(summary));
+	int width;
+
+	if (verbosity < 0)
+		return;
+
+	width = (summary_width + strlen(summary) - gettext_width(summary));
 
 	strbuf_addf(display, "%c %-*s ", code, width, summary);
 	if (!compact_format)

@@ -1202,13 +1210,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 					       "FETCH_HEAD", summary_width);
 			}
 			if (note.len) {
-				if (verbosity >= 0 && !shown_url) {
+				if (!shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
 							url_len, url);
 					shown_url = 1;
 				}
-				if (verbosity >= 0)
-					fprintf(stderr, " %s\n", note.buf);
+				fprintf(stderr, " %s\n", note.buf);
 			}
 		}
 	}

-- 
2.33.0

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help