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

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

[Ramkumar Ramachandra]

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

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Contrasting this with "git tag --list", one uses subcommand verb while the
other uses operating mode option and one could say they are inconsistent.

But it does not bother me that much, and for a good reason other than
inertia.

When a command whose primary purpose is very clear (e.g. "git tag" and
"git branch" are primarily to create these things, just like "git commit"
is to create a commit), it is more natural to give the primary mode the
main interface so that you do not have to say "git tag create v1.0"; hence
operating mode option makes sense than subcommand.

On the other hand, when a command does not have a clear primary mode
(e.g. if you save a stash you must be able to use (i.e. apply or pop) the
saved one and both are equally important feature), but its primary purpose
is to dispatch various different operation, it makes more sense to name
them as subcommands. You _could_ make all of them into operating mode
options, but that only requires more typing, i.e. "git stash --save"/"git
stash --pop", without adding much value to the command. In addition, it
invites unnecessary confusion "what is the default mode of operation, and
is it really that important to be the default?", because not requiring an
explicit "subcommand" but merely allowing "operation mode option" implies
that you can say "git cmd" without anything, i.e. there must be some
default.

For "stash", "save" has been the default merely by historical accident,
but that has been rectified (it now requires you do not have any message
for the quick stash "git satsh<ENTER>" to work). There really isn't any
"default" operating mode to the command, and the command is a dispatcher
to its subcommands.

[PATCH v2 0/6] Fix '&&' chaining in tests

[Ramkumar Ramachandra]

Hi,

This follows-up $gmane/186481.  Thanks to Jonathan and Matthieu for
going through it.  Changes are:
1. Changes in response to Jonathan's review.
2. Squash similar patches and re-order.

Thanks.

-- Ram

Ramkumar Ramachandra (6):
  t3040 (subprojects-basic): modernize style
  t3030 (merge-recursive): use test_expect_code
  t1006 (cat-file): use test_cmp
  t3200 (branch): fix '&&' chaining
  t1510 (worktree): fix '&&' chaining
  test: fix '&&' chaining

 t/t1006-cat-file.sh                   |  119 +++++++++++++--------------
 t/t1007-hash-object.sh                |    2 +-
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1501-worktree.sh                   |    6 +-
 t/t1510-repo-setup.sh                 |    4 +-
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3030-merge-recursive.sh            |   72 ++---------------
 t/t3040-subprojects-basic.sh          |  144 ++++++++++++++++----------------
 t/t3200-branch.sh                     |    4 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +-
 t/t3400-rebase.sh                     |    4 +-
 t/t3418-rebase-continue.sh            |    4 +-
 t/t3419-rebase-patch-id.sh            |    2 +-
 15 files changed, 156 insertions(+), 223 deletions(-)

-- 
1.7.7.3

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

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> -	if (argc < 3)
> -		usage(builtin_bundle_usage);
> -
> -	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);

Doesn't this make the usage completely confusing?  Before, if I wanted
to create bundle named "verify", I could write

	git bundle create verify

Afterwards, not only does that not work, but if I make a typo and
write

	git bundle ceate verify

then it will act like "git bundle verify ceate".

I am starting to suspect the first half of patch 1/2 was not such a
great idea, either. :)  Do you have other examples of how it would be
used?

Thanks for thining about these things, and hope that helps.
Jonathan

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> -     if (argc < 3)
>> -             usage(builtin_bundle_usage);
>> -
>> -     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);
>
> Doesn't this make the usage completely confusing?  Before, if I wanted
> to create bundle named "verify", I could write
>
>        git bundle create verify
>
> Afterwards, not only does that not work, but if I make a typo and
> write
>
>        git bundle ceate verify
>
> then it will act like "git bundle verify ceate".

No it won't -- it'll just print out the usage because "verify" and
"create" are mutually exclusive options.  Sure, you can't create a
bundle named "verify", but that's the compromise you'll have to make
if you don't want to type out "--" with each option, no?

> I am starting to suspect the first half of patch 1/2 was not such a
> great idea, either. :)  Do you have other examples of how it would be
> used?

Hehe, my lack of foresight is disturbing as usual.  I'm not yet sure
it'll be useful.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Sure, you can't create a
> bundle named "verify", but that's the compromise you'll have to make
> if you don't want to type out "--" with each option, no?

No, that's not a compromise I'll have to make.  I'm not making it today.

Having to type "--" or prefix with "./" to escape ordinary filenames
that do not start with "-" would be completely weird.  This is
important to me: I want you to see it, rather than relying on me as an
authority figure to say it (after all, I'm not such a great authority
anyway --- I make plenty of mistakes).

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Sure, you can't create a
>> bundle named "verify", but that's the compromise you'll have to make
>> if you don't want to type out "--" with each option, no?
>
> Having to type "--" or prefix with "./" to escape ordinary filenames
> that do not start with "-" would be completely weird.

