[PATCH v4] git remote: Separate usage strings for subcommands

[PATCH v4] git remote: Separate usage strings for subcommands

[Tim Henigan]

When the usage string for a subcommand must be printed,
only print the information relevant to that command.

This commit also removes the options from the first line
of the usage string (replacing them with '<options>'
and lets them be documented in the paragraph below.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This patch is based on:
http://article.gmane.org/gmane.comp.version-control.git/132953

All usage strings are still only located at the top of file.  However,
separate usage string arrays have been created for each subcommand.

v2 fixed line wrap issues present in v1.

v3 changes include:
 - Changed usage strings to use '<options>...' rather than document
   the options both in the actual string and in the OPT_x array
   (as recommended by Junio).
 - Removed the change made to the usage string array constructed
   in 'cmd_remote'. v2 was broken because that change had made
   the command stop recognizing the '-v' option. Added an extra
   note here which explains that '-v' is only valid when placed
   after 'git remote' and before any 'subcommand'.
 - Updated the man page.

v4 changes include:
 - Changed usage strings to use '<options>' rather than
   '<options>...' based on feedback from Jonathan Nieder.
   See [1] for details.
 - Corrected "git remote set-head" usage string to show the
   required [-a | -d | <branch>] portion of the command.
 - Corrected "git remote update" usage string to show the
   optional, but not otherwise documented [<group> | <remote>]
   portion of the command.
 - Removed 2 more instances of "<subcommand> specific options".

[1] http://thread.gmane.org/gmane.comp.version-control.git/133151/focus=133160


 Documentation/git-remote.txt |   13 +++++----
 builtin-remote.c             |   60 +++++++++++++++++++++++++----------------
 2 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 82a3d29..ee3dc80 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -9,14 +9,14 @@ git-remote - manage set of tracked repositories
 SYNOPSIS
 --------
 [verse]
-'git remote' [-v | --verbose]
-'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
+'git remote' [<options>]
+'git remote add' [<options>] <name> <url>
 'git remote rename' <old> <new>
 'git remote rm' <name>
-'git remote set-head' <name> [-a | -d | <branch>]
-'git remote show' [-n] <name>
-'git remote prune' [-n | --dry-run] <name>
-'git remote update' [-p | --prune] [group | remote]...
+'git remote set-head' <name> [<options>] [-a | -d | <branch>]
+'git remote show' [<options>] <name>
+'git remote prune' [<options>] <name>
+'git remote update' [<options>] [<group> | <remote>]...
 
 DESCRIPTION
 -----------
@@ -30,6 +30,7 @@ OPTIONS
 -v::
 --verbose::
 	Be a little more verbose and show remote url after name.
+	NOTE: This must be placed between `remote` and `subcommand`.
 
 
 COMMANDS
diff --git a/builtin-remote.c b/builtin-remote.c
index 0777dd7..24a3ec0 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -7,18 +7,35 @@
 #include "run-command.h"
 #include "refs.h"
 
+#define REMOTE_BARE_USAGE "git remote [<options>]"
+#define REMOTE_ADD_USAGE "git remote add [<options>] <name> <url>"
+#define REMOTE_RENAME_USAGE "git remote rename <old> <new>"
+#define REMOTE_RM_USAGE "git remote rm <name>"
+#define REMOTE_SETHEAD_USAGE "git remote set-head <name> [-a | -d | <branch>]"
+#define REMOTE_SHOW_USAGE "git remote show [<options>] <name>"
+#define REMOTE_PRUNE_USAGE "git remote prune [<options>] <name>"
+#define REMOTE_UPDATE_USAGE "git remote update [<options>] [<group> | <remote>]..."
+
 static const char * const builtin_remote_usage[] = {
-	"git remote [-v | --verbose]",
-	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
-	"git remote rename <old> <new>",
-	"git remote rm <name>",
-	"git remote set-head <name> [-a | -d | <branch>]",
-	"git remote show [-n] <name>",
-	"git remote prune [-n | --dry-run] <name>",
-	"git remote [-v | --verbose] update [-p | --prune] [group]",
+	REMOTE_BARE_USAGE,
+	REMOTE_ADD_USAGE,
+	REMOTE_RENAME_USAGE,
+	REMOTE_RM_USAGE,
+	REMOTE_SETHEAD_USAGE,
+	REMOTE_SHOW_USAGE,
+	REMOTE_PRUNE_USAGE,
+	REMOTE_UPDATE_USAGE,
 	NULL
 };
 
+static const char * const builtin_remote_add_usage[] = { REMOTE_ADD_USAGE, NULL };
+static const char * const builtin_remote_rename_usage[] = { REMOTE_RENAME_USAGE, NULL };
+static const char * const builtin_remote_rm_usage[] = { REMOTE_RM_USAGE, NULL };
+static const char * const builtin_remote_sethead_usage[] = { REMOTE_SETHEAD_USAGE, NULL };
+static const char * const builtin_remote_show_usage[] = { REMOTE_SHOW_USAGE, NULL };
+static const char * const builtin_remote_prune_usage[] = { REMOTE_PRUNE_USAGE, NULL };
+static const char * const builtin_remote_update_usage[] = { REMOTE_UPDATE_USAGE, NULL };
+
 #define GET_REF_STATES (1<<0)
 #define GET_HEAD_NAMES (1<<1)
 #define GET_PUSH_REF_STATES (1<<2)
@@ -70,7 +87,6 @@ static int add(int argc, const char **argv)
 	int i;
 
 	struct option options[] = {
-		OPT_GROUP("add specific options"),
 		OPT_BOOLEAN('f', "fetch", &fetch, "fetch the remote branches"),
 		OPT_CALLBACK('t', "track", &track, "branch",
 			"branch(es) to track", opt_parse_track),
@@ -79,11 +95,11 @@ static int add(int argc, const char **argv)
 		OPT_END()
 	};
 
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,
 			     0);
 
 	if (argc < 2)
