On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <[email protected]> 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 <[email protected]>
> ---
>
> % 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
