[PATCH 0/2] Parsing a subcommand using parse-options

DORMANTno replies

3 messages, 1 author, 2016-06-15 · open the first message on its own page

[PATCH 0/2] Parsing a subcommand using parse-options

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:35

Hi,

It's been itching me for some time now: I want parse-options to be
able to parse a "subcommand".  It all started when I noticed how ugly
my implementation of '--continue' and '--quit' in git-revert were:
using an OPT_BOOLEAN to parse the option into separate integer
variables, and then using if-else constructs to turn that into an
enum.  Yuck!  Also, wouldn't we all love something like this in the
future?

  git stash show
            ^^ -- The subcommand "show" to the git-stash builtin

Ofcourse, git-stash.sh is just a shell script, and the full motivation
doesn't arise until someone decides to turn it into a C builtin.  So,
I went hunting for a C builtin that used subcommands and found
something relatively obscure: git-bundle.  It does option-parsing by
hand; changing it to use parse-options is the perfect opportunity to
implement this subcommand feature!

Thoughts?

-- Ram

Ramkumar Ramachandra (2):
  parse-options: introduce OPT_SUBCOMMAND
  bundle: rewrite builtin to use parse-options

 builtin/bundle.c         |  111 ++++++++++++++++++++++++++++++----------------
 parse-options.c          |    5 +-
 parse-options.h          |    3 +
 t/t0040-parse-options.sh |   31 +++++++++++++
 test-parse-options.c     |    4 ++
 5 files changed, 114 insertions(+), 40 deletions(-)

-- 
1.7.7.3

[PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:35

Several git programs take long dash-less options on the command-line
to indicate different modes of operation like:

  git stash show
  git bundle verify test.bundle
  git bisect start

Currently, the parse-options framework forbids the use of
opts->long_name and OPT_PARSE_NODASH, and the parsing has to be done
by hand as a result.  Lift this restriction, and create a new
OPT_SUBCOMMAND; this is built on top of OPTION_BIT to allow for the
detection of more than one subcommand.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 parse-options.c          |    5 +++--
 parse-options.h          |    3 +++
 t/t0040-parse-options.sh |   31 +++++++++++++++++++++++++++++++
 test-parse-options.c     |    4 ++++
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index f0098eb..079616a 100644
--- a/parse-options.c
+++ b/parse-options.c

@@ -278,6 +278,8 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
 			continue;
 		if (options->short_name == arg[0] && arg[1] == '\0')
 			return get_value(p, options, OPT_SHORT);
+		if (options->long_name && !strcmp(options->long_name, arg))
+			return get_value(p, options, OPT_SHORT);
 	}
 	return -2;
 }