-		usage_with_options(builtin_remote_usage, options);
+		usage_with_options(builtin_remote_add_usage, options);
 
 	name = argv[0];
 	url = argv[1];
@@ -540,7 +556,7 @@ static int mv(int argc, const char **argv)
 	int i;
 
 	if (argc != 3)
-		usage_with_options(builtin_remote_usage, options);
+		usage_with_options(builtin_remote_rename_usage, options);
 
 	rename.old = argv[1];
 	rename.new = argv[2];
@@ -681,7 +697,7 @@ static int rm(int argc, const char **argv)
 	int i, result;
 
 	if (argc != 2)
-		usage_with_options(builtin_remote_usage, options);
+		usage_with_options(builtin_remote_rm_usage, options);
 
 	remote = remote_get(argv[1]);
 	if (!remote)
@@ -976,7 +992,6 @@ static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0, query_flag = 0;
 	struct option options[] = {
-		OPT_GROUP("show specific options"),
 		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
 		OPT_END()
 	};
@@ -984,7 +999,7 @@ static int show(int argc, const char **argv)
 	struct string_list info_list = { NULL, 0, 0, 0 };
 	struct show_info info;
 
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_show_usage,
 			     0);
 
 	if (argc < 1)
@@ -1081,14 +1096,13 @@ static int set_head(int argc, const char **argv)
 	char *head_name = NULL;
 
 	struct option options[] = {
-		OPT_GROUP("set-head specific options"),
 		OPT_BOOLEAN('a', "auto", &opt_a,
 			    "set refs/remotes/<name>/HEAD according to remote"),
 		OPT_BOOLEAN('d', "delete", &opt_d,
 			    "delete refs/remotes/<name>/HEAD"),
 		OPT_END()
 	};
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_sethead_usage,
 			     0);
 	if (argc)
 		strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]);
@@ -1114,7 +1128,7 @@ static int set_head(int argc, const char **argv)
 		if (delete_ref(buf.buf, NULL, REF_NODEREF))
 			result |= error("Could not delete %s", buf.buf);
 	} else
