[PATCH 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s

[PATCH 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s

[Stephen Boyd]

Printing the usage message when encountering bad option combinations is
not very helpful. Instead, die with a message which tells the user
exactly what combination is invalid.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-read-tree.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 82e25ea..887e177 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -145,9 +145,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			continue;
 		}
 
-		/* using -u and -i at the same time makes no sense */
 		if (1 < opts.index_only + opts.update)
-			usage(read_tree_usage);
+			die("-u and -i at the same time makes no sense");
 
 		if (get_sha1(arg, sha1))
 			die("Not a valid object name %s", arg);
@@ -156,7 +155,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		stage++;
 	}
 	if ((opts.update||opts.index_only) && !opts.merge)
-		usage(read_tree_usage);
+		die("%s is meaningless without -m",
+		    opts.update ? "-u" : "-i");
 	if ((opts.dir && !opts.update))
 		die("--exclude-per-directory is meaningless unless -u");
 	if (opts.merge && !opts.index_only)
-- 
1.6.3.3.334.g916e1

[PATCH 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Cleanup the documentation to explicitly state that --exclude-directory
is only meaningful when used with -u. Also make the documentation more
consistent with the usage message printed with read-tree --help-all.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 Documentation/git-read-tree.txt |    5 +-
 builtin-read-tree.c             |  220 +++++++++++++++++++++-----------------
 2 files changed, 126 insertions(+), 99 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 7160fa1..1e0cc8f 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index
 
 SYNOPSIS
 --------
-'git read-tree' (<tree-ish> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]])
+'git read-tree' [--index-output=<file>] <treeish>
+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
+		[-u [--exclude-per-directory=<gitignore>] | -i]]
+		[--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]
 
 
 DESCRIPTION
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 887e177..adca739 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -12,6 +12,7 @@
 #include "unpack-trees.h"
 #include "dir.h"
 #include "builtin.h"
+#include "parse-options.h"
 
 static int nr_trees;
 static struct tree *trees[MAX_UNPACK_TREES];
@@ -29,7 +30,83 @@ static int list_tree(unsigned char *sha1)
 	return 0;
 }
 
-static const char read_tree_usage[] = "git read-tree (<sha> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <sha1> [<sha2> [<sha3>]])";
+static const char * const read_tree_usage[] = {
+	"git read-tree [--index-output=<file>] <treeish>",
+	"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u [--exclude-per-directory=<gitignore>] | -i]]  [--index-output=<file>] <treeish1> [<treeish2> [<treeish3>]]",
+	NULL
+};
+
+static int index_output_cb(const struct option *opt, const char *arg,
+				 int unset)
+{
+	set_alternate_index_output(arg);
+	return 0;
+}
+
+static int prefix_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct unpack_trees_options *opts;
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->merge || opts->prefix)
+		return 1;
+	opts->prefix = arg;
+	opts->merge = 1;
+	if (read_cache_unmerged())
+		die("you need to resolve your current index first");
+
+	return 0;
+}
+
+static int reset_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct unpack_trees_options *opts;
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->merge || opts->prefix)
+		return -1;
+	opts->reset = 1;
+	opts->merge = 1;
+	read_cache_unmerged();
+
+	return 0;
+}
+
+static int merge_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct unpack_trees_options *opts;
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->merge || opts->prefix)
+		return -1;
+	if (read_cache_unmerged())
+		die("you need to resolve your current index first");
+	opts->merge = 1;
+
+	return 0;
+}
+
+static int exclude_per_directory_cb(const struct option *opt, const char *arg,
+				    int unset)
+{
+	struct dir_struct *dir;
+	struct unpack_trees_options *opts;
+
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->dir)
+		die("more than one --exclude-per-directory given.");
+
+	dir = xcalloc(1, sizeof(*opts->dir));
+	dir->flags |= DIR_SHOW_IGNORED;
+	dir->exclude_per_dir = arg;
+	opts->dir = dir;
+	/* We do not need to nor want to do read-directory
+	 * here; we are merely interested in reusing the
+	 * per directory ignore stack mechanism.
+	 */
+	return 0;
+}
 
 static struct lock_file lock_file;
 
