[RFC/PATCH] parse-options: introduce parse_subcommands

[RFC/PATCH] parse-options: introduce parse_subcommands

[Ramkumar Ramachandra]

Some git builtins like git-notes use subcommands to switch between
different modes of operation.  Introduce a parse_subcommands similar
to parse_options to aid parsing these options.  The main advantage of
using it is that subcommands can have a descriptive help text attached
to them.

Make the git-notes builtin use parse_subcommands.  As a result, the
following error:

  $ git notes foo
  error: Unknown subcommand: foo
  usage: git notes [--ref <notes_ref>] [list [<object>]]
     or: git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>
     or: git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] edit [<object>]
     or: git notes [--ref <notes_ref>] show [<object>]
     or: git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>
     or: git notes merge --commit [-v | -q]
     or: git notes merge --abort [-v | -q]
     or: git notes [--ref <notes_ref>] remove [<object>...]
     or: git notes [--ref <notes_ref>] prune [-n | -v]
     or: git notes [--ref <notes_ref>] get-ref

    --ref <notes_ref>     use notes from <notes_ref>

is replaced by a more helpful:

  $ git notes foo
  error: unknown subcommand: foo
  usage: git notes [<options>] [<subcommand>] [<options>] [<object>..]

  available subcommands:
      list                  list notes for given object
      add                   add notes for given object
      copy                  copy notes for first object onto second object
      append                append notes to existing object
      edit                  edit notes for given object
      show                  show notes for given object
      merge                 merge given notes ref into current notes ref
      remove                remove notes for given objects
      prune                 remove all notes for non-existing/unreachable objects
      get-ref               print the current notes ref

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I found this patch in one of my branches from a long time ago.  I
 haven't written documentation and tests yet because I'm not fully
 convinced that I like this.

 Thoughts?

 builtin/notes.c |   56 ++++++++++++++++++++++---------------------------------
 parse-options.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.h |   10 ++++++++++
 3 files changed, 79 insertions(+), 34 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..b1d6206 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,18 +20,7 @@
 #include "notes-merge.h"
 
 static const char * const git_notes_usage[] = {
-	"git notes [--ref <notes_ref>] [list [<object>]]",
-	"git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>",
-	"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] edit [<object>]",
-	"git notes [--ref <notes_ref>] show [<object>]",
-	"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
-	"git notes merge --commit [-v | -q]",
-	"git notes merge --abort [-v | -q]",
-	"git notes [--ref <notes_ref>] remove [<object>...]",
-	"git notes [--ref <notes_ref>] prune [-n | -v]",
-	"git notes [--ref <notes_ref>] get-ref",
+	"git notes [<options>] [<subcommand>] [<options>] [<object>..]",
 	NULL
 };
 
@@ -1081,28 +1070,27 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 		strbuf_release(&sb);
 	}
 
-	if (argc < 1 || !strcmp(argv[0], "list"))
-		result = list(argc, argv, prefix);
-	else if (!strcmp(argv[0], "add"))
-		result = add(argc, argv, prefix);
-	else if (!strcmp(argv[0], "copy"))
-		result = copy(argc, argv, prefix);
-	else if (!strcmp(argv[0], "append") || !strcmp(argv[0], "edit"))
-		result = append_edit(argc, argv, prefix);
-	else if (!strcmp(argv[0], "show"))
-		result = show(argc, argv, prefix);
-	else if (!strcmp(argv[0], "merge"))
-		result = merge(argc, argv, prefix);
-	else if (!strcmp(argv[0], "remove"))
-		result = remove_cmd(argc, argv, prefix);
-	else if (!strcmp(argv[0], "prune"))
-		result = prune(argc, argv, prefix);
-	else if (!strcmp(argv[0], "get-ref"))
-		result = get_ref(argc, argv, prefix);
-	else {
-		result = error(_("Unknown subcommand: %s"), argv[0]);
-		usage_with_options(git_notes_usage, options);
-	}
+	struct subcommand subcmds[] = {
+		{ "list", "list notes for given object", list },
+		{ "add", "add notes for given object", add },
+		{ "copy",
+		  "copy notes for first object onto second object", copy },
+		{ "append", "append notes to existing object", append_edit },
+		{ "edit", "edit notes for given object", append_edit },
+		{ "show", "show notes for given object", show },
+		{ "merge",
+		  "merge given notes ref into current notes ref", merge },
+		{ "remove", "remove notes for given objects", remove_cmd },
+		{ "prune",
+		  "remove all notes for non-existing/unreachable objects", prune },
+		{ "get-ref", "print the current notes ref", get_ref },
+		{ NULL }
+	};
 
