{"thread":{"id":"19914","subject":"[PATCH 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s","startedAt":"2009-06-24T04:27:51Z","lastAt":"2009-06-27T02:00:17Z","messageCount":12,"participants":["Stephen Boyd","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"116845","messageId":"1245817672-25483-1-git-send-email-bebarino@gmail.com","threadId":"19914","inReplyTo":null,"subject":"[PATCH 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-24T04:27:51Z","receivedAt":"2009-06-24T04:27:51Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Printing the usage message when encountering bad option combinations is\nnot very helpful. Instead, die with a message which tells the user\nexactly what combination is invalid.\n\nSigned-off-by: Stephen Boyd \n---\n builtin-read-tree.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 82e25ea..887e177 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -145,9 +145,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* using -u and -i at the same time makes no sense */\n \t\tif (1 < opts.index_only + opts.update)\n-\t\t\tusage(read_tree_usage);\n+\t\t\tdie(\"-u and -i at the same time makes no sense\");\n \n \t\tif (get_sha1(arg, sha1))\n \t\t\tdie(\"Not a valid object name %s\", arg);\n@@ -156,7 +155,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tstage++;\n \t}\n \tif ((opts.update||opts.index_only) && !opts.merge)\n-\t\tusage(read_tree_usage);\n+\t\tdie(\"%s is meaningless without -m\",\n+\t\t opts.update ? \"-u\" : \"-i\");\n \tif ((opts.dir && !opts.update))\n \t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n \tif (opts.merge && !opts.index_only)\n-- \n1.6.3.3.334.g916e1\n"},{"id":"116847","messageId":"1245817672-25483-2-git-send-email-bebarino@gmail.com","threadId":"19914","inReplyTo":"1245817672-25483-1-git-send-email-bebarino@gmail.com","subject":"[PATCH 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-24T04:27:52Z","receivedAt":"2009-06-24T04:27:52Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Cleanup the documentation to explicitly state that --exclude-directory\nis only meaningful when used with -u. Also make the documentation more\nconsistent with the usage message printed with read-tree --help-all.\n\nSigned-off-by: Stephen Boyd \n---\n Documentation/git-read-tree.txt | 5 +-\n builtin-read-tree.c | 220 +++++++++++++++++++++-----------------\n 2 files changed, 126 insertions(+), 99 deletions(-)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex 7160fa1..1e0cc8f 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index\n \n SYNOPSIS\n --------\n-'git read-tree' ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\n+'git read-tree' [--index-output=] \n+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=]\n+\t\t[-u [--exclude-per-directory=] | -i]]\n+\t\t[--index-output=] [ []]\n \n \n DESCRIPTION\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 887e177..adca739 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -12,6 +12,7 @@\n #include \"unpack-trees.h\"\n #include \"dir.h\"\n #include \"builtin.h\"\n+#include \"parse-options.h\"\n \n static int nr_trees;\n static struct tree *trees[MAX_UNPACK_TREES];\n@@ -29,7 +30,83 @@ static int list_tree(unsigned char *sha1)\n \treturn 0;\n }\n \n-static const char read_tree_usage[] = \"git read-tree ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\";\n+static const char * const read_tree_usage[] = {\n+\t\"git read-tree [--index-output=] \",\n+\t\"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u [--exclude-per-directory=] | -i]] [--index-output=] [ []]\",\n+\tNULL\n+};\n+\n+static int index_output_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tset_alternate_index_output(arg);\n+\treturn 0;\n+}\n+\n+static int prefix_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct unpack_trees_options *opts;\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->merge || opts->prefix)\n+\t\treturn 1;\n+\topts->prefix = arg;\n+\topts->merge = 1;\n+\tif (read_cache_unmerged())\n+\t\tdie(\"you need to resolve your current index first\");\n+\n+\treturn 0;\n+}\n+\n+static int reset_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct unpack_trees_options *opts;\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->merge || opts->prefix)\n+\t\treturn -1;\n+\topts->reset = 1;\n+\topts->merge = 1;\n+\tread_cache_unmerged();\n+\n+\treturn 0;\n+}\n+\n+static int merge_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct unpack_trees_options *opts;\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->merge || opts->prefix)\n+\t\treturn -1;\n+\tif (read_cache_unmerged())\n+\t\tdie(\"you need to resolve your current index first\");\n+\topts->merge = 1;\n+\n+\treturn 0;\n+}\n+\n+static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tstruct dir_struct *dir;\n+\tstruct unpack_trees_options *opts;\n+\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->dir)\n+\t\tdie(\"more than one --exclude-per-directory given.\");\n+\n+\tdir = xcalloc(1, sizeof(*opts->dir));\n+\tdir->flags |= DIR_SHOW_IGNORED;\n+\tdir->exclude_per_dir = arg;\n+\topts->dir = dir;\n+\t/* We do not need to nor want to do read-directory\n+\t * here; we are merely interested in reusing the\n+\t * per directory ignore stack mechanism.\n+\t */\n+\treturn 0;\n+}\n \n static struct lock_file lock_file;\n \n@@ -39,6 +116,37 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tunsigned char sha1[20];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n+\tint update = 0, index_only = 0, trivial_merges_only = 0, aggressive = 0,\n+\t verbose = 0;\n+\tconst struct option read_tree_options[] = {\n+\t\t{ OPTION_CALLBACK, 0, \"index-output\", NULL, \"FILE\",\n+\t\t \"write resulting index to \",\n+\t\t PARSE_OPT_NONEG, index_output_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"prefix\", &opts, \"/\",\n+\t\t \"read the tree into the index under \",\n+\t\t PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, prefix_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n+\t\t \"gitignore\",\n+\t\t \"allow explicitly ignored files to be overwritten\",\n+\t\t PARSE_OPT_NONEG, exclude_per_directory_cb },\n+\t\tOPT__VERBOSE(&verbose),\n+\t\tOPT_GROUP(\"Merging\"),\n+\t\t{ OPTION_CALLBACK, 'm', NULL, &opts, NULL,\n+\t\t \"perform a merge in addition to a read\",\n+\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, merge_cb },\n+\t\t{ OPTION_CALLBACK, 0, \"reset\", &opts, NULL,\n+\t\t \"same as -m, except unmerged entries are discarded\",\n+\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, reset_cb },\n+\t\tOPT_BOOLEAN('u', NULL, &update,\n+\t\t\t \"update working tree with merge result\"),\n+\t\tOPT_BOOLEAN('i', NULL, &index_only,\n+\t\t\t \"don't check the working tree after merging\"),\n+\t\tOPT_BOOLEAN(0, \"trivial\", &trivial_merges_only,\n+\t\t\t \"3-way merge if no file level merging required\"),\n+\t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive,\n+\t\t\t \"3-way merge in presence of adds and removes\"),\n+\t\tOPT_END()\n+\t};\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n@@ -49,104 +157,18 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tnewfd = hold_locked_index(&lock_file, 1);\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\n-\t\t/* \"-u\" means \"update\", meaning that a merge will update\n-\t\t * the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-u\")) {\n-\t\t\topts.update = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"-v\")) {\n-\t\t\topts.verbose_update = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"-i\" means \"index only\", meaning that a merge will\n-\t\t * not even look at the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-i\")) {\n-\t\t\topts.index_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!prefixcmp(arg, \"--index-output=\")) {\n-\t\t\tset_alternate_index_output(arg + 15);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"--prefix=/\" means keep the current index\n-\t\t * entries and put the entries from the tree under the\n-\t\t * given subdirectory.\n-\t\t */\n-\t\tif (!prefixcmp(arg, \"--prefix=\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.prefix = arg + 9;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tcontinue;\n-\t\t}\n+\targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n+\t\t\t read_tree_usage, 0);\n \n-\t\t/* This differs from \"-m\" in that we'll silently ignore\n-\t\t * unmerged entries and overwrite working tree files that\n-\t\t * correspond to them.\n-\t\t */\n-\t\tif (!strcmp(arg, \"--reset\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.reset = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tread_cache_unmerged();\n-\t\t\tcontinue;\n-\t\t}\n+\topts.update = update ? 1 : 0;\n+\topts.index_only = index_only ? 1 : 0;\n+\topts.trivial_merges_only = trivial_merges_only ? 1 : 0;\n+\topts.aggressive = aggressive ? 1 : 0;\n+\topts.verbose_update = verbose ? 1 : 0;\n+\tstage = opts.merge;\n \n-\t\tif (!strcmp(arg, \"--trivial\")) {\n-\t\t\topts.trivial_merges_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"--aggressive\")) {\n-\t\t\topts.aggressive = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"-m\" stands for \"merge\", meaning we start in stage 1 */\n-\t\tif (!strcmp(arg, \"-m\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tstage = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!prefixcmp(arg, \"--exclude-per-directory=\")) {\n-\t\t\tstruct dir_struct *dir;\n-\n-\t\t\tif (opts.dir)\n-\t\t\t\tdie(\"more than one --exclude-per-directory are given.\");\n-\n-\t\t\tdir = xcalloc(1, sizeof(*opts.dir));\n-\t\t\tdir->flags |= DIR_SHOW_IGNORED;\n-\t\t\tdir->exclude_per_dir = arg + 24;\n-\t\t\topts.dir = dir;\n-\t\t\t/* We do not need to nor want to do read-directory\n-\t\t\t * here; we are merely interested in reusing the\n-\t\t\t * per directory ignore stack mechanism.\n-\t\t\t */\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (1 < opts.index_only + opts.update)\n-\t\t\tdie(\"-u and -i at the same time makes no sense\");\n+\tfor (i = 0; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tif (get_sha1(arg, sha1))\n \t\t\tdie(\"Not a valid object name %s\", arg);\n@@ -154,6 +176,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\tdie(\"failed to unpack tree object %s\", arg);\n \t\tstage++;\n \t}\n+\tif (1 < opts.index_only + opts.update)\n+\t\tdie(\"-u and -i at the same time makes no sense\");\n \tif ((opts.update||opts.index_only) && !opts.merge)\n \t\tdie(\"%s is meaningless without -m\",\n \t\t opts.update ? \"-u\" : \"-i\");\n-- \n1.6.3.3.334.g916e1\n"},{"id":"116849","messageId":"7vws721ao9.fsf@alter.siamese.dyndns.org","threadId":"19914","inReplyTo":"1245817672-25483-2-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH 2/2] read-tree: migrate to parse-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-24T05:08:06Z","receivedAt":"2009-06-24T05:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd writes:\n\n> Cleanup the documentation to explicitly state that --exclude-directory\n> is only meaningful when used with -u. Also make the documentation more\n> consistent with the usage message printed with read-tree --help-all.\n>\n> Signed-off-by: Stephen Boyd \n> ---\n> Documentation/git-read-tree.txt | 5 +-\n> builtin-read-tree.c | 220 +++++++++++++++++++++-----------------\n> 2 files changed, 126 insertions(+), 99 deletions(-)\n\nSorry, but I have to ask: Why?\n"},{"id":"116900","messageId":"4A42D49F.8010805@gmail.com","threadId":"19914","inReplyTo":"7vws721ao9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-25T01:36:31Z","receivedAt":"2009-06-25T01:36:31Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Junio C Hamano wrote:\n> Sorry, but I have to ask: Why?\n\nI think there are advantages to parse-optification, for plumbing and\nporcelains. Two reasons I see are:\n\n1. Providing a unified way of handling options\n2. Providing a consistent usage message format\n\nObviously, 1 reduces the bugs associated with parsing options (strcmp\nvs. prefixcmp, incorrect argv+offset). For number 2, I think it helps\nusers when they see the same style of usage messages with each command.\nIt's also nice to get a quick help message without opening the man pages\nor using git help .\n\nAdmittedly this patch ends up adding 20 or so lines. Do the above points\njustify this? I think so. I think the added lines can be attributed to\nthe rougher edges of parse-opts exposed by this patch. You can't take\nthe address of bit fields, so 6 lines are dealing with this problem.\nWhere are the other 15 lines coming from?\n\nLooking over it again, I think I may be able to cut the overhead down by\nrefactoring three of the callbacks into boolean options. There's a lot\nof duplication there, which can be simplified. I was trying to make this\na straight port which is probably not so good for convincing you that\nit's worthwhile.\n\nFor now, I don't mind this patch being dropped (there's an ambiguity in\nthe callbacks returning non-zero I'd like to fix too). I'll try and get\na new patch (or maybe this patch with the oneline fix and a refactoring\npatch) out later tonight that actually reduces the amount of lines,\ninstead of grossly adding to them.\n"},{"id":"116902","messageId":"1245906361-20644-1-git-send-email-bebarino@gmail.com","threadId":"19914","inReplyTo":"1245817672-25483-2-git-send-email-bebarino@gmail.com","subject":"[PATCHv2 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-25T05:06:01Z","receivedAt":"2009-06-25T05:06:01Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Cleanup the documentation to explicitly state that --exclude-directory\nis only meaningful when used with -u. Also make the documentation more\nconsistent with the usage message printed with read-tree --help-all.\n\nThe -m, --prefix, and --reset options are performing similar actions\n(setting some flags, read_cache_unmerged(), checking for illegal option\ncombinations). Instead of performing these actions when the options are\nparsed, we delay doing them until after parse-opts has finished.\n\nSigned-off-by: Stephen Boyd \n---\n\nI updated the two usage strings for --prefix and --reset too.\n\n Documentation/git-read-tree.txt | 5 +-\n builtin-read-tree.c | 183 ++++++++++++++++++---------------------\n 2 files changed, 89 insertions(+), 99 deletions(-)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex 7160fa1..1e0cc8f 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index\n \n SYNOPSIS\n --------\n-'git read-tree' ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\n+'git read-tree' [--index-output=] \n+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=]\n+\t\t[-u [--exclude-per-directory=] | -i]]\n+\t\t[--index-output=] [ []]\n \n \n DESCRIPTION\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 887e177..ba3ae6b 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -12,6 +12,7 @@\n #include \"unpack-trees.h\"\n #include \"dir.h\"\n #include \"builtin.h\"\n+#include \"parse-options.h\"\n \n static int nr_trees;\n static struct tree *trees[MAX_UNPACK_TREES];\n@@ -29,7 +30,40 @@ static int list_tree(unsigned char *sha1)\n \treturn 0;\n }\n \n-static const char read_tree_usage[] = \"git read-tree ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\";\n+static const char * const read_tree_usage[] = {\n+\t\"git read-tree [--index-output=] \",\n+\t\"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u [--exclude-per-directory=] | -i]] [--index-output=] [ []]\",\n+\tNULL\n+};\n+\n+static int index_output_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tset_alternate_index_output(arg);\n+\treturn 0;\n+}\n+\n+static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tstruct dir_struct *dir;\n+\tstruct unpack_trees_options *opts;\n+\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->dir)\n+\t\tdie(\"more than one --exclude-per-directory given.\");\n+\n+\tdir = xcalloc(1, sizeof(*opts->dir));\n+\tdir->flags |= DIR_SHOW_IGNORED;\n+\tdir->exclude_per_dir = arg;\n+\topts->dir = dir;\n+\t/* We do not need to nor want to do read-directory\n+\t * here; we are merely interested in reusing the\n+\t * per directory ignore stack mechanism.\n+\t */\n+\treturn 0;\n+}\n \n static struct lock_file lock_file;\n \n@@ -39,6 +73,35 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tunsigned char sha1[20];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n+\tint update = 0, index_only = 0, trivial_merges_only = 0, aggressive = 0,\n+\t verbose = 0, merge = 0, reset = 0, prefix_set = 0;\n+\tconst struct option read_tree_options[] = {\n+\t\t{ OPTION_CALLBACK, 0, \"index-output\", NULL, \"FILE\",\n+\t\t \"write resulting index to \",\n+\t\t PARSE_OPT_NONEG, index_output_cb },\n+\t\t{ OPTION_STRING, 0, \"prefix\", &opts.prefix, \"/\",\n+\t\t \"read the tree into the index under /\",\n+\t\t PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n+\t\t \"gitignore\",\n+\t\t \"allow explicitly ignored files to be overwritten\",\n+\t\t PARSE_OPT_NONEG, exclude_per_directory_cb },\n+\t\tOPT__VERBOSE(&verbose),\n+\t\tOPT_GROUP(\"Merging\"),\n+\t\tOPT_BOOLEAN('m', NULL, &merge,\n+\t\t\t \"perform a merge in addition to a read\"),\n+\t\tOPT_BOOLEAN(0, \"reset\", &reset,\n+\t\t\t \"same as -m, but discard unmerged entries\"),\n+\t\tOPT_BOOLEAN('u', NULL, &update,\n+\t\t\t \"update working tree with merge result\"),\n+\t\tOPT_BOOLEAN('i', NULL, &index_only,\n+\t\t\t \"don't check the working tree after merging\"),\n+\t\tOPT_BOOLEAN(0, \"trivial\", &trivial_merges_only,\n+\t\t\t \"3-way merge if no file level merging required\"),\n+\t\tOPT_BOOLEAN(0, \"aggressive\", &aggressive,\n+\t\t\t \"3-way merge in presence of adds and removes\"),\n+\t\tOPT_END()\n+\t};\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n@@ -49,104 +112,24 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tnewfd = hold_locked_index(&lock_file, 1);\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n+\targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n+\t\t\t read_tree_usage, 0);\n \n-\t\t/* \"-u\" means \"update\", meaning that a merge will update\n-\t\t * the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-u\")) {\n-\t\t\topts.update = 1;\n-\t\t\tcontinue;\n-\t\t}\n+\tprefix_set = opts.prefix ? 1 : 0;\n \n-\t\tif (!strcmp(arg, \"-v\")) {\n-\t\t\topts.verbose_update = 1;\n-\t\t\tcontinue;\n-\t\t}\n+\tif (read_cache_unmerged() && (prefix_set || merge))\n+\t\tdie(\"You need to resolve your current index first\");\n \n-\t\t/* \"-i\" means \"index only\", meaning that a merge will\n-\t\t * not even look at the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-i\")) {\n-\t\t\topts.index_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n+\topts.update = update ? 1 : 0;\n+\topts.index_only = index_only ? 1 : 0;\n+\topts.trivial_merges_only = trivial_merges_only ? 1 : 0;\n+\topts.aggressive = aggressive ? 1 : 0;\n+\topts.verbose_update = verbose ? 1 : 0;\n+\topts.reset = reset ? 1 : 0;\n+\tstage = opts.merge = (reset || merge || prefix_set) ? 1 : 0;\n \n-\t\tif (!prefixcmp(arg, \"--index-output=\")) {\n-\t\t\tset_alternate_index_output(arg + 15);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"--prefix=/\" means keep the current index\n-\t\t * entries and put the entries from the tree under the\n-\t\t * given subdirectory.\n-\t\t */\n-\t\tif (!prefixcmp(arg, \"--prefix=\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.prefix = arg + 9;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* This differs from \"-m\" in that we'll silently ignore\n-\t\t * unmerged entries and overwrite working tree files that\n-\t\t * correspond to them.\n-\t\t */\n-\t\tif (!strcmp(arg, \"--reset\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.reset = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tread_cache_unmerged();\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"--trivial\")) {\n-\t\t\topts.trivial_merges_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"--aggressive\")) {\n-\t\t\topts.aggressive = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"-m\" stands for \"merge\", meaning we start in stage 1 */\n-\t\tif (!strcmp(arg, \"-m\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tstage = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!prefixcmp(arg, \"--exclude-per-directory=\")) {\n-\t\t\tstruct dir_struct *dir;\n-\n-\t\t\tif (opts.dir)\n-\t\t\t\tdie(\"more than one --exclude-per-directory are given.\");\n-\n-\t\t\tdir = xcalloc(1, sizeof(*opts.dir));\n-\t\t\tdir->flags |= DIR_SHOW_IGNORED;\n-\t\t\tdir->exclude_per_dir = arg + 24;\n-\t\t\topts.dir = dir;\n-\t\t\t/* We do not need to nor want to do read-directory\n-\t\t\t * here; we are merely interested in reusing the\n-\t\t\t * per directory ignore stack mechanism.\n-\t\t\t */\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (1 < opts.index_only + opts.update)\n-\t\t\tdie(\"-u and -i at the same time makes no sense\");\n+\tfor (i = 0; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tif (get_sha1(arg, sha1))\n \t\t\tdie(\"Not a valid object name %s\", arg);\n@@ -154,6 +137,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\tdie(\"failed to unpack tree object %s\", arg);\n \t\tstage++;\n \t}\n+\tif (1 < opts.merge + opts.reset + prefix_set)\n+\t\tdie(\"Which one? -m, --reset, or --prefix?\");\n+\tif (1 < opts.index_only + opts.update)\n+\t\tdie(\"-u and -i at the same time makes no sense\");\n \tif ((opts.update||opts.index_only) && !opts.merge)\n \t\tdie(\"%s is meaningless without -m\",\n \t\t opts.update ? \"-u\" : \"-i\");\n-- \n1.6.3.3.334.g916e1\n"},{"id":"116904","messageId":"4A431F5E.6070109@viscovery.net","threadId":"19914","inReplyTo":"1245906361-20644-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCHv2 2/2] read-tree: migrate to parse-options","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-06-25T06:55:26Z","receivedAt":"2009-06-25T06:55:26Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Stephen Boyd schrieb:\n> @@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index\n> \n> SYNOPSIS\n> --------\n> -'git read-tree' ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\n> +'git read-tree' [--index-output=] \n> +'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=]\n> +\t\t[-u [--exclude-per-directory=] | -i]]\n> +\t\t[--index-output=] [ []]\n\nMulti-line synopsis must begin with [verse].\n\nIf you write more than one mode of operation, the subsequent text should\nbetter reference them, but the current text does not do that. I think it\nis OK if you leave only the second, particularly because the first is only\na subset of the second.\n\n> +\topts.update = update ? 1 : 0;\n> +\topts.index_only = index_only ? 1 : 0;\n> +\topts.trivial_merges_only = trivial_merges_only ? 1 : 0;\n> +\topts.aggressive = aggressive ? 1 : 0;\n> +\topts.verbose_update = verbose ? 1 : 0;\n> +\topts.reset = reset ? 1 : 0;\n> +\tstage = opts.merge = (reset || merge || prefix_set) ? 1 : 0;\n\nI don't think that the bitfields of struct unpack_trees_options are cast\nin stone. IMHO it is fine to make them regular struct members, so that you\ncan take their address for read_tree_options and these foo ? 1 : 0 become\nunnecessary.\n\n-- Hannes\n"},{"id":"116988","messageId":"4A443D6D.7060009@gmail.com","threadId":"19914","inReplyTo":"4A431F5E.6070109@viscovery.net","subject":"Re: [PATCHv2 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-26T03:15:57Z","receivedAt":"2009-06-26T03:15:57Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Johannes Sixt wrote:\n>\n> If you write more than one mode of operation, the subsequent text should\n> better reference them, but the current text does not do that. I think it\n> is OK if you leave only the second, particularly because the first is only\n> a subset of the second.\n\nI was contemplating this change, but I left it out because the single\ntree case felt special. So special that I felt the merging and the\nreading were two different modes. The description section hints at the\ntwo types of uses, but I think you want it to be more explicit? I'll\nhave to think about this more.\n\n> I don't think that the bitfields of struct unpack_trees_options are cast\n> in stone. IMHO it is fine to make them regular struct members, so that you\n> can take their address for read_tree_options and these foo ? 1 : 0 become\n> unnecessary\n\nThanks. I'll fix this up and resend the series.\n"},{"id":"116998","messageId":"1245993250-8213-1-git-send-email-bebarino@gmail.com","threadId":"19914","inReplyTo":"1245817672-25483-1-git-send-email-bebarino@gmail.com","subject":"[PATCHv3 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-26T05:14:09Z","receivedAt":"2009-06-26T05:14:09Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Printing the usage message when encountering bad option combinations is\nnot very helpful. Instead, die with a message which tells the user\nexactly what combination is invalid.\n\nSigned-off-by: Stephen Boyd \n---\n\nAdded --prefix and --reset to the second die message.\n\n builtin-read-tree.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 82e25ea..17c9631 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -145,9 +145,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* using -u and -i at the same time makes no sense */\n \t\tif (1 < opts.index_only + opts.update)\n-\t\t\tusage(read_tree_usage);\n+\t\t\tdie(\"-u and -i at the same time makes no sense\");\n \n \t\tif (get_sha1(arg, sha1))\n \t\t\tdie(\"Not a valid object name %s\", arg);\n@@ -156,7 +155,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tstage++;\n \t}\n \tif ((opts.update||opts.index_only) && !opts.merge)\n-\t\tusage(read_tree_usage);\n+\t\tdie(\"%s is meaningless without -m, --reset, or --prefix\",\n+\t\t opts.update ? \"-u\" : \"-i\");\n \tif ((opts.dir && !opts.update))\n \t\tdie(\"--exclude-per-directory is meaningless unless -u\");\n \tif (opts.merge && !opts.index_only)\n-- \n1.6.3.3.334.g916e1\n"},{"id":"116999","messageId":"1245993250-8213-2-git-send-email-bebarino@gmail.com","threadId":"19914","inReplyTo":"1245993250-8213-1-git-send-email-bebarino@gmail.com","subject":"[PATCHv3 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-26T05:14:10Z","receivedAt":"2009-06-26T05:14:10Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Cleanup the documentation to explicitly state that --exclude-directory\nis only meaningful when used with -u. Also make the documentation more\nconsistent with the usage message printed with read-tree --help-all.\n\nThe -m, --prefix, --reset options are performing similar actions\n(setting some flags, read_cache_unmerged(), checking for illegal option\ncombinations). Instead of performing these actions when the options are\nparsed, we delay performing them until after parse-opts has finished.\n\nThe bit fields in struct unpack_trees_options have been promoted to full\nunsigned ints. This is necessary to avoid \"foo ? 1 : 0\" constructs to\nset these fields.\n\nSigned-off-by: Stephen Boyd \n---\n\nAfter some more thought, Hannes has convinced me that removing the first\nmode is OK. I was trying to rewrite the documentation, and it just seemed\nbetter the way it was.\n\nA couple changes here:\n - reordered read_tree_options a bit to group the options with their\n dependent options better.\n - replaced OPT_BOOLEAN with OPT_SET_INT to avoid increment nature of\n OPT_BOOLEAN messing up logic depending on 1's and 0's only.\n\n Documentation/git-read-tree.txt | 5 +-\n builtin-read-tree.c | 174 +++++++++++++++++----------------------\n unpack-trees.h | 24 +++---\n 3 files changed, 92 insertions(+), 111 deletions(-)\n\ndiff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt\nindex 7160fa1..4a932b0 100644\n--- a/Documentation/git-read-tree.txt\n+++ b/Documentation/git-read-tree.txt\n@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index\n \n SYNOPSIS\n --------\n-'git read-tree' ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\n+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=]\n+\t\t[-u [--exclude-per-directory=] | -i]]\n+\t\t[--index-output=]\n+\t\t [ []]\n \n \n DESCRIPTION\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 17c9631..9c2d634 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -12,6 +12,7 @@\n #include \"unpack-trees.h\"\n #include \"dir.h\"\n #include \"builtin.h\"\n+#include \"parse-options.h\"\n \n static int nr_trees;\n static struct tree *trees[MAX_UNPACK_TREES];\n@@ -29,7 +30,39 @@ static int list_tree(unsigned char *sha1)\n \treturn 0;\n }\n \n-static const char read_tree_usage[] = \"git read-tree ( | [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u | -i]] [--exclude-per-directory=] [--index-output=] [ []])\";\n+static const char * const read_tree_usage[] = {\n+\t\"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=] [-u [--exclude-per-directory=] | -i]] [--index-output=] [ []]\",\n+\tNULL\n+};\n+\n+static int index_output_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tset_alternate_index_output(arg);\n+\treturn 0;\n+}\n+\n+static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n+\t\t\t\t int unset)\n+{\n+\tstruct dir_struct *dir;\n+\tstruct unpack_trees_options *opts;\n+\n+\topts = (struct unpack_trees_options *)opt->value;\n+\n+\tif (opts->dir)\n+\t\tdie(\"more than one --exclude-per-directory given.\");\n+\n+\tdir = xcalloc(1, sizeof(*opts->dir));\n+\tdir->flags |= DIR_SHOW_IGNORED;\n+\tdir->exclude_per_dir = arg;\n+\topts->dir = dir;\n+\t/* We do not need to nor want to do read-directory\n+\t * here; we are merely interested in reusing the\n+\t * per directory ignore stack mechanism.\n+\t */\n+\treturn 0;\n+}\n \n static struct lock_file lock_file;\n \n@@ -39,6 +72,34 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tunsigned char sha1[20];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n+\tint prefix_set = 0;\n+\tconst struct option read_tree_options[] = {\n+\t\t{ OPTION_CALLBACK, 0, \"index-output\", NULL, \"FILE\",\n+\t\t \"write resulting index to \",\n+\t\t PARSE_OPT_NONEG, index_output_cb },\n+\t\tOPT__VERBOSE(&opts.verbose_update),\n+\t\tOPT_GROUP(\"Merging\"),\n+\t\tOPT_SET_INT('m', NULL, &opts.merge,\n+\t\t\t \"perform a merge in addition to a read\", 1),\n+\t\tOPT_SET_INT(0, \"trivial\", &opts.trivial_merges_only,\n+\t\t\t \"3-way merge if no file level merging required\", 1),\n+\t\tOPT_SET_INT(0, \"aggressive\", &opts.aggressive,\n+\t\t\t \"3-way merge in presence of adds and removes\", 1),\n+\t\tOPT_SET_INT(0, \"reset\", &opts.reset,\n+\t\t\t \"same as -m, but discard unmerged entries\", 1),\n+\t\t{ OPTION_STRING, 0, \"prefix\", &opts.prefix, \"/\",\n+\t\t \"read the tree into the index under /\",\n+\t\t PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },\n+\t\tOPT_SET_INT('u', NULL, &opts.update,\n+\t\t\t \"update working tree with merge result\", 1),\n+\t\t{ OPTION_CALLBACK, 0, \"exclude-per-directory\", &opts,\n+\t\t \"gitignore\",\n+\t\t \"allow explicitly ignored files to be overwritten\",\n+\t\t PARSE_OPT_NONEG, exclude_per_directory_cb },\n+\t\tOPT_SET_INT('i', NULL, &opts.index_only,\n+\t\t\t \"don't check the working tree after merging\", 1),\n+\t\tOPT_END()\n+\t};\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n@@ -49,104 +110,19 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tnewfd = hold_locked_index(&lock_file, 1);\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\n-\t\t/* \"-u\" means \"update\", meaning that a merge will update\n-\t\t * the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-u\")) {\n-\t\t\topts.update = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"-v\")) {\n-\t\t\topts.verbose_update = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"-i\" means \"index only\", meaning that a merge will\n-\t\t * not even look at the working tree.\n-\t\t */\n-\t\tif (!strcmp(arg, \"-i\")) {\n-\t\t\topts.index_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!prefixcmp(arg, \"--index-output=\")) {\n-\t\t\tset_alternate_index_output(arg + 15);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* \"--prefix=/\" means keep the current index\n-\t\t * entries and put the entries from the tree under the\n-\t\t * given subdirectory.\n-\t\t */\n-\t\tif (!prefixcmp(arg, \"--prefix=\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.prefix = arg + 9;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/* This differs from \"-m\" in that we'll silently ignore\n-\t\t * unmerged entries and overwrite working tree files that\n-\t\t * correspond to them.\n-\t\t */\n-\t\tif (!strcmp(arg, \"--reset\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\topts.reset = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tstage = 1;\n-\t\t\tread_cache_unmerged();\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"--trivial\")) {\n-\t\t\topts.trivial_merges_only = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(arg, \"--aggressive\")) {\n-\t\t\topts.aggressive = 1;\n-\t\t\tcontinue;\n-\t\t}\n+\targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n+\t\t\t read_tree_usage, 0);\n \n-\t\t/* \"-m\" stands for \"merge\", meaning we start in stage 1 */\n-\t\tif (!strcmp(arg, \"-m\")) {\n-\t\t\tif (stage || opts.merge || opts.prefix)\n-\t\t\t\tusage(read_tree_usage);\n-\t\t\tif (read_cache_unmerged())\n-\t\t\t\tdie(\"you need to resolve your current index first\");\n-\t\t\tstage = 1;\n-\t\t\topts.merge = 1;\n-\t\t\tcontinue;\n-\t\t}\n+\tif (read_cache_unmerged() && (opts.prefix || opts.merge))\n+\t\tdie(\"You need to resolve your current index first\");\n \n-\t\tif (!prefixcmp(arg, \"--exclude-per-directory=\")) {\n-\t\t\tstruct dir_struct *dir;\n-\n-\t\t\tif (opts.dir)\n-\t\t\t\tdie(\"more than one --exclude-per-directory are given.\");\n-\n-\t\t\tdir = xcalloc(1, sizeof(*opts.dir));\n-\t\t\tdir->flags |= DIR_SHOW_IGNORED;\n-\t\t\tdir->exclude_per_dir = arg + 24;\n-\t\t\topts.dir = dir;\n-\t\t\t/* We do not need to nor want to do read-directory\n-\t\t\t * here; we are merely interested in reusing the\n-\t\t\t * per directory ignore stack mechanism.\n-\t\t\t */\n-\t\t\tcontinue;\n-\t\t}\n+\tprefix_set = opts.prefix ? 1 : 0;\n+\tif (1 < opts.merge + opts.reset + prefix_set)\n+\t\tdie(\"Which one? -m, --reset, or --prefix?\");\n+\tstage = opts.merge = (opts.reset || opts.merge || prefix_set);\n \n-\t\tif (1 < opts.index_only + opts.update)\n-\t\t\tdie(\"-u and -i at the same time makes no sense\");\n+\tfor (i = 0; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n \n \t\tif (get_sha1(arg, sha1))\n \t\t\tdie(\"Not a valid object name %s\", arg);\n@@ -154,6 +130,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\t\tdie(\"failed to unpack tree object %s\", arg);\n \t\tstage++;\n \t}\n+\tif (1 < opts.index_only + opts.update)\n+\t\tdie(\"-u and -i at the same time makes no sense\");\n \tif ((opts.update||opts.index_only) && !opts.merge)\n \t\tdie(\"%s is meaningless without -m, --reset, or --prefix\",\n \t\t opts.update ? \"-u\" : \"-i\");\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 1e0e232..d19df44 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -17,18 +17,18 @@ struct unpack_trees_error_msgs {\n };\n \n struct unpack_trees_options {\n-\tunsigned int reset:1,\n-\t\t merge:1,\n-\t\t update:1,\n-\t\t index_only:1,\n-\t\t nontrivial_merge:1,\n-\t\t trivial_merges_only:1,\n-\t\t verbose_update:1,\n-\t\t aggressive:1,\n-\t\t skip_unmerged:1,\n-\t\t initial_checkout:1,\n-\t\t diff_index_cached:1,\n-\t\t gently:1;\n+\tunsigned int reset,\n+\t\t merge,\n+\t\t update,\n+\t\t index_only,\n+\t\t nontrivial_merge,\n+\t\t trivial_merges_only,\n+\t\t verbose_update,\n+\t\t aggressive,\n+\t\t skip_unmerged,\n+\t\t initial_checkout,\n+\t\t diff_index_cached,\n+\t\t gently;\n \tconst char *prefix;\n \tint pos;\n \tstruct dir_struct *dir;\n-- \n1.6.3.3.334.g916e1\n"},{"id":"117002","messageId":"4A445CB0.4010506@gmail.com","threadId":"19914","inReplyTo":"1245993250-8213-2-git-send-email-bebarino@gmail.com","subject":"Re: [PATCHv3 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-26T05:29:20Z","receivedAt":"2009-06-26T05:29:20Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Stephen Boyd wrote:\n> struct unpack_trees_options {\n> -\tunsigned int reset:1,\n> -\t\t merge:1,\n> -\t\t update:1,\n> -\t\t index_only:1,\n> -\t\t nontrivial_merge:1,\n> -\t\t trivial_merges_only:1,\n> -\t\t verbose_update:1,\n> -\t\t aggressive:1,\n> -\t\t skip_unmerged:1,\n> -\t\t initial_checkout:1,\n> -\t\t diff_index_cached:1,\n> -\t\t gently:1;\n> +\tunsigned int reset,\n> +\t\t merge,\n> +\t\t update,\n> +\t\t index_only,\n> +\t\t nontrivial_merge,\n> +\t\t trivial_merges_only,\n> +\t\t verbose_update,\n> +\t\t aggressive,\n> +\t\t skip_unmerged,\n> +\t\t initial_checkout,\n> +\t\t diff_index_cached,\n> +\t\t gently;\n\nSorry I went a little overboard with s/:1// on unpack_tree_options.\nYou'll probably want to squash this on top.\n\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex d19df44..f344679 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -21,14 +21,14 @@ struct unpack_trees_options {\n merge,\n update,\n index_only,\n- nontrivial_merge,\n+ nontrivial_merge:1,\n trivial_merges_only,\n verbose_update,\n aggressive,\n- skip_unmerged,\n- initial_checkout,\n- diff_index_cached,\n- gently;\n+ skip_unmerged:1,\n+ initial_checkout:1,\n+ diff_index_cached:1,\n+ gently:1;\n const char *prefix;\n int pos;\n struct dir_struct *dir;\n"},{"id":"117032","messageId":"7vab3uucxi.fsf@alter.siamese.dyndns.org","threadId":"19914","inReplyTo":"4A445CB0.4010506@gmail.com","subject":"Re: [PATCHv3 2/2] read-tree: migrate to parse-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-26T17:23:21Z","receivedAt":"2009-06-26T17:23:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd writes:\n\n> Sorry I went a little overboard with s/:1// on unpack_tree_options.\n> You'll probably want to squash this on top.\n>\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index d19df44..f344679 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -21,14 +21,14 @@ struct unpack_trees_options {\n> merge,\n> update,\n> index_only,\n> - nontrivial_merge,\n> + nontrivial_merge:1,\n> trivial_merges_only,\n> verbose_update,\n> aggressive,\n> - skip_unmerged,\n> - initial_checkout,\n> - diff_index_cached,\n> - gently;\n> + skip_unmerged:1,\n> + initial_checkout:1,\n> + diff_index_cached:1,\n> + gently:1;\n\nLet's look at this (not this follow-up patch) the other way around.\n\nSix months down the load, somebody may ask you:\n\n Is there a good reason why many are not bitfields but only selected\n few are bitfields in this structure? Most of these can and should be\n bitfields, as far as I can see, because the code uses them as\n booleans, and the only breakage it may cause if we change them to\n bitfields to shrink this structure would be the option parsing code.\n\nWhat would be your answer? Doesn't it feel wrong to do such a conversion\nonly to work around the current limitation of parseopt?\n\nBy the way, has it been verified that all the users of these fields are OK\nwith this change when they use these fields? I am not worried about them\nreading the value command line option parser set, but more worried about\nreading after other codepaths set/modified these fields. The command line\nparser that uses parseopt may correctly set only 0 or 1 to these fields\ninitially and we should be able to verify that from the patch text, but\nthere is no guarantee that this conversion is even correct at runtime\nwithout an audit, no?\n\nThe callers have long relied on the fact that reading from these bitfields\nyields either 0 or 1 and never 2 or larger, but they are now widened to\nfull-width unsigned. A pattern like this:\n\n\tuto.field = ~uto.field;\n if (uto.field == 1) {\n \tfield now set;\n\t} else {\n \tfield now unset;\n\t}\n\nwould be broken by widening \"unsigned field:1\" to \"unsigned field\", right?\nI am not saying this is the only pattern that would break, nor I know\nthere are codepaths that use this pattern, but I think you got my point.\n"},{"id":"117070","messageId":"4A457D31.9030407@gmail.com","threadId":"19914","inReplyTo":"7vab3uucxi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3 2/2] read-tree: migrate to parse-options","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-06-27T02:00:17Z","receivedAt":"2009-06-27T02:00:17Z","isPatch":false,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Junio C Hamano wrote:\n>\n> Let's look at this (not this follow-up patch) the other way around.\n>\n> Six months down the load, somebody may ask you:\n>\n> Is there a good reason why many are not bitfields but only selected\n> few are bitfields in this structure? Most of these can and should be\n> bitfields, as far as I can see, because the code uses them as\n> booleans, and the only breakage it may cause if we change them to\n> bitfields to shrink this structure would be the option parsing code.\n>\n> What would be your answer? Doesn't it feel wrong to do such a conversion\n> only to work around the current limitation of parseopt?\n\nI understand. It does feel wrong to do this just to workaround\nparseopts, but I was assuming this wasn't a performance critical struct\nbecause Hannes said it wasn't set in stone. Of course, it also feels\nwrong to have the foo ? 1 : 0, but I think it's less wrong. This is why\nI had the foo ? 1 : 0 constructs in v2, because I felt that making this\nmore radical change would lead to just this question. As a bonus, having\nthese ugly constructs encourages someone to come up with a way to handle\nbit fields in parseopts.\n\n>\n> By the way, has it been verified that all the users of these fields are OK\n> with this change when they use these fields? I am not worried about them\n> reading the value command line option parser set, but more worried about\n> reading after other codepaths set/modified these fields. The command line\n> parser that uses parseopt may correctly set only 0 or 1 to these fields\n> initially and we should be able to verify that from the patch text, but\n> there is no guarantee that this conversion is even correct at runtime\n> without an audit, no?\n>\n> The callers have long relied on the fact that reading from these bitfields\n> yields either 0 or 1 and never 2 or larger, but they are now widened to\n> full-width unsigned. A pattern like this:\n>\n> \tuto.field = ~uto.field;\n> if (uto.field == 1) {\n> \tfield now set;\n> \t} else {\n> \tfield now unset;\n> \t}\n>\n> would be broken by widening \"unsigned field:1\" to \"unsigned field\", right?\n> I am not saying this is the only pattern that would break, nor I know\n> there are codepaths that use this pattern, but I think you got my point.\n\nYes. I glanced over the users of unpack_trees_options and didn't see\nanything dangerous like the above example. It wasn't a really thorough\naudit though because I was hoping that the callers were treating them as\nbooleans, and not bits.\n"}]}