[PATCH] remote: align columns on -v

[PATCH] remote: align columns on -v

[Kir Kolyshkin]

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(-)

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

Re: [PATCH] remote: align columns on -v

[Torsten Bögershausen]

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
>

Re: [PATCH] remote: align columns on -v

[Junio C Hamano]

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.

Re: [PATCH] remote: align columns on -v

[Kirill Kolyshkin]

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.

Re: [PATCH] remote: align columns on -v

[Junio C Hamano]

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.

Re: [PATCH] remote: align columns on -v

[Kirill Kolyshkin]

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.

[PATCH] remote: align columns on -v

[Kir Kolyshkin]

Currently, git remote -v produces a misaligned output when some remote
names are shorter and some are longer than a tab step. For example:

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 of remote and use it for alignment.
With this change in place, the output looks 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)

[v2: use utf8_strwidth and padding, fix description]

Reported-by: Roman Dodin <dodin.roman@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 builtin/remote.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 729f6f3643..654472a87c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -13,6 +13,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]",
@@ -1245,14 +1246,26 @@ 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 w = utf8_strwidth((list.items + i)->string);
+				if (w > width)
+					width = w;
+			}
+		}
 
 		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,
+			if (verbose) {
+				int padding = width - utf8_strwidth(item->string);
+				if (padding < 0)
+					padding = 0;
+				printf("%*s%s %s\n", padding, "", item->string,
 					item->util ? (const char *)item->util : "");
+			}
 			else {
 				if (i && !strcmp((item - 1)->string, item->string))
 					continue;
-- 
2.39.0