Re: [PATCH v2 3/3] fetch: reduce duplicate in ref update status lines

[Junio C Hamano <gitster@pobox.com>]

Duy Nguyen <pclouds@gmail.com> writes:

>> Peff suggested that a two-pass approach might not be too bad, but had 
>> problems when he tried it with extra-long refs.  Maybe those problems 
>> could be dealt with, and we could get a simple, aligned output?
>
> The good thing about 2/3 is we can customize the output easily and
> even switch format with config variables. But let's play without
> config vars for now. A 3/3 replacement that adds another pass to
> calculate column width is at the end.

Yes, I agree with you that 2/3 lays a very good groundwork,
regardless of the final output format.

> The patch does not do fancy stuff like this yet, but it can because
> lines exceeding terminal width is already excluded from column width
> calculation. So far the output looks good on my terminal (192 chars,
> can't overflow or refnames are insanely long)

Sounds like a sensible approach....

> -- 8< --
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a7f152a..5e1e5c9 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -15,6 +15,7 @@
>  #include "submodule.h"
>  #include "connected.h"
>  #include "argv-array.h"
> +#include "utf8.h"
>  
>  static const char * const builtin_fetch_usage[] = {
>  	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
> @@ -449,14 +450,26 @@ fail:
>  			   : STORE_REF_ERROR_OTHER;
>  }
>  
> -#define REFCOL_WIDTH  10
> +static int refcol_width = 10;
> +
> +static void adjust_refcol_width(const char *remote, const char *local)
> +{
> +	int max = term_columns();
> +	int rlen = utf8_strwidth(remote);
> +	int llen = utf8_strwidth(local);
> +
> +	if (21 /* flag summary */ + rlen + 4 /* => */ + llen >= max)
> +		return;
> +	if (refcol_width < rlen)
> +		refcol_width = rlen;
> +}
>  
>  static void format_display(struct strbuf *display, char code,
>  			   const char *summary, const char *error,
>  			   const char *remote, const char *local)
>  {
>  	strbuf_addf(display, "%c %-*s ", code, TRANSPORT_SUMMARY(summary));
> -	strbuf_addf(display, "%-*s -> %s", REFCOL_WIDTH, remote, local);
> +	strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);

... and if I understand correctly, this is the only place where you
need to decide if you need to switch to two lines, right?  You would
measure width of the remote and compare it with refcol_width or
something like that.

>  	if (error)
>  		strbuf_addf(display, "  (%s)", error);
>  }
> @@ -618,6 +631,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  		goto abort;
>  	}
>  
> +	/*
> +	 * go through all refs to determine column size for
> +	 * "remote -> local" output
> +	 */
> +	for (rm = ref_map; rm; rm = rm->next) {
> +		if (rm->status == REF_STATUS_REJECT_SHALLOW ||
> +		    !rm->peer_ref ||
> +		    !strcmp(rm->name, "HEAD"))
> +			continue;
> +
> +		adjust_refcol_width(prettify_refname(rm->name),
> +				    prettify_refname(rm->peer_ref->name));
> +	}
> +
>  	/*
>  	 * 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
> -- 8< --