Re: [PATCH v3 21/36] doc txt & -h consistency: add missing options and labels

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Oct 01 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> [...]
>> diff --git a/Documentation/git-update-server-info.txt b/Documentation/git-update-server-info.txt
>> index 969bb2e15f1..4e6bf44427f 100644
>> --- a/Documentation/git-update-server-info.txt
>> +++ b/Documentation/git-update-server-info.txt
>> @@ -9,7 +9,7 @@ git-update-server-info - Update auxiliary info file to help dumb servers
>>  SYNOPSIS
>>  --------
>>  [verse]
>> -'git update-server-info'
>> +'git update-server-info' [-f | --force]
>>  
>>  DESCRIPTION
>>  -----------
>> @@ -19,6 +19,12 @@ $GIT_OBJECT_DIRECTORY/info directories to help clients discover
>>  what references and packs the server has.  This command
>>  generates such auxiliary files.
>>  
>> +OPTIONS
>> +-------
>> +-f::
>> +--force::
>> +	Allow adding otherwise ignored files.
>
> The option help text says this:
>
>     N_("update the info files from scratch"), 0),

That was a copy/paste error, thanks!

> [...]
>> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
>> index df44e5cc0d1..5d5ac038716 100644
>> --- a/builtin/ls-remote.c
>> +++ b/builtin/ls-remote.c
>> @@ -7,7 +7,7 @@
>>  
>>  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"
>> +	   "              [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]\n"
>>  	   "              [--symref] [<repository> [<refs>...]]"),
>>  	NULL
>>  };
>
> OK.

I've triped to fix all the issues you noted above (including what I elided).

> We do not need to express that --sort=<key0> --sort=<key1>... can be
> given multiple times without triggering the usual last-one-wins?  In
> the documentation, the option description can say "this option can
> be given multiple times", but here we cannot, so the best we could
> do would be [--sort=<key>...] and I wonder if that is worth it.

Yes, FWIW I've left this at an explanation in the commit message. It
should be [(--sort=<key>)...], but also far git-branch.txt etc., but
fixing everything everywhere is a much bigger change.

>> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
>> index cfbd5c36c76..27c2ca06acb 100644
>> --- a/builtin/pack-refs.c
>> +++ b/builtin/pack-refs.c
>> @@ -5,7 +5,7 @@
>>  #include "repository.h"
>>  
>>  static char const * const pack_refs_usage[] = {
>> -	N_("git pack-refs [<options>]"),
>> +	N_("git pack-refs [--all] [--no-prune]"),
>>  	NULL
>>  };
>
> Even though "[--[no-]all] [--[no-]prune" would be more complete, I
> do not mind listing only the side that makes it behave differently
> from the default.  But if you reroll with a more complete version
> with documentation updates, I wouldn't complain at all.

For now I'm just trying to make commands consistent with their "other
side", but not all things consistent with all other things.

Having said that I really prefer when we have an "--all" as opposed to
"[--no-all | --all]", or the briefer "[--[no-]all]", it makes it clearer
that "--no-all" is the default, and that your "--all" is turning on the
non-default.

Likewise "--no-prune".

>>  static const char * const cherry_pick_usage[] = {
>> -	N_("git cherry-pick [<options>] <commit-ish>..."),
>> -	N_("git cherry-pick <subcommand>"),
>> +	N_("git cherry-pick [--edit] [-n] [-m <parent-number>] [-s] [-x] [--ff]\n"
>> +	   "                [-S[<keyid>]] <commit>..."),
>> +	N_("git cherry-pick (--continue | --skip | --abort | --quit)"),
>>  	NULL
>>  };
>
> And here we see "[--edit]", which is not exactly consistent.

*Nod*, but I'll leave this, per the "not trying to make all things
consistent with everything else", it's now consistent with its own
other side.

>> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
>> index 64962be0168..4c5d125fa0a 100644
>> --- a/builtin/send-pack.c
>> +++ b/builtin/send-pack.c
>> @@ -20,6 +20,7 @@ static const char * const send_pack_usage[] = {
>>  	N_("git send-pack [--mirror] [--dry-run] [--force]\n"
>>  	   "              [--receive-pack=<git-receive-pack>]\n"
>>  	   "              [--verbose] [--thin] [--atomic]\n"
>> +	   "              [--[no-]signed | --signed=(true|false|if-asked)]\n"
>>  	   "              [<host>:]<directory> (--all | <ref>...)"),
>>  	NULL,
>>  };
>
> Excellent.
>
>> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
>> index 1b0f10225f0..50b6df78df0 100644
>> --- a/builtin/symbolic-ref.c
>> +++ b/builtin/symbolic-ref.c
>> @@ -5,8 +5,9 @@
>>  #include "parse-options.h"
>>  
>>  static const char * const git_symbolic_ref_usage[] = {
>> -	N_("git symbolic-ref [<options>] <name> [<ref>]"),
>> -	N_("git symbolic-ref -d [-q] <name>"),
>> +	N_("git symbolic-ref [-m <reason>] <name> <ref>"),
>> +	N_("git symbolic-ref [-q] [--short] <name>"),
>> +	N_("git symbolic-ref --delete [-q] <name>"),
>>  	NULL
>>  };
>
> Why spell out --delete without allowing -d?  Especially when listing
> only -q for OPT__QUIET()?
>
>     git symbolic-ref (-d|--delete) [-q|--quiet] <name>
>
> would be more complete but I think the original (i.e. "-d [-q]") is
> more consistent.

I can do this if you'd like, but didn't per the above, i.e. the *.txt
uses just "symbolic-ref --delete".