+	if (argc < 1)
+		result = list(argc, argv, prefix);
+	else
+		result = parse_subcommands(argc, argv, prefix,
+					subcmds, git_notes_usage);
 	return result ? 1 : 0;
 }
diff --git a/parse-options.c b/parse-options.c
index f0098eb..76640fb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -583,3 +583,50 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 	return usage_with_options_internal(ctx, usagestr, opts, 0, err);
 }
 
+static void subcommand_usage(const char * const *usagestr,
+			const struct subcommand *subcmds)
+{
+	const struct subcommand *subcmd;
+
+	fprintf(stderr, "usage: %s\n", *usagestr++);
+	while (*usagestr && **usagestr)
+		fprintf(stderr, "   or: %s\n", *usagestr++);
+	while (*usagestr) {
+		fprintf(stderr, "%s%s\n",
+				**usagestr ? "    " : "",
+				*usagestr);
+		usagestr++;
+	}
+
+	fputc('\n', stderr);
+	fprintf(stderr, "available subcommands:\n");
+	for (subcmd = subcmds; subcmd->name != NULL; subcmd ++) {
+		size_t pos;
+		int pad;
+
+		pos = fprintf(stderr, "    ");
+		pos += fprintf(stderr, "%s", subcmd->name);
+
+		if (pos <= USAGE_OPTS_WIDTH)
+			pad = USAGE_OPTS_WIDTH - pos;
+		else {
+			fputc('\n', stderr);
+			pad = USAGE_OPTS_WIDTH;
+		}
+		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", subcmd->help);
+	}
+	fputc('\n', stderr);
+}
+
+int parse_subcommands(int argc, const char **argv, const char *prefix,
+		const struct subcommand *subcmds, const char * const usagestr[])
+{
+	const struct subcommand *subcmd;
+
+	for (subcmd = subcmds; subcmd->name != NULL; subcmd ++)
+		if (!strcmp(subcmd->name, argv[0]))
+			return (*subcmd->callback)(argc, argv, prefix);
+	error("unknown subcommand: %s", argv[0]);
+	subcommand_usage(usagestr, subcmds);
+	exit(129);
+}
diff --git a/parse-options.h b/parse-options.h
index 2e811dc..e95ad10 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -46,6 +46,7 @@ enum parse_opt_option_flags {
 
 struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
+typedef int subcmd_cb(int argc, const char **argv, const char *prefix);
 
 struct parse_opt_ctx_t;
 typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
@@ -117,6 +118,12 @@ struct option {
 	intptr_t defval;
 };
 
+struct subcommand {
+	const char *name;
+	const char *help;
+	subcmd_cb *callback;
+};
+
 #define OPT_END()                   { OPTION_END }
 #define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
 				      (h), PARSE_OPT_NOARG}
@@ -169,6 +176,9 @@ extern int parse_options(int argc, const char **argv, const char *prefix,
                          const struct option *options,
                          const char * const usagestr[], int flags);
 
+extern int parse_subcommands(int argc, const char **argv, const char *prefix,
+			const struct subcommand *subcmds, const char * const usagestr[]);
+
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
-- 
1.7.10

Re: [RFC/PATCH] parse-options: introduce parse_subcommands

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>  I found this patch in one of my branches from a long time ago.  I
>  haven't written documentation and tests yet because I'm not fully
>  convinced that I like this.
>
>  Thoughts?

It looks like a static version of string_list_lookup() and does not
have much reason to tie it to "subcommand".

Re: [RFC/PATCH] parse-options: introduce parse_subcommands

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