@@ -39,6 +116,37 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	unsigned char sha1[20];
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
+	int update = 0, index_only = 0, trivial_merges_only = 0, aggressive = 0,
+	    verbose = 0;
+	const struct option read_tree_options[] = {
+		{ OPTION_CALLBACK, 0, "index-output", NULL, "FILE",
+		  "write resulting index to <FILE>",
+		  PARSE_OPT_NONEG, index_output_cb },
+		{ OPTION_CALLBACK, 0, "prefix", &opts, "<subdirectory>/",
+		  "read the tree into the index under <subdirectory>",
+		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, prefix_cb },
+		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
+		  "gitignore",
+		  "allow explicitly ignored files to be overwritten",
+		  PARSE_OPT_NONEG, exclude_per_directory_cb },
+		OPT__VERBOSE(&verbose),
+		OPT_GROUP("Merging"),
+		{ OPTION_CALLBACK, 'm', NULL, &opts, NULL,
+		  "perform a merge in addition to a read",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, merge_cb },
+		{ OPTION_CALLBACK, 0, "reset", &opts, NULL,
+		  "same as -m, except unmerged entries are discarded",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, reset_cb },
+		OPT_BOOLEAN('u', NULL, &update,
+			    "update working tree with merge result"),
+		OPT_BOOLEAN('i', NULL, &index_only,
+			    "don't check the working tree after merging"),
+		OPT_BOOLEAN(0, "trivial", &trivial_merges_only,
+			    "3-way merge if no file level merging required"),
+		OPT_BOOLEAN(0, "aggressive", &aggressive,
+			    "3-way merge in presence of adds and removes"),
+		OPT_END()
+	};
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
@@ -49,104 +157,18 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	newfd = hold_locked_index(&lock_file, 1);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		/* "-u" means "update", meaning that a merge will update
-		 * the working tree.
-		 */
-		if (!strcmp(arg, "-u")) {
-			opts.update = 1;
-			continue;
-		}
-
-		if (!strcmp(arg, "-v")) {
-			opts.verbose_update = 1;
-			continue;
-		}
-
-		/* "-i" means "index only", meaning that a merge will
-		 * not even look at the working tree.
-		 */
-		if (!strcmp(arg, "-i")) {
-			opts.index_only = 1;
-			continue;
-		}
-
-		if (!prefixcmp(arg, "--index-output=")) {
-			set_alternate_index_output(arg + 15);
-			continue;
-		}
-
-		/* "--prefix=<subdirectory>/" means keep the current index
-		 *  entries and put the entries from the tree under the
-		 * given subdirectory.
-		 */
-		if (!prefixcmp(arg, "--prefix=")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.prefix = arg + 9;
-			opts.merge = 1;
-			stage = 1;
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			continue;
-		}
+	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
+			     read_tree_usage, 0);
 
-		/* This differs from "-m" in that we'll silently ignore
-		 * unmerged entries and overwrite working tree files that
-		 * correspond to them.
-		 */
-		if (!strcmp(arg, "--reset")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.reset = 1;
-			opts.merge = 1;
-			stage = 1;
-			read_cache_unmerged();
-			continue;
-		}
+	opts.update = update ? 1 : 0;
+	opts.index_only = index_only ? 1 : 0;
+	opts.trivial_merges_only = trivial_merges_only ? 1 : 0;
+	opts.aggressive = aggressive ? 1 : 0;
+	opts.verbose_update = verbose ? 1 : 0;
+	stage = opts.merge;
 
-		if (!strcmp(arg, "--trivial")) {
-			opts.trivial_merges_only = 1;
-			continue;
-		}
-
-		if (!strcmp(arg, "--aggressive")) {
-			opts.aggressive = 1;
-			continue;
-		}
-
-		/* "-m" stands for "merge", meaning we start in stage 1 */
-		if (!strcmp(arg, "-m")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			stage = 1;
-			opts.merge = 1;
-			continue;
-		}
-
-		if (!prefixcmp(arg, "--exclude-per-directory=")) {
-			struct dir_struct *dir;
-
-			if (opts.dir)
-				die("more than one --exclude-per-directory are given.");
-
-			dir = xcalloc(1, sizeof(*opts.dir));
-			dir->flags |= DIR_SHOW_IGNORED;
-			dir->exclude_per_dir = arg + 24;
-			opts.dir = dir;
-			/* We do not need to nor want to do read-directory
-			 * here; we are merely interested in reusing the
-			 * per directory ignore stack mechanism.
-			 */
-			continue;
-		}
-
-		if (1 < opts.index_only + opts.update)
-			die("-u and -i at the same time makes no sense");
+	for (i = 0; i < argc; i++) {
+		const char *arg = argv[i];
 
 		if (get_sha1(arg, sha1))
 			die("Not a valid object name %s", arg);
@@ -154,6 +176,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			die("failed to unpack tree object %s", arg);
 		stage++;
 	}
