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

> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> > 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.

> 
> > 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).
> 
> > Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> > ---
> >
> >   % 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

> 
> > 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'"

> 
> > +                                     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?

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