>   $ git notes foo
>   error: Unknown subcommand: foo
>   usage: git notes [--ref <notes_ref>] [list [<object>]]
>      or: git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
>      or: git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>
[...]
> is replaced by a more helpful:
>
>   $ git notes foo
>   error: unknown subcommand: foo
>   usage: git notes [<options>] [<subcommand>] [<options>] [<object>..]
>
>   available subcommands:
>       list                  list notes for given object
>       add                   add notes for given object
>       copy                  copy notes for first object onto second object
>       append                append notes to existing object
>       edit                  edit notes for given object
>       show                  show notes for given object
>       merge                 merge given notes ref into current notes ref
>       remove                remove notes for given objects
>       prune                 remove all notes for non-existing/unreachable objects
>       get-ref               print the current notes ref

The goal looks good.  As Junio said, the code looks simplifiable.

Hope that helps,
Jonathan

Re: [RFC/PATCH] parse-options: introduce parse_subcommands

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>  I found this patch in one of my branches from a long time ago.  I
>>  haven't written documentation and tests yet because I'm not fully
>>  convinced that I like this.
>>
>>  Thoughts?
>
> It looks like a static version of string_list_lookup() and does not
> have much reason to tie it to "subcommand".

How so?  I can use a string_list to keep subcommand->name and stuff
the callback into util.  Where do I put subcommand->help then?
I agree that there's no reason to tie it to subcommand though.

Ram

Re: [RFC/PATCH] parse-options: introduce parse_subcommands

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> It looks like a static version of string_list_lookup() and does not
>> have much reason to tie it to "subcommand".
>
> How so?  I can use a string_list to keep subcommand->name and stuff
> the callback into util.  Where do I put subcommand->help then?

A string_list is a mapping from a string to an arbitrary piece of
data; there is nothing that stops you from placing a pointer to a
structure in its util field.

> I agree that there's no reason to tie it to subcommand though.

Yeah, if it were a generic API for a mapping from a string to an
arbitrary piece of data that is determined at compile time, it would
be a useful addition, and at that point, it is misleading to call
that a "parse-subcommand" API.  It is just a look-up mechansim in
a fixed table keyed by strings.

[RFC] notes: attach help text to subcommands

[Ramkumar Ramachandra]

Subcommands now have descriptive help text attached to them so that
the following error message:

  $ git notes foo
  error: Unknown subcommand: foo
  usage: git notes [--ref <notes_ref>] [list [<object>]]
     or: git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>
     or: git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] edit [<object>]
     or: git notes [--ref <notes_ref>] show [<object>]
     or: git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>
     or: git notes merge --commit [-v | -q]
     or: git notes merge --abort [-v | -q]
     or: git notes [--ref <notes_ref>] remove [<object>...]
     or: git notes [--ref <notes_ref>] prune [-n | -v]
     or: git notes [--ref <notes_ref>] get-ref

    --ref <notes_ref>     use notes from <notes_ref>

is replaced by a more helpful:

  $ git notes foo
  error: unknown subcommand: foo
  usage: git notes [<options>] [<subcommand>] [<options>] [<object>..]

  available subcommands:
      list                  list notes for given object
      add                   add notes for given object
      copy                  copy notes for first object onto second object
      append                append notes to existing object
      edit                  edit notes for given object
      show                  show notes for given object
      merge                 merge given notes ref into current notes ref
      remove                remove notes for given objects
      prune                 remove all notes for non-existing/unreachable objects
      get-ref               print the current notes ref

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I coerced my patch to use the string_list API, and this is the
 result.  It looks terribly ugly, and I have no idea how to proceed.

 Thoughts/ suggestions?

 builtin/notes.c |  119 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 37 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..d5fcf45 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,18 +20,7 @@
 #include "notes-merge.h"
 
 static const char * const git_notes_usage[] = {
-	"git notes [--ref <notes_ref>] [list [<object>]]",
-	"git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>",
-	"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] edit [<object>]",
-	"git notes [--ref <notes_ref>] show [<object>]",
-	"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
-	"git notes merge --commit [-v | -q]",
-	"git notes merge --abort [-v | -q]",
-	"git notes [--ref <notes_ref>] remove [<object>...]",
-	"git notes [--ref <notes_ref>] prune [-n | -v]",
-	"git notes [--ref <notes_ref>] get-ref",
+	"git notes [<options>] [<subcommand>] [<options>] [<object>..]",
 	NULL
 };
 
