| Message ID | patch-v2-4.6-4547cc968b1-20210910T153147Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parse-options: properly align continued usage output & related | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Let's "fix" various "git <cmd> -h" output by "properly" aligning the > output in cases where we continue usage output after a "\n". The "fix" > and "properly" scare quotes are because this actually makes things The "A" and "B" *in* scare quotes? > The issue is that we should have whitespace corresponding to the > length of the command name here, e.g. in the case of "git ls-remote" > it should be 14 characters, or the length of ""git ls-remote > ". Instead we had 21 characters in builtin/ls-remote.c, those extra 7 > characters are the length of "usage: " (and also " or:"). So in the C > locale the resulting output aligns nicely: > > $ git ls-remote -h > usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>] > [-q | --quiet] [--exit-code] [--get-url] > [--symref] [<repository> [<refs>...]] Isn't this aiming for a wrong end goal? With an overly long subcommand name, we'd end up with overly deep indentation of the subsequent lines (hence too narrow a space to fit the options). Rather, wouldn't it be better to aim for a consistent and wide enough indentation, like say two tabs, everywhere, no matter how long the subcommand name is? Thanks.
On Fri, Sep 10 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Let's "fix" various "git <cmd> -h" output by "properly" aligning the >> output in cases where we continue usage output after a "\n". The "fix" >> and "properly" scare quotes are because this actually makes things > > The "A" and "B" *in* scare quotes? Perhaps I should take a stab at rewriting this, something like: Let's start by aligning the strings in the C code so that their indentation is correct, in the end it'll still be off due to the indentation parse_options() itself adds, but that'll be fixed in a subsequent commit. That subsequent commit relies on the indentation in the source being consistent. >> The issue is that we should have whitespace corresponding to the >> length of the command name here, e.g. in the case of "git ls-remote" >> it should be 14 characters, or the length of ""git ls-remote >> ". Instead we had 21 characters in builtin/ls-remote.c, those extra 7 >> characters are the length of "usage: " (and also " or:"). So in the C >> locale the resulting output aligns nicely: >> >> $ git ls-remote -h >> usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>] >> [-q | --quiet] [--exit-code] [--get-url] >> [--symref] [<repository> [<refs>...]] > > Isn't this aiming for a wrong end goal? With an overly long > subcommand name, we'd end up with overly deep indentation of the > subsequent lines (hence too narrow a space to fit the options). > > Rather, wouldn't it be better to aim for a consistent and wide > enough indentation, like say two tabs, everywhere, no matter how > long the subcommand name is? Similarly to the discussion on another patch about whether we should convert these long lines to [<options>] or not, I think this is really outside the scope of this series. In this particular case the indentation remains exactly the same before this series and after, for the other cases there's usually a change of 1-3 spaces or so, i.e. they were slightly off. Should we have no indentation at all? Auto re-indent or whatever? Maybe, but let's start by narrowly fixing code that's clearly a bit off in functionality from what it's doing & intending to do right now.
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index f4fd823af83..318949c3d75 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -7,8 +7,8 @@ static const char * const ls_remote_usage[] = { N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n" - " [-q | --quiet] [--exit-code] [--get-url]\n" - " [--symref] [<repository> [<refs>...]]"), + " [-q | --quiet] [--exit-code] [--get-url]\n" + " [--symref] [<repository> [<refs>...]]"), NULL }; diff --git a/builtin/show-branch.c b/builtin/show-branch.c index d77ce7aeb38..a82cd1534fc 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -11,9 +11,9 @@ static const char* show_branch_usage[] = { N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n" - " [--current] [--color[=<when>] | --no-color] [--sparse]\n" - " [--more=<n> | --list | --independent | --merge-base]\n" - " [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"), + " [--current] [--color[=<when>] | --no-color] [--sparse]\n" + " [--more=<n> | --list | --independent | --merge-base]\n" + " [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"), N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"), NULL }; diff --git a/builtin/stash.c b/builtin/stash.c index 8f42360ca91..45b19007d7c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = { static const char * const git_stash_save_usage[] = { N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" - " [-u|--include-untracked] [-a|--all] [<message>]"), + " [-u|--include-untracked] [-a|--all] [<message>]"), NULL }; diff --git a/builtin/tag.c b/builtin/tag.c index 065b6bf093e..6535ed27ee9 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -23,10 +23,10 @@ static const char * const git_tag_usage[] = { N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n" - "\t\t<tagname> [<head>]"), + " <tagname> [<head>]"), N_("git tag -d <tagname>..."), N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n" - "\t\t[--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"), + " [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"), N_("git tag -v [--format=<format>] <tagname>..."), NULL };
Let's "fix" various "git <cmd> -h" output by "properly" aligning the output in cases where we continue usage output after a "\n". The "fix" and "properly" scare quotes are because this actually makes things worse in some cases, because e.g. in the case of "git tag -h" the "\t\t" effectively works around how parse-options.c aligns this output. But two wrongs don't make a right, let's "fix" this by making it worse temporarily, in anticipation of improving parse-options.c to handle this alignment. The issue is that we should have whitespace corresponding to the length of the command name here, e.g. in the case of "git ls-remote" it should be 14 characters, or the length of ""git ls-remote ". Instead we had 21 characters in builtin/ls-remote.c, those extra 7 characters are the length of "usage: " (and also " or:"). So in the C locale the resulting output aligns nicely: $ git ls-remote -h usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>] [-q | --quiet] [--exit-code] [--get-url] [--symref] [<repository> [<refs>...]] But that's fragile, we might not be under the C locale. We really should have parse-options.c itself add this padding. In a subsequent commit I'll make it do that. In the case of "tag" and "show-branch" and "stash save" the output was not properly aligned, although in the "git tag" case it was near-enough (aligned with the "-" in "git tag -l") to look good, assuming C locale & a tab-width of 8. In any case, let's align this in a way that looks obviously correct when looking at the source itself, and then improve parse-options.c itself. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/ls-remote.c | 4 ++-- builtin/show-branch.c | 6 +++--- builtin/stash.c | 2 +- builtin/tag.c | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)