remote: align --verbose output with spaces

From: Wang Bing-hua <[email protected]>

Remote names exceeding a tab width could cause misalignment. Align --verbose output with spaces instead of a tab.

Signed-off-by Wang Bing-hua <[email protected]>

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

builtin/remote.c +1 -0

index 1ad3e70a6b4..876274d9dca 100644
@@ -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]",

builtin/remote.c +0 -0

 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;

Good enhancement.

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/[email protected]

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.

Quoted 3 lines · Wang Bing-hua via GitGitGadget

From: Wang Bing-hua <[email protected]>

Remote names exceeding a tab width could cause misalignment.

Quoted 2 lines

Align --verbose output with spaces instead of a tab.

Good enhancement.

Quoted 4 lines · Wang Bing-hua via GitGitGadget

Quoted 3 lines

From: Wang Bing-hua <[email protected]>

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

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.

Quoted 4 lines · shejialuo

Quoted 3 lines · Wang Bing-hua via GitGitGadget

From: Wang Bing-hua <[email protected]>

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

Quoted 3 lines

Quoted 2 lines

Align --verbose output with spaces instead of a tab.

Quoted 1 line

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.

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.

From: Wang Bing-hua <[email protected]>

Remote names exceeding a tab width could cause misalignment. Align --verbose output with spaces instead of a tab.

builtin/remote.c +14 -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;

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.

builtin/remote.c static int show_all(void) · line 1311

Quoted 10 lines

builtin/remote.c +1 -7

 		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.

builtin/remote.c static int show_all(void) · line 1320

Quoted 10 lines

builtin/remote.c +9 -0

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

Quoted 7 lines

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:

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?

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.

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.