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

[Eric Sunshine <sunshine@sunshineco.com>]

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