@@ -314,8 +316,7 @@ static void parse_options_check(const struct option *opts)
 		if (opts->flags & PARSE_OPT_NODASH &&
 		    ((opts->flags & PARSE_OPT_OPTARG) ||
 		     !(opts->flags & PARSE_OPT_NOARG) ||
-		     !(opts->flags & PARSE_OPT_NONEG) ||
-		     opts->long_name))
+		     !(opts->flags & PARSE_OPT_NONEG)))
 			err |= optbug(opts, "uses feature "
 					"not supported for dashless options");
 		switch (opts->type) {

diff --git a/parse-options.h b/parse-options.h
index 2e811dc..9267d46 100644
--- a/parse-options.h
+++ b/parse-options.h

@@ -123,6 +123,9 @@ struct option {
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \
 				      PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_SUBCOMMAND(l, v, h, i)  { OPTION_BIT, 0, (l), (v), NULL, (h), \
+				      PARSE_OPT_NONEG | PARSE_OPT_NOARG | \
+				      PARSE_OPT_NODASH | PARSE_OPT_HIDDEN, NULL, (i) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a1e4616..47a402e 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh

@@ -55,6 +55,7 @@ mv expect expect.err
 
 cat > expect << EOF
 boolean: 2
+subcommand: 0
 integer: 1729
 timestamp: 0
 string: 123

@@ -74,6 +75,7 @@ test_expect_success 'short options' '
 
 cat > expect << EOF
 boolean: 2
+subcommand: 0
 integer: 1729
 timestamp: 0
 string: 321

@@ -103,6 +105,7 @@ test_expect_success 'missing required value' '
 
 cat > expect << EOF
 boolean: 1
+subcommand: 0
 integer: 13
 timestamp: 0
 string: 123

@@ -125,6 +128,7 @@ test_expect_success 'intermingled arguments' '
 
 cat > expect << EOF
 boolean: 0
+subcommand: 0
 integer: 2
 timestamp: 0
 string: (not set)

@@ -154,6 +158,7 @@ test_expect_success 'ambiguously abbreviated option' '
 
 cat > expect << EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: 123

@@ -182,6 +187,7 @@ test_expect_success 'detect possible typos' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)

@@ -201,6 +207,7 @@ test_expect_success 'keep some options as arguments' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 1
 string: default

@@ -222,6 +229,7 @@ test_expect_success 'OPT_DATE() and OPT_SET_PTR() work' '
 cat > expect <<EOF
 Callback: "four", 0
 boolean: 5
+subcommand: 0
 integer: 4
 timestamp: 0
 string: (not set)

@@ -250,6 +258,7 @@ test_expect_success 'OPT_CALLBACK() and callback errors work' '
 
 cat > expect <<EOF
 boolean: 1
+subcommand: 0
 integer: 23
 timestamp: 0
 string: (not set)

@@ -274,6 +283,7 @@ test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
 
 cat > expect <<EOF
 boolean: 6
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)

@@ -304,6 +314,26 @@ test_expect_success 'OPT_BOOLEAN() with PARSE_OPT_NODASH works' '
 
 cat > expect <<EOF
 boolean: 0
+subcommand: 4
+integer: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'OPT_SUBCOMMAND() works' '
+	test-parse-options sub4 > output 2> output.err &&
+	test ! -s output.err &&
+	test_cmp expect output
+'
+
+cat > expect <<EOF
+boolean: 0
+subcommand: 0
 integer: 12345
 timestamp: 0
 string: (not set)

@@ -322,6 +352,7 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 
 cat >expect <<EOF
 boolean: 0
+subcommand: 0
 integer: 0
 timestamp: 0
 string: (not set)

diff --git a/test-parse-options.c b/test-parse-options.c
index 36487c4..8d5fcd4 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c

@@ -3,6 +3,7 @@
 #include "string-list.h"
 
 static int boolean = 0;
+static int subcommand = 0;
 static int integer = 0;
 static unsigned long timestamp;
 static int abbrev = 7;

@@ -40,6 +41,8 @@ int main(int argc, const char **argv)
 		OPT_BOOLEAN('b', "boolean", &boolean, "get a boolean"),
 		OPT_BIT('4', "or4", &boolean,
 			"bitwise-or boolean with ...0100", 4),
+		OPT_SUBCOMMAND("sub4", &subcommand,
+			"bitwise-or subcommand with ...0100", 4),
 		OPT_NEGBIT(0, "neg-or4", &boolean, "same as --no-or4", 4),
 		OPT_GROUP(""),
 		OPT_INTEGER('i', "integer", &integer, "get a integer"),

@@ -80,6 +83,7 @@ int main(int argc, const char **argv)
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	printf("boolean: %d\n", boolean);
+	printf("subcommand: %d\n", subcommand);
 	printf("integer: %u\n", integer);
 	printf("timestamp: %lu\n", timestamp);
 	printf("string: %s\n", string ? string : "(not set)");

-- 
1.7.7.3

[PATCH 2/2] bundle: rewrite builtin to use parse-options

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:52:35

The git-bundle builtin currently parses command-line options by hand;
this is both fragile and cryptic on failure.  Since we now have an
OPT_SUBCOMMAND, make use of it to parse the correct subcommand, while
forbidding the use of more than one subcommand in the same invocation.

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/bundle.c |  111 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 73 insertions(+), 38 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 92a8a60..c977d9f 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c

@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "bundle.h"
 
 /*

@@ -9,57 +10,91 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create <file> <git-rev-list args>\n"
-  "   or: git bundle verify <file>\n"
-  "   or: git bundle list-heads <file> [<refname>...]\n"
-  "   or: git bundle unbundle <file> [<refname>...]";
+static const char * builtin_bundle_usage[] = {
+	"git bundle create <file> <git-rev-list args>",
+	"git bundle verify <file>",
+	"git bundle list-heads <file> [<refname>...]",
+	"git bundle unbundle <file> [<refname>...]",
+	NULL
+};
+
+enum bundle_subcommand {
+	BUNDLE_NONE = 0,
+	BUNDLE_CREATE = 1,
+	BUNDLE_VERIFY = 2,
+	BUNDLE_LIST_HEADS = 4,
+	BUNDLE_UNBUNDLE = 8
+};
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
-	struct bundle_header header;
-	const char *cmd, *bundle_file;
+	int prefix_length;
 	int bundle_fd = -1;
-	char buffer[PATH_MAX];
+	const char *bundle_file;
+	struct bundle_header header;
+	enum bundle_subcommand subcommand = BUNDLE_NONE;
 
-	if (argc < 3)
-		usage(builtin_bundle_usage);
+	struct option options[] = {
+		OPT_SUBCOMMAND("create", &subcommand,
+			"create a new bundle",
+			BUNDLE_CREATE),
+		OPT_SUBCOMMAND("verify", &subcommand,
+			"verify clean application of the bundle",
+			BUNDLE_VERIFY),
+		OPT_SUBCOMMAND("list-heads", &subcommand,
+			"list references defined in the bundle",
+			BUNDLE_LIST_HEADS),
+		OPT_SUBCOMMAND("unbundle", &subcommand,
+			"pass objects in the bundle to 'git index-pack'",
+			BUNDLE_UNBUNDLE),
+		OPT_END(),
+	};
 
-	cmd = argv[1];
-	bundle_file = argv[2];
-	argc -= 2;
-	argv += 2;
+	argc = parse_options(argc, argv, NULL,
+			options, builtin_bundle_usage,
+			PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
 
-	if (prefix && bundle_file[0] != '/') {
-		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-		bundle_file = buffer;
-	}
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
 
-	memset(&header, 0, sizeof(header));
-	if (strcmp(cmd, "create") && (bundle_fd =
-				read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	/* The next parameter on the command line is bundle_file */
+	prefix_length = prefix ? strlen(prefix) : 0;
+	bundle_file = prefix_filename(prefix, prefix_length, argv[1]);
+	argc -= 1;
+	argv += 1;
 
-	if (!strcmp(cmd, "verify")) {
+	/* Read out bundle header, except in BUNDLE_CREATE case */
+	if (subcommand == BUNDLE_VERIFY || subcommand == BUNDLE_LIST_HEADS ||
+		subcommand == BUNDLE_UNBUNDLE) {
+		memset(&header, 0, sizeof(header));
+		bundle_fd = read_bundle_header(bundle_file, &header);
+		if (bundle_fd < 0)
+			die_errno(_("Failed to open bundle file '%s'"), bundle_file);
+	}
+
+	switch (subcommand) {
+	case BUNDLE_CREATE:
+		if (!startup_info->have_repository)
+			die(_("Need a repository to create a bundle."));
+		return create_bundle(&header, bundle_file, argc, argv);
+	case BUNDLE_VERIFY:
 		close(bundle_fd);
 		if (verify_bundle(&header, 1))
-			return 1;
+			return -1; /* Error already reported */
 		fprintf(stderr, _("%s is okay\n"), bundle_file);
-		return 0;
-	}
-	if (!strcmp(cmd, "list-heads")) {
+		break;
+	case BUNDLE_LIST_HEADS:
 		close(bundle_fd);
-		return !!list_bundle_refs(&header, argc, argv);
-	}
-	if (!strcmp(cmd, "create")) {
-		if (!startup_info->have_repository)
-			die(_("Need a repository to create a bundle."));
-		return !!create_bundle(&header, bundle_file, argc, argv);
-	} else if (!strcmp(cmd, "unbundle")) {
-		if (!startup_info->have_repository)
+		return list_bundle_refs(&header, argc, argv);
+	case BUNDLE_UNBUNDLE:
+		if (!startup_info->have_repository) {
+			close(bundle_fd);
 			die(_("Need a repository to unbundle."));
-		return !!unbundle(&header, bundle_fd, 0) ||
+		}
+		return unbundle(&header, bundle_fd, 0) ||
 			list_bundle_refs(&header, argc, argv);
-	} else
-		usage(builtin_bundle_usage);
+	default:
+		usage_with_options(builtin_bundle_usage, options);
+	}
+	return 0;
 }

-- 
1.7.7.3

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help