Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:

On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi [off-list ref] wrote:

quoted

When a branch is checked out by current working tree, deleting the
branch is forbidden. However when the branch is checked out only by
other working trees, deleting is allowed.

It's not quite clear from this description that it is bad for deletion
to succeed in the second case. Perhaps:

    s/deleting is allowed/deletion incorrectly succeeds/

would make it more clear.

Thanks.

quoted

Use find_shared_symref() to check if the branch is in use, not just
comparing with the current working tree's HEAD.

This version of the patch is nicer. Thanks. See a couple minor
comments below which may or may not be worth a re-roll (you decide).

quoted

Signed-off-by: Kazuki Yamaguchi <redacted>
---

  % git worktree list
  /path/to      2c3c5f2 [master]
  /path/to/wt   2c3c5f2 [branch-a]
  % git branch -d branch-a
  error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'

Thanks for an example of the new behavior. It's also helpful to
reviewers if you use this space to explain what changed since the
previous version, and to provide a link to the previous attempt, like
this[1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932

I'll do from next time.

quoted

diff --git a/builtin/branch.c b/builtin/branch.c

@@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                int flags = 0;

                strbuf_branchname(&bname, argv[i]);
-               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
-                       error(_("Cannot delete the branch '%s' "
-                             "which you are currently on."), bname.buf);
-                       ret = 1;
-                       continue;
-               }
-
                free(name);
-
                name = mkpathdup(fmt, bname.buf);
+
+               if (kinds == FILTER_REFS_BRANCHES) {
+                       char *worktree = find_shared_symref("HEAD", name);
+                       if (worktree) {
+                               error(_("Cannot delete the branch '%s' "
+                                       "which is currently checked out at '%s'"),

This could be stated more concisely as:

    "Cannot delete branch '%s' checked out at '%s'"

I'll use it. Thanks.

quoted

+                                     bname.buf, worktree);
+                               free(worktree);

Would it make sense to show all worktrees at which this branch is
checked out, rather than only one, or is that not worth the effort and
extra code ugliness?

I thought one is enough.
I think the worktrees usually won't be more than one, considering
"git worktree add" requires additional option to check out an already
checked out branch. Also, since the branch is not actually deleted at
that time, the user can safely retry after checking "git worktree list".


Thanks,

quoted

+                               ret = 1;
+                               continue;
+                       }
+               }
+
                target = resolve_ref_unsafe(name,
                                            RESOLVE_REF_READING
                                            | RESOLVE_REF_NO_RECURSE