Re: [PATCH v3 21/36] doc txt & -h consistency: add missing options and labels
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 4, 2022, 10:13 UTC
- Message-ID
- <221004.86bkqruc2i.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqwn9j2xmu.fsf@gitster.g>
On Sat, Oct 01 2022, Junio C Hamano wrote:
Show 28 quoted lines
> Æ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!
Show 16 quoted lines
> [...]
>> 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).
Show 5 quoted lines
> 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.
Show 17 quoted lines
>> 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".
Show 10 quoted lines
>> 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.
Show 38 quoted lines
>> 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".