[PATCH/RFC] Restructure some of the checkout opts.

[PATCH/RFC] Restructure some of the checkout opts.

[Jared Hance]

Previously, most opts in `builtin.checkout.c' had been an instance of
`struct checkout_opts'. It appears that some of the opts that were
perhaps added later were not in the struct. Move them into the struct
in order to maintain consistency.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---

 builtin/checkout.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7250e5c..3a35cde 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -31,6 +31,9 @@ struct checkout_opts {
 	int force;
 	int writeout_stage;
 	int writeout_error;
+	int patch_mode;
+	int dwim_new_local_branch;
+	char *conflict_style;
 
 	/* not set by parse_options */
 	int branch_exists;
@@ -660,9 +663,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	const char *arg;
 	struct branch_info new;
 	struct tree *source_tree = NULL;
-	char *conflict_style = NULL;
-	int patch_mode = 0;
-	int dwim_new_local_branch = 1;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "branch",
@@ -679,10 +679,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    3),
 		OPT_BOOLEAN('f', "force", &opts.force, "force"),
 		OPT_BOOLEAN('m', "merge", &opts.merge, "merge"),
-		OPT_STRING(0, "conflict", &conflict_style, "style",
+		OPT_STRING(0, "conflict", &opts.conflict_style, "style",
 			   "conflict style (merge or diff3)"),
-		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
-		{ OPTION_BOOLEAN, 0, "guess", &dwim_new_local_branch, NULL,
+		OPT_BOOLEAN('p', "patch", &opts.patch_mode, "select hunks interactively"),
+		{ OPTION_BOOLEAN, 0, "guess", &opts.dwim_new_local_branch, NULL,
 		  "second guess 'git checkout no-such-branch'",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 		OPT_END(),
@@ -695,6 +695,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	git_config(git_checkout_config, NULL);
 
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
+	opts.dwim_new_local_branch = 1;
 
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
@@ -707,7 +708,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch_force)
 		opts.new_branch = opts.new_branch_force;
 
-	if (patch_mode && (opts.track > 0 || opts.new_branch
+	if (opts.patch_mode && (opts.track > 0 || opts.new_branch
 			   || opts.new_branch_log || opts.merge || opts.force))
 		die ("--patch is incompatible with all other options");
 
@@ -734,9 +735,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		opts.new_branch = opts.new_orphan_branch;
 	}
 
-	if (conflict_style) {
+	if (opts.conflict_style) {
 		opts.merge = 1; /* implied */
-		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
+		git_xmerge_config("merge.conflictstyle", opts.conflict_style, NULL);
 	}
 
 	if (opts.force && opts.merge)
@@ -787,8 +788,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (get_sha1_mb(arg, rev)) {
 			if (has_dash_dash)          /* case (1) */
 				die("invalid reference: %s", arg);
-			if (!patch_mode &&
-			    dwim_new_local_branch &&
+			if (!opts.patch_mode &&
+			    opts.dwim_new_local_branch &&
 			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			    !opts.new_branch &&
 			    !check_filename(NULL, arg) &&
@@ -851,7 +852,7 @@ no_reference:
 		if (!pathspec)
 			die("invalid path specification");
 
-		if (patch_mode)
+		if (opts.patch_mode)
 			return interactive_checkout(new.name, pathspec, &opts);
 
 		/* Checkout paths */
@@ -869,7 +870,7 @@ no_reference:
 		return checkout_paths(source_tree, pathspec, &opts);
 	}
 
-	if (patch_mode)
+	if (opts.patch_mode)
 		return interactive_checkout(new.name, NULL, &opts);
 
 	if (opts.new_branch) {
-- 
1.7.2.2

Re: [PATCH/RFC] Restructure some of the checkout opts.

[Jens Lehmann]

Am 05.09.2010 14:09, schrieb Jared Hance:
> Previously, most opts in `builtin.checkout.c' had been an instance of
> `struct checkout_opts'. It appears that some of the opts that were
> perhaps added later were not in the struct. Move them into the struct
> in order to maintain consistency.

Hm, they aren't used outside of cmd_checkout() (as the other members
are), so maybe it is ok that they aren't in the struct?

Re: [PATCH/RFC] Restructure some of the checkout opts.

[Jared Hance]

On Sun, Sep 05, 2010 at 02:24:09PM +0200, Jens Lehmann wrote:
> Am 05.09.2010 14:09, schrieb Jared Hance:
> > Previously, most opts in `builtin.checkout.c' had been an instance of
> > `struct checkout_opts'. It appears that some of the opts that were
> > perhaps added later were not in the struct. Move them into the struct
> > in order to maintain consistency.
> 
> Hm, they aren't used outside of cmd_checkout() (as the other members
> are), so maybe it is ok that they aren't in the struct?

I guessed that this was the reason that they were overlooked, but it
seemed a little odd that every other opt was stored in the struct and
it just seemed to be more consistent to have _everything_ in the
struct.

It also makes it more clear that they opts, whereas before, you might
think they weren't at a first glance because they weren't in the
struct like the others.

I'll leave this to you guys to decide what to do with it.

Re: [PATCH/RFC] Restructure some of the checkout opts.

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 05.09.2010 14:09, schrieb Jared Hance:
>> Previously, most opts in `builtin.checkout.c' had been an instance of
>> `struct checkout_opts'. It appears that some of the opts that were
>> perhaps added later were not in the struct. Move them into the struct
>> in order to maintain consistency.
>
> Hm, they aren't used outside of cmd_checkout() (as the other members
> are), so maybe it is ok that they aren't in the struct?

Correct.  I do not think they are outside the structure because they are
overlooked.

They simply do not make sense outside the context of "builtin/checkout.c"
where "struct checkout" is used (e.g. what would patch-mode possibly mean
in the context of "builtin/apply" where the struct is used to check out a
path that is not checked out to the working tree).  Moving them into the
structure would thus make no sense and _will_ confuse people.