[PATCH 0/6] Miscellaneous merge fixes

[PATCH 0/6] Miscellaneous merge fixes

[Elijah Newren]

This series has four independent miscellaneous fixes, three split out
from my RFC series at $gmane/291007 and one new fix.  These four fixes
have no dependencies on each other, so I can trivially split them into
four submissions if preferred.  I only batched them because I want my
index-only merge series to be able to simultaneously depend on three
of them.

The first patch is a very minor code cleanup, which has already been
reviewed by Junio.

The second patch is also a minor code cleanup.

The third and fourth patches test and fix a bug with octopus merges.
Since last submitting these patches with my RFC series, I discovered
our documentation used to explicitly state the expectation I enforced
with these patches.  I added a note about that in the commit message.

The fifth and sixth patches test and fix a bug with trivial merges;
these patches are new since my RFC series.


Elijah Newren (6):
  Remove duplicate code
  Avoid checking working copy when creating a virtual merge base
  Add merge testcases for when index doesn't match HEAD
  merge-octopus: Abort if index does not match HEAD
  Add a testcase demonstrating a bug with trivial merges
  builtin/merge.c: Fix a bug with trivial merges

 builtin/merge.c                          |   8 ++
 git-merge-octopus.sh                     |   6 ++
 merge-recursive.c                        |   8 +-
 t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++
 t/t7605-merge-resolve.sh                 |   6 +-
 5 files changed, 175 insertions(+), 6 deletions(-)
 create mode 100755 t/t6044-merge-unrelated-index-changes.sh

-- 
2.8.0.21.g229f62a

[PATCH 1/6] Remove duplicate code

[Elijah Newren]

In commit 51931bf (merge-recursive: Improve handling of rename target vs.
directory addition, 2011-08-11) I apparently added two lines of code that
were immediately duplicated a few lines later.  No idea why, other than
it seems pretty clear this was a mistake: there is no need to remove the
same file twice; removing it once is sufficient...especially since the
intervening line was working with a different file entirely.