+	if (1 < opts.index_only + opts.update)
+		die("-u and -i at the same time makes no sense");
 	if ((opts.update||opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m",
 		    opts.update ? "-u" : "-i");
-- 
1.6.3.3.334.g916e1

Re: [PATCH 2/2] read-tree: migrate to parse-options

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Cleanup the documentation to explicitly state that --exclude-directory
> is only meaningful when used with -u. Also make the documentation more
> consistent with the usage message printed with read-tree --help-all.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
>  Documentation/git-read-tree.txt |    5 +-
>  builtin-read-tree.c             |  220 +++++++++++++++++++++-----------------
>  2 files changed, 126 insertions(+), 99 deletions(-)

Sorry, but I have to ask: Why?

Re: [PATCH 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Junio C Hamano wrote:
> Sorry, but I have to ask: Why?

I think there are advantages to parse-optification, for plumbing and
porcelains. Two reasons I see are:

1. Providing a unified way of handling options
2. Providing a consistent usage message format

Obviously, 1 reduces the bugs associated with parsing options (strcmp
vs. prefixcmp, incorrect argv+offset). For number 2, I think it helps
users when they see the same style of usage messages with each command.
It's also nice to get a quick help message without opening the man pages
or using git help <command>.

Admittedly this patch ends up adding 20 or so lines. Do the above points
justify this? I think so. I think the added lines can be attributed to
the rougher edges of parse-opts exposed by this patch. You can't take
the address of bit fields, so 6 lines are dealing with this problem.
Where are the other 15 lines coming from?

Looking over it again, I think I may be able to cut the overhead down by
refactoring three of the callbacks into boolean options. There's a lot
of duplication there, which can be simplified. I was trying to make this
a straight port which is probably not so good for convincing you that
it's worthwhile.

For now, I don't mind this patch being dropped (there's an ambiguity in
the callbacks returning non-zero I'd like to fix too). I'll try and get
a new patch (or maybe this patch with the oneline fix and a refactoring
patch) out later tonight that actually reduces the amount of lines,
instead of grossly adding to them.

[PATCHv2 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Cleanup the documentation to explicitly state that --exclude-directory
is only meaningful when used with -u. Also make the documentation more
consistent with the usage message printed with read-tree --help-all.

The -m, --prefix, and --reset options are performing similar actions
(setting some flags, read_cache_unmerged(), checking for illegal option
combinations). Instead of performing these actions when the options are
parsed, we delay doing them until after parse-opts has finished.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

I updated the two usage strings for --prefix and --reset too.

 Documentation/git-read-tree.txt |    5 +-
 builtin-read-tree.c             |  183 ++++++++++++++++++---------------------
 2 files changed, 89 insertions(+), 99 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 7160fa1..1e0cc8f 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index
 
 SYNOPSIS
 --------
-'git read-tree' (<tree-ish> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]])
+'git read-tree' [--index-output=<file>] <treeish>
+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
+		[-u [--exclude-per-directory=<gitignore>] | -i]]
+		[--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]
 
 
 DESCRIPTION
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 887e177..ba3ae6b 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -12,6 +12,7 @@
 #include "unpack-trees.h"
 #include "dir.h"
 #include "builtin.h"
+#include "parse-options.h"
 
 static int nr_trees;
 static struct tree *trees[MAX_UNPACK_TREES];
@@ -29,7 +30,40 @@ static int list_tree(unsigned char *sha1)
 	return 0;
 }
 
-static const char read_tree_usage[] = "git read-tree (<sha> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <sha1> [<sha2> [<sha3>]])";
+static const char * const read_tree_usage[] = {
+	"git read-tree [--index-output=<file>] <treeish>",
+	"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u [--exclude-per-directory=<gitignore>] | -i]]  [--index-output=<file>] <treeish1> [<treeish2> [<treeish3>]]",
+	NULL
+};
+
+static int index_output_cb(const struct option *opt, const char *arg,
+				 int unset)
+{
+	set_alternate_index_output(arg);
+	return 0;
+}
+
+static int exclude_per_directory_cb(const struct option *opt, const char *arg,
+				    int unset)
+{
+	struct dir_struct *dir;
+	struct unpack_trees_options *opts;
+
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->dir)
+		die("more than one --exclude-per-directory given.");
+
+	dir = xcalloc(1, sizeof(*opts->dir));
+	dir->flags |= DIR_SHOW_IGNORED;
+	dir->exclude_per_dir = arg;
+	opts->dir = dir;
+	/* We do not need to nor want to do read-directory
+	 * here; we are merely interested in reusing the
+	 * per directory ignore stack mechanism.
+	 */
+	return 0;
+}
 
 static struct lock_file lock_file;
 