Hm, do you have any suggestions to work around this?  Can we use
something like a parse-stopper after the the first subcommand is
encountered, and treat the next argument as a non-subcommand (filename
or whatever else)?

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> Having to type "--" or prefix with "./" to escape ordinary filenames
>> that do not start with "-" would be completely weird.
>
> Hm, do you have any suggestions to work around this?  Can we use
> something like a parse-stopper after the the first subcommand is
> encountered, and treat the next argument as a non-subcommand (filename
> or whatever else)?

What's the desired behavior?  Then we can talk about how to implement it.

If the goal is "use parse-options for a command that has subcommands",
see builtin/notes.c.

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

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> What's the desired behavior?  Then we can talk about how to implement it.
>
> If the goal is "use parse-options for a command that has subcommands",
> see builtin/notes.c.

Uses strcmp() to match argv[0].  And you can't specify the options for
a certain subcommand before the subcommand itself on the command-line,
although I don't consider this a serious limitation.  I was going for
something prettier with subcommand-specific help text, albeit a
serious limitation.  I'll try working towards this for a few more
hours to see if anything useful comes out of it -- otherwise, I'll
just drop this patch and focus on eliminating the ugliness in
builtin/revert.c around '--continue', '--quit' parsing.

That being said, do you see value in lifting the restriction on
opts->long_name and PARSE_OPTS_NODASH not allowed together?  The
restriction seems quite arbitrary, but I can't justify lifting it
unless I can show some valid usecase.

Thanks.

-- Ram

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> That being said, do you see value in lifting the restriction on
> opts->long_name and PARSE_OPTS_NODASH not allowed together?  The
> restriction seems quite arbitrary, but I can't justify lifting it
> unless I can show some valid usecase.

True and true.

As to the first "true", it is because the nodash was introduced only to
parse "find ... \( ... \)" style parentheses as if they are part of option
sequence, and that use case only needed a single letter.

As to the second "true", it is because so far we didn't need anything
longer.

I do not think the name of a subcommand is not a good use case example for
it, by the way. Unlike parentheses on the command line of "find" that can
come anywhere and there can be more than one, the subcommand must be the
first thing on the command line and only one subcommand is given at one
command invocation.

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

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > That being said, do you see value in lifting the restriction on
> > opts->long_name and PARSE_OPTS_NODASH not allowed together?  The
> > restriction seems quite arbitrary, but I can't justify lifting it
> > unless I can show some valid usecase.
> 
> True and true.
> 
> As to the first "true", it is because the nodash was introduced only to
> parse "find ... \( ... \)" style parentheses as if they are part of option
> sequence, and that use case only needed a single letter.
> 
> As to the second "true", it is because so far we didn't need anything
> longer.
> 
> I do not think the name of a subcommand is not a good use case example for
> it, by the way. Unlike parentheses on the command line of "find" that can
> come anywhere and there can be more than one, the subcommand must be the
> first thing on the command line and only one subcommand is given at one
> command invocation.

Well, I think it doesn't have to be true: there can be some options
like e.g. '-n' / '--dry-run' that are common to all subcommands, and
in my opinion they could come before subcommand name.

But if restriction that subcommand name must be first simplifies code,
then let's do it this way.


I agree that subcommands are and must be mutually exclusive --
otherwise they better be implemented as options, not subcommands.

-- 
Jakub Narębski

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Well, I think it doesn't have to be true: there can be some options
> like e.g. '-n' / '--dry-run' that are common to all subcommands, and
> in my opinion they could come before subcommand name.

That is just a crazy talk.

If you have three subcommands a, b and c, and only a and b shares -n, c
must learn to reject the option that does not apply. If you have more
subcommands and you need to add an option to one of them, you are forcing
logic to reject that new option to all other subcommands.

That is not a proper way to share option specification. Different
subcommands support different options, and if you want to share some
options among them, you add a new facility to _allow_ them to share _some_
options, while still allowing them to keep their own option specification
table. Otherwise the resulting mess will be unmaintainable.

[PATCH 0/2] Improve git-bundle builtin

[Ramkumar Ramachandra]

Hi,

My OPT_SUBCOMMAND idea crashed and burned; I decided to salvage some
of the work that went into improving the git-bundle builtin and put it
into another series along with some additional tests.  I hope this
benefits people who use git-bundle to do incremental backups on their
servers or otherwise.

A couple of thoughts:

1. There's a SP between the OBJID and the ref name in list-heads as
opposed to the TAB used by other git commands such as ls-remote,
diff-tree.  Will fixing it break someone's parser somewhere?

2. Is it worth fixing the "--stdin" tests?  What is the usecase?  A
quick blame points to f62e0a39 (t5704 (bundle): add tests for bundle
--stdin, 2010-04-19): Jonathan, Joey?

Cheers.

-- Ram