Signed-off-by: Elijah Newren <newren@gmail.com>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---
 merge-recursive.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index b880ae5..d4292de 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1773,8 +1773,6 @@ static int process_entry(struct merge_options *o,
 			output(o, 1, _("CONFLICT (%s): There is a directory with name %s in %s. "
 			       "Adding %s as %s"),
 			       conf, path, other_branch, path, new_path);
-			if (o->call_depth)
-				remove_file_from_cache(path);
 			update_file(o, 0, sha, mode, new_path);
 			if (o->call_depth)
 				remove_file_from_cache(path);
-- 
2.8.0.21.g229f62a

[PATCH 2/6] Avoid checking working copy when creating a virtual merge base

[Elijah Newren]

There were a few cases in merge-recursive that could result in a check for
the presence of files in the working copy while trying to create a virtual
merge base.  These were rare and innocuous, but somewhat illogical.  The
two cases were:

  * When there was naming conflicts (e.g. a D/F conflict) and we had to
    pick a new unique name for a file.  Since the new name is somewhat
    arbitrary, it didn't matter that we consulted the working copy to
    avoid picking a filename it has, but since the virtual merge base is
    never checked out, it's a waste of time and slightly odd to do so.

  * When two different files get renamed to the same name (on opposite
    sides of the merge), we needed to delete the original filenames from
    the cache and possibly also the working directory.  The caller's check
    for determining whether to delete from the working directory was a
    call to would_lose_untracked().  It turns out this didn't matter
    because remove_file() had logic to avoid modifying the working
    directory when creating a virtual merge base, but there is no reason
    for the caller to check the working directory in such circumstances.
    It's a waste of time, if not also a bit weird.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d4292de..06d31ed 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -622,7 +622,7 @@ static char *unique_path(struct merge_options *o, const char *path, const char *
 	base_len = newpath.len;
 	while (string_list_has_string(&o->current_file_set, newpath.buf) ||
 	       string_list_has_string(&o->current_directory_set, newpath.buf) ||
-	       file_exists(newpath.buf)) {
+	       (!o->call_depth && file_exists(newpath.buf))) {
 		strbuf_setlen(&newpath, base_len);
 		strbuf_addf(&newpath, "_%d", suffix++);
 	}
@@ -1234,8 +1234,8 @@ static void conflict_rename_rename_2to1(struct merge_options *o,
 	       a->path, c1->path, ci->branch1,
 	       b->path, c2->path, ci->branch2);
 
-	remove_file(o, 1, a->path, would_lose_untracked(a->path));
-	remove_file(o, 1, b->path, would_lose_untracked(b->path));
+	remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
+	remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
 
 	mfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other,
 					    o->branch1, c1->path,
-- 
2.8.0.21.g229f62a

Re: [PATCH 2/6] Avoid checking working copy when creating a virtual merge base

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> There were a few cases in merge-recursive that could result in a check for
> the presence of files in the working copy while trying to create a virtual
> merge base.  These were rare and innocuous, but somewhat illogical.  The
> two cases were:
>
>   * When there was naming conflicts (e.g. a D/F conflict) and we had to
>     pick a new unique name for a file.  Since the new name is somewhat
>     arbitrary, it didn't matter that we consulted the working copy to
>     avoid picking a filename it has, but since the virtual merge base is
>     never checked out, it's a waste of time and slightly odd to do so.
>
>   * When two different files get renamed to the same name (on opposite
>     sides of the merge), we needed to delete the original filenames from
>     the cache and possibly also the working directory.  The caller's check
>     for determining whether to delete from the working directory was a
>     call to would_lose_untracked().  It turns out this didn't matter
>     because remove_file() had logic to avoid modifying the working
>     directory when creating a virtual merge base, but there is no reason
>     for the caller to check the working directory in such circumstances.
>     It's a waste of time, if not also a bit weird.

I think "avoid checking" and "waste of time" are understatements, in
that they make it sound as if the current code is OK and this change
is only to reduce waste.  But doesn't the code misbehave if you had
a file in the working tree whose path happens to be the same as the
one involved in these codepaths (iow, if they did not check these
paths in the working tree, the internal merge may succeed, but it
would unnecessarily fail)?

Subject: merge-recursive: do not check working tree during an internal merge

or something, perhaps?

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-recursive.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d4292de..06d31ed 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -622,7 +622,7 @@ static char *unique_path(struct merge_options *o, const char *path, const char *
>  	base_len = newpath.len;
>  	while (string_list_has_string(&o->current_file_set, newpath.buf) ||
>  	       string_list_has_string(&o->current_directory_set, newpath.buf) ||
> -	       file_exists(newpath.buf)) {
> +	       (!o->call_depth && file_exists(newpath.buf))) {
>  		strbuf_setlen(&newpath, base_len);
>  		strbuf_addf(&newpath, "_%d", suffix++);
>  	}
> @@ -1234,8 +1234,8 @@ static void conflict_rename_rename_2to1(struct merge_options *o,
>  	       a->path, c1->path, ci->branch1,
>  	       b->path, c2->path, ci->branch2);
>  
> -	remove_file(o, 1, a->path, would_lose_untracked(a->path));
> -	remove_file(o, 1, b->path, would_lose_untracked(b->path));
> +	remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
> +	remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
>  
>  	mfi_c1 = merge_file_special_markers(o, a, c1, &ci->ren1_other,
>  					    o->branch1, c1->path,

[PATCH 3/6] Add merge testcases for when index doesn't match HEAD

[Elijah Newren]

With one exception, we require the index to exactly match the current HEAD
commit at the time git merge is invoked.  This expectation was even
documented in git-merge.txt until commit ebef7e5 (Documentation: simplify
How Merge Works, 2010-01-23).  Most merge strategies enforced this
requirement, but it turns out not all did.  The current exceptions were
the following two:
  * ff updates
  * octopus merges

ff updates actually will error out if the staged change is to a path
modified between HEAD and the commit being merged.  If the path(s) that
are staged are files unrelated to the changes between these two commits,
though, then an ff update will just keep these staged changes around after
the merge.  This is the one exception we expected to the abort-merge-if-
index-doesn't-match-HEAD rule.

For octopus merges, the rule should be enforced.  Unfortunately, the
current behavior of the code is to ignore the difference and use the
staged changes in place of whatever is in HEAD as it proceeds to perform
the merge.  So if the staged changes can be cleanly merged with all the
other heads, then the staged changes will just be incorported into the
resulting commit.  If the staged changes cannot be cleanly merged with all
the other heads, the merge is not aborted -- merge conflicts are simply
reported as if HEAD had originally contained whatever the index did.

Add testcases that check our expectations.  A subsequent commit will
correct the erroneous octopus merge behavior.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)
 create mode 100755 t/t6044-merge-unrelated-index-changes.sh

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
new file mode 100755
index 0000000..eed5d95
--- /dev/null
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description="merges with unrelated index changes"
+
+. ./test-lib.sh
+
+# Testcase for some simple merges
+#   A
+#   o-----o B
+#    \
+#     \---o C
+#      \
+#       \-o D
+#        \
+#         o E
+#   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
+
+test_expect_success 'setup trivial merges' '
+	seq 1 10 >a &&
+	git add a &&
+	test_tick && git commit -m A &&
+
+	git branch A &&
+	git branch B &&
+	git branch C &&
+	git branch D &&
+	git branch E &&
+
+	git checkout B &&
+	echo b >b &&
+	echo 11 >>a &&
+	git add a b &&
+	test_tick && git commit -m B &&
+
+	git checkout C &&
+	echo c >c &&
+	git add c &&
+	test_tick && git commit -m C &&
+
+	git checkout D &&
+	seq 2 10 >a &&
+	echo d >d &&
+	git add a d &&
+	test_tick && git commit -m D &&
+
+	git checkout E &&
+	mkdir subdir &&
+	git mv a subdir/a &&
+	echo e >subdir/e &&
+	git add subdir &&
+	test_tick && git commit -m E
+'
+
+test_expect_success 'ff update' '
+	git reset --hard &&
+	git checkout A^0 &&
+
+	touch random_file && git add random_file &&
+
+	git merge E^0 &&
+
+	test_must_fail git rev-parse HEAD:random_file &&
+	test "$(git diff --name-only --cached E)" = "random_file"
+'
+
+test_expect_success 'ff update, important file modified' '
+	git reset --hard &&
+	git checkout A^0 &&
+
+	mkdir subdir &&
+	touch subdir/e &&
+	git add subdir/e &&
+
+	test_must_fail git merge E^0
+'
+
+test_expect_success 'resolve, trivial' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s resolve C^0
+'
+
+test_expect_success 'resolve, non-trivial' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s resolve D^0
+'
+
+test_expect_success 'recursive' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s recursive C^0
+'
+
+test_expect_failure 'octopus, unrelated file touched' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge C^0 D^0
+'
+
+test_expect_failure 'octopus, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm b &&
+
+	test_must_fail git merge C^0 D^0
+'
+
+test_expect_failure 'octopus, related file modified' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	echo 12 >>a && git add a &&
+
+	test_must_fail git merge C^0 D^0
+'
+
+test_expect_success 'ours' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s ours C^0
+'
+
+test_expect_success 'subtree' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	touch random_file && git add random_file &&
+
+	test_must_fail git merge -s subtree E^0
+'
+
+test_done
-- 
2.8.0.21.g229f62a

[PATCH 4/6] merge-octopus: Abort if index does not match HEAD

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 git-merge-octopus.sh                     | 6 ++++++
 t/t6044-merge-unrelated-index-changes.sh | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
index 8643f74..dc2fd1b 100755
--- a/git-merge-octopus.sh
+++ b/git-merge-octopus.sh
@@ -44,6 +44,12 @@ esac
 # MRC is the current "merge reference commit"
 # MRT is the current "merge result tree"
 
+if ! git diff-index --quiet --cached HEAD --
+then
+    echo "Error: Your local changes to the following files would be overwritten by merge"
+    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
+    exit 2
+fi
 MRC=$(git rev-parse --verify -q $head)
 MRT=$(git write-tree)
 NON_FF_MERGE=0
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index eed5d95..20a3ffe 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -105,7 +105,7 @@ test_expect_success 'recursive' '
 	test_must_fail git merge -s recursive C^0
 '
 
-test_expect_failure 'octopus, unrelated file touched' '
+test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -114,7 +114,7 @@ test_expect_failure 'octopus, unrelated file touched' '
 	test_must_fail git merge C^0 D^0
 '
 
-test_expect_failure 'octopus, related file removed' '
+test_expect_success 'octopus, related file removed' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -123,7 +123,7 @@ test_expect_failure 'octopus, related file removed' '
 	test_must_fail git merge C^0 D^0
 '
 
-test_expect_failure 'octopus, related file modified' '
+test_expect_success 'octopus, related file modified' '
 	git reset --hard &&
 	git checkout B^0 &&
 
-- 
2.8.0.21.g229f62a

[PATCH 5/6] Add a testcase demonstrating a bug with trivial merges

[Elijah Newren]

Repeating a trivially merge more than once will leave the index out of
sync, despite being clean before the merge and operating on the exact
same heads as the first run.  The recorded merge has the correct tree and
the working tree is brought up to date, it is just the index that is left
as it was before the merge.  Every attempt to repeat the merge beyond the
first will leave the index in the same weird out-of-sync state.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7605-merge-resolve.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 0cb9d11..2f80037 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -27,7 +27,7 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'merge c1 to c2' '
+merge_c1_to_c2_cmds='
 	git reset --hard c1 &&
 	git merge -s resolve c2 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
@@ -41,6 +41,10 @@ test_expect_success 'merge c1 to c2' '
 	test 3 = $(git ls-files | wc -l)
 '
 
+test_expect_success 'merge c1 to c2'        "$merge_c1_to_c2_cmds"
+
+test_expect_failure 'merge c1 to c2, again' "$merge_c1_to_c2_cmds"
+
 test_expect_success 'merge c2 to c3 (fails)' '
 	git reset --hard c2 &&
 	test_must_fail git merge -s resolve c3
-- 
2.8.0.21.g229f62a

[PATCH 6/6] builtin/merge.c: Fix a bug with trivial merges

[Elijah Newren]

If read_tree_trivial succeeds and produces a tree that is already in the
object store, then the index is not written to disk, leaving it
out-of-sync with both HEAD and the working tree.

In order to write the index back out to disk after a merge,
write_index_locked() needs to be called.  For most merge strategies, this
is done from try_merge_strategy().  For fast forward updates, this is
done from checkout_fast_forward().  When trivial merges work, the call to
write_index_locked() is buried a little deeper:
  merge_trivial()
  -> write_tree_trivial()
     -> write_cache_as_tree()
        -> write_index_as_tree()
           -> write_locked_index()
However, it is only called when !cache_tree_fully_valid(), which is how
this bug is triggered.  But that also shows why this bug doesn't affect
any other merge strategies or cases.

Add a direct call to write_index_locked() from merge_trivial() to fix
this issue.  Since the indirect call to write_locked_index() was
conditional on cache_tree_fully_valid(), it won't be written twice.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge.c          | 8 ++++++++
 t/t7605-merge-resolve.sh | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 101ffef..8615343 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -819,6 +819,14 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 {
 	unsigned char result_tree[20], result_commit[20];
 	struct commit_list *parents, **pptr = &parents;
+	static struct lock_file lock;
+
+	hold_locked_index(&lock, 1);
+	refresh_cache(REFRESH_QUIET);
+	if (active_cache_changed &&
+	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+		return error(_("Unable to write index."));
+	rollback_lock_file(&lock);
 
 	write_tree_trivial(result_tree);
 	printf(_("Wonderful.\n"));
diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 2f80037..5d56c38 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -43,7 +43,7 @@ merge_c1_to_c2_cmds='
 
 test_expect_success 'merge c1 to c2'        "$merge_c1_to_c2_cmds"
 
-test_expect_failure 'merge c1 to c2, again' "$merge_c1_to_c2_cmds"
+test_expect_success 'merge c1 to c2, again' "$merge_c1_to_c2_cmds"
 
 test_expect_success 'merge c2 to c3 (fails)' '
 	git reset --hard c2 &&
-- 
2.8.0.21.g229f62a

Re: [PATCH 0/6] Miscellaneous merge fixes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Elijah Newren (6):
>   Remove duplicate code
>   Avoid checking working copy when creating a virtual merge base
>   Add merge testcases for when index doesn't match HEAD
>   merge-octopus: Abort if index does not match HEAD
>   Add a testcase demonstrating a bug with trivial merges
>   builtin/merge.c: Fix a bug with trivial merges

Please be careful when giving titles to your patches.  They will be
shown in a context that is much wider than the area your attention
is currently concentrated on.  E.g. does "Remove duplicate code"
tell readers of "git shortlog --no-merges v2.8.0..v2.9.0" what the
change was about when it eventually appears in the upcoming release?

>
>  builtin/merge.c                          |   8 ++
>  git-merge-octopus.sh                     |   6 ++
>  merge-recursive.c                        |   8 +-
>  t/t6044-merge-unrelated-index-changes.sh | 153 +++++++++++++++++++++++++++++++
>  t/t7605-merge-resolve.sh                 |   6 +-
>  5 files changed, 175 insertions(+), 6 deletions(-)
>  create mode 100755 t/t6044-merge-unrelated-index-changes.sh

Re: [PATCH 0/6] Miscellaneous merge fixes

[Elijah Newren]

Hi,

On Tue, Apr 12, 2016 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> Elijah Newren (6):
>>   Remove duplicate code
>>   Avoid checking working copy when creating a virtual merge base
>>   Add merge testcases for when index doesn't match HEAD
>>   merge-octopus: Abort if index does not match HEAD
>>   Add a testcase demonstrating a bug with trivial merges
>>   builtin/merge.c: Fix a bug with trivial merges
>
> Please be careful when giving titles to your patches.  They will be
> shown in a context that is much wider than the area your attention
> is currently concentrated on.  E.g. does "Remove duplicate code"
> tell readers of "git shortlog --no-merges v2.8.0..v2.9.0" what the
> change was about when it eventually appears in the upcoming release?

I will try to do better on that.  For the patch you mention, how about:
   merge-recursive: Remove a few lines of duplicate code
?

I can resubmit the series correcting both that subject and the subject
of the second patch using your suggestion elsewhere in this thread.
The other four already provide some context on their area; am I
correct to assume those provide enough?

Elijah