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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 1, 2022, 18:43 UTC
Message-ID: <xmqqwn9j2xmu.fsf@gitster.g>
In-Reply-To: <patch-v3-21.36-1713227d497-20220930T180415Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix various issues of SYNOPSIS and -h output syntax where:
>
>  * Options such as --force were missing entirely
>  * ...or the short option, such as -f

In the full option listing (e.g. what usage_with_options() gives for options with both short and long form, such as "git commit -h"), it indeed is good to show both, i.e.

	-q, --quiet    run quietly
	-v, --verbose  run verbosely

is good and we have enough room to do so.

But for commands with so small number of options that we cram the options on the summary line without hiding them behind a single "[<options>]" placeholder, I am not sure if it is a good idea to show both and do

	usage: git cmd [-f|--force] [-q|--quiet] [-v|--verbose] <pathspec>

in the first place. It would be easier to understand without distracting choice, i.e.

	usage: git cmd [--force] [--quiet] [--verbose] <pathspec>

or even with only the short-form (but that risks to become unfriendly to new people who haven't learned what a single letter option "-x" means, so I would not recommend it).

Especially with the mandatory space around vertical bar, it would look horrible,

	usage: git cmd [-f | --force] [-q | --quiet] [-v | --verbose] <pathspec>

as the visual binding between the short and long form of the same option becomes weaker.

So, I dunno.

>  * We said "opts" or "options", but could instead enumerate
>    the (small) set of supported options

Yes, see above.

>  * argument labels were missing entirely (ls-remote)

I do not know what an "argument label" is, but I can see that ls-remote.c lacked the description for the --sort option.

>  * How we referred to an argument was inconsistent between the two,
>    e.g. <pack> v.s. <pack>.idx.

I didn't see anything in this category in this patch.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

> -'git send-pack' [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
> +'git send-pack' [--mirror] [--dry-run] [--force]
> +		[--receive-pack=<git-receive-pack>]

It may not matter what order the options are given, but having anything before "--dry-run" looks somewhat strange. It is not like this list is given in the same order as they appear in the options[] array.

I wonder if "--mirror" is the most closely related to (read: easier to understand if it is listed close to) "--all" and <ref>, i.e. immediately before the destination repository?

We do not really care about these commands that end-users will never invoke themselves, so I'll let it pass, even though this one looks a bit questionable.

Show 10 quoted lines

> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index 872436d7426..68392d2a56e 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -9,7 +9,7 @@ git-sparse-checkout - Reduce your working tree to a subset of tracked files
>  SYNOPSIS
>  --------
>  [verse]
> -'git sparse-checkout' <subcommand> [<options>]
> +'git sparse-checkout' (init | list | set | add | reapply | disable) [<options>]

OK. We have only limited number that fits on a line; otherwise "<subcommand>" -> "<command>" which refers reders to the COMMANDS section of the documentation.

Show 22 quoted lines

> 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),

Show 13 quoted lines

> diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
> index 92097f6673d..96d10cfdffe 100644
> --- a/Documentation/git-verify-commit.txt
> +++ b/Documentation/git-verify-commit.txt
> @@ -8,7 +8,7 @@ git-verify-commit - Check the GPG signature of commits
>  SYNOPSIS
>  --------
>  [verse]
> -'git verify-commit' <commit>...
> +'git verify-commit' [-v | --verbose] <commit>...
>  
>  DESCRIPTION
>  -----------

The DESCRIPTION section also mentions "--raw".

Show 13 quoted lines

> diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
> index 0b8075dad96..a454df2711e 100644
> --- a/Documentation/git-verify-tag.txt
> +++ b/Documentation/git-verify-tag.txt
> @@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags
>  SYNOPSIS
>  --------
>  [verse]
> -'git verify-tag' [--format=<format>] <tag>...
> +'git verify-tag' [-v | --verbose] [--format=<format>] <tag>...
>  
>  DESCRIPTION
>  -----------

The DESCRIPTION section also mentions "--raw".

Show 13 quoted lines

> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index ca672a6a619..d4eb0097d24 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -267,7 +267,7 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
>  	const char *socket_path;
>  	int ignore_sighup = 0;
>  	static const char *usage[] = {
> -		"git-credential-cache--daemon [opts] <socket-path>",
> +		"git-credential-cache--daemon [--debug] <socket-path>",
>  		NULL
>  	};
>  	int debug = 0;

OK.

Show 15 quoted lines

> diff --git a/builtin/describe.c b/builtin/describe.c
> index e17c4b4c69b..23e3f05fb10 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -23,8 +23,9 @@
>  define_commit_slab(commit_names, struct commit_name *);
>  
>  static const char * const describe_usage[] = {
> -	N_("git describe [<options>] [<commit-ish>...]"),
> -	N_("git describe [<options>] --dirty"),
> +	N_("git describe [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]"),
> +	N_("git describe [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]"),
> +	N_("git describe <blob>"),
>  	NULL
>  };

OK.

Show 13 quoted lines

> diff --git a/builtin/diff-index.c b/builtin/diff-index.c
> index e667cf52e7d..aea139b9d8f 100644
> --- a/builtin/diff-index.c
> +++ b/builtin/diff-index.c
> @@ -9,7 +9,7 @@
>  #include "submodule.h"
>  
>  static const char diff_cache_usage[] =
> -"git diff-index [-m] [--cached] "
> +"git diff-index [-m] [--cached] [--merge-base] "
>  "[<common-diff-options>] <tree-ish> [<path>...]"
>  "\n"
>  COMMON_DIFF_OPTIONS_HELP;

OK.

Show 13 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.

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.

Show 12 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.

Show 14 quoted lines

> diff --git a/builtin/revert.c b/builtin/revert.c
> index ee2a0807f01..ee32c714a76 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -21,14 +21,15 @@
>   */
>  
>  static const char * const revert_usage[] = {
> -	N_("git revert [<options>] <commit-ish>..."),
> -	N_("git revert <subcommand>"),
> +	N_("git revert [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>..."),
> +	N_("git revert (--continue | --skip | --abort | --quit)"),
>  	NULL
>  };

This one uses "[--[no-]edit]", which is OK. Also <keyid> unlike the other ones I saw in my yesterdays review is not <key-id>, which is somewhat curious.

Show 8 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.

Show 12 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.

Show 15 quoted lines

> 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.

Show 12 quoted lines

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 75dece0e4f1..d428c45dc8d 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -23,11 +23,13 @@
>  #include "date.h"
>  
>  static const char * const git_tag_usage[] = {
> -	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
> -	   "        <tagname> [<head>]"),
> +	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]\n"
> +	   "        <tagname> [<commit> | <object>]"),