@@ -1059,15 +1048,75 @@ static int get_ref(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+
+#define USAGE_OPTS_WIDTH 24
+#define USAGE_GAP         2
+
+typedef int subcmd_cb(int argc, const char **argv, const char *prefix);
+
+struct subcommand {
+	const char *help;
+	subcmd_cb *callback;
+};
+
+static void subcommand_usage(const char * const *usagestr,
+			struct string_list *subcmds)
+{
+	int i;
+
+	fprintf(stderr, "usage: %s\n", *usagestr++);
+	while (*usagestr && **usagestr)
+		fprintf(stderr, "   or: %s\n", *usagestr++);
+	while (*usagestr) {
+		fprintf(stderr, "%s%s\n",
+				**usagestr ? "    " : "",
+				*usagestr);
+		usagestr++;
+	}
+
+	fputc('\n', stderr);
+	fprintf(stderr, "available subcommands:\n");
+	for (i = 0; i < subcmds->nr; i++) {
+		size_t pos;
+		int pad;
+
+		pos = fprintf(stderr, "    ");
+		pos += fprintf(stderr, "%s", subcmds->items[i].string);
+
+		if (pos <= USAGE_OPTS_WIDTH)
+			pad = USAGE_OPTS_WIDTH - pos;
+		else {
+			fputc('\n', stderr);
+			pad = USAGE_OPTS_WIDTH;
+		}
+		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "",
+			((struct subcommand *)(subcmds->items[i].util))->help);
+	}
+	fputc('\n', stderr);
+}
+
 int cmd_notes(int argc, const char **argv, const char *prefix)
 {
-	int result;
 	const char *override_notes_ref = NULL;
+	struct string_list subcmds = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
 	struct option options[] = {
 		OPT_STRING(0, "ref", &override_notes_ref, "notes_ref",
 			   "use notes from <notes_ref>"),
 		OPT_END()
 	};
+	struct subcommand util[] = {
+		{ "list notes for given object", list },
+		{ "add notes for given object", add },
+		{ "copy notes for first object onto second object", copy },
+		{ "append notes to existing object", append_edit },
+		{ "edit notes for given object", append_edit },
+		{ "show notes for given object", show },
+		{ "merge given notes ref into current notes ref", merge },
+		{ "remove notes for given objects", remove_cmd },
+		{ "remove all notes for non-existing/unreachable objects", prune },
+		{ "print the current notes ref", get_ref }
+	};
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, git_notes_usage,
@@ -1081,28 +1130,24 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 		strbuf_release(&sb);
 	}
 
-	if (argc < 1 || !strcmp(argv[0], "list"))
-		result = list(argc, argv, prefix);
-	else if (!strcmp(argv[0], "add"))
-		result = add(argc, argv, prefix);
-	else if (!strcmp(argv[0], "copy"))
-		result = copy(argc, argv, prefix);
-	else if (!strcmp(argv[0], "append") || !strcmp(argv[0], "edit"))
-		result = append_edit(argc, argv, prefix);
-	else if (!strcmp(argv[0], "show"))
-		result = show(argc, argv, prefix);
-	else if (!strcmp(argv[0], "merge"))
-		result = merge(argc, argv, prefix);
-	else if (!strcmp(argv[0], "remove"))
-		result = remove_cmd(argc, argv, prefix);
-	else if (!strcmp(argv[0], "prune"))
-		result = prune(argc, argv, prefix);
-	else if (!strcmp(argv[0], "get-ref"))
-		result = get_ref(argc, argv, prefix);
-	else {
-		result = error(_("Unknown subcommand: %s"), argv[0]);
-		usage_with_options(git_notes_usage, options);
-	}
-
-	return result ? 1 : 0;
+	string_list_insert(&subcmds, "list")->util = &util[0];
+	string_list_insert(&subcmds, "add")->util = &util[1];
+	string_list_insert(&subcmds, "copy")->util = &util[2];
+	string_list_insert(&subcmds, "append")->util = &util[3];
+	string_list_insert(&subcmds, "edit")->util = &util[4];
+	string_list_insert(&subcmds, "show")->util = &util[5];
+	string_list_insert(&subcmds, "merge")->util = &util[6];
+	string_list_insert(&subcmds, "remove")->util = &util[7];
+	string_list_insert(&subcmds, "prune")->util = &util[8];
+	string_list_insert(&subcmds, "get-ref")->util = &util[9];
+
+	if (argc < 1)
+		return list(argc, argv, prefix) ? 1 : 0;
+
+	item = unsorted_string_list_lookup(&subcmds, argv[0]);
+	if (item)
+		return ((struct subcommand *)(item->util))->callback(argc, argv, prefix) ? 1 : 0;
+	error("unknown subcommand: %s", argv[0]);
+	subcommand_usage(git_notes_usage, &subcmds);
+	exit(129);
 }
