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

["Wang Bing-hua" <louiswpf@gmail.com>]

On 17/12/2024 12:47, Junio C Hamano wrote:
> 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.

This is a great idea.

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

Thanks for the reminder.

> 
>> +			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?

I did try to use printf at first.

				printf("%-*s %s\n", maxwidth, item->string,
				       item->util ? (const char *)item->util :
						    "");

But it broke when there are non-ASCII characters. For example:

$ git remote -v
a  url (fetch)
a  url (push)
å url (fetch)
å url (push)
åå url (fetch)
åå url (push)


Thank you for reviewing. I'm also debating. It's great to align
"remote -v" and make it behave similarly to "branch -v". But it might
not be worth it to complicate the code and break machine readers.
Do we continue working on this?