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

From: Glen Choo <chooglen@google.com>
Date: May 12, 2023, 00:49 UTC
Message-ID: <kl6lilcyz855.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <bb1a591c2f46b78eb26eb72ec677c65d10cb714c.1683721293.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

> 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.

Show 15 quoted lines

> -	/*
> -	 * 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) {"...

Show 9 quoted lines

> +		/*
> +		 * 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.