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