OK.

>  	N_("git tag -d <tagname>..."),

Show 6 quoted lines

> -	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
> -	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
> +	N_("git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]\n"
> +	   "        [--points-at <object>] [--column[=<options>] | --no-column]\n"
> +	   "        [--create-reflog] [--sort=<key>] [--format=<format>]\n"
> +	   "        [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),

OK.

>  	N_("git tag -v [--format=<format>] <tagname>..."),
>  	NULL
>  };

Show 26 quoted lines

> diff --git a/builtin/update-server-info.c b/builtin/update-server-info.c
> index 880fffec587..d2239c9ef4d 100644
> --- a/builtin/update-server-info.c
> +++ b/builtin/update-server-info.c
> @@ -4,7 +4,7 @@
>  #include "parse-options.h"
>  
>  static const char * const update_server_info_usage[] = {
> -	"git update-server-info [--force]",
> +	"git update-server-info [-f | --force]",
>  	NULL
>  };
>  
> diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> index 125af53885f..25b69da2bf2 100644
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -8,7 +8,8 @@
>  #include "serve.h"
>  
>  static const char * const upload_pack_usage[] = {
> -	N_("git upload-pack [<options>] <dir>"),
> +	N_("git-upload-pack [--[no-]strict] [--timeout=<n>] [--stateless-rpc]\n"
> +	   "                [--advertise-refs] <directory>"),
>  	NULL
>  };

OK. We do not really care about these commands that end-users will never invoke themselves, but these look good.

Show 13 quoted lines

> diff --git a/help.c b/help.c
> index d04542d8261..f1e090a4428 100644
> --- a/help.c
> +++ b/help.c
> @@ -757,7 +757,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
>  	struct strbuf buf = STRBUF_INIT;
>  	int build_options = 0;
>  	const char * const usage[] = {
> -		N_("git version [<options>]"),
> +		N_("git version [--build-options]"),
>  		NULL
>  	};
>  	struct option options[] = {

OK.