Re: [PATCH v2] remote: align --verbose output with spaces
- From
- Wang Bing-hua <louiswpf@gmail.com>
- Date
- Dec 18, 2024, 08:37 UTC
- Message-ID
- <1812389a7b6fe18d.7946240d95ed36ca.1b2132d5c888cc21@host>
- In-Reply-To
- <xmqq4j32yr3c.fsf@gitster.g>
On 17/12/2024 12:47, Junio C Hamano wrote:
Show 17 quoted lines
> 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.
Show 15 quoted lines
>
>> 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.
Show 18 quoted lines
>
>> + 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?