| Message ID | pull.1837.git.1734439176360.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remote: align --verbose output with spaces | expand |
On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote: > 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. > Good enhancement. > Signed-off-by: Wang Bing-hua <louiswpf@gmail.com> > --- > remote: align --verbose output with spaces > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1837%2Flouiswpf%2Fremote-align-verbose-output-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1837/louiswpf/remote-align-verbose-output-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1837 > > builtin/remote.c | 30 ++++++++++++++++++++++++++---- > t/t5505-remote.sh | 4 ++-- > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index 1ad3e70a6b4..876274d9dca 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; > + > + for (int i = 0; i < list->nr; i++) { Nit: we should use "size_t" to declare/define loop variable `i` because the type of `list-nr` is `size_t`. Recently, Patrick has provided a patch to start warn unsigned value compared with signed value in [1] which has not been merged into the master. [1] https://lore.kernel.org/git/20241206-pks-sign-compare-v4-0-0344c6dfb219@pks.im > + struct string_list_item *item = list->items + i; > + int w = utf8_strwidth(item->string); > + > + if (w > max) > + max = w; > + } > + return max; > +} > + So, here we traverse the list to find the max "utf8_strwidth". However, we should not EXPLICITLY traverse the string list. There are two ways you could do: 1. Use the helper macro "for_each_string_list_item" in "string-list.h" to do above. 2. Use the helper function "for_each_string_list" in "string-list.c" to do above. > static int show_all(void) > { > struct string_list list = STRING_LIST_INIT_DUP; > @@ -1292,10 +1307,17 @@ static int show_all(void) > 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 (verbose) { > + struct strbuf s = STRBUF_INIT; > + > + strbuf_utf8_align(&s, ALIGN_LEFT, > + calc_maxwidth(&list) + 1, > + item->string); So, here we call `calc_maxwidth` in the loop. That does not make sense. We should not call this function when we are traversing the string list. I think we should firstly calculate the max width outside of the loop. Thanks, Jialuo
On 17/12/2024 21:23, shejialuo wrote: > > Good enhancement. > Thank you. > > Nit: we should use "size_t" to declare/define loop variable `i` > because the type of `list-nr` is `size_t`. > > Recently, Patrick has provided a patch to start warn unsigned value > compared with signed value in [1] which has not been merged into the > master. > > [1] https://lore.kernel.org/git/20241206-pks-sign-compare-v4-0-0344c6dfb219@pks.im > > > So, here we traverse the list to find the max "utf8_strwidth". However, > we should not EXPLICITLY traverse the string list. There are two ways > you could do: > > 1. Use the helper macro "for_each_string_list_item" in "string-list.h" > to do above. > 2. Use the helper function "for_each_string_list" in "string-list.c" to > do above. > > > So, here we call `calc_maxwidth` in the loop. That does not make sense. > We should not call this function when we are traversing the string list. > I think we should firstly calculate the max width outside of the loop. > Thank you for the detailed comments. It really helps. I will address these issues in v2.
shejialuo <shejialuo@gmail.com> writes: > On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote: >> From: Wang Bing-hua <louiswpf@gmail.com> >> >> Remote names exceeding a tab width could cause misalignment. If all of them are named with ten ASCII characters, on a terminal with fixed-width font, things will still display aligned ;-) >> Align --verbose output with spaces instead of a tab. >> > > Good enhancement. With a Devil's advocate hat on, a change like this will completely break tools when they are reading the "--verbose" output and expecting that the fields are separated with a TAB (in other words, the tab is *not* about alignment in the first place for them). Now with that hat off. For users with that many remotes where the alignment of URLs in the interactive "git remote -v" output matter, I am not sure if this change is a real improvement enough that it is worth the possible risk of breaking existing tools. With that many remotes defined, wouldn't they be doing "git remote -v" piped to "grep '^name<TAB>'" or something? That use case would break with the change, too. Thanks.
On 17/12/2024 12:21, Junio C Hamano wrote: >> On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote: >>> From: Wang Bing-hua <louiswpf@gmail.com> >>> >>> Remote names exceeding a tab width could cause misalignment. > > If all of them are named with ten ASCII characters, on a terminal > with fixed-width font, things will still display aligned ;-) Indeed :) I did consider this scenario and wrote "could". I should have worded this more clearly. > With a Devil's advocate hat on, a change like this will completely > break tools when they are reading the "--verbose" output and > expecting that the fields are separated with a TAB (in other words, > the tab is *not* about alignment in the first place for them). > > Now with that hat off. > > For users with that many remotes where the alignment of URLs in the > interactive "git remote -v" output matter, I am not sure if this > change is a real improvement enough that it is worth the possible > risk of breaking existing tools. With that many remotes defined, > wouldn't they be doing "git remote -v" piped to "grep '^name<TAB>'" > or something? That use case would break with the change, too. Agreed on all points. Another point to consider is to align with "git branch -v" since it also uses spaces for alignment. This patch was also originally lifted from "builtin/branch.c".
diff --git a/builtin/remote.c b/builtin/remote.c index 1ad3e70a6b4..876274d9dca 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; + + for (int i = 0; i < list->nr; i++) { + struct string_list_item *item = list->items + i; + 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; @@ -1292,10 +1307,17 @@ static int show_all(void) 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 (verbose) { + struct strbuf s = STRBUF_INIT; + + strbuf_utf8_align(&s, ALIGN_LEFT, + calc_maxwidth(&list) + 1, + item->string); + if (item->util) + strbuf_addstr(&s, item->util); + printf("%s\n", s.buf); + strbuf_release(&s); + } else { if (i && !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