Re: [PATCH v8 4/4] worktree: teach "add" to check out existing branches
From: Eric Sunshine <hidden>
Date: 2018-04-24 04:25:20
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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.
quoted hunk ↗ jump to hunk
+ } 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.
quoted hunk ↗ jump to hunk
+ 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 ---