git merge commits staged files (when two trees are identical)

git merge commits staged files (when two trees are identical)

[Andreas Krey]

Hi everybody,

we just stumbled over a situation in which a merge commits
staged changes into the merge commit. This happens when the
merged-in branch does have commits ('main') but has the same
tree ('--allow-empty') as the merge base:

    git init
    echo eins >a
    git add a
    git commit -m initial
    git branch sub
    git commit -m main --allow-empty
    git checkout sub
    : two
    echo zwei >>a
    git add a
    git commit -m underway
    : three
    echo drei >>a
    git add a              # important
    git status
    git diff --cached
    git merge master -m 'merge'
    git status
    git log --cc -1

If the change isn't staged (comment out the '# important' line)
the change survives as unstaged.

That is a bug?

- Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: git merge commits staged files (when two trees are identical)

[Elijah Newren]

On Wed, Dec 20, 2017 at 3:43 AM, Andreas Krey <a.krey@gmx.de> wrote:
> Hi everybody,
>
> we just stumbled over a situation in which a merge commits
> staged changes into the merge commit. This happens when the
> merged-in branch does have commits ('main') but has the same
> tree ('--allow-empty') as the merge base:
>
>     git init
>     echo eins >a
>     git add a
>     git commit -m initial
>     git branch sub
>     git commit -m main --allow-empty
>     git checkout sub
>     : two
>     echo zwei >>a
>     git add a
>     git commit -m underway
>     : three
>     echo drei >>a
>     git add a              # important
>     git status
>     git diff --cached
>     git merge master -m 'merge'
>     git status
>     git log --cc -1
>
> If the change isn't staged (comment out the '# important' line)
> the change survives as unstaged.
>
> That is a bug?

Yes, it's definitely a bug; thanks for reporting it.  We have a
specific set of tests for this type of situation in t6044, which was
intended to test all the merge strategies and ensure they didn't have
this class of bug.  It turns out that there is an additional codepath
within one of the strategies that wasn't tested, and which you've
tripped over.  I've got some patches to fix this up that I'll respond
with shortly.

[PATCH 1/3] t6044: recursive can silently incorporate dirty changes in a merge

[Elijah Newren]

The recursive merge strategy has some special handling when the tree for
the merge branch exactly matches the merge base, but that code path is
missing checks for the index having changes relative to HEAD.  Add a
testcase covering this scenario.

Reported-by: Andreas Krey <a.krey@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6044-merge-unrelated-index-changes.sh | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 01023486c5..5e472be92b 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -6,18 +6,21 @@ test_description="merges with unrelated index changes"
 
 # Testcase for some simple merges
 #   A
-#   o-----o B
+#   o-------o B
 #    \
-#     \---o C
+#     \-----o C
 #      \
-#       \-o D
+#       \---o D
 #        \
-#         o E
+#         \-o E
+#          \
+#           o F
 #   Commit A: some file a
 #   Commit B: adds file b, modifies end of a
 #   Commit C: adds file c
 #   Commit D: adds file d, modifies beginning of a
 #   Commit E: renames a->subdir/a, adds subdir/e
+#   Commit F: empty commit
 
 test_expect_success 'setup trivial merges' '
 	test_seq 1 10 >a &&
@@ -29,6 +32,7 @@ test_expect_success 'setup trivial merges' '
 	git branch C &&
 	git branch D &&
 	git branch E &&
+	git branch F &&
 
 	git checkout B &&
 	echo b >b &&
@@ -52,7 +56,10 @@ test_expect_success 'setup trivial merges' '
 	git mv a subdir/a &&
 	echo e >subdir/e &&
 	git add subdir &&
-	test_tick && git commit -m E
+	test_tick && git commit -m E &&
+
+	git checkout F &&
+	test_tick && git commit --allow-empty -m F
 '
 
 test_expect_success 'ff update' '
@@ -105,6 +112,15 @@ test_expect_success 'recursive' '
 	test_must_fail git merge -s recursive C^0
 '
 
+test_expect_failure 'recursive, when merge branch matches merge base' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s recursive F^0
+'
+
 test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
-- 
2.15.1.436.g63a861020b

[PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Elijah Newren]

index_has_changes() is a function we want to reuse outside of just am,
making it also available for merge-recursive and merge-ort.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/am.c | 37 -------------------------------------
 cache.h      |  9 +++++++++
 merge.c      | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3d98e52085..a02d5186cb 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1142,43 +1142,6 @@ static void refresh_and_write_cache(void)
 		die(_("unable to write index file"));
 }
 
