Re: [PATCH v8 4/4] worktree: teach "add" to check out existing branches

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add <path>' creates a new branch named after the
> basename of the path by default.  If a branch with that name already
> exists, the command refuses to do anything, unless the '--force' option
> is given.
>
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else.  This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -356,9 +356,12 @@ static int add_worktree(const char *path, const char *refname,
>  static void print_preparing_worktree_line(int detach,
>                                           const char *branch,
>                                           const char *new_branch,
> -                                         const char *new_branch_force)
> +                                         const char *new_branch_force,
> +                                         int checkout_existing_branch)
>  {
> -       if (new_branch_force) {
> +       if (checkout_existing_branch) {
> +               printf_ln(_("Preparing worktree (checking out '%s')"), branch);

Is the new 'checkout_existing_branch' argument and this new
conditional really needed? Both 'new_branch' and 'new_branch_force'
will be NULL in the case of an existing branch, so there should be no
need for 'checkout_existing_branch' to state explicitly what is
already indicated by their NULLness. With both of them NULL, the
existing "(checking out '...')" message later in the function will
kick in, so this new condition isn't needed.

> +       } else if (new_branch_force) {
> @@ -387,11 +390,23 @@ static void print_preparing_worktree_line(int detach,
> +static const char *dwim_branch(const char *path, const char **new_branch,
> +                              int *checkout_existing_branch)
>  {
>         int n;
>         const char *s = worktree_basename(path, &n);
> -       *new_branch = xstrndup(s, n);
> +       const char *branchname = xstrndup(s, n);
> +       struct strbuf ref = STRBUF_INIT;
> +
> +       if (!strbuf_check_branch_ref(&ref, branchname) &&
> +           ref_exists(ref.buf)) {
> +               *checkout_existing_branch = 1;

See above regarding apparent lack of need for this new variable.

> +               strbuf_release(&ref);
> +               UNLEAK(branchname);
> +               return branchname;
> +       }

Taking the above observation into account, I applied, atop this patch,
the following fixup! which removes the 'checkout_existing_branch'
gunk and still get the same output:

--- >8 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -356,12 +356,9 @@ static int add_worktree(const char *path, const char *refname,
 static void print_preparing_worktree_line(int detach,
 					  const char *branch,
 					  const char *new_branch,
-					  const char *new_branch_force,
-					  int checkout_existing_branch)
+					  const char *new_branch_force)
 {
-	if (checkout_existing_branch) {
-		printf_ln(_("Preparing worktree (checking out '%s')"), branch);
-	} else if (new_branch_force) {
+	if (new_branch_force) {
 		struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
 		if (!commit)
 			printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
@@ -390,8 +387,7 @@ static void print_preparing_worktree_line(int detach,
 	}
 }
 
-static const char *dwim_branch(const char *path, const char **new_branch,
-			       int *checkout_existing_branch)
+static const char *dwim_branch(const char *path, const char **new_branch)
 {
 	int n;
 	const char *s = worktree_basename(path, &n);
@@ -400,7 +396,6 @@ static const char *dwim_branch(const char *path, const char **new_branch,
 
 	if (!strbuf_check_branch_ref(&ref, branchname) &&
 	    ref_exists(ref.buf)) {
-		*checkout_existing_branch = 1;
 		strbuf_release(&ref);
 		UNLEAK(branchname);
 		return branchname;
@@ -421,7 +416,6 @@ static int add(int ac, const char **av, const char *prefix)
 	struct add_opts opts;
 	const char *new_branch_force = NULL;
 	char *path;
-	int checkout_existing_branch = 0;
 	const char *branch;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
@@ -469,8 +463,7 @@ static int add(int ac, const char **av, const char *prefix)
 	}
 
 	if (ac < 2 && !new_branch && !opts.detach) {
-		const char *s = dwim_branch(path, &new_branch,
-					    &checkout_existing_branch);
+		const char *s = dwim_branch(path, &new_branch);
 		if (s)
 			branch = s;
 	}
@@ -490,8 +483,7 @@ static int add(int ac, const char **av, const char *prefix)
 		}
 	}
 
-	print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force,
-				      checkout_existing_branch);
+	print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
 
 	if (new_branch) {
 		struct child_process cp = CHILD_PROCESS_INIT;
--- >8 ---