Subject: [PATCH 3/3] checkout: move branch guessing code out as a separate function

From: Nguyễn Thái Ngọc Duy <[email-masked]>
Date: Tue, 28 Aug 2012 20:49:08 +0700

This makes cmd_checkout() shorter, therefore easier to get the big
picture.

Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>
---
 In general I like it, except that parse_branchname_arg() pulls so many
 options from cmd_checkout(). I would rather have update_new_branch()
 that only takes "struct checkout_opts *" and returns a new branch. So
 I'm not sure if we should do this. If we do, perhaps we can merge it
 into the previous patch.

 builtin/checkout.c | 184 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 96 insertions(+), 88 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3f1a436..e9d503d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -914,6 +914,99 @@ static int parse_branchname_arg(int argc, const char **argv,
 	return argcount;
 }
 
+static const char *update_new_branch(const struct checkout_opts *opts,
+				     int argc, const char **argv,
+				     const char *prefix, const char ***pathspec,
+				     int dwim_ok,
+				     struct branch_info *new,
+				     struct tree **source_tree,
+				     unsigned char *rev)
+{
+	const char *branch = opts->new_branch;
+
+	if (opts->new_branch_force) {
+		/* we can assume from now on new_branch = !new_branch_force */
+		if (branch)
+			die(_("New branch '%s' is already specified by other options"),
+			    branch);
+
+		/* copy -B over to -b, so that we can just check the latter */
+		if (opts->new_branch_force)
+			branch = opts->new_branch_force;
+	}
+
+	if (opts->new_orphan_branch) {
+		if (opts->track > 0)
+			die(_("--orphan cannot be used with -t"));
+		if (branch)
+			die(_("New branch '%s' is already specified by other options"),
+			    branch);
+		branch = opts->new_orphan_branch;
+	}
+
+	/* --track without -b should DWIM */
+	if (0 < opts->track && !branch) {
+		const char *argv0 = argv[0];
+		if (!argc || !strcmp(argv0, "--"))
+			die (_("--track needs a branch name"));
+		if (!prefixcmp(argv0, "refs/"))
+			argv0 += 5;
+		if (!prefixcmp(argv0, "remotes/"))
+			argv0 += 8;
+		argv0 = strchr(argv0, '/');
+		if (!argv0 || !argv0[1])
+			die (_("Missing branch name; try -b"));
+		branch = argv0 + 1;
+	}
+
+	/*
+	 * Extract branch name from command line arguments, so
+	 * all that is left is pathspecs.
+	 *
+	 * Handle
+	 *
+	 *  1) git checkout <tree> -- [<paths>]
+	 *  2) git checkout -- [<paths>]
+	 *  3) git checkout <something> [<paths>]
+	 *
+	 * including "last branch" syntax and DWIM-ery for names of
+	 * remote branches, erroring out for invalid or ambiguous cases.
+	 */
+	if (argc) {
+		dwim_ok = dwim_ok &&
+			opts->track == BRANCH_TRACK_UNSPECIFIED &&
+			!branch;
+		int n = parse_branchname_arg(argc, argv, dwim_ok,
+					     new, source_tree, rev, &branch);
+		argv += n;
+		argc -= n;
+	}
+
+	/* After DWIM, these must be pathspec */
+	if (argc) {
+		*pathspec = get_pathspec(prefix, argv);
+
+		if (!*pathspec)
+			die(_("invalid path specification"));
+
+		/* Try to give more helpful suggestion, new_branch &&
+		   argc > 1 will be caught later */
+		if (branch && argc == 1)
+			die(_("Cannot update paths and switch to branch '%s' at the same time.\n"
+			      "Did you intend to checkout '%s' which can not be resolved as commit?"),
+			    branch, argv[0]);
+
+		if (opts->force_detach)
+			die(_("git checkout: --detach does not take a path argument"));
+
+		if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
+			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
+			      "checking out of the index."));
+	}
+
+	return branch;
+}
+
 static int switch_unborn_to_new_branch(const struct checkout_opts *opts)
 {
 	int status;
@@ -989,94 +1082,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
 	}
 
-	/*
-	 * opts.new_branch calculation, it could be from -b or -B or
-	 * --track or --orphan or other command line arguments.
-	 *
-	 * The following code until new branch validation should not
-	 * update any fields in opts but opts.new_branch.
-	 */
-	if (opts.new_branch_force) {
-		/* we can assume from now on new_branch = !new_branch_force */
-		if (opts.new_branch)
-			die(_("New branch '%s' is already specified by other options"),
-			    opts.new_branch);
-
-		/* copy -B over to -b, so that we can just check the latter */
-		if (opts.new_branch_force)
-			opts.new_branch = opts.new_branch_force;
-	}
-
-	if (opts.new_orphan_branch) {
-		if (opts.track > 0)
-			die(_("--orphan cannot be used with -t"));
-		if (opts.new_branch)
-			die(_("New branch '%s' is already specified by other options"),
-			    opts.new_branch);
-		opts.new_branch = opts.new_orphan_branch;
-	}
-
-	/* --track without -b should DWIM */
-	if (0 < opts.track && !opts.new_branch) {
-		const char *argv0 = argv[0];
-		if (!argc || !strcmp(argv0, "--"))
-			die (_("--track needs a branch name"));
-		if (!prefixcmp(argv0, "refs/"))
-			argv0 += 5;
-		if (!prefixcmp(argv0, "remotes/"))
-			argv0 += 8;
-		argv0 = strchr(argv0, '/');
-		if (!argv0 || !argv0[1])
-			die (_("Missing branch name; try -b"));
-		opts.new_branch = argv0 + 1;
-	}
-
-	/*
-	 * Extract branch name from command line arguments, so
-	 * all that is left is pathspecs.
-	 *
-	 * Handle
-	 *
-	 *  1) git checkout <tree> -- [<paths>]
-	 *  2) git checkout -- [<paths>]
-	 *  3) git checkout <something> [<paths>]
-	 *
-	 * including "last branch" syntax and DWIM-ery for names of
-	 * remote branches, erroring out for invalid or ambiguous cases.
-	 */
-	if (argc) {
-		int dwim_ok =
-			!patch_mode &&
-			dwim_new_local_branch &&
-			opts.track == BRANCH_TRACK_UNSPECIFIED &&
-			!opts.new_branch;
-		int n = parse_branchname_arg(argc, argv, dwim_ok,
-				&new, &source_tree, rev, &opts.new_branch);
-		argv += n;
-		argc -= n;
-	}
-
-	/* After DWIM, these must be pathspec */
-	if (argc) {
-		pathspec = get_pathspec(prefix, argv);
-
-		if (!pathspec)
-			die(_("invalid path specification"));
-
-		/* Try to give more helpful suggestion, new_branch &&
-		   argc > 1 will be caught later */
-		if (opts.new_branch && argc == 1)
-			die(_("Cannot update paths and switch to branch '%s' at the same time.\n"
-			      "Did you intend to checkout '%s' which can not be resolved as commit?"),
-			    opts.new_branch, argv[0]);
-
-		if (opts.force_detach)
-			die(_("git checkout: --detach does not take a path argument"));
-
-		if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)
-			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
-			      "checking out of the index."));
-	}
+	opts.new_branch = update_new_branch(&opts, argc, argv, prefix, &pathspec,
+					    !patch_mode && dwim_new_local_branch,
+					    &new, &source_tree, rev);
 
 	/* new branch calculation done, validate it */
 	if (opts.new_branch) {
-- 
1.7.12.rc2.18.g61b472e