@@ -39,6 +73,35 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	unsigned char sha1[20];
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
+	int update = 0, index_only = 0, trivial_merges_only = 0, aggressive = 0,
+	    verbose = 0, merge = 0, reset = 0, prefix_set = 0;
+	const struct option read_tree_options[] = {
+		{ OPTION_CALLBACK, 0, "index-output", NULL, "FILE",
+		  "write resulting index to <FILE>",
+		  PARSE_OPT_NONEG, index_output_cb },
+		{ OPTION_STRING, 0, "prefix", &opts.prefix, "<subdirectory>/",
+		  "read the tree into the index under <subdirectory>/",
+		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
+		  "gitignore",
+		  "allow explicitly ignored files to be overwritten",
+		  PARSE_OPT_NONEG, exclude_per_directory_cb },
+		OPT__VERBOSE(&verbose),
+		OPT_GROUP("Merging"),
+		OPT_BOOLEAN('m', NULL, &merge,
+			    "perform a merge in addition to a read"),
+		OPT_BOOLEAN(0, "reset", &reset,
+			    "same as -m, but discard unmerged entries"),
+		OPT_BOOLEAN('u', NULL, &update,
+			    "update working tree with merge result"),
+		OPT_BOOLEAN('i', NULL, &index_only,
+			    "don't check the working tree after merging"),
+		OPT_BOOLEAN(0, "trivial", &trivial_merges_only,
+			    "3-way merge if no file level merging required"),
+		OPT_BOOLEAN(0, "aggressive", &aggressive,
+			    "3-way merge in presence of adds and removes"),
+		OPT_END()
+	};
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
@@ -49,104 +112,24 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	newfd = hold_locked_index(&lock_file, 1);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
+			     read_tree_usage, 0);
 
-		/* "-u" means "update", meaning that a merge will update
-		 * the working tree.
-		 */
-		if (!strcmp(arg, "-u")) {
-			opts.update = 1;
-			continue;
-		}
+	prefix_set = opts.prefix ? 1 : 0;
 
-		if (!strcmp(arg, "-v")) {
-			opts.verbose_update = 1;
-			continue;
-		}
+	if (read_cache_unmerged() && (prefix_set || merge))
+		die("You need to resolve your current index first");
 
-		/* "-i" means "index only", meaning that a merge will
-		 * not even look at the working tree.
-		 */
-		if (!strcmp(arg, "-i")) {
-			opts.index_only = 1;
-			continue;
-		}
+	opts.update = update ? 1 : 0;
+	opts.index_only = index_only ? 1 : 0;
+	opts.trivial_merges_only = trivial_merges_only ? 1 : 0;
+	opts.aggressive = aggressive ? 1 : 0;
+	opts.verbose_update = verbose ? 1 : 0;
+	opts.reset = reset ? 1 : 0;
+	stage = opts.merge = (reset || merge || prefix_set) ? 1 : 0;
 
-		if (!prefixcmp(arg, "--index-output=")) {
-			set_alternate_index_output(arg + 15);
-			continue;
-		}
-
-		/* "--prefix=<subdirectory>/" means keep the current index
-		 *  entries and put the entries from the tree under the
-		 * given subdirectory.
-		 */
-		if (!prefixcmp(arg, "--prefix=")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.prefix = arg + 9;
-			opts.merge = 1;
-			stage = 1;
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			continue;
-		}
-
-		/* This differs from "-m" in that we'll silently ignore
-		 * unmerged entries and overwrite working tree files that
-		 * correspond to them.
-		 */
-		if (!strcmp(arg, "--reset")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.reset = 1;
-			opts.merge = 1;
-			stage = 1;
-			read_cache_unmerged();
-			continue;
-		}
-
-		if (!strcmp(arg, "--trivial")) {
-			opts.trivial_merges_only = 1;
-			continue;
-		}
-
-		if (!strcmp(arg, "--aggressive")) {
-			opts.aggressive = 1;
-			continue;
-		}
-
-		/* "-m" stands for "merge", meaning we start in stage 1 */
-		if (!strcmp(arg, "-m")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			stage = 1;
-			opts.merge = 1;
-			continue;
-		}
-
-		if (!prefixcmp(arg, "--exclude-per-directory=")) {
-			struct dir_struct *dir;
-
-			if (opts.dir)
-				die("more than one --exclude-per-directory are given.");
-
-			dir = xcalloc(1, sizeof(*opts.dir));
-			dir->flags |= DIR_SHOW_IGNORED;
-			dir->exclude_per_dir = arg + 24;
-			opts.dir = dir;
-			/* We do not need to nor want to do read-directory
-			 * here; we are merely interested in reusing the
-			 * per directory ignore stack mechanism.
-			 */
-			continue;
-		}
-
-		if (1 < opts.index_only + opts.update)
-			die("-u and -i at the same time makes no sense");
+	for (i = 0; i < argc; i++) {
+		const char *arg = argv[i];
 
 		if (get_sha1(arg, sha1))
 			die("Not a valid object name %s", arg);
@@ -154,6 +137,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			die("failed to unpack tree object %s", arg);
 		stage++;
 	}
