[PATCH] remote: align --verbose output with spaces

[PATCH] remote: align --verbose output with spaces

[Wang Bing-hua via GitGitGadget]

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.

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++) {
+		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

base-commit: 2ccc89b0c16c51561da90d21cfbb4b58cc877bf6
-- 
gitgitgadget

Re: [PATCH] remote: align --verbose output with spaces

[shejialuo]

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

Re: [PATCH] remote: align --verbose output with spaces

[Wang Bing-hua]

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.

[PATCH v2] remote: align --verbose output with spaces

[Wang Bing-hua via GitGitGadget]

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.

Signed-off-by: Wang Bing-hua <louiswpf@gmail.com>
---
    remote: align --verbose output with spaces
    
    Changes in v2:
    
     * Use for_each_string_list_item() to traverse string lists.
     * Calculate the max width outside of the loop.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1837%2Flouiswpf%2Fremote-align-verbose-output-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1837/louiswpf/remote-align-verbose-output-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1837

Range-diff vs v1:

 1:  960a18efc36 ! 1:  648881dbf0d remote: align --verbose output with spaces
     @@ builtin/remote.c: static int get_one_entry(struct remote *remote, void *priv)
      +static int calc_maxwidth(struct string_list *list)
      +{
      +	int max = 0;
     ++	struct string_list_item *item;
      +
     -+	for (int i = 0; i < list->nr; i++) {
     -+		struct string_list_item *item = list->items + i;
     ++	for_each_string_list_item (item, list) {
      +		int w = utf8_strwidth(item->string);
      +
      +		if (w > max)
     @@ builtin/remote.c: static int get_one_entry(struct remote *remote, void *priv)
       {
       	struct string_list list = STRING_LIST_INIT_DUP;
      @@ builtin/remote.c: 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;
     +-		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,
     -+						  calc_maxwidth(&list) + 1,
     ++				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 (i && !strcmp((item - 1)->string, item->string))
     ++				if (item != list.items &&
     ++				    !strcmp((item - 1)->string, item->string))
       					continue;
       				printf("%s\n", item->string);
     + 			}
      
       ## t/t5505-remote.sh ##
      @@ t/t5505-remote.sh: test_expect_success 'without subcommand' '


 builtin/remote.c  | 40 ++++++++++++++++++++++++++++++++--------
 t/t5505-remote.sh |  4 ++--
 2 files changed, 34 insertions(+), 10 deletions(-)

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

base-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086
-- 
gitgitgadget

Re: [PATCH] remote: align --verbose output with spaces

[Junio C Hamano]

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.

Re: [PATCH v2] remote: align --verbose output with spaces

[Junio C Hamano]

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

Re: [PATCH] remote: align --verbose output with spaces

[Wang Bing-hua]

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".

Re: [PATCH v2] remote: align --verbose output with spaces

[Wang Bing-hua]

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?

Re: [PATCH] remote: align --verbose output with spaces

[shejialuo]

On Tue, Dec 17, 2024 at 12:21:37PM -0800, Junio C Hamano wrote:
> 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).
> 

Yes, I agree. Although we may provide convenience for the end user, but
we may break other tools parsing the output. I think I bring confusion
to the Wang. Actually, I have seen that Wang contributes to Git in the
first time, so I just want to courage.

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

Re: [PATCH] remote: align --verbose output with spaces

[Wang Bing-hua]

On 18/12/2024 21:52, shejialuo wrote:
> Yes, I agree. Although we may provide convenience for the end user, but
> we may break other tools parsing the output. I think I bring confusion
> to the Wang. Actually, I have seen that Wang contributes to Git in the
> first time, so I just want to courage.

No worries. Thank you for encouraging words :)

Re: [PATCH v2] remote: align --verbose output with spaces

[Junio C Hamano]

"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.

Re: [PATCH v2] remote: align --verbose output with spaces

[Wang Bing-hua]

On 18/12/2024 07:39, Junio C Hamano wrote:
> 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.

Understood. Thanks for the explanation.

Here is a tested code snippet for the record:

	printf("%s%*s %s\n",
	       item->string,
	       maxwidth - utf8_strwidth(item->string), "",
	       item->util ? (const char *)item->util : "");

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

I see. Thanks.