-- 
1.7.10

Re: [RFC] notes: attach help text to subcommands

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:


> +	string_list_insert(&subcmds, "list")->util = &util[0];
> +	string_list_insert(&subcmds, "add")->util = &util[1];
> ...

If you make "struct subcommand" to contain entry's own name, you
could simply do this, no?

	for (i = 0; i < ARRAY_SIZE(util); i++)
        	string_list_insert(&subcmds, util[i].name)->util = &util[i];

Perhaps append all and then sort once at the end?

[PATCH] notes: attach help text to subcommands

[Ramkumar Ramachandra]

Attach descriptive help text to subcommands of 'git notes' so that the
following error message:

  $ git notes foo
  error: Unknown subcommand: foo
  usage: git notes [--ref <notes_ref>] [list [<object>]]
     or: git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>
     or: git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]
     or: git notes [--ref <notes_ref>] edit [<object>]
     or: git notes [--ref <notes_ref>] show [<object>]
     or: git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>
     or: git notes merge --commit [-v | -q]
     or: git notes merge --abort [-v | -q]
     or: git notes [--ref <notes_ref>] remove [<object>...]
     or: git notes [--ref <notes_ref>] prune [-n | -v]
     or: git notes [--ref <notes_ref>] get-ref

    --ref <notes_ref>     use notes from <notes_ref>

is replaced by a more helpful:

  $ git notes foo
  error: unknown subcommand: foo
  usage: git notes [<options>] [<subcommand>] [<options>] [<object>..]

  available subcommands:
      list                  list notes for given object
      add                   add notes for given object
      copy                  copy notes for first object onto second object
      append                append notes to existing object
      edit                  edit notes for given object
      show                  show notes for given object
      merge                 merge given notes ref into current notes ref
      remove                remove notes for given objects
      prune                 remove all notes for non-existing/unreachable objects
      get-ref               print the current notes ref

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Junio C Hamano wrote:
 > If you make "struct subcommand" to contain entry's own name, you
 > could simply do this, no?
 >
 >        for (i = 0; i < ARRAY_SIZE(util); i++)
 >                string_list_insert(&subcmds, util[i].name)->util = &util[i];
 >
 > Perhaps append all and then sort once at the end?
 
 Thanks.  I wish we could do something aout USAGE_OPTS_WIDTH and
 USAGE_GAP; I stole them from parse-options.c.  Plus, `struct
 subcommand` and `subcommand_usage` need a new home if they're going
 to be used by more builtins with subcommands.
 
 Thoughts?

 builtin/notes.c |  112 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3644d14..223f990 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,18 +20,7 @@
 #include "notes-merge.h"
 
 static const char * const git_notes_usage[] = {
-	"git notes [--ref <notes_ref>] [list [<object>]]",
-	"git notes [--ref <notes_ref>] add [-f] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] copy [-f] <from-object> <to-object>",
-	"git notes [--ref <notes_ref>] append [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]",
-	"git notes [--ref <notes_ref>] edit [<object>]",
-	"git notes [--ref <notes_ref>] show [<object>]",
-	"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
-	"git notes merge --commit [-v | -q]",
-	"git notes merge --abort [-v | -q]",
-	"git notes [--ref <notes_ref>] remove [<object>...]",
-	"git notes [--ref <notes_ref>] prune [-n | -v]",
-	"git notes [--ref <notes_ref>] get-ref",
+	"git notes [<options>] [<subcommand>] [<options>] [<object>..]",
 	NULL
 };
 