+	if (1 < opts.merge + opts.reset + prefix_set)
+		die("Which one? -m, --reset, or --prefix?");
+	if (1 < opts.index_only + opts.update)
+		die("-u and -i at the same time makes no sense");
 	if ((opts.update||opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m",
 		    opts.update ? "-u" : "-i");
-- 
1.6.3.3.334.g916e1

Re: [PATCHv2 2/2] read-tree: migrate to parse-options

[Johannes Sixt]

Stephen Boyd schrieb:
> @@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index
>  
>  SYNOPSIS
>  --------
> -'git read-tree' (<tree-ish> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]])
> +'git read-tree' [--index-output=<file>] <treeish>
> +'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
> +		[-u [--exclude-per-directory=<gitignore>] | -i]]
> +		[--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]

Multi-line synopsis must begin with [verse].

If you write more than one mode of operation, the subsequent text should
better reference them, but the current text does not do that. I think it
is OK if you leave only the second, particularly because the first is only
a subset of the second.

> +	opts.update = update ? 1 : 0;
> +	opts.index_only = index_only ? 1 : 0;
> +	opts.trivial_merges_only = trivial_merges_only ? 1 : 0;
> +	opts.aggressive = aggressive ? 1 : 0;
> +	opts.verbose_update = verbose ? 1 : 0;
> +	opts.reset = reset ? 1 : 0;
> +	stage = opts.merge = (reset || merge || prefix_set) ? 1 : 0;

I don't think that the bitfields of struct unpack_trees_options are cast
in stone. IMHO it is fine to make them regular struct members, so that you
can take their address for read_tree_options and these foo ? 1 : 0 become
unnecessary.

-- Hannes

Re: [PATCHv2 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Johannes Sixt wrote:
>
> If you write more than one mode of operation, the subsequent text should
> better reference them, but the current text does not do that. I think it
> is OK if you leave only the second, particularly because the first is only
> a subset of the second.

I was contemplating this change, but I left it out because the single
tree case felt special. So special that I felt the merging and the
reading were two different modes. The description section hints at the
two types of uses, but I think you want it to be more explicit? I'll
have to think about this more.

> I don't think that the bitfields of struct unpack_trees_options are cast
> in stone. IMHO it is fine to make them regular struct members, so that you
> can take their address for read_tree_options and these foo ? 1 : 0 become
> unnecessary

Thanks. I'll fix this up and resend the series.

[PATCHv3 1/2] read-tree: convert unhelpful usage()'s to helpful die()'s

[Stephen Boyd]

Printing the usage message when encountering bad option combinations is
not very helpful. Instead, die with a message which tells the user
exactly what combination is invalid.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

Added --prefix and --reset to the second die message.

 builtin-read-tree.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 82e25ea..17c9631 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -145,9 +145,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			continue;
 		}
 
-		/* using -u and -i at the same time makes no sense */
 		if (1 < opts.index_only + opts.update)
-			usage(read_tree_usage);
+			die("-u and -i at the same time makes no sense");
 
 		if (get_sha1(arg, sha1))
 			die("Not a valid object name %s", arg);
@@ -156,7 +155,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		stage++;
 	}
 	if ((opts.update||opts.index_only) && !opts.merge)
-		usage(read_tree_usage);
+		die("%s is meaningless without -m, --reset, or --prefix",
+		    opts.update ? "-u" : "-i");
 	if ((opts.dir && !opts.update))
 		die("--exclude-per-directory is meaningless unless -u");
 	if (opts.merge && !opts.index_only)
-- 
1.6.3.3.334.g916e1

[PATCHv3 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Cleanup the documentation to explicitly state that --exclude-directory
is only meaningful when used with -u. Also make the documentation more
consistent with the usage message printed with read-tree --help-all.

The -m, --prefix, --reset options are performing similar actions
(setting some flags, read_cache_unmerged(), checking for illegal option
combinations). Instead of performing these actions when the options are
parsed, we delay performing them until after parse-opts has finished.

The bit fields in struct unpack_trees_options have been promoted to full
unsigned ints. This is necessary to avoid "foo ? 1 : 0" constructs to
set these fields.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

After some more thought, Hannes has convinced me that removing the first
mode is OK. I was trying to rewrite the documentation, and it just seemed
better the way it was.

A couple changes here:
    - reordered read_tree_options a bit to group the options with their
      dependent options better.
    - replaced OPT_BOOLEAN with OPT_SET_INT to avoid increment nature of
      OPT_BOOLEAN messing up logic depending on 1's and 0's only.

 Documentation/git-read-tree.txt |    5 +-
 builtin-read-tree.c             |  174 +++++++++++++++++----------------------
 unpack-trees.h                  |   24 +++---
 3 files changed, 92 insertions(+), 111 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 7160fa1..4a932b0 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -8,7 +8,10 @@ git-read-tree - Reads tree information into the index
 
 SYNOPSIS
 --------
-'git read-tree' (<tree-ish> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]])
+'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
+		[-u [--exclude-per-directory=<gitignore>] | -i]]
+		[--index-output=<file>]
+		<tree-ish1> [<tree-ish2> [<tree-ish3>]]
 
 
 DESCRIPTION
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 17c9631..9c2d634 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -12,6 +12,7 @@
 #include "unpack-trees.h"
 #include "dir.h"
 #include "builtin.h"
