git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
WBWang Bing-hua <louiswpf@gmail.com>
Date
Dec 18, 2024, 08:37 UTC
Message-ID
<1812389a7b6fe18d.7946240d95ed36ca.1b2132d5c888cc21@host>
In-Reply-To
<xmqq4j32yr3c.fsf@gitster.g>
On 17/12/2024 12:47, Junio C Hamano wrote:
Show 17 quoted lines
> 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.
Show 15 quoted lines
> 
>>  		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.
Show 18 quoted lines
> 
>> +			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?

Previous: Junio C HamanoNext: Junio C Hamano
Message 10 of 12 in “remote: align --verbose output with spaces”
  1. remote: align --verbose output with spacesWang Bing-hua via GitGitGadget, Dec 17, 2024
  2. shejialuoDec 17, 2024
  3. Wang Bing-huaDec 17, 2024
  4. Junio C HamanoDec 17, 2024
  5. Wang Bing-huaDec 18, 2024
  6. shejialuoDec 18, 2024
  7. Wang Bing-huaDec 18, 2024
  8. remote: align --verbose output with spacesWang Bing-hua via GitGitGadget, Dec 17, 2024
  9. Junio C HamanoDec 17, 2024
  10. Wang Bing-huaDec 18, 2024
  11. Junio C HamanoDec 18, 2024
  12. Wang Bing-huaDec 19, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.