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

From: shejialuo <shejialuo@gmail.com>
Date: Dec 17, 2024, 13:23 UTC
Message-ID: <Z2F7VAsTjfBCVCrG@ArchLinux>
In-Reply-To: <pull.1837.git.1734439176360.gitgitgadget@gmail.com>

On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:

Show 5 quoted lines

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

Show 33 quoted lines

> 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

Show 9 quoted lines

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

Show 17 quoted lines

>  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