+#include "parse-options.h"
 
 static int nr_trees;
 static struct tree *trees[MAX_UNPACK_TREES];
@@ -29,7 +30,39 @@ static int list_tree(unsigned char *sha1)
 	return 0;
 }
 
-static const char read_tree_usage[] = "git read-tree (<sha> | [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u | -i]] [--exclude-per-directory=<gitignore>] [--index-output=<file>] <sha1> [<sha2> [<sha3>]])";
+static const char * const read_tree_usage[] = {
+	"git read-tree [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>] [-u [--exclude-per-directory=<gitignore>] | -i]]  [--index-output=<file>] <tree-ish1> [<tree-ish2> [<tree-ish3>]]",
+	NULL
+};
+
+static int index_output_cb(const struct option *opt, const char *arg,
+				 int unset)
+{
+	set_alternate_index_output(arg);
+	return 0;
+}
+
+static int exclude_per_directory_cb(const struct option *opt, const char *arg,
+				    int unset)
+{
+	struct dir_struct *dir;
+	struct unpack_trees_options *opts;
+
+	opts = (struct unpack_trees_options *)opt->value;
+
+	if (opts->dir)
+		die("more than one --exclude-per-directory given.");
+
+	dir = xcalloc(1, sizeof(*opts->dir));
+	dir->flags |= DIR_SHOW_IGNORED;
+	dir->exclude_per_dir = arg;
+	opts->dir = dir;
+	/* We do not need to nor want to do read-directory
+	 * here; we are merely interested in reusing the
+	 * per directory ignore stack mechanism.
+	 */
+	return 0;
+}
 
 static struct lock_file lock_file;
 
@@ -39,6 +72,34 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	unsigned char sha1[20];
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
+	int prefix_set = 0;
+	const struct option read_tree_options[] = {
+		{ OPTION_CALLBACK, 0, "index-output", NULL, "FILE",
+		  "write resulting index to <FILE>",
+		  PARSE_OPT_NONEG, index_output_cb },
+		OPT__VERBOSE(&opts.verbose_update),
+		OPT_GROUP("Merging"),
+		OPT_SET_INT('m', NULL, &opts.merge,
+			    "perform a merge in addition to a read", 1),
+		OPT_SET_INT(0, "trivial", &opts.trivial_merges_only,
+			    "3-way merge if no file level merging required", 1),
+		OPT_SET_INT(0, "aggressive", &opts.aggressive,
+			    "3-way merge in presence of adds and removes", 1),
+		OPT_SET_INT(0, "reset", &opts.reset,
+			    "same as -m, but discard unmerged entries", 1),
+		{ OPTION_STRING, 0, "prefix", &opts.prefix, "<subdirectory>/",
+		  "read the tree into the index under <subdirectory>/",
+		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+		OPT_SET_INT('u', NULL, &opts.update,
+			    "update working tree with merge result", 1),
+		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
+		  "gitignore",
+		  "allow explicitly ignored files to be overwritten",
+		  PARSE_OPT_NONEG, exclude_per_directory_cb },
+		OPT_SET_INT('i', NULL, &opts.index_only,
+			    "don't check the working tree after merging", 1),
+		OPT_END()
+	};
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
@@ -49,104 +110,19 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	newfd = hold_locked_index(&lock_file, 1);
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		/* "-u" means "update", meaning that a merge will update
-		 * the working tree.
-		 */
-		if (!strcmp(arg, "-u")) {
-			opts.update = 1;
-			continue;
-		}
-
-		if (!strcmp(arg, "-v")) {
-			opts.verbose_update = 1;
-			continue;
-		}
-
-		/* "-i" means "index only", meaning that a merge will
-		 * not even look at the working tree.
-		 */
-		if (!strcmp(arg, "-i")) {
-			opts.index_only = 1;
-			continue;
-		}
-
-		if (!prefixcmp(arg, "--index-output=")) {
-			set_alternate_index_output(arg + 15);
-			continue;
-		}
-
-		/* "--prefix=<subdirectory>/" means keep the current index
-		 *  entries and put the entries from the tree under the
-		 * given subdirectory.
-		 */
-		if (!prefixcmp(arg, "--prefix=")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.prefix = arg + 9;
-			opts.merge = 1;
-			stage = 1;
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			continue;
-		}
-
-		/* This differs from "-m" in that we'll silently ignore
-		 * unmerged entries and overwrite working tree files that
-		 * correspond to them.
-		 */
-		if (!strcmp(arg, "--reset")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			opts.reset = 1;
-			opts.merge = 1;
-			stage = 1;
-			read_cache_unmerged();
-			continue;
-		}
-
-		if (!strcmp(arg, "--trivial")) {
-			opts.trivial_merges_only = 1;
-			continue;
-		}
-
-		if (!strcmp(arg, "--aggressive")) {
-			opts.aggressive = 1;
-			continue;
-		}
+	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
+			     read_tree_usage, 0);
 
