{"thread":{"id":"62653","subject":"[PATCH] remote: align --verbose output with spaces","startedAt":"2024-12-17T12:39:39Z","lastAt":"2024-12-19T02:14:33Z","messageCount":12,"participants":["Wang Bing-hua via GitGitGadget","shejialuo","Wang Bing-hua","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509216","messageId":"pull.1837.git.1734439176360.gitgitgadget@gmail.com","threadId":"62653","inReplyTo":null,"subject":"[PATCH] remote: align --verbose output with spaces","fromName":"Wang Bing-hua via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-17T12:39:36Z","receivedAt":"2024-12-17T12:39:39Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"From: Wang Bing-hua <louiswpf@gmail.com>\n\nRemote names exceeding a tab width could cause misalignment.\nAlign --verbose output with spaces instead of a tab.\n\nSigned-off-by: Wang Bing-hua <louiswpf@gmail.com>\n---\n    remote: align --verbose output with spaces\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1837%2Flouiswpf%2Fremote-align-verbose-output-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1837/louiswpf/remote-align-verbose-output-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1837\n\n builtin/remote.c  | 30 ++++++++++++++++++++++++++----\n t/t5505-remote.sh |  4 ++--\n 2 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 1ad3e70a6b4..876274d9dca 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -16,6 +16,7 @@\n #include \"strvec.h\"\n #include \"commit-reach.h\"\n #include \"progress.h\"\n+#include \"utf8.h\"\n \n static const char * const builtin_remote_usage[] = {\n \t\"git remote [-v | --verbose]\",\n@@ -1279,6 +1280,20 @@ static int get_one_entry(struct remote *remote, void *priv)\n \treturn 0;\n }\n \n+static int calc_maxwidth(struct string_list *list)\n+{\n+\tint max = 0;\n+\n+\tfor (int i = 0; i < list->nr; i++) {\n+\t\tstruct string_list_item *item = list->items + i;\n+\t\tint w = utf8_strwidth(item->string);\n+\n+\t\tif (w > max)\n+\t\t\tmax = w;\n+\t}\n+\treturn max;\n+}\n+\n static int show_all(void)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n@@ -1292,10 +1307,17 @@ static int show_all(void)\n \t\tstring_list_sort(&list);\n \t\tfor (i = 0; i < list.nr; i++) {\n \t\t\tstruct string_list_item *item = list.items + i;\n-\t\t\tif (verbose)\n-\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n-\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n-\t\t\telse {\n+\t\t\tif (verbose) {\n+\t\t\t\tstruct strbuf s = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT,\n+\t\t\t\t\t\t  calc_maxwidth(&list) + 1,\n+\t\t\t\t\t\t  item->string);\n+\t\t\t\tif (item->util)\n+\t\t\t\t\tstrbuf_addstr(&s, item->util);\n+\t\t\t\tprintf(\"%s\\n\", s.buf);\n+\t\t\t\tstrbuf_release(&s);\n+\t\t\t} else {\n \t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n \t\t\t\t\tcontinue;\n \t\t\t\tprintf(\"%s\\n\", item->string);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 08424e878e1..6586f020f74 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -249,8 +249,8 @@ test_expect_success 'without subcommand' '\n \n test_expect_success 'without subcommand accepts -v' '\n \tcat >expect <<-EOF &&\n-\torigin\t$(pwd)/one (fetch)\n-\torigin\t$(pwd)/one (push)\n+\torigin $(pwd)/one (fetch)\n+\torigin $(pwd)/one (push)\n \tEOF\n \tgit -C test remote -v >actual &&\n \ttest_cmp expect actual\n\nbase-commit: 2ccc89b0c16c51561da90d21cfbb4b58cc877bf6\n-- \ngitgitgadget\n"},{"id":"509220","messageId":"Z2F7VAsTjfBCVCrG@ArchLinux","threadId":"62653","inReplyTo":"pull.1837.git.1734439176360.gitgitgadget@gmail.com","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-17T13:23:32Z","receivedAt":"2024-12-17T13:22:53Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:\n> From: Wang Bing-hua <louiswpf@gmail.com>\n> \n> Remote names exceeding a tab width could cause misalignment.\n> Align --verbose output with spaces instead of a tab.\n> \n\nGood enhancement.\n\n> Signed-off-by: Wang Bing-hua <louiswpf@gmail.com>\n> ---\n>     remote: align --verbose output with spaces\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1837%2Flouiswpf%2Fremote-align-verbose-output-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1837/louiswpf/remote-align-verbose-output-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1837\n> \n>  builtin/remote.c  | 30 ++++++++++++++++++++++++++----\n>  t/t5505-remote.sh |  4 ++--\n>  2 files changed, 28 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 1ad3e70a6b4..876274d9dca 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -16,6 +16,7 @@\n>  #include \"strvec.h\"\n>  #include \"commit-reach.h\"\n>  #include \"progress.h\"\n> +#include \"utf8.h\"\n>  \n>  static const char * const builtin_remote_usage[] = {\n>  \t\"git remote [-v | --verbose]\",\n> @@ -1279,6 +1280,20 @@ static int get_one_entry(struct remote *remote, void *priv)\n>  \treturn 0;\n>  }\n>  \n> +static int calc_maxwidth(struct string_list *list)\n> +{\n> +\tint max = 0;\n> +\n> +\tfor (int i = 0; i < list->nr; i++) {\n\nNit: we should use \"size_t\" to declare/define loop variable `i`\nbecause the type of `list-nr` is `size_t`.\n\nRecently, Patrick has provided a patch to start warn unsigned value\ncompared with signed value in [1] which has not been merged into the\nmaster.\n\n[1] https://lore.kernel.org/git/20241206-pks-sign-compare-v4-0-0344c6dfb219@pks.im\n\n> +\t\tstruct string_list_item *item = list->items + i;\n> +\t\tint w = utf8_strwidth(item->string);\n> +\n> +\t\tif (w > max)\n> +\t\t\tmax = w;\n> +\t}\n> +\treturn max;\n> +}\n> +\n\nSo, here we traverse the list to find the max \"utf8_strwidth\". However,\nwe should not EXPLICITLY traverse the string list. There are two ways\nyou could do:\n\n1. Use the helper macro \"for_each_string_list_item\" in \"string-list.h\"\nto do above.\n2. Use the helper function \"for_each_string_list\" in \"string-list.c\" to\ndo above.\n\n>  static int show_all(void)\n>  {\n>  \tstruct string_list list = STRING_LIST_INIT_DUP;\n> @@ -1292,10 +1307,17 @@ static int show_all(void)\n>  \t\tstring_list_sort(&list);\n>  \t\tfor (i = 0; i < list.nr; i++) {\n>  \t\t\tstruct string_list_item *item = list.items + i;\n> -\t\t\tif (verbose)\n> -\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n> -\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n> -\t\t\telse {\n> +\t\t\tif (verbose) {\n> +\t\t\t\tstruct strbuf s = STRBUF_INIT;\n> +\n> +\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT,\n> +\t\t\t\t\t\t  calc_maxwidth(&list) + 1,\n> +\t\t\t\t\t\t  item->string);\n\nSo, here we call `calc_maxwidth` in the loop. That does not make sense.\nWe should not call this function when we are traversing the string list.\nI think we should firstly calculate the max width outside of the loop.\n\nThanks,\nJialuo\n"},{"id":"509224","messageId":"1812003825ec9aea.c3843a5641fc0dd3.ac43970f069a1446@host","threadId":"62653","inReplyTo":"Z2F7VAsTjfBCVCrG@ArchLinux","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"Wang Bing-hua","fromEmail":"louiswpf@gmail.com","sentAt":"2024-12-17T15:24:07Z","receivedAt":"2024-12-17T15:24:10Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"\n\nOn 17/12/2024 21:23, shejialuo wrote:\n> \n> Good enhancement.\n> \n\nThank you.\n\n> \n> Nit: we should use \"size_t\" to declare/define loop variable `i`\n> because the type of `list-nr` is `size_t`.\n> \n> Recently, Patrick has provided a patch to start warn unsigned value\n> compared with signed value in [1] which has not been merged into the\n> master.\n> \n> [1] https://lore.kernel.org/git/20241206-pks-sign-compare-v4-0-0344c6dfb219@pks.im\n> \n\n> \n> So, here we traverse the list to find the max \"utf8_strwidth\". However,\n> we should not EXPLICITLY traverse the string list. There are two ways\n> you could do:\n> \n> 1. Use the helper macro \"for_each_string_list_item\" in \"string-list.h\"\n> to do above.\n> 2. Use the helper function \"for_each_string_list\" in \"string-list.c\" to\n> do above.\n> \n\n> \n> So, here we call `calc_maxwidth` in the loop. That does not make sense.\n> We should not call this function when we are traversing the string list.\n> I think we should firstly calculate the max width outside of the loop.\n> \n\nThank you for the detailed comments. It really helps.\nI will address these issues in v2.\n\n"},{"id":"509226","messageId":"pull.1837.v2.git.1734455884405.gitgitgadget@gmail.com","threadId":"62653","inReplyTo":"pull.1837.git.1734439176360.gitgitgadget@gmail.com","subject":"[PATCH v2] remote: align --verbose output with spaces","fromName":"Wang Bing-hua via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-17T17:18:04Z","receivedAt":"2024-12-17T17:18:08Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"From: Wang Bing-hua <louiswpf@gmail.com>\n\nRemote names exceeding a tab width could cause misalignment.\nAlign --verbose output with spaces instead of a tab.\n\nSigned-off-by: Wang Bing-hua <louiswpf@gmail.com>\n---\n    remote: align --verbose output with spaces\n    \n    Changes in v2:\n    \n     * Use for_each_string_list_item() to traverse string lists.\n     * Calculate the max width outside of the loop.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1837%2Flouiswpf%2Fremote-align-verbose-output-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1837/louiswpf/remote-align-verbose-output-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1837\n\nRange-diff vs v1:\n\n 1:  960a18efc36 ! 1:  648881dbf0d remote: align --verbose output with spaces\n     @@ builtin/remote.c: static int get_one_entry(struct remote *remote, void *priv)\n      +static int calc_maxwidth(struct string_list *list)\n      +{\n      +\tint max = 0;\n     ++\tstruct string_list_item *item;\n      +\n     -+\tfor (int i = 0; i < list->nr; i++) {\n     -+\t\tstruct string_list_item *item = list->items + i;\n     ++\tfor_each_string_list_item (item, list) {\n      +\t\tint w = utf8_strwidth(item->string);\n      +\n      +\t\tif (w > max)\n     @@ builtin/remote.c: static int get_one_entry(struct remote *remote, void *priv)\n       {\n       \tstruct string_list list = STRING_LIST_INIT_DUP;\n      @@ builtin/remote.c: static int show_all(void)\n     + \tresult = for_each_remote(get_one_entry, &list);\n     + \n     + \tif (!result) {\n     +-\t\tint i;\n     ++\t\tint maxwidth = 0;\n     ++\t\tstruct string_list_item *item;\n     + \n     ++\t\tif (verbose)\n     ++\t\t\tmaxwidth = calc_maxwidth(&list);\n       \t\tstring_list_sort(&list);\n     - \t\tfor (i = 0; i < list.nr; i++) {\n     - \t\t\tstruct string_list_item *item = list.items + i;\n     +-\t\tfor (i = 0; i < list.nr; i++) {\n     +-\t\t\tstruct string_list_item *item = list.items + i;\n      -\t\t\tif (verbose)\n      -\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n      -\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n      -\t\t\telse {\n     +-\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n     ++\t\tfor_each_string_list_item (item, &list) {\n      +\t\t\tif (verbose) {\n      +\t\t\t\tstruct strbuf s = STRBUF_INIT;\n      +\n     -+\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT,\n     -+\t\t\t\t\t\t  calc_maxwidth(&list) + 1,\n     ++\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT, maxwidth + 1,\n      +\t\t\t\t\t\t  item->string);\n      +\t\t\t\tif (item->util)\n      +\t\t\t\t\tstrbuf_addstr(&s, item->util);\n      +\t\t\t\tprintf(\"%s\\n\", s.buf);\n      +\t\t\t\tstrbuf_release(&s);\n      +\t\t\t} else {\n     - \t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n     ++\t\t\t\tif (item != list.items &&\n     ++\t\t\t\t    !strcmp((item - 1)->string, item->string))\n       \t\t\t\t\tcontinue;\n       \t\t\t\tprintf(\"%s\\n\", item->string);\n     + \t\t\t}\n      \n       ## t/t5505-remote.sh ##\n      @@ t/t5505-remote.sh: test_expect_success 'without subcommand' '\n\n\n builtin/remote.c  | 40 ++++++++++++++++++++++++++++++++--------\n t/t5505-remote.sh |  4 ++--\n 2 files changed, 34 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 1ad3e70a6b4..1e9106530c0 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -16,6 +16,7 @@\n #include \"strvec.h\"\n #include \"commit-reach.h\"\n #include \"progress.h\"\n+#include \"utf8.h\"\n \n static const char * const builtin_remote_usage[] = {\n \t\"git remote [-v | --verbose]\",\n@@ -1279,6 +1280,20 @@ static int get_one_entry(struct remote *remote, void *priv)\n \treturn 0;\n }\n \n+static int calc_maxwidth(struct string_list *list)\n+{\n+\tint max = 0;\n+\tstruct string_list_item *item;\n+\n+\tfor_each_string_list_item (item, list) {\n+\t\tint w = utf8_strwidth(item->string);\n+\n+\t\tif (w > max)\n+\t\t\tmax = w;\n+\t}\n+\treturn max;\n+}\n+\n static int show_all(void)\n {\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n@@ -1287,16 +1302,25 @@ static int show_all(void)\n \tresult = for_each_remote(get_one_entry, &list);\n \n \tif (!result) {\n-\t\tint i;\n+\t\tint maxwidth = 0;\n+\t\tstruct string_list_item *item;\n \n+\t\tif (verbose)\n+\t\t\tmaxwidth = calc_maxwidth(&list);\n \t\tstring_list_sort(&list);\n-\t\tfor (i = 0; i < list.nr; i++) {\n-\t\t\tstruct string_list_item *item = list.items + i;\n-\t\t\tif (verbose)\n-\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n-\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n-\t\t\telse {\n-\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n+\t\tfor_each_string_list_item (item, &list) {\n+\t\t\tif (verbose) {\n+\t\t\t\tstruct strbuf s = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT, maxwidth + 1,\n+\t\t\t\t\t\t  item->string);\n+\t\t\t\tif (item->util)\n+\t\t\t\t\tstrbuf_addstr(&s, item->util);\n+\t\t\t\tprintf(\"%s\\n\", s.buf);\n+\t\t\t\tstrbuf_release(&s);\n+\t\t\t} else {\n+\t\t\t\tif (item != list.items &&\n+\t\t\t\t    !strcmp((item - 1)->string, item->string))\n \t\t\t\t\tcontinue;\n \t\t\t\tprintf(\"%s\\n\", item->string);\n \t\t\t}\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 08424e878e1..6586f020f74 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -249,8 +249,8 @@ test_expect_success 'without subcommand' '\n \n test_expect_success 'without subcommand accepts -v' '\n \tcat >expect <<-EOF &&\n-\torigin\t$(pwd)/one (fetch)\n-\torigin\t$(pwd)/one (push)\n+\torigin $(pwd)/one (fetch)\n+\torigin $(pwd)/one (push)\n \tEOF\n \tgit -C test remote -v >actual &&\n \ttest_cmp expect actual\n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\n-- \ngitgitgadget\n"},{"id":"509238","messageId":"xmqqr066ysa6.fsf@gitster.g","threadId":"62653","inReplyTo":"Z2F7VAsTjfBCVCrG@ArchLinux","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-17T20:21:37Z","receivedAt":"2024-12-17T20:21:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"shejialuo <shejialuo@gmail.com> writes:\n\n> On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:\n>> From: Wang Bing-hua <louiswpf@gmail.com>\n>> \n>> Remote names exceeding a tab width could cause misalignment.\n\nIf all of them are named with ten ASCII characters, on a terminal\nwith fixed-width font, things will still display aligned ;-)\n\n>> Align --verbose output with spaces instead of a tab.\n>> \n>\n> Good enhancement.\n\nWith a Devil's advocate hat on, a change like this will completely\nbreak tools when they are reading the \"--verbose\" output and\nexpecting that the fields are separated with a TAB (in other words,\nthe tab is *not* about alignment in the first place for them).\n\nNow with that hat off.\n\nFor users with that many remotes where the alignment of URLs in the\ninteractive \"git remote -v\" output matter, I am not sure if this\nchange is a real improvement enough that it is worth the possible\nrisk of breaking existing tools.  With that many remotes defined,\nwouldn't they be doing \"git remote -v\" piped to \"grep '^name<TAB>'\"\nor something?  That use case would break with the change, too.\n\nThanks.\n\n"},{"id":"509240","messageId":"xmqq4j32yr3c.fsf@gitster.g","threadId":"62653","inReplyTo":"pull.1837.v2.git.1734455884405.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] remote: align --verbose output with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-17T20:47:19Z","receivedAt":"2024-12-17T20:47:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Wang Bing-hua via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Wang Bing-hua <louiswpf@gmail.com>\n>\n> Remote names exceeding a tab width could cause misalignment.\n> Align --verbose output with spaces instead of a tab.\n\nWhile I am still not convinced if this change is a good idea (see\nmy earlier comment in a separate message)...\n\n> +static int calc_maxwidth(struct string_list *list)\n> +{\n> +\tint max = 0;\n> +\tstruct string_list_item *item;\n> +\n> +\tfor_each_string_list_item (item, list) {\n> +\t\tint w = utf8_strwidth(item->string);\n> +\n> +\t\tif (w > max)\n> +\t\t\tmax = w;\n> +\t}\n> +\treturn max;\n> +}\n> +\n>  static int show_all(void)\n>  {\n>  \tstruct string_list list = STRING_LIST_INIT_DUP;\n> @@ -1287,16 +1302,25 @@ static int show_all(void)\n>  \tresult = for_each_remote(get_one_entry, &list);\n>  \n>  \tif (!result) {\n> -\t\tint i;\n> +\t\tint maxwidth = 0;\n> +\t\tstruct string_list_item *item;\n>  \n> +\t\tif (verbose)\n> +\t\t\tmaxwidth = calc_maxwidth(&list);\n\nI wonder if it is a better idea to extend get_one_entry() interface\nto take not just a string_list but something like\n\n\tstruct remotes_data {\n\t\tint maxwidth;\n\t\tstruct string_list *list_of_remotes;\n\t};\n\nif we think it is a good idea to give richer output to show_all()\nfunction (instead of keep it spartan and compatible for the sake of\nnot breaking machine readers).  There may be things other than\nmaxwidth that future changes to \"git remote [-v]\" may find needed.\nAnd with such a change, you do not need a separate iteration over\nthe list of remotes just to call calc_maxwidth() callback.  Keeping\na tally of \"max length we have seen\" inside get_one_entry() regardless\nof \"--verbose\" setting shouldn't be too costly and help reduce the\ncomplexity of the code.\n\n>  \t\tstring_list_sort(&list);\n> -\t\tfor (i = 0; i < list.nr; i++) {\n> -\t\t\tstruct string_list_item *item = list.items + i;\n> -\t\t\tif (verbose)\n> -\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n> -\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n> -\t\t\telse {\n> -\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n> +\t\tfor_each_string_list_item (item, &list) {\n\nUse of for_each_string_list_item() instead of a manual iteration is\nprobably a good idea here.  If this were a larger change, that may\ndeserve to be a preparatory step on its own, but it is probably OK\nto do so in the same patch.\n\n> +\t\t\tif (verbose) {\n> +\t\t\t\tstruct strbuf s = STRBUF_INIT;\n> +\n> +\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT, maxwidth + 1,\n> +\t\t\t\t\t\t  item->string);\n> +\t\t\t\tif (item->util)\n> +\t\t\t\t\tstrbuf_addstr(&s, item->util);\n> +\t\t\t\tprintf(\"%s\\n\", s.buf);\n> +\t\t\t\tstrbuf_release(&s);\n\nWouldn't it work to just do (totally untested code snippet below;\nmay have off-by-one around maxwidth)\n\n\t\t\t\tprintf(\"%.*s%s\", maxwidth, item->string,\n\t\t\t\t\titem->util ? \"\" : item->util);\n\nwithout using any strbuf operation?\n"},{"id":"509275","messageId":"18122f6da7729ff9.8ab23eb43f7bb6f4.c74125d7d6163423@host","threadId":"62653","inReplyTo":"xmqqr066ysa6.fsf@gitster.g","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"Wang Bing-hua","fromEmail":"louiswpf@gmail.com","sentAt":"2024-12-18T05:49:14Z","receivedAt":"2024-12-18T05:49:17Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"\n\nOn 17/12/2024 12:21, Junio C Hamano wrote:\n>> On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:\n>>> From: Wang Bing-hua <louiswpf@gmail.com>\n>>> \n>>> Remote names exceeding a tab width could cause misalignment.\n> \n> If all of them are named with ten ASCII characters, on a terminal\n> with fixed-width font, things will still display aligned ;-)\n\nIndeed :)\nI did consider this scenario and wrote \"could\". I should have worded\nthis more clearly.\n\n> With a Devil's advocate hat on, a change like this will completely\n> break tools when they are reading the \"--verbose\" output and\n> expecting that the fields are separated with a TAB (in other words,\n> the tab is *not* about alignment in the first place for them).\n> \n> Now with that hat off.\n> \n> For users with that many remotes where the alignment of URLs in the\n> interactive \"git remote -v\" output matter, I am not sure if this\n> change is a real improvement enough that it is worth the possible\n> risk of breaking existing tools.  With that many remotes defined,\n> wouldn't they be doing \"git remote -v\" piped to \"grep '^name<TAB>'\"\n> or something?  That use case would break with the change, too.\n\nAgreed on all points. Another point to consider is to align with\n\"git branch -v\" since it also uses spaces for alignment.\nThis patch was also originally lifted from \"builtin/branch.c\".\n\n"},{"id":"509281","messageId":"1812389a7b6fe18d.7946240d95ed36ca.1b2132d5c888cc21@host","threadId":"62653","inReplyTo":"xmqq4j32yr3c.fsf@gitster.g","subject":"Re: [PATCH v2] remote: align --verbose output with spaces","fromName":"Wang Bing-hua","fromEmail":"louiswpf@gmail.com","sentAt":"2024-12-18T08:37:22Z","receivedAt":"2024-12-18T08:37:25Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"\n\nOn 17/12/2024 12:47, Junio C Hamano wrote:\n> I wonder if it is a better idea to extend get_one_entry() interface\n> to take not just a string_list but something like\n> \n> \tstruct remotes_data {\n> \t\tint maxwidth;\n> \t\tstruct string_list *list_of_remotes;\n> \t};\n> \n> if we think it is a good idea to give richer output to show_all()\n> function (instead of keep it spartan and compatible for the sake of\n> not breaking machine readers).  There may be things other than\n> maxwidth that future changes to \"git remote [-v]\" may find needed.\n> And with such a change, you do not need a separate iteration over\n> the list of remotes just to call calc_maxwidth() callback.  Keeping\n> a tally of \"max length we have seen\" inside get_one_entry() regardless\n> of \"--verbose\" setting shouldn't be too costly and help reduce the\n> complexity of the code.\n\nThis is a great idea.\n\n> \n>>  \t\tstring_list_sort(&list);\n>> -\t\tfor (i = 0; i < list.nr; i++) {\n>> -\t\t\tstruct string_list_item *item = list.items + i;\n>> -\t\t\tif (verbose)\n>> -\t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n>> -\t\t\t\t\titem->util ? (const char *)item->util : \"\");\n>> -\t\t\telse {\n>> -\t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n>> +\t\tfor_each_string_list_item (item, &list) {\n> \n> Use of for_each_string_list_item() instead of a manual iteration is\n> probably a good idea here.  If this were a larger change, that may\n> deserve to be a preparatory step on its own, but it is probably OK\n> to do so in the same patch.\n\nThanks for the reminder.\n\n> \n>> +\t\t\tif (verbose) {\n>> +\t\t\t\tstruct strbuf s = STRBUF_INIT;\n>> +\n>> +\t\t\t\tstrbuf_utf8_align(&s, ALIGN_LEFT, maxwidth + 1,\n>> +\t\t\t\t\t\t  item->string);\n>> +\t\t\t\tif (item->util)\n>> +\t\t\t\t\tstrbuf_addstr(&s, item->util);\n>> +\t\t\t\tprintf(\"%s\\n\", s.buf);\n>> +\t\t\t\tstrbuf_release(&s);\n> \n> Wouldn't it work to just do (totally untested code snippet below;\n> may have off-by-one around maxwidth)\n> \n> \t\t\t\tprintf(\"%.*s%s\", maxwidth, item->string,\n> \t\t\t\t\titem->util ? \"\" : item->util);\n> \n> without using any strbuf operation?\n\nI did try to use printf at first.\n\n\t\t\t\tprintf(\"%-*s %s\\n\", maxwidth, item->string,\n\t\t\t\t       item->util ? (const char *)item->util :\n\t\t\t\t\t\t    \"\");\n\nBut it broke when there are non-ASCII characters. For example:\n\n$ git remote -v\na  url (fetch)\na  url (push)\nå url (fetch)\nå url (push)\nåå url (fetch)\nåå url (push)\n\n\nThank you for reviewing. I'm also debating. It's great to align\n\"remote -v\" and make it behave similarly to \"branch -v\". But it might\nnot be worth it to complicate the code and break machine readers.\nDo we continue working on this?\n\n"},{"id":"509292","messageId":"Z2LTp60u03Y9iiUd@ArchLinux","threadId":"62653","inReplyTo":"xmqqr066ysa6.fsf@gitster.g","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-18T13:52:39Z","receivedAt":"2024-12-18T13:51:59Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Tue, Dec 17, 2024 at 12:21:37PM -0800, Junio C Hamano wrote:\n> shejialuo <shejialuo@gmail.com> writes:\n> \n> > On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:\n> >> From: Wang Bing-hua <louiswpf@gmail.com>\n> >> \n> >> Remote names exceeding a tab width could cause misalignment.\n> \n> If all of them are named with ten ASCII characters, on a terminal\n> with fixed-width font, things will still display aligned ;-)\n> \n> >> Align --verbose output with spaces instead of a tab.\n> >> \n> >\n> > Good enhancement.\n> \n> With a Devil's advocate hat on, a change like this will completely\n> break tools when they are reading the \"--verbose\" output and\n> expecting that the fields are separated with a TAB (in other words,\n> the tab is *not* about alignment in the first place for them).\n> \n\nYes, I agree. Although we may provide convenience for the end user, but\nwe may break other tools parsing the output. I think I bring confusion\nto the Wang. Actually, I have seen that Wang contributes to Git in the\nfirst time, so I just want to courage.\n\n> Now with that hat off.\n> \n> For users with that many remotes where the alignment of URLs in the\n> interactive \"git remote -v\" output matter, I am not sure if this\n> change is a real improvement enough that it is worth the possible\n> risk of breaking existing tools.  With that many remotes defined,\n> wouldn't they be doing \"git remote -v\" piped to \"grep '^name<TAB>'\"\n> or something?  That use case would break with the change, too.\n> \n> Thanks.\n"},{"id":"509293","messageId":"18124abbd9d6169f.305d7269b347c5c2.ae083dd7649e4539@host","threadId":"62653","inReplyTo":"Z2LTp60u03Y9iiUd@ArchLinux","subject":"Re: [PATCH] remote: align --verbose output with spaces","fromName":"Wang Bing-hua","fromEmail":"louiswpf@gmail.com","sentAt":"2024-12-18T14:09:37Z","receivedAt":"2024-12-18T14:09:39Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"\n\nOn 18/12/2024 21:52, shejialuo wrote:\n> Yes, I agree. Although we may provide convenience for the end user, but\n> we may break other tools parsing the output. I think I bring confusion\n> to the Wang. Actually, I have seen that Wang contributes to Git in the\n> first time, so I just want to courage.\n\nNo worries. Thank you for encouraging words :)\n\n"},{"id":"509302","messageId":"xmqqy10dt2zb.fsf@gitster.g","threadId":"62653","inReplyTo":"1812389a7b6fe18d.7946240d95ed36ca.1b2132d5c888cc21@host","subject":"Re: [PATCH v2] remote: align --verbose output with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-18T15:39:20Z","receivedAt":"2024-12-18T15:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Wang Bing-hua\" <louiswpf@gmail.com> writes:\n\n>> Wouldn't it work to just do (totally untested code snippet below;\n>> may have off-by-one around maxwidth)\n>> \n>> \t\t\t\tprintf(\"%.*s%s\", maxwidth, item->string,\n>> \t\t\t\t\titem->util ? \"\" : item->util);\n>> \n>> without using any strbuf operation?\n>\n> I did try to use printf at first.\n>\n> \t\t\t\tprintf(\"%-*s %s\\n\", maxwidth, item->string,\n> \t\t\t\t       item->util ? (const char *)item->util :\n> \t\t\t\t\t\t    \"\");\n>\n> But it broke when there are non-ASCII characters. For example:\n\nAh, of course, I should have double-checked, but it should be more\nlike\n\n\tprintf(\"%.*s%s%s\",\n\t       maxwidth + 1 - utf8_strwidth(item->string), \"\",\n\t       item->string,\n\t       item->util ? item->util : \"\");\n\nmeaning\n\n (1) the first output field must have maxwidth+1 - the display width\n     the second output field takes.  The field's contents is empty, so\n     we get enough SP padded to make the total of this first field\n     and the second field to make maxwidth+1.\n\n (2) the second output field is item->string itself.\n\n (3) the third output field has item->util if exists.\n\n> Thank you for reviewing. I'm also debating. It's great to align\n> \"remote -v\" and make it behave similarly to \"branch -v\". But it might\n> not be worth it to complicate the code and break machine readers.\n> Do we continue working on this?\n\nYou already know from my initial reaction what my answer would be,\nbut I am inclined to hear from others before we make a decision.\n\nTHanks.\n"},{"id":"509320","messageId":"1812724a6e98b9f0.772d5fa147f10a1c.6ab8f72f53fdfcab@host","threadId":"62653","inReplyTo":"xmqqy10dt2zb.fsf@gitster.g","subject":"Re: [PATCH v2] remote: align --verbose output with spaces","fromName":"Wang Bing-hua","fromEmail":"louiswpf@gmail.com","sentAt":"2024-12-19T02:14:30Z","receivedAt":"2024-12-19T02:14:33Z","isPatch":true,"sender":{"key":"louiswpf@gmail.com","avatar":null},"body":"\n\nOn 18/12/2024 07:39, Junio C Hamano wrote:\n> Ah, of course, I should have double-checked, but it should be more\n> like\n> \n> \tprintf(\"%.*s%s%s\",\n> \t       maxwidth + 1 - utf8_strwidth(item->string), \"\",\n> \t       item->string,\n> \t       item->util ? item->util : \"\");\n> \n> meaning\n> \n>  (1) the first output field must have maxwidth+1 - the display width\n>      the second output field takes.  The field's contents is empty, so\n>      we get enough SP padded to make the total of this first field\n>      and the second field to make maxwidth+1.\n> \n>  (2) the second output field is item->string itself.\n> \n>  (3) the third output field has item->util if exists.\n\nUnderstood. Thanks for the explanation.\n\nHere is a tested code snippet for the record:\n\n\tprintf(\"%s%*s %s\\n\",\n\t       item->string,\n\t       maxwidth - utf8_strwidth(item->string), \"\",\n\t       item->util ? (const char *)item->util : \"\");\n\n> You already know from my initial reaction what my answer would be,\n> but I am inclined to hear from others before we make a decision.\n\nI see. Thanks.\n\n"}]}