-		usage_with_options(builtin_remote_usage, options);
+		usage_with_options(builtin_remote_sethead_usage, options);
 
 	if (head_name) {
 		unsigned char sha1[20];
@@ -1138,16 +1152,15 @@ static int prune(int argc, const char **argv)
 {
 	int dry_run = 0, result = 0;
 	struct option options[] = {
-		OPT_GROUP("prune specific options"),
 		OPT__DRY_RUN(&dry_run),
 		OPT_END()
 	};
 
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
 			     0);
 
 	if (argc < 1)
-		usage_with_options(builtin_remote_usage, options);
+		usage_with_options(builtin_remote_prune_usage, options);
 
 	for (; argc; argc--, argv++)
 		result |= prune_remote(*argv, dry_run);
@@ -1228,13 +1241,12 @@ static int update(int argc, const char **argv)
 	struct string_list list = { NULL, 0, 0, 0 };
 	static const char *default_argv[] = { NULL, "default", NULL };
 	struct option options[] = {
-		OPT_GROUP("update specific options"),
 		OPT_BOOLEAN('p', "prune", &prune,
 			    "prune remotes after fetching"),
 		OPT_END()
 	};
 
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,
 			     PARSE_OPT_KEEP_ARGV0);
 	if (argc < 2) {
 		argc = 2;
@@ -1334,7 +1346,7 @@ static int show_all(void)
 int cmd_remote(int argc, const char **argv, const char *prefix)
 {
 	struct option options[] = {
-		OPT__VERBOSE(&verbose),
+		OPT_BOOLEAN('v', "verbose", &verbose, "be verbose; must be placed before a subcommand"),
 		OPT_END()
 	};
 	int result;
-- 
1.6.5.2.186.ga4d60

Re: [PATCH v4] git remote: Separate usage strings for subcommands

[Nanako Shiraishi]

Quoting Tim Henigan <tim.henigan@gmail.com>

> When the usage string for a subcommand must be printed,
> only print the information relevant to that command.

I think this is a huge improvement.

> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 82a3d29..ee3dc80 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -9,14 +9,14 @@ git-remote - manage set of tracked repositories
>  SYNOPSIS
>  --------
>  [verse]
> -'git remote' [-v | --verbose]
> -'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
> +'git remote' [<options>]
> +'git remote add' [<options>] <name> <url>
>  'git remote rename' <old> <new>
>  'git remote rm' <name>
> -'git remote set-head' <name> [-a | -d | <branch>]
> -'git remote show' [-n] <name>
> -'git remote prune' [-n | --dry-run] <name>
> -'git remote update' [-p | --prune] [group | remote]...
> +'git remote set-head' <name> [<options>] [-a | -d | <branch>]
> +'git remote show' [<options>] <name>
> +'git remote prune' [<options>] <name>
> +'git remote update' [<options>] [<group> | <remote>]...

Often people look at this part of the manual page to quickly remind 
themselves what options are available, and it is better to keep the 
current text. Some manual pages have to use [options...] when there 
are too many to list, but each subcommand of git-remote doesn't have 
that many options.

> diff --git a/builtin-remote.c b/builtin-remote.c
> index 0777dd7..24a3ec0 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -7,18 +7,35 @@
>  #include "run-command.h"
>  #include "refs.h"
>  
> +#define REMOTE_BARE_USAGE "git remote [<options>]"
> +#define REMOTE_ADD_USAGE "git remote add [<options>] <name> <url>"
> +#define REMOTE_RENAME_USAGE "git remote rename <old> <new>"
> +#define REMOTE_RM_USAGE "git remote rm <name>"
> +#define REMOTE_SETHEAD_USAGE "git remote set-head <name> [-a | -d | <branch>]"
> +#define REMOTE_SHOW_USAGE "git remote show [<options>] <name>"
> +#define REMOTE_PRUNE_USAGE "git remote prune [<options>] <name>"
> +#define REMOTE_UPDATE_USAGE "git remote update [<options>] [<group> | <remote>]..."
> +
>  static const char * const builtin_remote_usage[] = {
> -	"git remote [-v | --verbose]",
> -	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
> -	"git remote rename <old> <new>",
> -	"git remote rm <name>",
> -	"git remote set-head <name> [-a | -d | <branch>]",
> -	"git remote show [-n] <name>",
> -	"git remote prune [-n | --dry-run] <name>",
> -	"git remote [-v | --verbose] update [-p | --prune] [group]",
> +	REMOTE_BARE_USAGE,
> +	REMOTE_ADD_USAGE,
> +	REMOTE_RENAME_USAGE,
> +	REMOTE_RM_USAGE,
> +	REMOTE_SETHEAD_USAGE,
> +	REMOTE_SHOW_USAGE,
> +	REMOTE_PRUNE_USAGE,
> +	REMOTE_UPDATE_USAGE,
>  	NULL
>  };

For the same reason, I don't think this is a good change, if these
lines are used to show the first lines of 'git-remote -h' output.

Re: [PATCH v4] git remote: Separate usage strings for subcommands

[Tim Henigan]

On Wed, Nov 18, 2009 at 10:40 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Quoting Tim Henigan <tim.henigan@gmail.com>
>
>> When the usage string for a subcommand must be printed,
>> only print the information relevant to that command.
>
> I think this is a huge improvement.
>
> Often people look at this part of the manual page to quickly remind
> themselves what options are available, and it is better to keep the
> current text. Some manual pages have to use [options...] when there
> are too many to list, but each subcommand of git-remote doesn't have
> that many options.

... snip ...

> For the same reason, I don't think this is a good change, if these
> lines are used to show the first lines of 'git-remote -h' output.

The original version of this patch [1] left the contents of the usage
strings intact.  However, Junio expressed a preference to change
them to use the generic <options>.  See this thread for the
discussion [2].

[1] http://article.gmane.org/gmane.comp.version-control.git/133048/
[2] http://thread.gmane.org/gmane.comp.version-control.git/132968/focus=133050

Re: [PATCH v4] git remote: Separate usage strings for subcommands

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> The original version of this patch [1] left the contents of the usage
> strings intact.  However, Junio expressed a preference to change
> them to use the generic <options>.  See this thread for the
> discussion [2].
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/133048/
> [2] http://thread.gmane.org/gmane.comp.version-control.git/132968/focus=133050

Sorry, but I think you misunderstood what I meant, then.  in [2], I said...

    > ... hunk to remove literal strings from builtin_remote_usage[] 
    > ... and replace them with REMOTE_BARE_USAGE, REMOTE_ADD_USAGE

    I am not sure about the value of reusing option string like this, and for

Here, please note that I was objecting to the use of _the same string_ in
both contexts ("reusing").

    all other subcommands the same comment applies.  For example, in the case
    of "remote add -h", you would use

    "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"

    from REMOTE_ADD_USAGE, but ...

    > ... hunk to give the builtin_remote_add_usage[] that  uses the same
    > ... REMOTE_ADD_USAGE to parse_options() 

    ... the options list is used to reproduce the information in a major part
    of that string already.  So I would prefer builtin_remote_add_usage[] to
    be something like:

        "git remote add [<options>...] <name> <url>"

I meant that we want to change "remote add -h" to show this here; and the
reason why I doubted "the value of reusing option string" was because I
wanted to do so without touching the concise list of the subcommands and
their options given by "remote -h".  Otherwise, it would have made perfect
sense to use preprocessor macros to share the two identical strings.

In your response to my above comment, you indicated that you wanted to do
the [<options>] thing as a separate patch (your original patch spelled
options in full).  I took it to mean that you would do that only for the
subcommand help, and did not respond, because

 (1) doing that to the subcommand help would be a good idea anyway; and

 (2) you will realize what I said in the message about "the value of
     reusing option string" was correct when you see the end result, which
     will regress "remote -h" output.

In this case, unfortunately (2) didn't happen before Nana pointed it out.

Re: [PATCH v4] git remote: Separate usage strings for subcommands

[Tim Henigan]

On Thu, Nov 19, 2009 at 1:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> The original version of this patch [1] left the contents of the usage
>> strings intact.  However, Junio expressed a preference to change
>> them to use the generic <options>.  See this thread for the
>> discussion [2].
>>
>> [1] http://article.gmane.org/gmane.comp.version-control.git/133048/
>> [2] http://thread.gmane.org/gmane.comp.version-control.git/132968/focus=133050
>
> Sorry, but I think you misunderstood what I meant, then.  in [2], I said...

... snip ...

>    ... the options list is used to reproduce the information in a major part
>    of that string already.  So I would prefer builtin_remote_add_usage[] to
>    be something like:
>
>        "git remote add [<options>...] <name> <url>"
>
> I meant that we want to change "remote add -h" to show this here; and the
> reason why I doubted "the value of reusing option string" was because I
> wanted to do so without touching the concise list of the subcommands and
> their options given by "remote -h".  Otherwise, it would have made perfect
> sense to use preprocessor macros to share the two identical strings.

Okay, I believe I understand now, but let's test that theory ;)

Using the 'add' subcommand as an example, the desired output is:

Output of 'git remote -h':
    "git remote [-v | --verbose]"
    "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"
    etc.

Output of 'git remote add -h':
    "git remote add [<options>...] <name> <url>"
    followed by the detailed description given by 'parse_options()'.

Text in 'man git-remote':
    "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"
    with the options explained in detail later in the file.

Thanks for your patience,
Tim

Re: [PATCH v4] git remote: Separate usage strings for subcommands

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> Using the 'add' subcommand as an example, the desired output is:
>
> Output of 'git remote -h':
>     "git remote [-v | --verbose]"
>     "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"
>     etc.
>
> Output of 'git remote add -h':
>     "git remote add [<options>...] <name> <url>"
>     followed by the detailed description given by 'parse_options()'.
>
> Text in 'man git-remote':
>     "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"
>     with the options explained in detail later in the file.
>
> Thanks for your patience,

I think the above looks good; thank _you_ for your patience.

I often wonder if we want to add to the parse-options library a function
that takes a "const struct option *" and some other unspecified hints, and
fills a strbuf with a one-line description, e.g.

    "[-t <branch>] [-m <master>] [-f] [--mirror]"

I expect we would eventually want to handle something like (this example
is from "git push"):

    "[--all | --mirror | --tags] [-n | --dry-run]"

and walking elements in a "struct option" one by one wouldn't give us
enough information to group all/mirror/tags in "one of these" brackets,
and that is what I mean by "some other unspecified hints".  Obviously we
could do the "[-n|--dry-run]" with existing information.

A helper function like that may make things a bit easier.  parse_options()
may need to take a custom callback function of some sort so that you can
override what parse_options_usage() does when responding to "remote -h"
and generate the list of subcommands and their options on the fly, though.

But that is all outside the scope of this particular patch.