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

[Kazuki Yamaguchi <k@rhe.jp>]

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:
> 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.

Thanks.

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

I'll do from next time.

> 
> > 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.

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

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