{"thread":{"id":"59210","subject":"[PATCH] remote: align columns on -v","startedAt":"2023-02-07T23:52:56Z","lastAt":"2023-02-09T01:12:33Z","messageCount":7,"participants":["Kir Kolyshkin","Torsten Bögershausen","Junio C Hamano","Kirill Kolyshkin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471742","messageId":"20230207235238.1850757-1-kolyshkin@gmail.com","threadId":"59210","inReplyTo":null,"subject":"[PATCH] remote: align columns on -v","fromName":"Kir Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2023-02-07T23:52:38Z","receivedAt":"2023-02-07T23:52:56Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"Currently, git remote -v produces a misaligned output when a remote name\nis more than 8 characters long (i.e. longer than a tab step). Here's how\nit looks like:\n\ngiuseppe\thttps://github.com/giuseppe/runc (fetch)\ngiuseppe\thttps://github.com/giuseppe/runc (push)\nkir\tgit@github.com:kolyshkin/runc.git (fetch)\nkir\tgit@github.com:kolyshkin/runc.git (push)\nlifubang\thttps://github.com/lifubang/runc (fetch)\nlifubang\thttps://github.com/lifubang/runc (push)\nmarquiz\thttps://github.com/marquiz/runc (fetch)\nmarquiz\thttps://github.com/marquiz/runc (push)\n\nLet's find the maximum width and use it for alignment.\n\nWhile at it, let's keep the \\t in case some tools depend on it\nfor parsing (there will still be trailing spaces in the remote name).\n\nWith this change, the output is like this now:\n\ngiuseppe \thttps://github.com/giuseppe/runc (fetch)\ngiuseppe \thttps://github.com/giuseppe/runc (push)\nkir \tgit@github.com:kolyshkin/runc.git (fetch)\nkir \tgit@github.com:kolyshkin/runc.git (push)\nlifubang \thttps://github.com/lifubang/runc (fetch)\nlifubang \thttps://github.com/lifubang/runc (push)\nmarquiz \thttps://github.com/marquiz/runc (fetch)\nmarquiz \thttps://github.com/marquiz/runc (push)\n\nReported-by: Roman Dodin \nSigned-off-by: Kir Kolyshkin \n---\n builtin/remote.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 729f6f3643..116417574d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1245,13 +1245,21 @@ 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 i, width = 7;\n+\n+\t\tif (verbose) {\n+\t\t\tfor (i = 0; i < list.nr; i++) {\n+\t\t\t\tint len = strlen((list.items + i)->string);\n+\t\t\t\tif (len > width)\n+\t\t\t\t\twidth = len;\n+\t\t\t}\n+\t\t}\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\tprintf(\"%-*s\\t%s\\n\", width, 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-- \n2.39.0\n\n"},{"id":"471771","messageId":"20230208161222.guqqwlceqiy3jq7e@tb-raspi4","threadId":"59210","inReplyTo":"20230207235238.1850757-1-kolyshkin@gmail.com","subject":"Re: [PATCH] remote: align columns on -v","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-02-08T16:12:22Z","receivedAt":"2023-02-08T16:12:29Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Feb 07, 2023 at 03:52:38PM -0800, Kir Kolyshkin wrote:\n> Currently, git remote -v produces a misaligned output when a remote name\n> is more than 8 characters long (i.e. longer than a tab step). Here's how\n> it looks like:\n>\n> giuseppe\thttps://github.com/giuseppe/runc (fetch)\n> giuseppe\thttps://github.com/giuseppe/runc (push)\n> kir\tgit@github.com:kolyshkin/runc.git (fetch)\n> kir\tgit@github.com:kolyshkin/runc.git (push)\n> lifubang\thttps://github.com/lifubang/runc (fetch)\n> lifubang\thttps://github.com/lifubang/runc (push)\n> marquiz\thttps://github.com/marquiz/runc (fetch)\n> marquiz\thttps://github.com/marquiz/runc (push)\n>\n> Let's find the maximum width and use it for alignment.\n>\n> While at it, let's keep the \\t in case some tools depend on it\n> for parsing (there will still be trailing spaces in the remote name).\n>\n> With this change, the output is like this now:\n>\n> giuseppe \thttps://github.com/giuseppe/runc (fetch)\n> giuseppe \thttps://github.com/giuseppe/runc (push)\n> kir \tgit@github.com:kolyshkin/runc.git (fetch)\n> kir \tgit@github.com:kolyshkin/runc.git (push)\n> lifubang \thttps://github.com/lifubang/runc (fetch)\n> lifubang \thttps://github.com/lifubang/runc (push)\n> marquiz \thttps://github.com/marquiz/runc (fetch)\n> marquiz \thttps://github.com/marquiz/runc (push)\n>\n\nThanks for working on that - I had the same wish as well.\nHowever, I am tempted to comment on some details here.\nEspecially, what happens if a remote is named with a non-ASCII\ncharacter (unicode code point would be a better term) ?\nTo determine the width on screen for aligment, strlen()\ndoes the wrong thing here.\nThis has been done at other place (being UTF-8 aware),\nyou may want to have a look at this change:\n\n commit 12fc4ad89e23af642a8614371ff80bc67cb3315d\n Author: Torsten Bögershausen \n Date: Wed Sep 14 17:13:33 2022 +0200\n\n diff.c: use utf8_strwidth() to count display width\n\n\n\n> Reported-by: Roman Dodin \n> Signed-off-by: Kir Kolyshkin \n> ---\n> builtin/remote.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 729f6f3643..116417574d 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1245,13 +1245,21 @@ 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 i, width = 7;\n> +\n> +\t\tif (verbose) {\n> +\t\t\tfor (i = 0; i < list.nr; i++) {\n> +\t\t\t\tint len = strlen((list.items + i)->string);\n> +\t\t\t\tif (len > width)\n> +\t\t\t\t\twidth = len;\n> +\t\t\t}\n> +\t\t}\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\tprintf(\"%-*s\\t%s\\n\", width, 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> --\n> 2.39.0\n>\n"},{"id":"471772","messageId":"xmqq5yccuo2s.fsf@gitster.g","threadId":"59210","inReplyTo":"20230207235238.1850757-1-kolyshkin@gmail.com","subject":"Re: [PATCH] remote: align columns on -v","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T16:19:39Z","receivedAt":"2023-02-08T16:19:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kir Kolyshkin writes:\n\n> Currently, git remote -v produces a misaligned output when a remote name\n> is more than 8 characters long (i.e. longer than a tab step). Here's how\n> it looks like:\n\nThe condition under which URLs do not align is not when they are\nmore than 8 characters long. If all of your remotes have\n10-character names, their URLs would perfectly align, no? The\ndescription may need to be tightened if we really wanted to do this.\n\nBut I am skeptical, even without my devil's advocate hat on.\n\n> giuseppe\thttps://github.com/giuseppe/runc (fetch)\n> giuseppe\thttps://github.com/giuseppe/runc (push)\n> kir\tgit@github.com:kolyshkin/runc.git (fetch)\n> ...\n\nThe current output allows programs to post-process by splitting each\nline with a tab, but this change will break such practice and force\nthose who use such practice to do something different (like \"split\nat the first run of whitespaces or tabs\").\n\n> While at it, let's keep the \\t in case some tools depend on it\n> for parsing (there will still be trailing spaces in the remote name).\n\nThat will not help avoid breaking the behaviour for existing\npractice (they did not need to strip the whitespaces, but now they\nare forced to). It only make the output uglier by putting mixture\nof whitespaces and tabs.\n\nSo, I dunno.\n\nThanks.\n"},{"id":"471774","messageId":"xmqq1qn0ungo.fsf@gitster.g","threadId":"59210","inReplyTo":"20230208161222.guqqwlceqiy3jq7e@tb-raspi4","subject":"Re: [PATCH] remote: align columns on -v","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T16:32:55Z","receivedAt":"2023-02-08T16:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Especially, what happens if a remote is named with a non-ASCII\n> character (unicode code point would be a better term) ?\n> To determine the width on screen for aligment, strlen()\n> does the wrong thing here.\n\nInteresting point. I am still skeptical about the patch and suspect\nthat it is better to stick to easy-to-parse output, but if we switch\nto align for humans, we should definitely count display columns, not\nbyte count with strlen().\n\nThanks.\n\n\n"},{"id":"471824","messageId":"CAGmPdrzpEvjCh8u4-FjsaE3tmqfiE3xyytO-++56JrjcMEgP=A@mail.gmail.com","threadId":"59210","inReplyTo":"20230208161222.guqqwlceqiy3jq7e@tb-raspi4","subject":"Re: [PATCH] remote: align columns on -v","fromName":"Kirill Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2023-02-09T01:02:00Z","receivedAt":"2023-02-09T01:02:16Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"On Wed, Feb 8, 2023 at 8:12 AM Torsten Bögershausen wrote:\n>\n> On Tue, Feb 07, 2023 at 03:52:38PM -0800, Kir Kolyshkin wrote:\n> > Currently, git remote -v produces a misaligned output when a remote name\n> > is more than 8 characters long (i.e. longer than a tab step). Here's how\n> > it looks like:\n> >\n> > giuseppe https://github.com/giuseppe/runc (fetch)\n> > giuseppe https://github.com/giuseppe/runc (push)\n> > kir git@github.com:kolyshkin/runc.git (fetch)\n> > kir git@github.com:kolyshkin/runc.git (push)\n> > lifubang https://github.com/lifubang/runc (fetch)\n> > lifubang https://github.com/lifubang/runc (push)\n> > marquiz https://github.com/marquiz/runc (fetch)\n> > marquiz https://github.com/marquiz/runc (push)\n> >\n> > Let's find the maximum width and use it for alignment.\n> >\n> > While at it, let's keep the \\t in case some tools depend on it\n> > for parsing (there will still be trailing spaces in the remote name).\n> >\n> > With this change, the output is like this now:\n> >\n> > giuseppe https://github.com/giuseppe/runc (fetch)\n> > giuseppe https://github.com/giuseppe/runc (push)\n> > kir git@github.com:kolyshkin/runc.git (fetch)\n> > kir git@github.com:kolyshkin/runc.git (push)\n> > lifubang https://github.com/lifubang/runc (fetch)\n> > lifubang https://github.com/lifubang/runc (push)\n> > marquiz https://github.com/marquiz/runc (fetch)\n> > marquiz https://github.com/marquiz/runc (push)\n> >\n>\n> Thanks for working on that - I had the same wish as well.\n> However, I am tempted to comment on some details here.\n> Especially, what happens if a remote is named with a non-ASCII\n> character (unicode code point would be a better term) ?\n> To determine the width on screen for aligment, strlen()\n> does the wrong thing here.\n> This has been done at other place (being UTF-8 aware),\n> you may want to have a look at this change:\n>\n> commit 12fc4ad89e23af642a8614371ff80bc67cb3315d\n> Author: Torsten Bögershausen \n> Date: Wed Sep 14 17:13:33 2022 +0200\n>\n> diff.c: use utf8_strwidth() to count display width\n\nYes, I have seen now that neither strlen nor printf with the specified width\ndo a good job with unicode. Thank you for pointing this out, I'll send v2 soon.\n"},{"id":"471825","messageId":"CAGmPdrxcb+ztx96=aF2+ok7DgOkEnZ+znSrG4D=AE_nQ57A17w@mail.gmail.com","threadId":"59210","inReplyTo":"xmqq5yccuo2s.fsf@gitster.g","subject":"Re: [PATCH] remote: align columns on -v","fromName":"Kirill Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2023-02-09T01:11:40Z","receivedAt":"2023-02-09T01:12:16Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"On Wed, Feb 8, 2023 at 8:19 AM Junio C Hamano wrote:\n>\n> Kir Kolyshkin writes:\n>\n> > Currently, git remote -v produces a misaligned output when a remote name\n> > is more than 8 characters long (i.e. longer than a tab step). Here's how\n> > it looks like:\n>\n> The condition under which URLs do not align is not when they are\n> more than 8 characters long. If all of your remotes have\n> 10-character names, their URLs would perfectly align, no? The\n> description may need to be tightened if we really wanted to do this.\n>\n> But I am skeptical, even without my devil's advocate hat on.\n>\n> > giuseppe https://github.com/giuseppe/runc (fetch)\n> > giuseppe https://github.com/giuseppe/runc (push)\n> > kir git@github.com:kolyshkin/runc.git (fetch)\n> > ...\n>\n> The current output allows programs to post-process by splitting each\n> line with a tab, but this change will break such practice and force\n> those who use such practice to do something different (like \"split\n> at the first run of whitespaces or tabs\").\n>\n> > While at it, let's keep the \\t in case some tools depend on it\n> > for parsing (there will still be trailing spaces in the remote name).\n>\n> That will not help avoid breaking the behaviour for existing\n> practice (they did not need to strip the whitespaces, but now they\n> are forced to). It only make the output uglier by putting mixture\n> of whitespaces and tabs.\n>\n> So, I dunno.\n\nYes, I agree that this can break someone's scripts, and adding \\t was\nnot a bright idea either, as having it may hide the issue (of\nincorrect splitting)\nrather than break things entirely (and thus urging to fix it).\n\nI assume that any decent script would split by \"any whitespace\", plus\nthe output of git remote -v is mostly for the user's eyes, not scripts.\n\nPlease take a look at v2 which I am sending now.\n"},{"id":"471826","messageId":"20230209011201.1974721-1-kolyshkin@gmail.com","threadId":"59210","inReplyTo":"20230207235238.1850757-1-kolyshkin@gmail.com","subject":"[PATCH] remote: align columns on -v","fromName":"Kir Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2023-02-09T01:12:01Z","receivedAt":"2023-02-09T01:12:33Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"Currently, git remote -v produces a misaligned output when some remote\nnames are shorter and some are longer than a tab step. For example:\n\ngiuseppe\thttps://github.com/giuseppe/runc (fetch)\ngiuseppe\thttps://github.com/giuseppe/runc (push)\nkir\tgit@github.com:kolyshkin/runc.git (fetch)\nkir\tgit@github.com:kolyshkin/runc.git (push)\nlifubang\thttps://github.com/lifubang/runc (fetch)\nlifubang\thttps://github.com/lifubang/runc (push)\nmarquiz\thttps://github.com/marquiz/runc (fetch)\nmarquiz\thttps://github.com/marquiz/runc (push)\n\nLet's find the maximum width of remote and use it for alignment.\nWith this change in place, the output looks like this now:\n\ngiuseppe https://github.com/giuseppe/runc (fetch)\ngiuseppe https://github.com/giuseppe/runc (push)\nkir git@github.com:kolyshkin/runc.git (fetch)\nkir git@github.com:kolyshkin/runc.git (push)\nlifubang https://github.com/lifubang/runc (fetch)\nlifubang https://github.com/lifubang/runc (push)\nmarquiz https://github.com/marquiz/runc (fetch)\nmarquiz https://github.com/marquiz/runc (push)\n\n[v2: use utf8_strwidth and padding, fix description]\n\nReported-by: Roman Dodin \nSigned-off-by: Kir Kolyshkin \n---\n builtin/remote.c | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 729f6f3643..654472a87c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -13,6 +13,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@@ -1245,14 +1246,26 @@ 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 i, width = 7;\n+\n+\t\tif (verbose) {\n+\t\t\tfor (i = 0; i < list.nr; i++) {\n+\t\t\t\tint w = utf8_strwidth((list.items + i)->string);\n+\t\t\t\tif (w > width)\n+\t\t\t\t\twidth = w;\n+\t\t\t}\n+\t\t}\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\tif (verbose) {\n+\t\t\t\tint padding = width - utf8_strwidth(item->string);\n+\t\t\t\tif (padding < 0)\n+\t\t\t\t\tpadding = 0;\n+\t\t\t\tprintf(\"%*s%s %s\\n\", padding, \"\", item->string,\n \t\t\t\t\titem->util ? (const char *)item->util : \"\");\n+\t\t\t}\n \t\t\telse {\n \t\t\t\tif (i && !strcmp((item - 1)->string, item->string))\n \t\t\t\t\tcontinue;\n-- \n2.39.0\n\n"}]}