-		/* "-m" stands for "merge", meaning we start in stage 1 */
-		if (!strcmp(arg, "-m")) {
-			if (stage || opts.merge || opts.prefix)
-				usage(read_tree_usage);
-			if (read_cache_unmerged())
-				die("you need to resolve your current index first");
-			stage = 1;
-			opts.merge = 1;
-			continue;
-		}
+	if (read_cache_unmerged() && (opts.prefix || opts.merge))
+		die("You need to resolve your current index first");
 
-		if (!prefixcmp(arg, "--exclude-per-directory=")) {
-			struct dir_struct *dir;
-
-			if (opts.dir)
-				die("more than one --exclude-per-directory are given.");
-
-			dir = xcalloc(1, sizeof(*opts.dir));
-			dir->flags |= DIR_SHOW_IGNORED;
-			dir->exclude_per_dir = arg + 24;
-			opts.dir = dir;
-			/* We do not need to nor want to do read-directory
-			 * here; we are merely interested in reusing the
-			 * per directory ignore stack mechanism.
-			 */
-			continue;
-		}
+	prefix_set = opts.prefix ? 1 : 0;
+	if (1 < opts.merge + opts.reset + prefix_set)
+		die("Which one? -m, --reset, or --prefix?");
+	stage = opts.merge = (opts.reset || opts.merge || prefix_set);
 
-		if (1 < opts.index_only + opts.update)
-			die("-u and -i at the same time makes no sense");
+	for (i = 0; i < argc; i++) {
+		const char *arg = argv[i];
 
 		if (get_sha1(arg, sha1))
 			die("Not a valid object name %s", arg);
@@ -154,6 +130,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			die("failed to unpack tree object %s", arg);
 		stage++;
 	}
