diff mbox series

[v5,5/9] fetch: refactor calculation of the display table width

Message ID bb1a591c2f46b78eb26eb72ec677c65d10cb714c.1683721293.git.ps@pks.im (mailing list archive)
State New, archived
Headers show
Series fetch: introduce machine-parseable output | expand

Commit Message

Patrick Steinhardt May 10, 2023, 12:34 p.m. UTC
When displaying reference updates, we try to print the references in a
neat table. As the table's width is determined its contents we thus need
to precalculate the overall width before we can start printing updated
references.

The calculation is driven by `display_state_init()`, which invokes
`refcol_width()` for every reference that is to be printed. This split
is somewhat confusing. For one, we filter references that shall be
attributed to the overall width in both places. And second, we
needlessly recalculate the maximum line length based on the terminal
columns and display format for every reference.

Refactor the code so that the complete width calculations are neatly
contained in `refcol_width()`.

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

Comments

Glen Choo May 12, 2023, 12:49 a.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> When displaying reference updates, we try to print the references in a
> neat table. As the table's width is determined its contents we thus need
> to precalculate the overall width before we can start printing updated
> references.
>
> The calculation is driven by `display_state_init()`, which invokes
> `refcol_width()` for every reference that is to be printed. This split
> is somewhat confusing. For one, we filter references that shall be
> attributed to the overall width in both places. And second, we
> needlessly recalculate the maximum line length based on the terminal
> columns and display format for every reference.
>
> Refactor the code so that the complete width calculations are neatly
> contained in `refcol_width()`.

Through no fault of yours, I have to admit that I found this refactor
quite hard to read. I ended up redoing the refactor and ended up with a
result very similar to yours. That was probably overkill since we have
pretty extensive tests in this area, but I'm quite happy with the change
since it's far more readable.

> -	/*
> -	 * rough estimation to see if the output line is too long and
> -	 * should not be counted (we can't do precise calculation
> -	 * anyway because we don't know if the error explanation part
> -	 * will be printed in update_local_ref)
> -	 */
> -	if (compact_format) {
> -		llen = 0;
> +	max = term_columns();
> +	if (compact_format)
>  		max = max * 2 / 3;
> -	}
> -	len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
> -	if (len >= max)
> -		return 0;

The one thing that changed for the better (vs keeping the lines in the
same order as before) is that this comment that used to be anchored on
"if (compact_format) {"...

> +		/*
> +		 * rough estimation to see if the output line is too long and
> +		 * should not be counted (we can't do precise calculation
> +		 * anyway because we don't know if the error explanation part
> +		 * will be printed in update_local_ref)
> +		 */
> +		len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
> +		if (len >= max)
> +			continue;

is now more accurately anchored to the check that throws away the line.

Looks good.
diff mbox series

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6aecf549e8..007eb3693d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -751,40 +751,51 @@  static int s_update_ref(const char *action,
 	return ret;
 }
 
-static int refcol_width(const struct ref *ref, int compact_format)
+static int refcol_width(const struct ref *ref_map, int compact_format)
 {
-	int max, rlen, llen, len;
+	const struct ref *ref;
+	int max, width = 10;
 
-	/* uptodate lines are only shown on high verbosity level */
-	if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
-		return 0;
-
-	max    = term_columns();
-	rlen   = utf8_strwidth(prettify_refname(ref->name));
-
-	llen   = utf8_strwidth(prettify_refname(ref->peer_ref->name));
-
-	/*
-	 * rough estimation to see if the output line is too long and
-	 * should not be counted (we can't do precise calculation
-	 * anyway because we don't know if the error explanation part
-	 * will be printed in update_local_ref)
-	 */
-	if (compact_format) {
-		llen = 0;
+	max = term_columns();
+	if (compact_format)
 		max = max * 2 / 3;
-	}
-	len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
-	if (len >= max)
-		return 0;
 
-	return rlen;
+	for (ref = ref_map; ref; ref = ref->next) {
+		int rlen, llen = 0, len;
+
+		if (ref->status == REF_STATUS_REJECT_SHALLOW ||
+		    !ref->peer_ref ||
+		    !strcmp(ref->name, "HEAD"))
+			continue;
+
+		/* uptodate lines are only shown on high verbosity level */
+		if (verbosity <= 0 && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
+			continue;
+
+		rlen = utf8_strwidth(prettify_refname(ref->name));
+		if (!compact_format)
+			llen = utf8_strwidth(prettify_refname(ref->peer_ref->name));
+
+		/*
+		 * rough estimation to see if the output line is too long and
+		 * should not be counted (we can't do precise calculation
+		 * anyway because we don't know if the error explanation part
+		 * will be printed in update_local_ref)
+		 */
+		len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
+		if (len >= max)
+			continue;
+
+		if (width < rlen)
+			width = rlen;
+	}
+
+	return width;
 }
 
 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;
 
@@ -816,25 +827,7 @@  static void display_state_init(struct display_state *display_state, struct ref *
 		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;
-
-		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;
-	}
+	display_state->refcol_width = refcol_width(ref_map, display_state->compact_format);
 }
 
 static void display_state_release(struct display_state *display_state)