-/**
- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn
- * branch, returns 1 if there are entries in the index, 0 otherwise. If an
- * strbuf is provided, the space-separated list of files that differ will be
- * appended to it.
- */
-static int index_has_changes(struct strbuf *sb)
-{
-	struct object_id head;
-	int i;
-
-	if (!get_oid_tree("HEAD", &head)) {
-		struct diff_options opt;
-
-		diff_setup(&opt);
-		opt.flags.exit_with_status = 1;
-		if (!sb)
-			opt.flags.quick = 1;
-		do_diff_cache(&head, &opt);
-		diffcore_std(&opt);
-		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
-			if (i)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
-		}
-		diff_flush(&opt);
-		return opt.flags.has_changes != 0;
-	} else {
-		for (i = 0; sb && i < active_nr; i++) {
-			if (i)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, active_cache[i]->name);
-		}
-		return !!active_nr;
-	}
-}
-
 /**
  * Dies with a user-friendly message on how to proceed after resolving the
  * problem. This message can be overridden with state->resolvemsg.
diff --git a/cache.h b/cache.h
index a2ec8c0b55..d8b975a571 100644
--- a/cache.h
+++ b/cache.h
@@ -644,6 +644,15 @@ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsi
 extern int discard_index(struct index_state *);
 extern void move_index_extensions(struct index_state *dst, struct index_state *src);
 extern int unmerged_index(const struct index_state *);
+
+/**
+ * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn
+ * branch, returns 1 if there are entries in the index, 0 otherwise. If an
+ * strbuf is provided, the space-separated list of files that differ will be
+ * appended to it.
+ */
+extern int index_has_changes(struct strbuf *sb);
+
 extern int verify_path(const char *path);
 extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);
 extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
diff --git a/merge.c b/merge.c
index e5d796c9f2..195b578700 100644
--- a/merge.c
+++ b/merge.c
@@ -1,4 +1,6 @@
 #include "cache.h"
+#include "diff.h"
+#include "diffcore.h"
 #include "lockfile.h"
 #include "commit.h"
 #include "run-command.h"
@@ -15,6 +17,37 @@ static const char *merge_argument(struct commit *commit)
 		return EMPTY_TREE_SHA1_HEX;
 }
 
+int index_has_changes(struct strbuf *sb)
+{
+	struct object_id head;
+	int i;
+
+	if (!get_oid_tree("HEAD", &head)) {
+		struct diff_options opt;
+
+		diff_setup(&opt);
+		opt.flags.exit_with_status = 1;
+		if (!sb)
+			opt.flags.quick = 1;
+		do_diff_cache(&head, &opt);
+		diffcore_std(&opt);
+		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
+			if (i)
+				strbuf_addch(sb, ' ');
+			strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
+		}
+		diff_flush(&opt);
+		return opt.flags.has_changes != 0;
+	} else {
+		for (i = 0; sb && i < active_nr; i++) {
+			if (i)
+				strbuf_addch(sb, ' ');
+			strbuf_addstr(sb, active_cache[i]->name);
+		}
+		return !!active_nr;
+	}
+}
+
 int try_merge_command(const char *strategy, size_t xopts_nr,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
-- 
2.15.1.436.g63a861020b

Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Elijah Newren]

On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:
> index_has_changes() is a function we want to reuse outside of just am,
> making it also available for merge-recursive and merge-ort.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

Note: These patches built on master, and merge cleanly with next and
pu.  However, this patch has a minor conflict with maint.  If you'd
prefer a version that applies on top of maint, let me know and I'll
resubmit.

Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:
>> index_has_changes() is a function we want to reuse outside of just am,
>> making it also available for merge-recursive and merge-ort.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>
> Note: These patches built on master, and merge cleanly with next and
> pu.  However, this patch has a minor conflict with maint.  If you'd
> prefer a version that applies on top of maint, let me know and I'll
> resubmit.

I think I managed to create two topics, one that is with these three
patches (2/3 backported) on top of maint and the other one merges
the former on top of master.  Please see if you found a mismerge
when I push the results out.

Thanks.

Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Elijah Newren]

On Fri, Dec 22, 2017 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:
>>> index_has_changes() is a function we want to reuse outside of just am,
>>> making it also available for merge-recursive and merge-ort.
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>
>> Note: These patches built on master, and merge cleanly with next and
>> pu.  However, this patch has a minor conflict with maint.  If you'd
>> prefer a version that applies on top of maint, let me know and I'll
>> resubmit.
>
> I think I managed to create two topics, one that is with these three
> patches (2/3 backported) on top of maint and the other one merges
> the former on top of master.  Please see if you found a mismerge
> when I push the results out.

