Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 28, 2016, 16:51 UTC
- Message-ID
- <CAPig+cSzTwup6ojboVkP8nMR91-ZUU9FCbAK5NcrcohfFh2taQ@mail.gmail.com>
- In-Reply-To
- <1459149771-14790-1-git-send-email-k@rhe.jp>
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).
Show 8 quoted lines
> 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
Show 21 quoted lines
> 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?
Show 8 quoted lines
> + ret = 1; > + continue; > + } > + } > + > target = resolve_ref_unsafe(name, > RESOLVE_REF_READING > | RESOLVE_REF_NO_RECURSE