@@ -1059,15 +1048,76 @@ static int get_ref(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+
+struct subcommand {
+	const char *name;
+	const char *help;
+	int (*callback)(int, const char **, const char *);
+};
+
+#define USAGE_OPTS_WIDTH 24
+#define USAGE_GAP         2
+
+static void subcommand_usage(const char * const *usagestr,
+			struct string_list *subcmds)
+{
+	int i;
+
+	fprintf(stderr, "usage: %s\n", *usagestr++);
+	while (*usagestr && **usagestr)
+		fprintf(stderr, "   or: %s\n", *usagestr++);
+	while (*usagestr) {
+		fprintf(stderr, "%s%s\n",
+				**usagestr ? "    " : "",
+				*usagestr);
+		usagestr++;
+	}
+
+	fputc('\n', stderr);
+	fprintf(stderr, "available subcommands:\n");
+	for (i = 0; i < subcmds->nr; i++) {
+		size_t pos;
+		int pad;
+
+		pos = fprintf(stderr, "    ");
+		pos += fprintf(stderr, "%s", subcmds->items[i].string);
+
+		if (pos <= USAGE_OPTS_WIDTH)
+			pad = USAGE_OPTS_WIDTH - pos;
+		else {
+			fputc('\n', stderr);
+			pad = USAGE_OPTS_WIDTH;
+		}
+		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "",
+			((struct subcommand *)(subcmds->items[i].util))->help);
+	}
+	fputc('\n', stderr);
+}
+
 int cmd_notes(int argc, const char **argv, const char *prefix)
 {
-	int result;
+	int i;
 	const char *override_notes_ref = NULL;
+	struct string_list subcmds = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
 	struct option options[] = {
 		OPT_STRING(0, "ref", &override_notes_ref, "notes_ref",
 			   "use notes from <notes_ref>"),
 		OPT_END()
 	};
+	struct subcommand util[] = {
+		{ "list", "list notes for given object", list },
+		{ "add", "add notes for given object", add },
+		{ "copy", "copy notes for first object onto second object", copy },
+		{ "append", "append notes to existing object", append_edit },
+		{ "edit", "edit notes for given object", append_edit },
+		{ "show", "show notes for given object", show },
+		{ "merge", "merge given notes ref into current notes ref", merge },
+		{ "remove", "remove notes for given objects", remove_cmd },
+		{ "prune",
+		  "remove all notes for non-existing/unreachable objects", prune },
+		{ "get-ref", "print the current notes ref", get_ref }
+	};
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, git_notes_usage,
@@ -1081,28 +1131,18 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 		strbuf_release(&sb);
 	}
 
-	if (argc < 1 || !strcmp(argv[0], "list"))
-		result = list(argc, argv, prefix);
-	else if (!strcmp(argv[0], "add"))
-		result = add(argc, argv, prefix);
-	else if (!strcmp(argv[0], "copy"))
-		result = copy(argc, argv, prefix);
-	else if (!strcmp(argv[0], "append") || !strcmp(argv[0], "edit"))
-		result = append_edit(argc, argv, prefix);
-	else if (!strcmp(argv[0], "show"))
-		result = show(argc, argv, prefix);
-	else if (!strcmp(argv[0], "merge"))
-		result = merge(argc, argv, prefix);
-	else if (!strcmp(argv[0], "remove"))
-		result = remove_cmd(argc, argv, prefix);
-	else if (!strcmp(argv[0], "prune"))
-		result = prune(argc, argv, prefix);
-	else if (!strcmp(argv[0], "get-ref"))
-		result = get_ref(argc, argv, prefix);
-	else {
-		result = error(_("Unknown subcommand: %s"), argv[0]);
-		usage_with_options(git_notes_usage, options);
-	}
+	for (i = 0; i < ARRAY_SIZE(util); i++)
+		string_list_insert(&subcmds, util[i].name)->util = &util[i];
+	sort_string_list(&subcmds);
+
+	if (argc < 1)
+		return list(argc, argv, prefix) ? 1 : 0;
+
+	item = string_list_lookup(&subcmds, argv[0]);
+	if (item)
+		return ((struct subcommand *)(item->util))->callback(argc, argv, prefix) ? 1 : 0;
 
