| Message ID | patch-v2-1.5-b10bfd21f14-20210910T112545Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | help: fix usage nits & bugs, completion shellscript->C | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt > index 44fe8860b3f..568a0b606f3 100644 > --- a/Documentation/git-help.txt > +++ b/Documentation/git-help.txt > @@ -9,7 +9,7 @@ SYNOPSIS > -------- > [verse] > 'git help' [-a|--all [--[no-]verbose]] [-g|--guides] > - [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE] > + [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] This one is good, but ... > diff --git a/builtin/help.c b/builtin/help.c > index b7eec06c3de..44ea2798cda 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = { > }; > > static const char * const builtin_help_usage[] = { > - N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"), > + N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" > + " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), Aside from the addition of so-far-missing "verbose", which is obviously a good change, I am not sure if the other change is a good idea. This is because *_usage[] is designed to be always shown together with the list of options built from the option table the parse_options() uses, and the readers will see the correspondence between long and short options even more clear there. $ git help -h usage: git help [--all] [--guides] [--man | --web | --info] [<command>] -a, --all print all available commands -g, --guides print list of useful guides -c, --config print all configuration variable names -m, --man show man page -w, --web show manual in web browser -i, --info show info page -v, --verbose print command description If you look at output from $ git grep -A4 -e '_usage\[' builtin/\*.c you'll notice that many of them do not even spell out each option and instead have a single [<options>] placeholder unless the command has only very small number of options. With the number of options that "git help" takes, it might even be warranted to switch to the more generic "git help [<option>...] [<command>]". Not a strict veto, but just making sure if the over-cluttering of the early lines in "help -h" output has been considered as a possible downside before suggesting this change. Thanks.
On Fri, Sep 10 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt >> index 44fe8860b3f..568a0b606f3 100644 >> --- a/Documentation/git-help.txt >> +++ b/Documentation/git-help.txt >> @@ -9,7 +9,7 @@ SYNOPSIS >> -------- >> [verse] >> 'git help' [-a|--all [--[no-]verbose]] [-g|--guides] >> - [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE] >> + [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] > > This one is good, but ... > >> diff --git a/builtin/help.c b/builtin/help.c >> index b7eec06c3de..44ea2798cda 100644 >> --- a/builtin/help.c >> +++ b/builtin/help.c >> @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = { >> }; >> >> static const char * const builtin_help_usage[] = { >> - N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"), >> + N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" >> + " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), > > Aside from the addition of so-far-missing "verbose", which is > obviously a good change, I am not sure if the other change is a good > idea. > > This is because *_usage[] is designed to be always shown together > with the list of options built from the option table the > parse_options() uses, and the readers will see the correspondence > between long and short options even more clear there. > > $ git help -h > usage: git help [--all] [--guides] [--man | --web | --info] [<command>] > > -a, --all print all available commands > -g, --guides print list of useful guides > -c, --config print all configuration variable names > -m, --man show man page > -w, --web show manual in web browser > -i, --info show info page > -v, --verbose print command description > > If you look at output from > > $ git grep -A4 -e '_usage\[' builtin/\*.c > > you'll notice that many of them do not even spell out each option > and instead have a single [<options>] placeholder unless the command > has only very small number of options. With the number of options > that "git help" takes, it might even be warranted to switch to the > more generic "git help [<option>...] [<command>]". > > Not a strict veto, but just making sure if the over-cluttering of > the early lines in "help -h" output has been considered as a > possible downside before suggesting this change. That's a valid point, but if you look at: for cmd in $(git --list-cmds=builtins); do echo git-$cmd: && git -P $cmd -h | sed 's/^/ /'; done 2>&1|less I think it's fair to say it's a fairly mixed bag, some have the brevity you describe, others are attempting to list every possible option combo etc. This is one of the commands that's more like than the likes of git-branch, git-worktree, git-stash, git-index-pack etc. than git-add, git-apply, git-mv in the sense that it's trying to exhaustively mirror the options between the documentation and the usage string. I do think we should pick one, but I think migrating from one style to the other just for this one command would be an unnecessary distraction from a change that's not about changing that style in general, let's just stick with the pattern we find here. I do want to make them pretty and consistent eventually, this series is just one small step on the way there.
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt index 44fe8860b3f..568a0b606f3 100644 --- a/Documentation/git-help.txt +++ b/Documentation/git-help.txt @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git help' [-a|--all [--[no-]verbose]] [-g|--guides] - [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE] + [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE] DESCRIPTION ----------- diff --git a/builtin/help.c b/builtin/help.c index b7eec06c3de..44ea2798cda 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = { }; static const char * const builtin_help_usage[] = { - N_("git help [--all] [--guides] [--man | --web | --info] [<command>]"), + N_("git help [-a|--all] [-g|--guides] [--[no-]verbose]]\n" + " [[-i|--info] [-m|--man] [-w|--web]] [<command>]"), NULL };
Clarify the usage string in the documentation so we group e.g. -i and --info, and add the missing short options to the "-h" output. The alignment of the second line is off now, but will be fixed with another series of mine[1]. In the meantime let's just assume that fix will make it in eventually for the purposes of this patch, if it's misaligned for a bit it doesn't matter much. 1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-help.txt | 2 +- builtin/help.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)