+	if (1 < opts.index_only + opts.update)
+		die("-u and -i at the same time makes no sense");
 	if ((opts.update||opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m, --reset, or --prefix",
 		    opts.update ? "-u" : "-i");
diff --git a/unpack-trees.h b/unpack-trees.h
index 1e0e232..d19df44 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -17,18 +17,18 @@ struct unpack_trees_error_msgs {
 };
 
 struct unpack_trees_options {
-	unsigned int reset:1,
-		     merge:1,
-		     update:1,
-		     index_only:1,
-		     nontrivial_merge:1,
-		     trivial_merges_only:1,
-		     verbose_update:1,
-		     aggressive:1,
-		     skip_unmerged:1,
-		     initial_checkout:1,
-		     diff_index_cached:1,
-		     gently:1;
+	unsigned int reset,
+		     merge,
+		     update,
+		     index_only,
+		     nontrivial_merge,
+		     trivial_merges_only,
+		     verbose_update,
+		     aggressive,
+		     skip_unmerged,
+		     initial_checkout,
+		     diff_index_cached,
+		     gently;
 	const char *prefix;
 	int pos;
 	struct dir_struct *dir;
-- 
1.6.3.3.334.g916e1

Re: [PATCHv3 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Stephen Boyd wrote:
>  struct unpack_trees_options {
> -	unsigned int reset:1,
> -		     merge:1,
> -		     update:1,
> -		     index_only:1,
> -		     nontrivial_merge:1,
> -		     trivial_merges_only:1,
> -		     verbose_update:1,
> -		     aggressive:1,
> -		     skip_unmerged:1,
> -		     initial_checkout:1,
> -		     diff_index_cached:1,
> -		     gently:1;
> +	unsigned int reset,
> +		     merge,
> +		     update,
> +		     index_only,
> +		     nontrivial_merge,
> +		     trivial_merges_only,
> +		     verbose_update,
> +		     aggressive,
> +		     skip_unmerged,
> +		     initial_checkout,
> +		     diff_index_cached,
> +		     gently;

Sorry I went a little overboard with s/:1// on unpack_tree_options.
You'll probably want to squash this on top.

diff --git a/unpack-trees.h b/unpack-trees.h
index d19df44..f344679 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -21,14 +21,14 @@ struct unpack_trees_options {
                     merge,
                     update,
                     index_only,
-                    nontrivial_merge,
+                    nontrivial_merge:1,
                     trivial_merges_only,
                     verbose_update,
                     aggressive,
-                    skip_unmerged,
-                    initial_checkout,
-                    diff_index_cached,
-                    gently;
+                    skip_unmerged:1,
+                    initial_checkout:1,
+                    diff_index_cached:1,
+                    gently:1;
        const char *prefix;
        int pos;
        struct dir_struct *dir;

Re: [PATCHv3 2/2] read-tree: migrate to parse-options

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Sorry I went a little overboard with s/:1// on unpack_tree_options.
> You'll probably want to squash this on top.
>
> diff --git a/unpack-trees.h b/unpack-trees.h
> index d19df44..f344679 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -21,14 +21,14 @@ struct unpack_trees_options {
>                      merge,
>                      update,
>                      index_only,
> -                    nontrivial_merge,
> +                    nontrivial_merge:1,
>                      trivial_merges_only,
>                      verbose_update,
>                      aggressive,
> -                    skip_unmerged,
> -                    initial_checkout,
> -                    diff_index_cached,
> -                    gently;
> +                    skip_unmerged:1,
> +                    initial_checkout:1,
> +                    diff_index_cached:1,
> +                    gently:1;

Let's look at this (not this follow-up patch) the other way around.

Six months down the load, somebody may ask you:

    Is there a good reason why many are not bitfields but only selected
    few are bitfields in this structure?  Most of these can and should be
    bitfields, as far as I can see, because the code uses them as
    booleans, and the only breakage it may cause if we change them to
    bitfields to shrink this structure would be the option parsing code.

What would be your answer?  Doesn't it feel wrong to do such a conversion
only to work around the current limitation of parseopt?

By the way, has it been verified that all the users of these fields are OK
with this change when they use these fields?  I am not worried about them
reading the value command line option parser set, but more worried about
reading after other codepaths set/modified these fields.  The command line
parser that uses parseopt may correctly set only 0 or 1 to these fields
initially and we should be able to verify that from the patch text, but
there is no guarantee that this conversion is even correct at runtime
without an audit, no?

The callers have long relied on the fact that reading from these bitfields
yields either 0 or 1 and never 2 or larger, but they are now widened to
full-width unsigned.  A pattern like this:

	uto.field = ~uto.field;
        if (uto.field == 1) {
        	field now set;
	} else {
        	field now unset;
	}

would be broken by widening "unsigned field:1" to "unsigned field", right?
I am not saying this is the only pattern that would break, nor I know
there are codepaths that use this pattern, but I think you got my point.

Re: [PATCHv3 2/2] read-tree: migrate to parse-options

[Stephen Boyd]

Junio C Hamano wrote:
>
> Let's look at this (not this follow-up patch) the other way around.
>
> Six months down the load, somebody may ask you:
>
>     Is there a good reason why many are not bitfields but only selected
>     few are bitfields in this structure?  Most of these can and should be
>     bitfields, as far as I can see, because the code uses them as
>     booleans, and the only breakage it may cause if we change them to
>     bitfields to shrink this structure would be the option parsing code.
>
> What would be your answer?  Doesn't it feel wrong to do such a conversion
> only to work around the current limitation of parseopt?

I understand. It does feel wrong to do this just to workaround
parseopts, but I was assuming this wasn't a performance critical struct
because Hannes said it wasn't set in stone. Of course, it also feels
wrong to have the foo ? 1 : 0, but I think it's less wrong. This is why
I had the foo ? 1 : 0 constructs in v2, because I felt that making this
more radical change would lead to just this question. As a bonus, having
these ugly constructs encourages someone to come up with a way to handle
bit fields in parseopts.

>
> By the way, has it been verified that all the users of these fields are OK
> with this change when they use these fields?  I am not worried about them
> reading the value command line option parser set, but more worried about
> reading after other codepaths set/modified these fields.  The command line
> parser that uses parseopt may correctly set only 0 or 1 to these fields
> initially and we should be able to verify that from the patch text, but
> there is no guarantee that this conversion is even correct at runtime
> without an audit, no?
>
> The callers have long relied on the fact that reading from these bitfields
> yields either 0 or 1 and never 2 or larger, but they are now widened to
> full-width unsigned.  A pattern like this:
>
> 	uto.field = ~uto.field;
>         if (uto.field == 1) {
>         	field now set;
> 	} else {
>         	field now unset;
> 	}
>
> would be broken by widening "unsigned field:1" to "unsigned field", right?
> I am not saying this is the only pattern that would break, nor I know
> there are codepaths that use this pattern, but I think you got my point.

Yes. I glanced over the users of unpack_trees_options and didn't see
anything dangerous like the above example. It wasn't a really thorough
audit though because I was hoping that the callers were treating them as
booleans, and not bits.