Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

From: Jeff King <peff@peff.net>
Date: Oct 22, 2016, 05:04 UTC
Message-ID: <20161022050426.t2fifjqrldc6mebc@sigill.intra.peff.net>
In-Reply-To: <xmqqoa2d9eum.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> And this is the final one.
> 
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Fri, 21 Oct 2016 21:33:06 -0700
> Subject: [PATCH] transport: compute summary-width dynamically
> 
> Now all that is left to do is to actually iterate over the refs
> and measure the display width needed to show their abbreviation.

I think we crossed emails. :) This is obviously correct, if we don't mind paying the find_unique_abbrev cost twice for each sha1.

Show 9 quoted lines

>  int transport_summary_width(const struct ref *refs)
>  {
> -	return (2 * FALLBACK_DEFAULT_ABBREV + 3);
> +	int maxw;
> +
> +	for (maxw = -1; refs; refs = refs->next) {
> +		maxw = measure_abbrev(&refs->old_oid, maxw);
> +		maxw = measure_abbrev(&refs->new_oid, maxw);
> +	}

This is a minor style nit, but I think it's better to avoid mixing unrelated bits between the initialization, condition, and iteration bits of a for loop. IOW:

  int maxw = -1;

  for (; refs; refs = refs->next) {
	...
  }

makes the for-loop _just_ about iteration, and it is more clear that the manipulation of "maxw" is a side effect of the loop that is valid after it finishes.

Though in this particular case, the loop and the whole function are short enough that I don't care that much. Just raising a philosophical point. :)

-Peff