{"thread":{"id":"29108","subject":"[PATCH 0/2] Parsing a subcommand using parse-options","startedAt":"2011-12-08T12:07:06Z","lastAt":"2011-12-08T19:51:24Z","messageCount":4,"participants":["Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"180571","messageId":"1323346028-9201-1-git-send-email-artagnon@gmail.com","threadId":"29108","inReplyTo":null,"subject":"[PATCH 0/2] Parsing a subcommand using parse-options","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-08T12:07:06Z","receivedAt":"2011-12-08T12:07:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nIt's been itching me for some time now: I want parse-options to be\nable to parse a \"subcommand\". It all started when I noticed how ugly\nmy implementation of '--continue' and '--quit' in git-revert were:\nusing an OPT_BOOLEAN to parse the option into separate integer\nvariables, and then using if-else constructs to turn that into an\nenum. Yuck! Also, wouldn't we all love something like this in the\nfuture?\n\n git stash show\n ^^ -- The subcommand \"show\" to the git-stash builtin\n\nOfcourse, git-stash.sh is just a shell script, and the full motivation\ndoesn't arise until someone decides to turn it into a C builtin. So,\nI went hunting for a C builtin that used subcommands and found\nsomething relatively obscure: git-bundle. It does option-parsing by\nhand; changing it to use parse-options is the perfect opportunity to\nimplement this subcommand feature!\n\nThoughts?\n\n-- Ram\n\nRamkumar Ramachandra (2):\n parse-options: introduce OPT_SUBCOMMAND\n bundle: rewrite builtin to use parse-options\n\n builtin/bundle.c | 111 ++++++++++++++++++++++++++++++----------------\n parse-options.c | 5 +-\n parse-options.h | 3 +\n t/t0040-parse-options.sh | 31 +++++++++++++\n test-parse-options.c | 4 ++\n 5 files changed, 114 insertions(+), 40 deletions(-)\n\n-- \n1.7.7.3\n"},{"id":"180572","messageId":"1323346028-9201-2-git-send-email-artagnon@gmail.com","threadId":"29108","inReplyTo":"1323346028-9201-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/2] parse-options: introduce OPT_SUBCOMMAND","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-12-08T12:07:07Z","receivedAt":"2011-12-08T12:07:07Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Several git programs take long dash-less options on the command-line\nto indicate different modes of operation like:\n\n git stash show\n git bundle verify test.bundle\n git bisect start\n\nCurrently, the parse-options framework forbids the use of\nopts->long_name and OPT_PARSE_NODASH, and the parsing has to be done\nby hand as a result. Lift this restriction, and create a new\nOPT_SUBCOMMAND; this is built on top of OPTION_BIT to allow for the\ndetection of more than one subcommand.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n parse-options.c | 5 +++--\n parse-options.h | 3 +++\n t/t0040-parse-options.sh | 31 +++++++++++++++++++++++++++++++\n test-parse-options.c | 4 ++++\n 4 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex f0098eb..079616a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -278,6 +278,8 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\t\tcontinue;\n \t\tif (options->short_name == arg[0] && arg[1] == '\\0')\n \t\t\treturn get_value(p, options, OPT_SHORT);\n+\t\tif (options->long_name && !strcmp(options->long_name, arg))\n+\t\t\treturn get_value(p, options, OPT_SHORT);\n \t}\n \treturn -2;\n }\n@@ -314,8 +316,7 @@ static void parse_options_check(const struct option *opts)\n \t\tif (opts->flags & PARSE_OPT_NODASH &&\n \t\t ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t !(opts->flags & PARSE_OPT_NOARG) ||\n-\t\t !(opts->flags & PARSE_OPT_NONEG) ||\n-\t\t opts->long_name))\n+\t\t !(opts->flags & PARSE_OPT_NONEG)))\n \t\t\terr |= optbug(opts, \"uses feature \"\n \t\t\t\t\t\"not supported for dashless options\");\n \t\tswitch (opts->type) {\ndiff --git a/parse-options.h b/parse-options.h\nindex 2e811dc..9267d46 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -123,6 +123,9 @@ struct option {\n #define OPT_GROUP(h) { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }\n #define OPT_BIT(s, l, v, h, b) { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n \t\t\t\t PARSE_OPT_NOARG, NULL, (b) }\n+#define OPT_SUBCOMMAND(l, v, h, i) { OPTION_BIT, 0, (l), (v), NULL, (h), \\\n+\t\t\t\t PARSE_OPT_NONEG | PARSE_OPT_NOARG | \\\n+\t\t\t\t PARSE_OPT_NODASH | PARSE_OPT_HIDDEN, NULL, (i) }\n #define OPT_NEGBIT(s, l, v, h, b) { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, NULL, (b) }\n #define OPT_COUNTUP(s, l, v, h) { OPTION_COUNTUP, (s), (l), (v), NULL, \\\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex a1e4616..47a402e 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -55,6 +55,7 @@ mv expect expect.err\n \n cat > expect << EOF\n boolean: 2\n+subcommand: 0\n integer: 1729\n timestamp: 0\n string: 123\n@@ -74,6 +75,7 @@ test_expect_success 'short options' '\n \n cat > expect << EOF\n boolean: 2\n+subcommand: 0\n integer: 1729\n timestamp: 0\n string: 321\n@@ -103,6 +105,7 @@ test_expect_success 'missing required value' '\n \n cat > expect << EOF\n boolean: 1\n+subcommand: 0\n integer: 13\n timestamp: 0\n string: 123\n@@ -125,6 +128,7 @@ test_expect_success 'intermingled arguments' '\n \n cat > expect << EOF\n boolean: 0\n+subcommand: 0\n integer: 2\n timestamp: 0\n string: (not set)\n@@ -154,6 +158,7 @@ test_expect_success 'ambiguously abbreviated option' '\n \n cat > expect << EOF\n boolean: 0\n+subcommand: 0\n integer: 0\n timestamp: 0\n string: 123\n@@ -182,6 +187,7 @@ test_expect_success 'detect possible typos' '\n \n cat > expect < expect < expect < expect < expect < expect < output 2> output.err &&\n+\ttest ! -s output.err &&\n+\ttest_cmp expect output\n+'\n+\n+cat > expect <expect <\n---\n builtin/bundle.c | 111 +++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 73 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 92a8a60..c977d9f 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"parse-options.h\"\n #include \"bundle.h\"\n \n /*\n@@ -9,57 +10,91 @@\n * bundle supporting \"fetch\", \"pull\", and \"ls-remote\".\n */\n \n-static const char builtin_bundle_usage[] =\n- \"git bundle create \\n\"\n- \" or: git bundle verify \\n\"\n- \" or: git bundle list-heads [...]\\n\"\n- \" or: git bundle unbundle [...]\";\n+static const char * builtin_bundle_usage[] = {\n+\t\"git bundle create \",\n+\t\"git bundle verify \",\n+\t\"git bundle list-heads [...]\",\n+\t\"git bundle unbundle [...]\",\n+\tNULL\n+};\n+\n+enum bundle_subcommand {\n+\tBUNDLE_NONE = 0,\n+\tBUNDLE_CREATE = 1,\n+\tBUNDLE_VERIFY = 2,\n+\tBUNDLE_LIST_HEADS = 4,\n+\tBUNDLE_UNBUNDLE = 8\n+};\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n {\n-\tstruct bundle_header header;\n-\tconst char *cmd, *bundle_file;\n+\tint prefix_length;\n \tint bundle_fd = -1;\n-\tchar buffer[PATH_MAX];\n+\tconst char *bundle_file;\n+\tstruct bundle_header header;\n+\tenum bundle_subcommand subcommand = BUNDLE_NONE;\n \n-\tif (argc < 3)\n-\t\tusage(builtin_bundle_usage);\n+\tstruct option options[] = {\n+\t\tOPT_SUBCOMMAND(\"create\", &subcommand,\n+\t\t\t\"create a new bundle\",\n+\t\t\tBUNDLE_CREATE),\n+\t\tOPT_SUBCOMMAND(\"verify\", &subcommand,\n+\t\t\t\"verify clean application of the bundle\",\n+\t\t\tBUNDLE_VERIFY),\n+\t\tOPT_SUBCOMMAND(\"list-heads\", &subcommand,\n+\t\t\t\"list references defined in the bundle\",\n+\t\t\tBUNDLE_LIST_HEADS),\n+\t\tOPT_SUBCOMMAND(\"unbundle\", &subcommand,\n+\t\t\t\"pass objects in the bundle to 'git index-pack'\",\n+\t\t\tBUNDLE_UNBUNDLE),\n+\t\tOPT_END(),\n+\t};\n \n-\tcmd = argv[1];\n-\tbundle_file = argv[2];\n-\targc -= 2;\n-\targv += 2;\n+\targc = parse_options(argc, argv, NULL,\n+\t\t\toptions, builtin_bundle_usage,\n+\t\t\tPARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);\n \n-\tif (prefix && bundle_file[0] != '/') {\n-\t\tsnprintf(buffer, sizeof(buffer), \"%s/%s\", prefix, bundle_file);\n-\t\tbundle_file = buffer;\n-\t}\n+\tif (argc < 2)\n+\t\tusage_with_options(builtin_bundle_usage, options);\n \n-\tmemset(&header, 0, sizeof(header));\n-\tif (strcmp(cmd, \"create\") && (bundle_fd =\n-\t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\t/* The next parameter on the command line is bundle_file */\n+\tprefix_length = prefix ? strlen(prefix) : 0;\n+\tbundle_file = prefix_filename(prefix, prefix_length, argv[1]);\n+\targc -= 1;\n+\targv += 1;\n \n-\tif (!strcmp(cmd, \"verify\")) {\n+\t/* Read out bundle header, except in BUNDLE_CREATE case */\n+\tif (subcommand == BUNDLE_VERIFY || subcommand == BUNDLE_LIST_HEADS ||\n+\t\tsubcommand == BUNDLE_UNBUNDLE) {\n+\t\tmemset(&header, 0, sizeof(header));\n+\t\tbundle_fd = read_bundle_header(bundle_file, &header);\n+\t\tif (bundle_fd < 0)\n+\t\t\tdie_errno(_(\"Failed to open bundle file '%s'\"), bundle_file);\n+\t}\n+\n+\tswitch (subcommand) {\n+\tcase BUNDLE_CREATE:\n+\t\tif (!startup_info->have_repository)\n+\t\t\tdie(_(\"Need a repository to create a bundle.\"));\n+\t\treturn create_bundle(&header, bundle_file, argc, argv);\n+\tcase BUNDLE_VERIFY:\n \t\tclose(bundle_fd);\n \t\tif (verify_bundle(&header, 1))\n-\t\t\treturn 1;\n+\t\t\treturn -1; /* Error already reported */\n \t\tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(cmd, \"list-heads\")) {\n+\t\tbreak;\n+\tcase BUNDLE_LIST_HEADS:\n \t\tclose(bundle_fd);\n-\t\treturn !!list_bundle_refs(&header, argc, argv);\n-\t}\n-\tif (!strcmp(cmd, \"create\")) {\n-\t\tif (!startup_info->have_repository)\n-\t\t\tdie(_(\"Need a repository to create a bundle.\"));\n-\t\treturn !!create_bundle(&header, bundle_file, argc, argv);\n-\t} else if (!strcmp(cmd, \"unbundle\")) {\n-\t\tif (!startup_info->have_repository)\n+\t\treturn list_bundle_refs(&header, argc, argv);\n+\tcase BUNDLE_UNBUNDLE:\n+\t\tif (!startup_info->have_repository) {\n+\t\t\tclose(bundle_fd);\n \t\t\tdie(_(\"Need a repository to unbundle.\"));\n-\t\treturn !!unbundle(&header, bundle_fd, 0) ||\n+\t\t}\n+\t\treturn unbundle(&header, bundle_fd, 0) ||\n \t\t\tlist_bundle_refs(&header, argc, argv);\n-\t} else\n-\t\tusage(builtin_bundle_usage);\n+\tdefault:\n+\t\tusage_with_options(builtin_bundle_usage, options);\n+\t}\n+\treturn 0;\n }\n-- \n1.7.7.3\n"},{"id":"180612","messageId":"7v4nxaj0zn.fsf@alter.siamese.dyndns.org","threadId":"29108","inReplyTo":"1323346028-9201-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/2] Parsing a subcommand using parse-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-08T19:51:24Z","receivedAt":"2011-12-08T19:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> git stash show\n> ^^ -- The subcommand \"show\" to the git-stash builtin\n\nContrasting this with \"git tag --list\", one uses subcommand verb while the\nother uses operating mode option and one could say they are inconsistent.\n\nBut it does not bother me that much, and for a good reason other than\ninertia.\n\nWhen a command whose primary purpose is very clear (e.g. \"git tag\" and\n\"git branch\" are primarily to create these things, just like \"git commit\"\nis to create a commit), it is more natural to give the primary mode the\nmain interface so that you do not have to say \"git tag create v1.0\"; hence\noperating mode option makes sense than subcommand.\n\nOn the other hand, when a command does not have a clear primary mode\n(e.g. if you save a stash you must be able to use (i.e. apply or pop) the\nsaved one and both are equally important feature), but its primary purpose\nis to dispatch various different operation, it makes more sense to name\nthem as subcommands. You _could_ make all of them into operating mode\noptions, but that only requires more typing, i.e. \"git stash --save\"/\"git\nstash --pop\", without adding much value to the command. In addition, it\ninvites unnecessary confusion \"what is the default mode of operation, and\nis it really that important to be the default?\", because not requiring an\nexplicit \"subcommand\" but merely allowing \"operation mode option\" implies\nthat you can say \"git cmd\" without anything, i.e. there must be some\ndefault.\n\nFor \"stash\", \"save\" has been the default merely by historical accident,\nbut that has been rectified (it now requires you do not have any message\nfor the quick stash \"git satsh\" to work). There really isn't any\n\"default\" operating mode to the command, and the command is a dispatcher\nto its subcommands.\n"}]}