Nanako Shiraishi <nanako3@xxxxxxxxxxx> writes:
> Quoting Nicolas Sebrecht <nicolas.s.dev@xxxxxx>
>
>> The 30/12/09, Nanako Shiraishi wrote:
>>
>>> I think the safety feature should check if the branch to be deleted is merged to the remote tracking branch it was forked from, instead of checking the current branch.
>>>
>>> What do you think?
>>
>> I think we shouldn't. At first, every repository don't have a remote.
>> This may easily be passed by a "double check" with a logical OR between
>> the two statements.
>
> Sorry, I was unclear. What I meant was that checking with the branch the
> branch to be deleted was forked from, if and only if such a branch
> exists. Otherwise, we can keep using the old default behavior to check
> with the current branch.
Back when the original "safety valve" was added, there was no mechanical
"this branch _always_ merges from/rebases on that other one" settings.
The users were supposed to keep track of the correspondence, and the
canonical workflow was "checkout this && merge that && branch -d that".
But I actually think it is quite a natural thing to do in year 2010 to
change the safety valve as suggested.
A patch to do so would look like this.
-- >8 --
branch -d: base the "already-merged" safety on the branch it merges with
When a branch is marked to merge with another ref (e.g. local 'next' that
merges from and pushes back to origin's 'next', with 'branch.next.merge'
set to 'refs/heads/next'), it makes little sense to base the "branch -d"
safety, whose purpose is not to lose commits that are not merged to other
branches, on the current branch. It is much more sensible to check if it
is merged with the other branch it merges with.
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
builtin-branch.c | 64 ++++++++++++++++++++++++++++++++++++++++++++--------
t/t3200-branch.sh | 26 +++++++++++++++++++++
2 files changed, 80 insertions(+), 10 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index c87e63b..d2a35fe 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -93,9 +93,60 @@ static const char *branch_get_color(enum color_branch ix)
return "";
}
+static int branch_merged(int kind, const char *name,
+ struct commit *rev, struct commit *head_rev)
+{
+ /*
+ * This checks whether the merge bases of branch and HEAD (or
+ * the other branch this branch builds upon) contains the
+ * branch, which means that the branch has already been merged
+ * safely to HEAD (or the other branch).
+ */
+ struct commit *reference_rev = NULL;
+ const char *reference_name = NULL;
+ int merged;
+
+ if (kind == REF_LOCAL_BRANCH) {
+ struct branch *branch = branch_get(name);
+ unsigned char sha1[20];
+
+ if (branch &&
+ branch->merge &&
+ branch->merge[0] &&
+ branch->merge[0]->dst &&
+ (reference_name =
+ resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+ reference_rev = lookup_commit_reference(sha1);
+ }
+ if (!reference_rev)
+ reference_rev = head_rev;
+
+ merged = in_merge_bases(rev, &reference_rev, 1);
+
+ /*
+ * After the safety valve is fully redefined to "check with
+ * upstream, if any, otherwise with HEAD", we should just
+ * return the result of the in_merge_bases() above without
+ * any of the following code, but during the transition period,
+ * a gentle reminder is in order.
+ */
+ if ((head_rev != reference_rev) &&
+ in_merge_bases(rev, &head_rev, 1) != merged) {
+ if (merged)
+ warning("deleting branch '%s' that has been merged to\n"
+ " '%s', but it is not yet merged to HEAD.",
+ name, reference_name);
+ else
+ warning("not deleting branch '%s' that is not yet merged to\n"
+ " '%s', even though it is merged to HEAD.",
+ name, reference_name);
+ }
+ return merged;
+}
+
static int delete_branches(int argc, const char **argv, int force, int kinds)
{
- struct commit *rev, *head_rev = head_rev;
+ struct commit *rev, *head_rev = NULL;
unsigned char sha1[20];
char *name = NULL;
const char *fmt, *remote;
@@ -148,15 +199,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
continue;
}
- /* This checks whether the merge bases of branch and
- * HEAD contains branch -- which means that the HEAD
- * contains everything in both.
- */
-
- if (!force &&
- !in_merge_bases(rev, &head_rev, 1)) {
- error("The branch '%s' is not an ancestor of "
- "your current HEAD.\n"
+ if (!force && !branch_merged(kinds, bname.buf, rev, head_rev)) {
+ error("The branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
"run 'git branch -D %s'.", bname.buf, bname.buf);
ret = 1;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index d59a9b4..e0b7605 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -468,4 +468,30 @@ test_expect_success 'detect misconfigured autosetuprebase (no value)' '
git config --unset branch.autosetuprebase
'
+test_expect_success 'attempt to delete a branch without base and unmerged to HEAD' '
+ git checkout my9 &&
+ git config --unset branch.my8.merge &&
+ test_must_fail git branch -d my8
+'
+
+test_expect_success 'attempt to delete a branch merged to its base' '
+ # we are on my9 which is the initial commit; traditionally
+ # we would not have allowed deleting my8 that is not merged
+ # to my9, but it is set to track master that already has my8
+ git config branch.my8.merge refs/heads/master &&
+ git branch -d my8
+'
+
+test_expect_success 'attempt to delete a branch merged to its base' '
+ git checkout master &&
+ echo Third >>A &&
+ git commit -m "Third commit" A &&
+ git branch -t my10 my9 &&
+ git branch -f my10 HEAD^ &&
+ # we are on master which is at the third commit, and my10
+ # is behind us, so traditionally we would have allowed deleting
+ # it; but my10 is set to track my9 that is further behind.
+ test_must_fail git branch -d my10
+'
+
test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html