-	return result ? 1 : 0;
+	error("unknown subcommand: %s", argv[0]);
+	subcommand_usage(git_notes_usage, &subcmds);
+	exit(129);
 }
-- 
1.7.10

Re: [PATCH] notes: attach help text to subcommands

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  Thanks.  I wish we could do something aout USAGE_OPTS_WIDTH and
>  USAGE_GAP; I stole them from parse-options.

Expose them in parse-options.h?  Or put this functionality in a
parseopt-related file?

Copy+paste must die. :)

Re: [PATCH] notes: attach help text to subcommands

[Felipe Contreras]

On Fri, Jun 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>
>>  Thanks.  I wish we could do something aout USAGE_OPTS_WIDTH and
>>  USAGE_GAP; I stole them from parse-options.
>
> Expose them in parse-options.h?  Or put this functionality in a
> parseopt-related file?
>
> Copy+paste must die. :)

There's many commands that would benefit from this. In addition to
provide a better help for subcommands, this can be used to generate
automatically the shell completion list of subcommands.

Even more; in zsh it would be possible to show this help text directly
in the completion.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] notes: attach help text to subcommands

[Ramkumar Ramachandra]

Hi Jonathan and Felipe,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>>  Thanks.  I wish we could do something aout USAGE_OPTS_WIDTH and
>>  USAGE_GAP; I stole them from parse-options.
>
> Expose them in parse-options.h?  Or put this functionality in a
> parseopt-related file?

Exposing a `subcommand_usage` in parse-options.h may not be a bad idea.

Felipe Contreras wrote:
> There's many commands that would benefit from this. In addition to
> provide a better help for subcommands, this can be used to generate
> automatically the shell completion list of subcommands.
>
> Even more; in zsh it would be possible to show this help text directly
> in the completion.

How exactly is that going to work?  `git notes -h` doesn't output
this- so the only way to get this output is to invoke `git notes` with
an invalid subcommand.  To fix that, I'd have to teach parse-options
about subcommands.

Ram

Re: [PATCH] notes: attach help text to subcommands

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +struct subcommand {
> +	const char *name;
> +	const char *help;
> +	int (*callback)(int, const char **, const char *);
> +};

In what way is this a "callback"?  It is not like you call some API
function with this structure as its argument, and then the
implementation of the API function calls this function back.

Re: [PATCH] notes: attach help text to subcommands

[Felipe Contreras]

On Fri, Jun 8, 2012 at 6:34 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Hi Jonathan and Felipe,
>
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>
>>>  Thanks.  I wish we could do something aout USAGE_OPTS_WIDTH and
>>>  USAGE_GAP; I stole them from parse-options.
>>
>> Expose them in parse-options.h?  Or put this functionality in a
>> parseopt-related file?
>
> Exposing a `subcommand_usage` in parse-options.h may not be a bad idea.
>
> Felipe Contreras wrote:
>> There's many commands that would benefit from this. In addition to
>> provide a better help for subcommands, this can be used to generate
>> automatically the shell completion list of subcommands.
>>
>> Even more; in zsh it would be possible to show this help text directly
>> in the completion.
>
> How exactly is that going to work?  `git notes -h` doesn't output
> this- so the only way to get this output is to invoke `git notes` with
> an invalid subcommand.  To fix that, I'd have to teach parse-options
> about subcommands.

Yeah, something like that. See this proposal:

http://mid.gmane.org/1334140165-24958-2-git-send-email-bebarino@gmail.com

I would prefer something like 'git info --subcommands notes' or
something like that.

Cheers.

-- 
Felipe Contreras