remote: align --verbose output with spaces #1837
Conversation
Welcome to GitGitGadgetHi @louiswpf, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
louiswpf
force-pushed
the
remote-align-verbose-output
branch
from
25bcdd1 to
960a18e
Compare
|
/allow |
|
User louiswpf is now allowed to use GitGitGadget. WARNING: louiswpf has no public email address set on GitHub; |
|
/preview |
|
Error: Could not determine full name of louiswpf |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, shejialuo wrote (reply to this): On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:
> From: Wang Bing-hua <[email protected]>
>
> Remote names exceeding a tab width could cause misalignment.
> Align --verbose output with spaces instead of a tab.
>
Good enhancement.
> 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(-)
>
> 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/[email protected]
> + 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.
> 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 |
|
User |
louiswpf
force-pushed
the
remote-align-verbose-output
branch
from
960a18e to
8d5ef78
Compare
|
On the Git mailing list, "Wang Bing-hua" wrote (reply to this): On 17/12/2024 21:23, shejialuo wrote:
>
> Good enhancement.
>
Thank you.
>
> 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.
>
Thank you for the detailed comments. It really helps.
I will address these issues in v2.
|
|
/preview |
|
Preview email sent as [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]>
louiswpf
force-pushed
the
remote-align-verbose-output
branch
from
8d5ef78 to
648881d
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): shejialuo <[email protected]> writes:
> On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:
>> 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 ;-)
>> Align --verbose output with spaces instead of a tab.
>>
>
> 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.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Wang Bing-hua via GitGitGadget" <[email protected]> writes:
> From: Wang Bing-hua <[email protected]>
>
> Remote names exceeding a tab width could cause misalignment.
> Align --verbose output with spaces instead of a tab.
While I am still not convinced if this change is a good idea (see
my earlier comment in a separate message)...
> +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;
> @@ -1287,16 +1302,25 @@ static int show_all(void)
> result = for_each_remote(get_one_entry, &list);
>
> if (!result) {
> - int i;
> + int maxwidth = 0;
> + struct string_list_item *item;
>
> + if (verbose)
> + maxwidth = calc_maxwidth(&list);
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.
> 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.
> + 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? |
|
On the Git mailing list, "Wang Bing-hua" wrote (reply to this): On 17/12/2024 12:21, Junio C Hamano wrote:
>> On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:
>>> 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 ;-)
Indeed :)
I did consider this scenario and wrote "could". I should have worded
this more clearly.
> 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.
Agreed on all points. Another point to consider is to align with
"git branch -v" since it also uses spaces for alignment.
This patch was also originally lifted from "builtin/branch.c".
|
|
On the Git mailing list, "Wang Bing-hua" wrote (reply to this): On 17/12/2024 12:47, Junio C Hamano wrote:
> 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.
>
>> 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.
>
>> + 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?
|
|
On the Git mailing list, shejialuo wrote (reply to this): On Tue, Dec 17, 2024 at 12:21:37PM -0800, Junio C Hamano wrote:
> shejialuo <[email protected]> writes:
>
> > On Tue, Dec 17, 2024 at 12:39:36PM +0000, Wang Bing-hua via GitGitGadget wrote:
> >> 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 ;-)
>
> >> Align --verbose output with spaces instead of a tab.
> >>
> >
> > 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).
>
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.
> 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. |
|
On the Git mailing list, "Wang Bing-hua" wrote (reply to this): On 18/12/2024 21:52, shejialuo wrote:
> 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.
No worries. Thank you for encouraging words :)
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Wang Bing-hua" <[email protected]> writes:
>> 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:
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.
> 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?
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.
THanks. |
Changes in v2:
cc: shejialuo [email protected]