| Message ID | pull.1837.v2.git.1734455884405.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] remote: align --verbose output with spaces | expand |
"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?
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?
"Wang Bing-hua" <louiswpf@gmail.com> writes: >> 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: Ah, of course, I should have double-checked, but it should be more like printf("%.*s%s%s", maxwidth + 1 - utf8_strwidth(item->string), "", item->string, item->util ? item->util : ""); meaning (1) the first output field must have maxwidth+1 - the display width the second output field takes. The field's contents is empty, so we get enough SP padded to make the total of this first field and the second field to make maxwidth+1. (2) the second output field is item->string itself. (3) the third output field has item->util if exists. > 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? You already know from my initial reaction what my answer would be, but I am inclined to hear from others before we make a decision. THanks.
diff --git a/builtin/remote.c b/builtin/remote.c index 1ad3e70a6b4..1e9106530c0 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -16,6 +16,7 @@ #include "strvec.h" #include "commit-reach.h" #include "progress.h" +#include "utf8.h" static const char * const builtin_remote_usage[] = { "git remote [-v | --verbose]", @@ -1279,6 +1280,20 @@ static int get_one_entry(struct remote *remote, void *priv) return 0; } +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); 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) { + 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); + } else { + if (item != list.items && + !strcmp((item - 1)->string, item->string)) continue; printf("%s\n", item->string); } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 08424e878e1..6586f020f74 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -249,8 +249,8 @@ test_expect_success 'without subcommand' ' test_expect_success 'without subcommand accepts -v' ' cat >expect <<-EOF && - origin $(pwd)/one (fetch) - origin $(pwd)/one (push) + origin $(pwd)/one (fetch) + origin $(pwd)/one (push) EOF git -C test remote -v >actual && test_cmp expect actual