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?
