threads / rfc / 30697

RFC patchparse-options: introduce parse_subcommands

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

## tl;dr

13 messages between Jun 4, 2012 and Jun 13, 2012. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Ramkumar Ramachandra· Jun 4, 2012, 09:35 UTC · lore

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(-)
Show changes to 3 files +79 −34

builtin/notes.c, parse-options.c, parse-options.h

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
Junio C Hamano· Jun 4, 2012, 17:01 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
>  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".

Ramkumar Ramachandra· Jun 6, 2012, 14:10 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Junio C Hamano wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jun 6, 2012, 17:26 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
>> 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.

Ramkumar Ramachandra· Jun 8, 2012, 08:56 UTC · re: Junio C Hamano · lore

[RFC] notes: attach help text to subcommands

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(-)
Show changes to builtin/notes.c +82 −37
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
Junio C Hamano· Jun 8, 2012, 14:49 UTC · re: Ramkumar Ramachandra · lore

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

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?
Ramkumar Ramachandra· Jun 8, 2012, 15:28 UTC · re: Junio C Hamano · lore

[PATCH] notes: attach help text to subcommands

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(-)
Show changes to builtin/notes.c +76 −36
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
Jonathan Nieder· Jun 8, 2012, 15:36 UTC · re: Ramkumar Ramachandra · lore

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

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. :)
Felipe Contreras· Jun 8, 2012, 15:54 UTC · re: Jonathan Nieder · lore

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

On Fri, Jun 8, 2012 at 5:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> 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
Ramkumar Ramachandra· Jun 8, 2012, 16:34 UTC · re: Felipe Contreras · lore

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

Hi Jonathan and Felipe,
Jonathan Nieder wrote:
Show 7 quoted lines
> 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:
Show 6 quoted lines
> 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
Felipe Contreras· Jun 13, 2012, 15:04 UTC · re: Ramkumar Ramachandra · lore

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

On Fri, Jun 8, 2012 at 6:34 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 25 quoted lines
> 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
Junio C Hamano· Jun 8, 2012, 17:03 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
> +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.

Jonathan Nieder· Jun 5, 2012, 23:32 UTC · re: Ramkumar Ramachandra · lore

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

Hi,
Ramkumar Ramachandra wrote:
Show 5 quoted lines
>   $ 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>
[...]
Show 17 quoted lines
> 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

← back to recent threads