Re: [PATCH v2] remote: align --verbose output with spaces

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

"Wang Bing-hua via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Wang Bing-hua <louiswpf@gmail.com>
>
> Remote names exceeding a tab width could cause misalignment.
> Align --verbose output with spaces instead of a tab.

While I am still not convinced if this change is a good idea (see
my earlier comment in a separate message)...

> +static int calc_maxwidth(struct string_list *list)
> +{
> +	int max = 0;
> +	struct string_list_item *item;
> +
> +	for_each_string_list_item (item, list) {
> +		int w = utf8_strwidth(item->string);
> +
> +		if (w > max)
> +			max = w;
> +	}
> +	return max;
> +}
> +
>  static int show_all(void)
>  {
>  	struct string_list list = STRING_LIST_INIT_DUP;
> @@ -1287,16 +1302,25 @@ static int show_all(void)
>  	result = for_each_remote(get_one_entry, &list);
>  
>  	if (!result) {
> -		int i;
> +		int maxwidth = 0;
> +		struct string_list_item *item;
>  
> +		if (verbose)
> +			maxwidth = calc_maxwidth(&list);

I wonder if it is a better idea to extend get_one_entry() interface
to take not just a string_list but something like

	struct remotes_data {
		int maxwidth;
		struct string_list *list_of_remotes;
	};

if we think it is a good idea to give richer output to show_all()
function (instead of keep it spartan and compatible for the sake of
not breaking machine readers).  There may be things other than
maxwidth that future changes to "git remote [-v]" may find needed.
And with such a change, you do not need a separate iteration over
the list of remotes just to call calc_maxwidth() callback.  Keeping
a tally of "max length we have seen" inside get_one_entry() regardless
of "--verbose" setting shouldn't be too costly and help reduce the
complexity of the code.

>  		string_list_sort(&list);
> -		for (i = 0; i < list.nr; i++) {
> -			struct string_list_item *item = list.items + i;
> -			if (verbose)
> -				printf("%s\t%s\n", item->string,
> -					item->util ? (const char *)item->util : "");
> -			else {
> -				if (i && !strcmp((item - 1)->string, item->string))
> +		for_each_string_list_item (item, &list) {

Use of for_each_string_list_item() instead of a manual iteration is
probably a good idea here.  If this were a larger change, that may
deserve to be a preparatory step on its own, but it is probably OK
to do so in the same patch.

> +			if (verbose) {
> +				struct strbuf s = STRBUF_INIT;
> +
> +				strbuf_utf8_align(&s, ALIGN_LEFT, maxwidth + 1,
> +						  item->string);
> +				if (item->util)
> +					strbuf_addstr(&s, item->util);
> +				printf("%s\n", s.buf);
> +				strbuf_release(&s);

Wouldn't it work to just do (totally untested code snippet below;
may have off-by-one around maxwidth)

				printf("%.*s%s", maxwidth, item->string,
					item->util ? "" : item->util);

without using any strbuf operation?