Ramkumar Ramachandra (2):
  t5704 (bundle): rewrite for larger coverage
  bundle: rewrite builtin to use parse-options

 builtin/bundle.c  |   91 +++++++++++++++++++++++++++++---------------------
 t/t5704-bundle.sh |   95 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 122 insertions(+), 64 deletions(-)

-- 
1.7.4.1

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

[Ramkumar Ramachandra]

The git-bundle builtin currently parses command-line options by hand;
this is fragile, and reports cryptic errors on failure.  Use the
parse-options library to do the parsing instead.

Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/bundle.c  |   91 +++++++++++++++++++++++++++++++----------------------
 t/t5704-bundle.sh |    2 +-
 2 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 92a8a60..13ed770 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,71 @@
  * 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
+};
 
 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 *subcommand, *bundle_file;
+	struct bundle_header header;
+	struct option options[] = { OPT_END() };
 
-	if (argc < 3)
-		usage(builtin_bundle_usage);
+	argc = parse_options(argc, argv, prefix, options,
+			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);
 
-	cmd = argv[1];
-	bundle_file = argv[2];
-	argc -= 2;
-	argv += 2;
+	if (argc < 2)
+		usage_with_options(builtin_bundle_usage, options);
+	subcommand = argv[0];
 
-	if (prefix && bundle_file[0] != '/') {
-		snprintf(buffer, sizeof(buffer), "%s/%s", prefix, bundle_file);
-		bundle_file = buffer;
-	}
+	argc = parse_options(argc, argv, prefix, options,
+			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	/* Disallow stray arguments */
+	if ((strcmp(subcommand, "create") && argc > 2) ||
+		(!strcmp(subcommand, "verify") && argc > 1))
+		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;
+	prefix_length = prefix ? strlen(prefix) : 0;
+	bundle_file = prefix_filename(prefix, prefix_length, argv[0]);
 
-	if (!strcmp(cmd, "verify")) {
+	/* Read out bundle header, except in the "create" case */
+	if (strcmp(subcommand, "create")) {
+		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);
+	}
+
+	if (!strcmp(subcommand, "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(subcommand, "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")) {
+	} else if (!strcmp(subcommand, "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);
+	} else if (!strcmp(subcommand, "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);
+	} else {
+		close(bundle_fd);
+		usage_with_options(builtin_bundle_usage, options);
+	}
+
+	return 0;
 }
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 09ff4f1..8e3f677 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -53,7 +53,7 @@ test_expect_success 'disallow stray command-line options' '
 	test_must_fail git bundle create --junk bundle second third
 '
 
-test_expect_failure 'disallow stray command-line arguments' '
+test_expect_success 'disallow stray command-line arguments' '
 	git bundle create bundle second third &&
 	test_must_fail git bundle verify bundle junk
 '
-- 
1.7.4.1

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The git-bundle builtin currently parses command-line options by hand;
> this is fragile, and reports cryptic errors on failure.  Use the
> parse-options library to do the parsing instead.

I don't understand how this is fragile.  I haven't actually run into
error messages from "git bundle" I found to be cryptic, but if they
are, they surely can be improved locally.  Could you give an example
or something?

> Encouraged-by: Jonathan Nieder <jrnieder@gmail.com>

No, not encouraged.

But parseoptification does have some nice benefits, so let's see how
the patch looks...

[...]
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/bundle.c  |   91 +++++++++++++++++++++++++++++++----------------------
>  t/t5704-bundle.sh |    2 +-
>  2 files changed, 54 insertions(+), 39 deletions(-)
> 
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 92a8a60..13ed770 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
[...]
> @@ -9,57 +10,71 @@
[...]
>  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];
> -	if (argc < 3)
> -		usage(builtin_bundle_usage);
> +	const char *subcommand, *bundle_file;
> +	struct bundle_header header;
> +	struct option options[] = { OPT_END() };
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			builtin_bundle_usage, PARSE_OPT_STOP_AT_NON_OPTION);

No, just no.  Using parse-options with an empty option table is
complete overkill for handling the "-h" option.  Without a lot more
justification, this doesn't make it seem more sane or readable at all.

Stopping here.  I wouldn't mind seeing "git bundle" being
parseoptified, but not if the result looks like this.

I _do_ think that a systematic option-parsing library that handles
subcommands would be something possible and probably useful for git.
Its input might include a table with subcommand names, an option table
for each, and a function to call when that subcommand is used:

	struct parseopt_subcommand subcmds[] = {
		{ "list", no_options, notes_list },
		{ "add", add_options, notes_add },
		{ "copy", copy_options, notes_copy },
		{ "append", append_options, notes_append },
		{ "edit", no_options, notes_edit },
		{ "show", no_options, notes_show },
		...
	};

Then "git notes -h" might be able to automatically generate a table of
synopses, and "gite notes --help-all" might print help for all
subcommands.  Something like that.

Hope that helps,
Jonathan