| Message ID | 20230207235238.1850757-1-kolyshkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remote: align columns on -v | expand |
On Tue, Feb 07, 2023 at 03:52:38PM -0800, Kir Kolyshkin wrote: > Currently, git remote -v produces a misaligned output when a remote name > is more than 8 characters long (i.e. longer than a tab step). Here's how > it looks like: > > giuseppe https://github.com/giuseppe/runc (fetch) > giuseppe https://github.com/giuseppe/runc (push) > kir git@github.com:kolyshkin/runc.git (fetch) > kir git@github.com:kolyshkin/runc.git (push) > lifubang https://github.com/lifubang/runc (fetch) > lifubang https://github.com/lifubang/runc (push) > marquiz https://github.com/marquiz/runc (fetch) > marquiz https://github.com/marquiz/runc (push) > > Let's find the maximum width and use it for alignment. > > While at it, let's keep the \t in case some tools depend on it > for parsing (there will still be trailing spaces in the remote name). > > With this change, the output is like this now: > > giuseppe https://github.com/giuseppe/runc (fetch) > giuseppe https://github.com/giuseppe/runc (push) > kir git@github.com:kolyshkin/runc.git (fetch) > kir git@github.com:kolyshkin/runc.git (push) > lifubang https://github.com/lifubang/runc (fetch) > lifubang https://github.com/lifubang/runc (push) > marquiz https://github.com/marquiz/runc (fetch) > marquiz https://github.com/marquiz/runc (push) > Thanks for working on that - I had the same wish as well. However, I am tempted to comment on some details here. Especially, what happens if a remote is named with a non-ASCII character (unicode code point would be a better term) ? To determine the width on screen for aligment, strlen() does the wrong thing here. This has been done at other place (being UTF-8 aware), you may want to have a look at this change: commit 12fc4ad89e23af642a8614371ff80bc67cb3315d Author: Torsten Bögershausen <tboegi@web.de> Date: Wed Sep 14 17:13:33 2022 +0200 diff.c: use utf8_strwidth() to count display width > Reported-by: Roman Dodin <dodin.roman@gmail.com> > Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> > --- > builtin/remote.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index 729f6f3643..116417574d 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1245,13 +1245,21 @@ static int show_all(void) > result = for_each_remote(get_one_entry, &list); > > if (!result) { > - int i; > + int i, width = 7; > + > + if (verbose) { > + for (i = 0; i < list.nr; i++) { > + int len = strlen((list.items + i)->string); > + if (len > width) > + width = len; > + } > + } > > 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, > + printf("%-*s\t%s\n", width, item->string, > item->util ? (const char *)item->util : ""); > else { > if (i && !strcmp((item - 1)->string, item->string)) > -- > 2.39.0 >
Kir Kolyshkin <kolyshkin@gmail.com> writes: > Currently, git remote -v produces a misaligned output when a remote name > is more than 8 characters long (i.e. longer than a tab step). Here's how > it looks like: The condition under which URLs do not align is not when they are more than 8 characters long. If all of your remotes have 10-character names, their URLs would perfectly align, no? The description may need to be tightened if we really wanted to do this. But I am skeptical, even without my devil's advocate hat on. > giuseppe https://github.com/giuseppe/runc (fetch) > giuseppe https://github.com/giuseppe/runc (push) > kir git@github.com:kolyshkin/runc.git (fetch) > ... The current output allows programs to post-process by splitting each line with a tab, but this change will break such practice and force those who use such practice to do something different (like "split at the first run of whitespaces or tabs"). > While at it, let's keep the \t in case some tools depend on it > for parsing (there will still be trailing spaces in the remote name). That will not help avoid breaking the behaviour for existing practice (they did not need to strip the whitespaces, but now they are forced to). It only make the output uglier by putting mixture of whitespaces and tabs. So, I dunno. Thanks.
Torsten Bögershausen <tboegi@web.de> writes: > Especially, what happens if a remote is named with a non-ASCII > character (unicode code point would be a better term) ? > To determine the width on screen for aligment, strlen() > does the wrong thing here. Interesting point. I am still skeptical about the patch and suspect that it is better to stick to easy-to-parse output, but if we switch to align for humans, we should definitely count display columns, not byte count with strlen(). Thanks.
On Wed, Feb 8, 2023 at 8:12 AM Torsten Bögershausen <tboegi@web.de> wrote: > > On Tue, Feb 07, 2023 at 03:52:38PM -0800, Kir Kolyshkin wrote: > > Currently, git remote -v produces a misaligned output when a remote name > > is more than 8 characters long (i.e. longer than a tab step). Here's how > > it looks like: > > > > giuseppe https://github.com/giuseppe/runc (fetch) > > giuseppe https://github.com/giuseppe/runc (push) > > kir git@github.com:kolyshkin/runc.git (fetch) > > kir git@github.com:kolyshkin/runc.git (push) > > lifubang https://github.com/lifubang/runc (fetch) > > lifubang https://github.com/lifubang/runc (push) > > marquiz https://github.com/marquiz/runc (fetch) > > marquiz https://github.com/marquiz/runc (push) > > > > Let's find the maximum width and use it for alignment. > > > > While at it, let's keep the \t in case some tools depend on it > > for parsing (there will still be trailing spaces in the remote name). > > > > With this change, the output is like this now: > > > > giuseppe https://github.com/giuseppe/runc (fetch) > > giuseppe https://github.com/giuseppe/runc (push) > > kir git@github.com:kolyshkin/runc.git (fetch) > > kir git@github.com:kolyshkin/runc.git (push) > > lifubang https://github.com/lifubang/runc (fetch) > > lifubang https://github.com/lifubang/runc (push) > > marquiz https://github.com/marquiz/runc (fetch) > > marquiz https://github.com/marquiz/runc (push) > > > > Thanks for working on that - I had the same wish as well. > However, I am tempted to comment on some details here. > Especially, what happens if a remote is named with a non-ASCII > character (unicode code point would be a better term) ? > To determine the width on screen for aligment, strlen() > does the wrong thing here. > This has been done at other place (being UTF-8 aware), > you may want to have a look at this change: > > commit 12fc4ad89e23af642a8614371ff80bc67cb3315d > Author: Torsten Bögershausen <tboegi@web.de> > Date: Wed Sep 14 17:13:33 2022 +0200 > > diff.c: use utf8_strwidth() to count display width Yes, I have seen now that neither strlen nor printf with the specified width do a good job with unicode. Thank you for pointing this out, I'll send v2 soon.
On Wed, Feb 8, 2023 at 8:19 AM Junio C Hamano <gitster@pobox.com> wrote: > > Kir Kolyshkin <kolyshkin@gmail.com> writes: > > > Currently, git remote -v produces a misaligned output when a remote name > > is more than 8 characters long (i.e. longer than a tab step). Here's how > > it looks like: > > The condition under which URLs do not align is not when they are > more than 8 characters long. If all of your remotes have > 10-character names, their URLs would perfectly align, no? The > description may need to be tightened if we really wanted to do this. > > But I am skeptical, even without my devil's advocate hat on. > > > giuseppe https://github.com/giuseppe/runc (fetch) > > giuseppe https://github.com/giuseppe/runc (push) > > kir git@github.com:kolyshkin/runc.git (fetch) > > ... > > The current output allows programs to post-process by splitting each > line with a tab, but this change will break such practice and force > those who use such practice to do something different (like "split > at the first run of whitespaces or tabs"). > > > While at it, let's keep the \t in case some tools depend on it > > for parsing (there will still be trailing spaces in the remote name). > > That will not help avoid breaking the behaviour for existing > practice (they did not need to strip the whitespaces, but now they > are forced to). It only make the output uglier by putting mixture > of whitespaces and tabs. > > So, I dunno. Yes, I agree that this can break someone's scripts, and adding \t was not a bright idea either, as having it may hide the issue (of incorrect splitting) rather than break things entirely (and thus urging to fix it). I assume that any decent script would split by "any whitespace", plus the output of git remote -v is mostly for the user's eyes, not scripts. Please take a look at v2 which I am sending now.
diff --git a/builtin/remote.c b/builtin/remote.c index 729f6f3643..116417574d 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1245,13 +1245,21 @@ static int show_all(void) result = for_each_remote(get_one_entry, &list); if (!result) { - int i; + int i, width = 7; + + if (verbose) { + for (i = 0; i < list.nr; i++) { + int len = strlen((list.items + i)->string); + if (len > width) + width = len; + } + } 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, + printf("%-*s\t%s\n", width, item->string, item->util ? (const char *)item->util : ""); else { if (i && !strcmp((item - 1)->string, item->string))
Currently, git remote -v produces a misaligned output when a remote name is more than 8 characters long (i.e. longer than a tab step). Here's how it looks like: giuseppe https://github.com/giuseppe/runc (fetch) giuseppe https://github.com/giuseppe/runc (push) kir git@github.com:kolyshkin/runc.git (fetch) kir git@github.com:kolyshkin/runc.git (push) lifubang https://github.com/lifubang/runc (fetch) lifubang https://github.com/lifubang/runc (push) marquiz https://github.com/marquiz/runc (fetch) marquiz https://github.com/marquiz/runc (push) Let's find the maximum width and use it for alignment. While at it, let's keep the \t in case some tools depend on it for parsing (there will still be trailing spaces in the remote name). With this change, the output is like this now: giuseppe https://github.com/giuseppe/runc (fetch) giuseppe https://github.com/giuseppe/runc (push) kir git@github.com:kolyshkin/runc.git (fetch) kir git@github.com:kolyshkin/runc.git (push) lifubang https://github.com/lifubang/runc (fetch) lifubang https://github.com/lifubang/runc (push) marquiz https://github.com/marquiz/runc (fetch) marquiz https://github.com/marquiz/runc (push) Reported-by: Roman Dodin <dodin.roman@gmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> --- builtin/remote.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)