I'm about to head out on a multi-day trip, so I might not get to this
until the middle of next week.  I'll try to take a look as soon as I
can, though.

[PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

[Elijah Newren]

builtin/merge.c contains this important requirement for merge strategies:
	/*
	 * At this point, we need a real merge.  No matter what strategy
	 * we use, it would operate on the index, possibly affecting the
	 * working tree, and when resolved cleanly, have the desired
	 * tree in the index -- this means that the index must be in
	 * sync with the head commit.  The strategies are responsible
	 * to ensure this.
	 */

merge-recursive does not do this check directly, instead it relies on
unpack_trees() to do it.  However, merge_trees() has a special check for
the merge branch exactly matching the merge base; when it detects that
situation, it returns early without calling unpack_trees(), because it
knows that the HEAD commit already has the correct result.  Unfortunately,
it didn't check that the index matched HEAD, so after it returned, the
outer logic ended up creating a merge commit that included something
other than HEAD.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                        | 7 +++++++
 t/t6044-merge-unrelated-index-changes.sh | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

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;
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 5e472be92b..23b86fb977 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -112,7 +112,7 @@ test_expect_success 'recursive' '
 	test_must_fail git merge -s recursive C^0
 '
 
-test_expect_failure 'recursive, when merge branch matches merge base' '
+test_expect_success 'recursive, when merge branch matches merge base' '
 	git reset --hard &&
 	git checkout B^0 &&
 
-- 
2.15.1.436.g63a861020b

Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> builtin/merge.c contains this important requirement for merge strategies:
> 	/*
> 	 * At this point, we need a real merge.  No matter what strategy
> 	 * we use, it would operate on the index, possibly affecting the
> 	 * working tree, and when resolved cleanly, have the desired
> 	 * tree in the index -- this means that the index must be in
> 	 * sync with the head commit.  The strategies are responsible
> 	 * to ensure this.
> 	 */
>
> merge-recursive does not do this check directly, instead it relies on
> unpack_trees() to do it.  However, merge_trees() has a special check for
> the merge branch exactly matching the merge base; when it detects that
> situation, it returns early without calling unpack_trees(), because it
> knows that the HEAD commit already has the correct result.  Unfortunately,
> it didn't check that the index matched HEAD, so after it returned, the
> outer logic ended up creating a merge commit that included something
> other than HEAD.

Good.

I actually was imagining that you would shoot for creating an empty
commit and leaving a working tree and the index that are both dirty,
but I do not think it is worth the effort.  Besides, "you have to
start from a clean index" is a much simpler rule to explain than
with "unless the resulting tree is the same as HEAD", especially
when that "unless" is highly unlikely to happen anyway.

Thanks.

>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-recursive.c                        | 7 +++++++
>  t/t6044-merge-unrelated-index-changes.sh | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> 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;
> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
> index 5e472be92b..23b86fb977 100755
> --- a/t/t6044-merge-unrelated-index-changes.sh
> +++ b/t/t6044-merge-unrelated-index-changes.sh
> @@ -112,7 +112,7 @@ test_expect_success 'recursive' '
>  	test_must_fail git merge -s recursive C^0
>  '
>  
> -test_expect_failure 'recursive, when merge branch matches merge base' '
> +test_expect_success 'recursive, when merge branch matches merge base' '
>  	git reset --hard &&
>  	git checkout B^0 &&

Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

[Junio C Hamano]

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?

Perhaps something like this is needed?

 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 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;

[PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

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

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> + ...
> +	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) &&

Does not affect the validity of the test at all, but the log message
of the $O1 should be made with -m "O1", not with -m "O2".  That fix
will be in the version I'll be queuing.

> +
> +	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

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Eric Sunshine]

On Tue, Jan 9, 2018 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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>
> ---
> diff --git 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_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

s/childs/children,/

> +       # 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

Too many "into"s: s/merged into/merged/

> +       # 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.

Nit: Does "code before fix" belong in this comment? It sounds more
like something you'd say in the commit message.

> +
> +       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" &&

s/O2/O1/

> +       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
> +'

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Elijah Newren]

Hi,

On Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano <gitster@pobox.com> wrote:

>     > 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?

Eek, good catch.

>  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;

Yep, looks good to me; sorry for overlooking this.

Elijah

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Hi,
>
> On Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>     > 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?
>
> Eek, good catch.
>
>>  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;
>
> Yep, looks good to me; sorry for overlooking this.
>
> Elijah

Thanks.  The breakage is already in 'master' so this fix needs to be
fast-tracked.