From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>, a.krey@gmx.de
Subject: [PATCH] merge-recursive: do not look at the index during recursive merge
Date: Tue, 09 Jan 2018 10:19:24 -0800 [thread overview]
Message-ID: <xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <xmqqbmi484tw.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Mon, 08 Jan 2018 12:37:31 -0800")
When merging another branch into ours, if their tree is the same as
the common ancestor's, we can declare that our tree represents the
result of three-way merge. In such a case, the recursive merge
backend incorrectly used to create a commit out of our index, even
when the index has changes.
A recent fix attempted to prevent this by adding a comparison
between "our" tree and the index, but forgot that this check must be
restricted only to the outermost merge. Inner merges performed by
the recursive backend across merge bases are by definition made from
scratch without having any local changes added to the index. The
call to index_has_changes() during an inner merge is working on the
index that has no relation to the merge being performed, preventing
legitimate merges from getting carried out.
Fix it by limiting the check to the outermost merge.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
> Elijah Newren <newren@gmail.com> writes:
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 2ecf495cc2..780f81a8bd 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
>> }
>>
>> if (oid_eq(&common->object.oid, &merge->object.oid)) {
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + if (index_has_changes(&sb)) {
>> + err(o, _("Dirty index: cannot merge (dirty: %s)"),
>> + sb.buf);
>> + return 0;
>> + }
>> output(o, 0, _("Already up to date!"));
>> *result = head;
>> return 1;
>
> I haven't come up with an addition to the test suite, but I suspect
> this change is conceptually wrong. What if a call to this function
> is made during a recursive, inner merge?
Now I have.
merge-recursive.c | 2 +-
t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 780f81a8bd..0fc580d8ca 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,
if (oid_eq(&common->object.oid, &merge->object.oid)) {
struct strbuf sb = STRBUF_INIT;
- if (index_has_changes(&sb)) {
+ if (!o->call_depth && index_has_changes(&sb)) {
err(o, _("Dirty index: cannot merge (dirty: %s)"),
sb.buf);
return 0;
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 9a893b5fe7..12e3b1392d 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names of all base trees' '
test_cmp expect actual
'
+test_expect_success 'merge-recursive internal merge resolves to the sameness' '
+ git reset --hard HEAD &&
+
+ # We are going to create a history leading to two criss-cross
+ # branches A and B. The common ancestor at the bottom, O0,
+ # has two childs O1 and O2, both of which will be merge base
+ # between A and B, like so:
+ #
+ # O1---A
+ # / \ /
+ # O0 .
+ # \ / \
+ # O2---B
+ #
+ # The recently added "check to see if the index is different from
+ # the tree into which something else is getting merged into and
+ # reject" check must NOT kick in when an inner merge between O1
+ # and O2 is made. Both O1 and O2 happen to have the same tree
+ # as O0 in this test to trigger the bug---whether the inner merge
+ # is made by merging O2 into O1 or O1 into O2, their common ancestor
+ # O0 and the branch being merged have the same tree, and the code
+ # before fix will incorrectly try to look at the index.
+
+ echo "zero" >file &&
+ git add file &&
+ test_tick &&
+ git commit -m "O0" &&
+ O0=$(git rev-parse HEAD) &&
+
+ test_tick &&
+ git commit --allow-empty -m "O2" &&
+ O1=$(git rev-parse HEAD) &&
+
+ git reset --hard $O0 &&
+ test_tick &&
+ git commit --allow-empty -m "O2" &&
+ O2=$(git rev-parse HEAD) &&
+
+ test_tick &&
+ git merge -s ours $O1 &&
+ B=$(git rev-parse HEAD) &&
+
+ git reset --hard $O1 &&
+ test_tick &&
+ git merge -s ours $O2 &&
+ A=$(git rev-parse HEAD) &&
+
+ git merge $B
+'
+
test_done
--
2.16.0-rc1-164-gb9fca19b00
next prev parent reply other threads:[~2018-01-09 18:19 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-20 11:43 git merge commits staged files (when two trees are identical) Andreas Krey
2017-12-21 18:50 ` Elijah Newren
2017-12-21 19:19 ` [PATCH 1/3] t6044: recursive can silently incorporate dirty changes in a merge Elijah Newren
2017-12-21 19:19 ` [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse Elijah Newren
2017-12-21 19:36 ` Elijah Newren
2017-12-22 20:46 ` Junio C Hamano
2017-12-23 2:26 ` Elijah Newren
2017-12-21 19:19 ` [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge Elijah Newren
2017-12-22 20:38 ` Junio C Hamano
2018-01-08 20:37 ` Junio C Hamano
2018-01-09 18:19 ` Junio C Hamano [this message]
2018-01-09 18:25 ` [PATCH] merge-recursive: do not look at the index during recursive merge Junio C Hamano
2018-01-09 18:27 ` Eric Sunshine
2018-01-09 18:29 ` Elijah Newren
2018-01-09 18:49 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=a.krey@gmx.de \
--cc=git@vger.kernel.org \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).