[RFC PATCH 0/7] merge requirement: index matches head

[RFC PATCH 0/7] merge requirement: index matches head

[Elijah Newren]

Between working on some other things, I happened to be reading
Documentation/git-merge.txt and ran across the part that says:

    ...[merge will] abort if there are any changes registered in the
    index relative to the `HEAD` commit.  (One exception is when the
    changed index entries are in the state that would result from the
    merge already.)

I was pretty sure this statement was wrong, but did some digging to
uncover the details and the history.  What I thought would turn into a
simple three-line documentation fix, ballooned into this patch series.

This series might be best read in a different order; I'm not yet sure
the right way to structure it.  But:

  * Patch 5 demonstrates one of the ways that the parenthetical
    sentence is wrong (desirable perhaps, but not what is implemented)

  * Patch 7 explains the history, the trade-offs, the three ways the
    parenthetical sentence is wrong, and the many pitfalls we've run
    into trying to allow for such an exception.  Very small
    documentation fix with a huge commit message.

  * Patch 6 fixes the breakage demonstrated in patch 5, but if I only
    submitted patches 5-7, then the testsuite wouldn't pass because
    this fix uncovered multiple other bugs.  That's where patches 1-4
    came in.  This fix is also kind of opinionated; it takes the stance
    that allowing the exceptions isn't worth it.

Elijah Newren (7):
  t6044: verify that merges expected to abort actually abort
  t6044: add a testcase for index matching head, when head doesn't match HEAD
  merge-recursive: make sure when we say we abort that we actually abort
  merge-recursive: fix assumption that head tree being merged is HEAD
  t6044: add more testcases with staged changes before a merge is invoked
  merge-recursive: enforce rule that index matches head before merging
  merge: fix misleading pre-merge check documentation

 Documentation/git-merge.txt              |   6 +-
 builtin/am.c                             |   6 +-
 cache.h                                  |   8 --
 merge-recursive.c                        |  14 +--
 merge.c                                  |  10 +-
 t/t6044-merge-unrelated-index-changes.sh |  67 +++++++++++--
 t/t7504-commit-msg-hook.sh               |   4 +-
 t/t7611-merge-abort.sh                   | 118 -----------------------
 tree.h                                   |   8 ++
 9 files changed, 87 insertions(+), 154 deletions(-)

-- 
2.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 1/7] t6044: verify that merges expected to abort actually abort

[Elijah Newren]

t6044 has lots of tests for verifying that merge will abort as expected
when there are changes staged before the merge starts.  However, it only
checked for non-zero exit code, which could mean that the merge ran to
completion with conflicts.  Check that the merge was actually correctly
aborted, i.e. that .git/MERGE_HEAD is not present.

This changes one of the tests from expect_success to expect_failure.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6044-merge-unrelated-index-changes.sh | 32 ++++++++++++++++--------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 23b86fb977..f9c2f8179e 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -82,7 +82,8 @@ test_expect_success 'ff update, important file modified' '
 	touch subdir/e &&
 	git add subdir/e &&
 
-	test_must_fail git merge E^0
+	test_must_fail git merge E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, trivial' '
@@ -91,7 +92,8 @@ test_expect_success 'resolve, trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve C^0
+	test_must_fail git merge -s resolve C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, non-trivial' '
@@ -100,7 +102,8 @@ test_expect_success 'resolve, non-trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve D^0
+	test_must_fail git merge -s resolve D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'recursive' '
@@ -109,16 +112,18 @@ test_expect_success 'recursive' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s recursive C^0
+	test_must_fail git merge -s recursive C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
-test_expect_success 'recursive, when merge branch matches merge base' '
+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_must_fail git merge -s recursive F^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, unrelated file touched' '
@@ -127,7 +132,8 @@ test_expect_success 'octopus, unrelated file touched' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file removed' '
@@ -136,7 +142,8 @@ test_expect_success 'octopus, related file removed' '
 
 	git rm b &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file modified' '
@@ -145,7 +152,8 @@ test_expect_success 'octopus, related file modified' '
 
 	echo 12 >>a && git add a &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'ours' '
@@ -154,7 +162,8 @@ test_expect_success 'ours' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s ours C^0
+	test_must_fail git merge -s ours C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'subtree' '
@@ -163,7 +172,8 @@ test_expect_success 'subtree' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s subtree E^0
+	test_must_fail git merge -s subtree E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_done
-- 
2.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 2/7] t6044: add a testcase for index matching head, when head doesn't match HEAD

[Elijah Newren]

The `git merge-recursive` command allows the user to directly specify
three commits to merge -- base, head, and remote.  (More than three can be
specified in the case of multiple merge bases.)  Note that since the user
is allowed to specify head, it need not match HEAD.

Virtually every test and script in the current git.git codebase calls `git
merge-recursive` with head=HEAD, and likely external callers do as well,
which is why this has gone unnoticed.  There is one notable
counter-example: git-stash.sh.  However, git-stash called `git
merge-recursive` with an index that matches the expected merge result,
which happens to be a currently allowed exception to the "index must match
head" rule, so this never triggered an error previously.

Since we would like to tighten up the "index must match head" rule, we
need to make sure we are comparing to the correct head.  Add a testcase
that demonstrates the failure when we check the wrong HEAD.

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

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index f9c2f8179e..92ec552558 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
+	git reset --hard &&
+	git checkout C^0 &&
+
+	# Make index match B
+	git diff C B | git apply --cached &&
+	# Merge B & F, with B as "head"
+	git merge-recursive A -- B F > out &&
+	test_i18ngrep "Already up to date" out
+'
+
 test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
-- 
2.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 3/7] merge-recursive: make sure when we say we abort that we actually abort

[Elijah Newren]

In commit 65170c07d4 ("merge-recursive: avoid incorporating uncommitted
changes in a merge", 2017-12-21), it was noted that there was a special
case when merge-recursive didn't rely on unpack_trees() to enforce the
index == HEAD requirement, and thus that it needed to do that enforcement
itself.  Unfortunately, it returned the wrong exit status, signalling that
the merge completed but had conflicts, rather than that it was aborted.
Fix the return code, and while we're at it, change the error message to
match what unpack_trees() would have printed.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index ac27abbd4c..b3deb7b182 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3264,9 +3264,9 @@ int merge_trees(struct merge_options *o,
 		struct strbuf sb = STRBUF_INIT;
 
 		if (!o->call_depth && index_has_changes(&sb)) {
-			err(o, _("Dirty index: cannot merge (dirty: %s)"),
+			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 			    sb.buf);
-			return 0;
+			return -1;
 		}
 		output(o, 0, _("Already up to date!"));
 		*result = head;
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 92ec552558..3876cfa4fa 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -116,7 +116,7 @@ test_expect_success 'recursive' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
-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.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Elijah Newren]

`git merge-recursive` does a three-way merge between user-specified trees
base, head, and remote.  Since the user is allowed to specify head, we can
not necesarily assume that head == HEAD.

We modify index_has_changes() to take an extra argument specifying the
tree to compare the index to.  If NULL, it will compare to HEAD.  We then
use this from merge-recursive to make sure we compare to the
user-specified head.

Signed-off-by: Elijah Newren <newren@gmail.com>
---

I'm really unsure where the index_has_changes() declaration should go;
I stuck it in tree.h, but is there a better spot?

 builtin/am.c                             |  6 +++---
 cache.h                                  |  8 --------
 merge-recursive.c                        |  2 +-
 merge.c                                  | 10 ++++++----
 t/t6044-merge-unrelated-index-changes.sh |  2 +-
 tree.h                                   |  8 ++++++++
 6 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 2fc2d1e82c..c5b76cd095 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)
 
 	refresh_and_write_cache();
 
-	if (index_has_changes(&sb)) {
+	if (index_has_changes(&sb, NULL)) {
 		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)
 			 * Applying the patch to an earlier tree and merging
 			 * the result may have produced the same tree as ours.
 			 */
-			if (!apply_status && !index_has_changes(NULL)) {
+			if (!apply_status && !index_has_changes(NULL, NULL)) {
 				say(state, stdout, _("No changes -- Patch already applied."));
 				goto next;
 			}
@@ -1878,7 +1878,7 @@ static void am_resolve(struct am_state *state)
 
 	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
 
-	if (!index_has_changes(NULL)) {
+	if (!index_has_changes(NULL, NULL)) {
 		printf_ln(_("No changes - did you forget to use 'git add'?\n"
 			"If there is nothing left to stage, chances are that something else\n"
 			"already introduced the same changes; you might want to skip this patch."));
diff --git a/cache.h b/cache.h
index 89a107a7f7..439b9d9f6e 100644
--- a/cache.h
+++ b/cache.h
@@ -634,14 +634,6 @@ 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, unsigned mode);
 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-recursive.c b/merge-recursive.c
index b3deb7b182..762aa087d0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3263,7 +3263,7 @@ int merge_trees(struct merge_options *o,
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
 		struct strbuf sb = STRBUF_INIT;
 
-		if (!o->call_depth && index_has_changes(&sb)) {
+		if (!o->call_depth && index_has_changes(&sb, head)) {
 			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 			    sb.buf);
 			return -1;
diff --git a/merge.c b/merge.c
index 0783858739..965d287646 100644
--- a/merge.c
+++ b/merge.c
@@ -14,19 +14,21 @@ static const char *merge_argument(struct commit *commit)
 	return oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);
 }
 
-int index_has_changes(struct strbuf *sb)
+int index_has_changes(struct strbuf *sb, struct tree *tree)
 {
-	struct object_id head;
+	struct object_id cmp;
 	int i;
 
-	if (!get_oid_tree("HEAD", &head)) {
+	if (tree)
+		cmp = tree->object.oid;
+	if (tree || !get_oid_tree("HEAD", &cmp)) {
 		struct diff_options opt;
 
 		diff_setup(&opt);
 		opt.flags.exit_with_status = 1;
 		if (!sb)
 			opt.flags.quick = 1;
-		do_diff_cache(&head, &opt);
+		do_diff_cache(&cmp, &opt);
 		diffcore_std(&opt);
 		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
 			if (i)
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 3876cfa4fa..1d43712c52 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
-test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
+test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	git reset --hard &&
 	git checkout C^0 &&
 
diff --git a/tree.h b/tree.h
index e2a80be4ef..2e1d8ea720 100644
--- a/tree.h
+++ b/tree.h
@@ -37,4 +37,12 @@ extern int read_tree_recursive(struct tree *tree,
 extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,
 		     struct index_state *istate);
 
+/**
+ * Returns 1 if the index differs from tree, 0 otherwise.  If tree is NULL,
+ * compares to HEAD.  If tree is NULL and 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, struct tree *tree);
+
 #endif /* TREE_H */
-- 
2.18.0.rc0.49.g3c08dc0fef

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Ramsay Jones]

On 03/06/18 07:58, Elijah Newren wrote:
> `git merge-recursive` does a three-way merge between user-specified trees
> base, head, and remote.  Since the user is allowed to specify head, we can
> not necesarily assume that head == HEAD.
> 
> We modify index_has_changes() to take an extra argument specifying the
> tree to compare the index to.  If NULL, it will compare to HEAD.  We then
> use this from merge-recursive to make sure we compare to the
> user-specified head.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> 
> I'm really unsure where the index_has_changes() declaration should go;
> I stuck it in tree.h, but is there a better spot?

Err, leave it where it is and '#include "tree.h"' ? :-D

ATB,
Ramsay Jones

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On Sun, Jun 03, 2018 at 02:52:12PM +0100, Ramsay Jones wrote:
> On 03/06/18 07:58, Elijah Newren wrote:
> > I'm really unsure where the index_has_changes() declaration should go;
> > I stuck it in tree.h, but is there a better spot?
> 
> Err, leave it where it is and '#include "tree.h"' ? :-D

Or leave it where it is and use a forward structure declaration?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Ramsay Jones]

On 04/06/18 00:37, brian m. carlson wrote:
> On Sun, Jun 03, 2018 at 02:52:12PM +0100, Ramsay Jones wrote:
>> On 03/06/18 07:58, Elijah Newren wrote:
>>> I'm really unsure where the index_has_changes() declaration should go;
>>> I stuck it in tree.h, but is there a better spot?
>>
>> Err, leave it where it is and '#include "tree.h"' ? :-D
> 
> Or leave it where it is and use a forward structure declaration?

Indeed, I had intended to mention that possibility as well.

[Note: the "merge-recursive.h" header file references several
'struct tree *' parameters, but does not itself include a
declaration/definition from any source. So, in all of the six
files that #include it, it relies on a previous #include to
provide such a declaration/definition. I haven't checked, but
I think that it is usually provided by the "commit.h" header (even
on the single occasion that "tree.h" was included!).]


ATB,
Ramsay Jones

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> `git merge-recursive` does a three-way merge between user-specified trees
> base, head, and remote.  Since the user is allowed to specify head, we can
> not necesarily assume that head == HEAD.
>
> We modify index_has_changes() to take an extra argument specifying the
> tree to compare the index to.  If NULL, it will compare to HEAD.  We then
> use this from merge-recursive to make sure we compare to the
> user-specified head.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>
> I'm really unsure where the index_has_changes() declaration should go;
> I stuck it in tree.h, but is there a better spot?

I think I saw you tried to lift an assumption that we're always
working on the_index in a separate patch recently.  Should that
logic apply also to this part of the codebase?  IOW, shouldn't
index_has_changes() take a pointer to istate (as opposed to a
function that uses the implicit the_index that should be named as
"cache_has_changes()" or something?)

I tend to think this function as part of the larger read-cache.c
family whose definitions are in cache.h and accompanied by macros
that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we
were to move it elsewhere, I'd keep the header part as-is and
implementation to read-cache.c to keep it together with the family,
but I do not see a huge issue with the current placement, either.

> diff --git a/cache.h b/cache.h
> index 89a107a7f7..439b9d9f6e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -634,14 +634,6 @@ 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, unsigned mode);
>  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-recursive.c b/merge-recursive.c
> index b3deb7b182..762aa087d0 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3263,7 +3263,7 @@ int merge_trees(struct merge_options *o,
>  	if (oid_eq(&common->object.oid, &merge->object.oid)) {
>  		struct strbuf sb = STRBUF_INIT;
>  
> -		if (!o->call_depth && index_has_changes(&sb)) {
> +		if (!o->call_depth && index_has_changes(&sb, head)) {
>  			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
>  			    sb.buf);
>  			return -1;
> diff --git a/merge.c b/merge.c
> index 0783858739..965d287646 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -14,19 +14,21 @@ static const char *merge_argument(struct commit *commit)
>  	return oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);
>  }
>  
> -int index_has_changes(struct strbuf *sb)
> +int index_has_changes(struct strbuf *sb, struct tree *tree)
>  {
> -	struct object_id head;
> +	struct object_id cmp;
>  	int i;
>  
> -	if (!get_oid_tree("HEAD", &head)) {
> +	if (tree)
> +		cmp = tree->object.oid;
> +	if (tree || !get_oid_tree("HEAD", &cmp)) {
>  		struct diff_options opt;
>  
>  		diff_setup(&opt);
>  		opt.flags.exit_with_status = 1;
>  		if (!sb)
>  			opt.flags.quick = 1;
> -		do_diff_cache(&head, &opt);
> +		do_diff_cache(&cmp, &opt);
>  		diffcore_std(&opt);
>  		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
>  			if (i)
> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
> index 3876cfa4fa..1d43712c52 100755
> --- a/t/t6044-merge-unrelated-index-changes.sh
> +++ b/t/t6044-merge-unrelated-index-changes.sh
> @@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '
>  	test_path_is_missing .git/MERGE_HEAD
>  '
>  
> -test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
> +test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
>  	git reset --hard &&
>  	git checkout C^0 &&
>  
> diff --git a/tree.h b/tree.h
> index e2a80be4ef..2e1d8ea720 100644
> --- a/tree.h
> +++ b/tree.h
> @@ -37,4 +37,12 @@ extern int read_tree_recursive(struct tree *tree,
>  extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,
>  		     struct index_state *istate);
>  
> +/**
> + * Returns 1 if the index differs from tree, 0 otherwise.  If tree is NULL,
> + * compares to HEAD.  If tree is NULL and 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, struct tree *tree);
> +
>  #endif /* TREE_H */

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Elijah Newren]

On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> `git merge-recursive` does a three-way merge between user-specified trees
>> base, head, and remote.  Since the user is allowed to specify head, we can
>> not necesarily assume that head == HEAD.
>>
>> We modify index_has_changes() to take an extra argument specifying the
>> tree to compare the index to.  If NULL, it will compare to HEAD.  We then
>> use this from merge-recursive to make sure we compare to the
>> user-specified head.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>>
>> I'm really unsure where the index_has_changes() declaration should go;
>> I stuck it in tree.h, but is there a better spot?
>
> I think I saw you tried to lift an assumption that we're always
> working on the_index in a separate patch recently.  Should that
> logic apply also to this part of the codebase?  IOW, shouldn't
> index_has_changes() take a pointer to istate (as opposed to a
> function that uses the implicit the_index that should be named as
> "cache_has_changes()" or something?)
>
> I tend to think this function as part of the larger read-cache.c
> family whose definitions are in cache.h and accompanied by macros
> that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we
> were to move it elsewhere, I'd keep the header part as-is and
> implementation to read-cache.c to keep it together with the family,
> but I do not see a huge issue with the current placement, either.

That's good point; the goal to lift assumptions on the_index should
probably also apply here.  I'll make the change.
(And it was actually Duy's patch that I was reviewing, but close
enough.)   I'll take a look at moving it to read-cache.c as well.

Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

[Elijah Newren]

On Tue, Jun 5, 2018 at 12:14 AM, Elijah Newren <newren@gmail.com> wrote:
> On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> `git merge-recursive` does a three-way merge between user-specified trees
>>> base, head, and remote.  Since the user is allowed to specify head, we can
>>> not necesarily assume that head == HEAD.
>>>
>>> We modify index_has_changes() to take an extra argument specifying the
>>> tree to compare the index to.  If NULL, it will compare to HEAD.  We then
>>> use this from merge-recursive to make sure we compare to the
>>> user-specified head.
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>
>>> I'm really unsure where the index_has_changes() declaration should go;
>>> I stuck it in tree.h, but is there a better spot?
>>
>> I think I saw you tried to lift an assumption that we're always
>> working on the_index in a separate patch recently.  Should that
>> logic apply also to this part of the codebase?  IOW, shouldn't
>> index_has_changes() take a pointer to istate (as opposed to a
>> function that uses the implicit the_index that should be named as
>> "cache_has_changes()" or something?)
>>
>> I tend to think this function as part of the larger read-cache.c
>> family whose definitions are in cache.h and accompanied by macros
>> that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we
>> were to move it elsewhere, I'd keep the header part as-is and
>> implementation to read-cache.c to keep it together with the family,
>> but I do not see a huge issue with the current placement, either.
>
> That's good point; the goal to lift assumptions on the_index should
> probably also apply here.  I'll make the change.
> (And it was actually Duy's patch that I was reviewing, but close
> enough.)   I'll take a look at moving it to read-cache.c as well.

Making it not depend on the_index will require changes to make
diff-lib.c not depend on the_index first, so this is going to have to
wait for Duy's changes mentioned at
https://public-inbox.org/git/CACsJy8Ba74iSPf4_zFxuV=_uNJgL6Z2QunOvAvi3qab-6EWi5g@mail.gmail.com/.
I'll re-roll this series on top of Duy's when it comes out.

[RFC PATCH 5/7] t6044: add more testcases with staged changes before a merge is invoked

[Elijah Newren]

According to Documentation/git-merge.txt,

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed index
    entries are in the state that would result from the merge already.)

Add some tests showing that this exception, while it does accurately state
what would be a safe condition under which we could allow the merge to
proceed, is not what is actually implemented.

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

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 1d43712c52..e609f14f87 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -137,6 +137,35 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	test_i18ngrep "Already up to date" out
 '
 
+test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 10 >subdir/a &&
+	git add subdir/a &&
+
+	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
+	# Since subdir/a matches neither HEAD nor what the merge would write
+	# to that file, the merge should fail to avoid overwriting what is
+	# currently found in subdir/a
+	test_must_fail git merge -s recursive E^0
+'
+
+test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 11 >subdir/a &&
+	git add subdir/a &&
+
+	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
+	# Since subdir/a matches what the merge would write to that file,
+	# the merge should be safe to proceed
+	git merge -s recursive E^0
+'
+
 test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
-- 
2.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 6/7] merge-recursive: enforce rule that index matches head before merging

[Elijah Newren]

builtin/merge.c says that when we are about to perform a merge:

    ...the index must be in sync with the head commit.  The strategies are
    responsible to ensure this.

merge-recursive has always relied on unpack_trees() to enforce this
requirement, except in the case of an "Already up to date!" merge.
unpack-trees.c does not actually enforce this requirement, though.  It
allows for a pair of exceptions, in cases which it refers to as #14(ALT)
and #2ALT.  Documentation/technical/trivial-merge.txt can be consulted for
the precise meanings of the various case numbers and their meanings for
unpack-trees.c, but we have a high-level description of the intent behind
these two exceptions in a combined and summarized form in
Documentation/git-merge.txt:

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed index
    entries are in the state that would result from the merge already.)

While this high-level description does describe conditions under which it
would be safe to allow the index to diverge from HEAD, it does not match
what is actually implemented.  In particular, unpack-trees.c has no
knowledge of renames, and these two exceptions were written assuming that
no renames take place.  Once renames get into the mix, it is no longer
safe to allow the index to not match for #2ALT.  We could modify
unpack-trees to only allow #14(ALT) as an exception, but that would be
more strict than required for the resolve strategy (since the resolve
strategy doesn't handle renames at all).  Therefore, unpack_trees.c seems
like the wrong place to fix this.

Further, if someone fixes the combination of break and rename detection
and modifies merge-recursive to take advantage of the combination, then it
will also no longer be safe to allow the index to not match for #14(ALT)
when the recursive strategy is in use.  Therefore, leaving one of the
exceptions in place with the recursive merge strategy feels like we are
just leaving a latent bug in the code for folks in the future to stumble
across.

It may be possible to fix both unpack-trees and merge-recursive in a way
that implements the exception as stated in Documentation/git-merge.txt,
but it would be somewhat complex, possibly also buggy at first, and
ultimately, not all that valuable.  Instead, just enforce the requirement
stated in builtin/merge.c; error out if the index does not match the HEAD
commit, just like the 'ours' and 'octopus' strategies do.

Some testcase fixups were in order:
  t6044: We no longer expect stray staged changes to sometimes result
         in the merge continuing.  Also, fixes a case where a merge
         didn't abort but should have.
  t7504: had a few tests that had stray staged changes that were not
         actually part of the test under consideration
  t7611: had many tests designed to show that `git merge --abort` could
	 not always restore the index and working tree to the state they
	 were in before the merge started.  The tests that were associated
	 with having changes in the index before the merge started are no
         longer applicable, so they have been removed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                        |  14 +--
 t/t6044-merge-unrelated-index-changes.sh |  19 ++--
 t/t7504-commit-msg-hook.sh               |   4 +-
 t/t7611-merge-abort.sh                   | 118 -----------------------
 4 files changed, 18 insertions(+), 137 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 762aa087d0..4640b47a19 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3254,6 +3254,13 @@ int merge_trees(struct merge_options *o,
 		struct tree **result)
 {
 	int code, clean;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!o->call_depth && index_has_changes(&sb, head)) {
+		err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		    sb.buf);
+		return -1;
+	}
 
 	if (o->subtree_shift) {
 		merge = shift_tree_object(head, merge, o->subtree_shift);
@@ -3261,13 +3268,6 @@ int merge_trees(struct merge_options *o,
 	}
 
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
-		struct strbuf sb = STRBUF_INIT;
-
-		if (!o->call_depth && index_has_changes(&sb, head)) {
-			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
-			    sb.buf);
-			return -1;
-		}
 		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 e609f14f87..6e0ecab9c0 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -137,7 +137,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	test_i18ngrep "Already up to date" out
 '
 
-test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
+test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -145,14 +145,12 @@ test_expect_failure 'recursive, when file has staged changes not matching HEAD n
 	test_seq 1 10 >subdir/a &&
 	git add subdir/a &&
 
-	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
-	# Since subdir/a matches neither HEAD nor what the merge would write
-	# to that file, the merge should fail to avoid overwriting what is
-	# currently found in subdir/a
-	test_must_fail git merge -s recursive E^0
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
 '
 
-test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '
+test_expect_success 'recursive, when file has staged changes matching what a merge would give' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -160,10 +158,9 @@ test_expect_failure 'recursive, when file has staged changes matching what a mer
 	test_seq 1 11 >subdir/a &&
 	git add subdir/a &&
 
-	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
-	# Since subdir/a matches what the merge would write to that file,
-	# the merge should be safe to proceed
-	git merge -s recursive E^0
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
 '
 
 test_expect_success 'octopus, unrelated file touched' '
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 302a3a2082..31b9c6a2c1 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&
@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
-	echo "content" >> file &&
+	echo "content" >file &&
 	git add file &&
 	git commit -m "content"
 
@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&
diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index 7b4798e8e4..7c84a518aa 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -187,31 +187,6 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '
 	test_cmp expect actual
 '
 
-test_expect_success 'Abort clean merge with matching dirty index' '
-	git add bar &&
-	git diff --staged > expect &&
-	git merge --no-commit clean_branch &&
-	test -f .git/MERGE_HEAD &&
-	### When aborting the merge, git will discard all staged changes,
-	### including those that were staged pre-merge. In other words,
-	### --abort will LOSE any staged changes (the staged changes that
-	### are lost must match the merge result, or the merge would not
-	### have been allowed to start). Change expectations accordingly:
-	rm expect &&
-	touch expect &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff --staged > actual &&
-	test_cmp expect actual &&
-	test -z "$(git diff)"
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
 test_expect_success 'Fail conflicting merge with matching dirty worktree' '
 	echo barf > bar &&
 	git diff > expect &&
@@ -223,97 +198,4 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '
 	test_cmp expect actual
 '
 
-test_expect_success 'Abort conflicting merge with matching dirty index' '
-	git add bar &&
-	git diff --staged > expect &&
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	### When aborting the merge, git will discard all staged changes,
-	### including those that were staged pre-merge. In other words,
-	### --abort will LOSE any staged changes (the staged changes that
-	### are lost must match the merge result, or the merge would not
-	### have been allowed to start). Change expectations accordingly:
-	rm expect &&
-	touch expect &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff --staged > actual &&
-	test_cmp expect actual &&
-	test -z "$(git diff)"
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
-test_expect_success 'Abort merge with pre- and post-merge worktree changes' '
-	# Pre-merge worktree changes
-	echo xyzzy > foo &&
-	echo barf > bar &&
-	git add bar &&
-	git diff > expect &&
-	git diff --staged > expect-staged &&
-	# Perform merge
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	# Post-merge worktree changes
-	echo yzxxz > foo &&
-	echo blech > baz &&
-	### When aborting the merge, git will discard staged changes (bar)
-	### and unmerged changes (baz). Other changes that are neither
-	### staged nor marked as unmerged (foo), will be preserved. For
-	### these changed, git cannot tell pre-merge changes apart from
-	### post-merge changes, so the post-merge changes will be
-	### preserved. Change expectations accordingly:
-	git diff -- foo > expect &&
-	rm expect-staged &&
-	touch expect-staged &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff > actual &&
-	test_cmp expect actual &&
-	git diff --staged > actual-staged &&
-	test_cmp expect-staged actual-staged
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
-test_expect_success 'Abort merge with pre- and post-merge index changes' '
-	# Pre-merge worktree changes
-	echo xyzzy > foo &&
-	echo barf > bar &&
-	git add bar &&
-	git diff > expect &&
-	git diff --staged > expect-staged &&
-	# Perform merge
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	# Post-merge worktree changes
-	echo yzxxz > foo &&
-	echo blech > baz &&
-	git add foo bar &&
-	### When aborting the merge, git will discard all staged changes
-	### (foo, bar and baz), and no changes will be preserved. Whether
-	### the changes were staged pre- or post-merge does not matter
-	### (except for not preventing starting the merge).
-	### Change expectations accordingly:
-	rm expect expect-staged &&
-	touch expect &&
-	touch expect-staged &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff > actual &&
-	test_cmp expect actual &&
-	git diff --staged > actual-staged &&
-	test_cmp expect-staged actual-staged
-'
-
 test_done
-- 
2.18.0.rc0.49.g3c08dc0fef

[RFC PATCH 7/7] merge: fix misleading pre-merge check documentation

[Elijah Newren]

builtin/merge.c contains this important requirement for merge strategies:

    ...the index must be in sync with the head commit.  The strategies are
    responsible to ensure this.

However, Documentation/git-merge.txt says:

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed
    index entries are in the state that would result from the merge
    already.)

Interestingly, prior to commit c0be8aa06b85 ("Documentation/git-merge.txt:
Partial rewrite of How Merge Works", 2008-07-19),
Documentation/git-merge.txt said much more:

    ...the index file must match the tree of `HEAD` commit...
    [NOTE]
    This is a bit of a lite.  In certain special cases [explained
    in detail]...
    Otherwise, merge will refuse to do any harm to your repository
    (that is...your working tree...and index are left intact).

So, this suggests that the exceptions existed because there were special
cases where it would case no harm, and potentially be slightly more
convenient for the user.  While the current text in git-merge.txt does
list a condition under which it would be safe to proceed despite the index
not matching HEAD, it does not match what is actually implemented, in
three different ways:

    * The exception is written to describe what unpack-trees allows.  Not
      all merge strategies allow such an exception, though, making this
      description misleading.  'ours' and 'octopus' merges have strictly
      enforced index==HEAD for a while, and the commit previous to this
      one made 'recursive' do so as well.

    * If someone did a three-way content merge on a specific file using
      versions from the relevant commits and staged it prior to running
      merge, then that path would technically satisfy the exception listed
      in git-merge.txt.  unpack-trees.c would still error out on the path,
      though, because it defers the three-way content merge logic to other
      parts of the code (resolve, octopus, or recursive) and has no way of
      checking whether the index entry from before the merge will match
      the end result of the merge.

    * The exception as implemented in unpack-trees actually only checked
      that the index matched the MERGE_HEAD version of the file and that
      HEAD matched the merge base.  Assuming no renames, that would indeed
      provide cases where the index matches the end result we'd get from a
      merge.  But renames means unpack-trees is checking that it instead
      matches something other than what the final result will be, risking
      either erroring out when we shouldn't need to, or not erroring out
      when we should and overwriting the user's staged changes.

In addition to the wording behind this exception being misleading, it is
also somewhat surprising to see how many times the code for the special
cases were wrong or the check to make sure the index matched head was
forgotten altogether:

* Prior to commit ee6566e8d70d ("[PATCH] Rewrite read-tree", 2005-09-05),
  there were many cases where an unclean index entry was allowed (look for
  merged_entry_allow_dirty()); it appears that in those cases, the merge
  would have simply overwritten staged changes with the result of the
  merge.  Thus, the merge result would have been correct, but the user's
  uncommitted changes could be thrown away without warning.

* Prior to commit 160252f81626 ("git-merge-ours: make sure our index
  matches HEAD", 2005-11-03), the 'ours' merge strategy did not check
  whether the index matched HEAD.  If it didn't, the resulting merge
  would include all the staged changes, and thus wasn't really an 'ours'
  strategy.

* Prior to commit 3ec62ad9ffba ("merge-octopus: abort if index does not
  match HEAD", 2016-04-09), 'octopus' merges did not check whether the
  index matched HEAD, also resulting in any staged changes from before
  the commit silently being folded into the resulting merge.  commit
  a6ee883b8eb5 ("t6044: new merge testcases for when index doesn't match
  HEAD", 2016-04-09) was also added at the same time to try to test to
  make sure all strategies did the necessary checking for the requirement
  that the index match HEAD.  Sadly, it didn't catch all the cases, as
  evidenced by the remainder of this list...

* Prior to commit 65170c07d466 ("merge-recursive: avoid incorporating
  uncommitted changes in a merge", 2017-12-21), merge-recursive simply
  relied on unpack_trees() to do the necessary check, but in one special
  case it avoided calling unpack_trees() entirely and accidentally ended
  up silently including any staged changes from before the merge in the
  resulting merge commit.

* The commit immediately before this one in this series noted that the
  exceptions were written in a way that assumed no renames, making it
  unsafe for merge-recursive to use.  merge-recursive was modified to
  use its own check to enforce that index==HEAD.

This history makes it very tempting to go into builtin/merge.c and replace
the comment that strategies must enforce that index matches HEAD with code
that just enforces it.  At this point, that would only affect the
'resolve' strategy; all other strategies have each been modified to
manually enforce it.  (However, note that index==HEAD is not strictly
enforced for fast-forward merges, as those are not considered a merge
strategy and they trigger in builtin/merge.c before the section in the
code where the relevant comment is found.)

But, even if we don't take the step of just fixing these problems by
enforcing index==HEAD for all strategies, we at least need to update this
misleading documentation in git-merge.txt.  For now, just modify the claim
in Documentation/git-merge.txt to fix the error.  The precise details
around combination of merges strategies and special cases probably is not
relevant to most users, so simply state that exceptions may exist but are
narrow and vary depending upon which merge strategy is in use.

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

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index d5dfd8430f..141bd72284 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -122,9 +122,9 @@ merge' may need to update.
 
 To avoid recording unrelated changes in the merge commit,
 'git pull' and 'git merge' will also abort if there are any changes
-registered in the index relative to the `HEAD` commit.  (One
-exception is when the changed index entries are in the state that
-would result from the merge already.)
+registered in the index relative to the `HEAD` commit.  (Special
+narrow exceptions to this rule may exist depending on which merge
+strategy is in use, but generally, the index must match HEAD.)
 
 If all named commits are already ancestors of `HEAD`, 'git merge'
 will exit early with the message "Already up to date."
-- 
2.18.0.rc0.49.g3c08dc0fef

Re: [RFC PATCH 7/7] merge: fix misleading pre-merge check documentation

[Elijah Newren]

On Sat, Jun 2, 2018 at 11:58 PM, Elijah Newren <newren@gmail.com> wrote:
> builtin/merge.c contains this important requirement for merge strategies:
>
>     ...the index must be in sync with the head commit.  The strategies are
>     responsible to ensure this.
>
> However, Documentation/git-merge.txt says:
>
>     ...[merge will] abort if there are any changes registered in the index
>     relative to the `HEAD` commit.  (One exception is when the changed
>     index entries are in the state that would result from the merge
>     already.)
>
> Interestingly, prior to commit c0be8aa06b85 ("Documentation/git-merge.txt:
> Partial rewrite of How Merge Works", 2008-07-19),
> Documentation/git-merge.txt said much more:
>
>     ...the index file must match the tree of `HEAD` commit...
>     [NOTE]
>     This is a bit of a lite.  In certain special cases [explained
>     in detail]...
>     Otherwise, merge will refuse to do any harm to your repository
>     (that is...your working tree...and index are left intact).
>
> So, this suggests that the exceptions existed because there were special
> cases where it would case no harm, and potentially be slightly more
> convenient for the user.  While the current text in git-merge.txt does
> list a condition under which it would be safe to proceed despite the index
> not matching HEAD, it does not match what is actually implemented, in
> three different ways:
>
>     * The exception is written to describe what unpack-trees allows.  Not
>       all merge strategies allow such an exception, though, making this
>       description misleading.  'ours' and 'octopus' merges have strictly
>       enforced index==HEAD for a while, and the commit previous to this
>       one made 'recursive' do so as well.
>
>     * If someone did a three-way content merge on a specific file using
>       versions from the relevant commits and staged it prior to running
>       merge, then that path would technically satisfy the exception listed
>       in git-merge.txt.  unpack-trees.c would still error out on the path,
>       though, because it defers the three-way content merge logic to other
>       parts of the code (resolve, octopus, or recursive) and has no way of
>       checking whether the index entry from before the merge will match
>       the end result of the merge.
>
>     * The exception as implemented in unpack-trees actually only checked
>       that the index matched the MERGE_HEAD version of the file and that
>       HEAD matched the merge base.  Assuming no renames, that would indeed
>       provide cases where the index matches the end result we'd get from a
>       merge.  But renames means unpack-trees is checking that it instead
>       matches something other than what the final result will be, risking
>       either erroring out when we shouldn't need to, or not erroring out
>       when we should and overwriting the user's staged changes.
>
> In addition to the wording behind this exception being misleading, it is
> also somewhat surprising to see how many times the code for the special
> cases were wrong or the check to make sure the index matched head was
> forgotten altogether:
>
> * Prior to commit ee6566e8d70d ("[PATCH] Rewrite read-tree", 2005-09-05),
>   there were many cases where an unclean index entry was allowed (look for
>   merged_entry_allow_dirty()); it appears that in those cases, the merge
>   would have simply overwritten staged changes with the result of the
>   merge.  Thus, the merge result would have been correct, but the user's
>   uncommitted changes could be thrown away without warning.
>
> * Prior to commit 160252f81626 ("git-merge-ours: make sure our index
>   matches HEAD", 2005-11-03), the 'ours' merge strategy did not check
>   whether the index matched HEAD.  If it didn't, the resulting merge
>   would include all the staged changes, and thus wasn't really an 'ours'
>   strategy.
>
> * Prior to commit 3ec62ad9ffba ("merge-octopus: abort if index does not
>   match HEAD", 2016-04-09), 'octopus' merges did not check whether the
>   index matched HEAD, also resulting in any staged changes from before
>   the commit silently being folded into the resulting merge.  commit
>   a6ee883b8eb5 ("t6044: new merge testcases for when index doesn't match
>   HEAD", 2016-04-09) was also added at the same time to try to test to
>   make sure all strategies did the necessary checking for the requirement
>   that the index match HEAD.  Sadly, it didn't catch all the cases, as
>   evidenced by the remainder of this list...
>
> * Prior to commit 65170c07d466 ("merge-recursive: avoid incorporating
>   uncommitted changes in a merge", 2017-12-21), merge-recursive simply
>   relied on unpack_trees() to do the necessary check, but in one special
>   case it avoided calling unpack_trees() entirely and accidentally ended
>   up silently including any staged changes from before the merge in the
>   resulting merge commit.
>
> * The commit immediately before this one in this series noted that the
>   exceptions were written in a way that assumed no renames, making it
>   unsafe for merge-recursive to use.  merge-recursive was modified to
>   use its own check to enforce that index==HEAD.
>
> This history makes it very tempting to go into builtin/merge.c and replace
> the comment that strategies must enforce that index matches HEAD with code
> that just enforces it.  At this point, that would only affect the
> 'resolve' strategy; all other strategies have each been modified to
> manually enforce it.

I'm curious if anyone has comments on this last paragraph above.
Would anyone object to strictly enforcing index matches HEAD before
all types of merges?

[PATCH v2 0/9] Fix merge issues with index not matching HEAD

[Elijah Newren]

This series exists to fix problems for merges when the index doesn't
match HEAD.  We've had an almost comical number of these types of
problems in our history, as thoroughly documented in the commit
message for the final patch.

v1 can be found here:
  https://public-inbox.org/git/20180603065810.23841-1-newren@gmail.com/

Changes since v1 (full branch-diff below):
  * Minor wording tweaks to a few commit messages (fixing typos,
    rewrapping, etc.)
    
  * Move index_has_changes() to read-cache.c, and (partially) lift the
    assumption that we're always operating on the_index -- as
    suggested by Junio.  A full lift of the assumption would conflict
    with and duplicate work Duy is doing, so there is a simple BUG()
    check in place for now.
    
  * Add two new patches to the _beginning_ of the series, to implement
    the last point.  Reason: Since this series will probably conflict
    slightly with Duy's not-yet-submitted work to remove assumption of
    the_index throughout the codebase, I figured this would make it
    the clearest and easiest for him to fix up small conflicts.
    (Alternatively, if folks would rather that my series wait for his
    to go through, it should make it much easier for me to rebase my
    series on top of his work by having these placeholders at the
    beginning.)

Questions I'm particularly interested in reviewers addressing:

  * Do my two patches at the beginning make sense?  In particular, is
    the BUG() a reasonable way to limit conflicts with Duy for now,
    while he works on more thoroughly stamping out assumed the_index
    usage?

  * Does the series flow well?  I was curious if I should reorder the
    series when I submitted v1, but no one commented on that question.

  * The second to last patch points out that current git incorrectly
    implements what would be a safe exception to the index matching
    HEAD before a merge rule.  Would it be more desireable to
    correctly implement the safe exception (even though it would be
    somewhat difficult), rather than just disallow the exception for
    merge-recursive as I did in that patch?

  * Given the large number of problems we've had in this area -- as
    documented in the final commit message -- should we be more
    defensive and disallow all merge strategies from having even
    so-called safe exceptions?  We could do this in a single place,
    which would have prevented all the current and most if not all
    historical problems in this area, by just enforcing that the index
    match HEAD in builtin/merge.c.  (See the second to last paragraph
    of the last commit message for more details.)

Elijah Newren (9):
  read-cache.c: move index_has_changes() from merge.c
  index_has_changes(): avoid assuming operating on the_index
  t6044: verify that merges expected to abort actually abort
  t6044: add a testcase for index matching head, when head doesn't match
    HEAD
  merge-recursive: make sure when we say we abort that we actually abort
  merge-recursive: fix assumption that head tree being merged is HEAD
  t6044: add more testcases with staged changes before a merge is
    invoked
  merge-recursive: enforce rule that index matches head before merging
  merge: fix misleading pre-merge check documentation

 Documentation/git-merge.txt              |   6 +-
 builtin/am.c                             |   7 +-
 cache.h                                  |  16 +--
 merge-recursive.c                        |  14 +--
 merge.c                                  |  31 ------
 read-cache.c                             |  40 ++++++++
 t/t6044-merge-unrelated-index-changes.sh |  67 +++++++++++--
 t/t7504-commit-msg-hook.sh               |   4 +-
 t/t7611-merge-abort.sh                   | 118 -----------------------
 9 files changed, 124 insertions(+), 179 deletions(-)

-:  --------- > 1:  ff2501ac4 read-cache.c: move index_has_changes() from merge.c
-:  --------- > 2:  5813ca722 index_has_changes(): avoid assuming operating on the_index
1:  730e5e483 = 3:  ca11503bd t6044: verify that merges expected to abort actually abort
2:  36f7cc0a3 = 4:  386390899 t6044: add a testcase for index matching head, when head doesn't match HEAD
3:  70899afa3 ! 5:  8a900d2ee merge-recursive: make sure when we say we abort that we actually abort
    @@ -19,7 +19,7 @@
     @@
      		struct strbuf sb = STRBUF_INIT;
      
    - 		if (!o->call_depth && index_has_changes(&sb)) {
    + 		if (!o->call_depth && index_has_changes(&the_index, &sb)) {
     -			err(o, _("Dirty index: cannot merge (dirty: %s)"),
     +			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
      			    sb.buf);
4:  eab2f36a4 ! 6:  2564b29e9 merge-recursive: fix assumption that head tree being merged is HEAD
    @@ -6,10 +6,9 @@
         base, head, and remote.  Since the user is allowed to specify head, we can
         not necesarily assume that head == HEAD.
     
    -    We modify index_has_changes() to take an extra argument specifying the
    -    tree to compare the index to.  If NULL, it will compare to HEAD.  We then
    -    use this from merge-recursive to make sure we compare to the
    -    user-specified head.
    +    Modify index_has_changes() to take an extra argument specifying the tree
    +    to compare against.  If NULL, it will compare to HEAD.  We then use this
    +    from merge-recursive to make sure we compare to the user-specified head.
     
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
    @@ -20,8 +19,8 @@
      
      	refresh_and_write_cache();
      
    --	if (index_has_changes(&sb)) {
    -+	if (index_has_changes(&sb, NULL)) {
    +-	if (index_has_changes(&the_index, &sb)) {
    ++	if (index_has_changes(&the_index, NULL, &sb)) {
      		write_state_bool(state, "dirtyindex", 1);
      		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
      	}
    @@ -29,8 +28,9 @@
      			 * Applying the patch to an earlier tree and merging
      			 * the result may have produced the same tree as ours.
      			 */
    --			if (!apply_status && !index_has_changes(NULL)) {
    -+			if (!apply_status && !index_has_changes(NULL, NULL)) {
    +-			if (!apply_status && !index_has_changes(&the_index, NULL)) {
    ++			if (!apply_status &&
    ++			    !index_has_changes(&the_index, NULL, NULL)) {
      				say(state, stdout, _("No changes -- Patch already applied."));
      				goto next;
      			}
    @@ -38,8 +38,8 @@
      
      	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
      
    --	if (!index_has_changes(NULL)) {
    -+	if (!index_has_changes(NULL, NULL)) {
    +-	if (!index_has_changes(&the_index, NULL)) {
    ++	if (!index_has_changes(&the_index, NULL, NULL)) {
      		printf_ln(_("No changes - did you forget to use 'git add'?\n"
      			"If there is nothing left to stage, chances are that something else\n"
      			"already introduced the same changes; you might want to skip this patch."));
    @@ -48,20 +48,32 @@
     --- a/cache.h
     +++ b/cache.h
     @@
    - extern void move_index_extensions(struct index_state *dst, struct index_state *src);
    + /* Forward structure decls */
    + struct pathspec;
    + struct child_process;
    ++struct tree;
    + 
    + /*
    +  * Copy the sha1 and stat state of a cache entry from one to
    +@@
      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);
    --
    + /**
    +- * Returns 1 if istate differs from HEAD, 0 otherwise.  When on an unborn
    +- * branch, returns 1 if there are entries in istate, 0 otherwise.  If an
    +- * strbuf is provided, the space-separated list of files that differ will
    +- * be appended to it.
    ++ * Returns 1 if istate differs from tree, 0 otherwise.  If tree is NULL,
    ++ * compares istate to HEAD.  If tree is NULL and on an unborn branch,
    ++ * returns 1 if there are entries in istate, 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(const struct index_state *istate,
    ++			     struct tree *tree,
    + 			     struct strbuf *sb);
    + 
      extern int verify_path(const char *path, unsigned mode);
    - 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-recursive.c b/merge-recursive.c
     --- a/merge-recursive.c
    @@ -70,26 +82,31 @@
      	if (oid_eq(&common->object.oid, &merge->object.oid)) {
      		struct strbuf sb = STRBUF_INIT;
      
    --		if (!o->call_depth && index_has_changes(&sb)) {
    -+		if (!o->call_depth && index_has_changes(&sb, head)) {
    +-		if (!o->call_depth && index_has_changes(&the_index, &sb)) {
    ++		if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
      			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
      			    sb.buf);
      			return -1;
     
    -diff --git a/merge.c b/merge.c
    ---- a/merge.c
    -+++ b/merge.c
    +diff --git a/read-cache.c b/read-cache.c
    +--- a/read-cache.c
    ++++ b/read-cache.c
     @@
    - 	return oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);
    + 	return 0;
      }
      
    --int index_has_changes(struct strbuf *sb)
    -+int index_has_changes(struct strbuf *sb, struct tree *tree)
    +-int index_has_changes(const struct index_state *istate, struct strbuf *sb)
    ++int index_has_changes(const struct index_state *istate,
    ++		      struct tree *tree,
    ++		      struct strbuf *sb)
      {
     -	struct object_id head;
     +	struct object_id cmp;
      	int i;
      
    + 	if (istate != &the_index) {
    + 		BUG("index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.");
    + 	}
     -	if (!get_oid_tree("HEAD", &head)) {
     +	if (tree)
     +		cmp = tree->object.oid;
    @@ -118,20 +135,3 @@
      	git reset --hard &&
      	git checkout C^0 &&
      
    -
    -diff --git a/tree.h b/tree.h
    ---- a/tree.h
    -+++ b/tree.h
    -@@
    - extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,
    - 		     struct index_state *istate);
    - 
    -+/**
    -+ * Returns 1 if the index differs from tree, 0 otherwise.  If tree is NULL,
    -+ * compares to HEAD.  If tree is NULL and 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, struct tree *tree);
    -+
    - #endif /* TREE_H */
5:  4aa0684c0 ! 7:  88a8e44a2 t6044: add more testcases with staged changes before a merge is invoked
    @@ -5,8 +5,9 @@
         According to Documentation/git-merge.txt,
     
             ...[merge will] abort if there are any changes registered in the index
    -        relative to the `HEAD` commit.  (One exception is when the changed index
    -        entries are in the state that would result from the merge already.)
    +        relative to the `HEAD` commit.  (One exception is when the changed
    +        index entries are in the state that would result from the merge
    +        already.)
     
         Add some tests showing that this exception, while it does accurately state
         what would be a safe condition under which we could allow the merge to
6:  905f2683f ! 8:  c0049b788 merge-recursive: enforce rule that index matches head before merging
    @@ -48,16 +48,16 @@
         commit, just like the 'ours' and 'octopus' strategies do.
     
         Some testcase fixups were in order:
    -      t6044: We no longer expect stray staged changes to sometimes result
    -             in the merge continuing.  Also, fixes a case where a merge
    -             didn't abort but should have.
    -      t7504: had a few tests that had stray staged changes that were not
    -             actually part of the test under consideration
           t7611: had many tests designed to show that `git merge --abort` could
                  not always restore the index and working tree to the state they
                  were in before the merge started.  The tests that were associated
                  with having changes in the index before the merge started are no
                  longer applicable, so they have been removed.
    +      t7504: had a few tests that had stray staged changes that were not
    +             actually part of the test under consideration
    +      t6044: We no longer expect stray staged changes to sometimes result
    +             in the merge continuing.  Also, fix a case where a merge
    +             didn't abort but should have.
     
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
    @@ -70,7 +70,7 @@
      	int code, clean;
     +	struct strbuf sb = STRBUF_INIT;
     +
    -+	if (!o->call_depth && index_has_changes(&sb, head)) {
    ++	if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
     +		err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
     +		    sb.buf);
     +		return -1;
    @@ -84,7 +84,7 @@
      	if (oid_eq(&common->object.oid, &merge->object.oid)) {
     -		struct strbuf sb = STRBUF_INIT;
     -
    --		if (!o->call_depth && index_has_changes(&sb, head)) {
    +-		if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
     -			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
     -			    sb.buf);
     -			return -1;
7:  69f6fe8b1 ! 9:  557c5d94c merge: fix misleading pre-merge check documentation
    @@ -20,7 +20,7 @@
     
             ...the index file must match the tree of `HEAD` commit...
             [NOTE]
    -        This is a bit of a lite.  In certain special cases [explained
    +        This is a bit of a lie.  In certain special cases [explained
             in detail]...
             Otherwise, merge will refuse to do any harm to your repository
             (that is...your working tree...and index are left intact).

-- 
2.18.0.137.g2a11d05a6.dirty

[PATCH v2 1/9] read-cache.c: move index_has_changes() from merge.c

[Elijah Newren]

Since index_has_change() is an index-related function, move it to
read-cache.c, only modifying it to avoid uses of the active_cache and
active_nr macros.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge.c      | 31 -------------------------------
 read-cache.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/merge.c b/merge.c
index 078385873..e30e03fb8 100644
--- a/merge.c
+++ b/merge.c
@@ -14,37 +14,6 @@ static const char *merge_argument(struct commit *commit)
 	return oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);
 }
 
-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)
diff --git a/read-cache.c b/read-cache.c
index 372588260..5a9a93da3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -6,6 +6,8 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "config.h"
+#include "diff.h"
+#include "diffcore.h"
 #include "tempfile.h"
 #include "lockfile.h"
 #include "cache-tree.h"
@@ -1984,6 +1986,37 @@ int unmerged_index(const struct index_state *istate)
 	return 0;
 }
 
+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 < the_index.cache_nr; i++) {
+			if (i)
+				strbuf_addch(sb, ' ');
+			strbuf_addstr(sb, the_index.cache[i]->name);
+		}
+		return !!the_index.cache_nr;
+	}
+}
+
 #define WRITE_BUFFER_SIZE 8192
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;
-- 
2.18.0.137.g2a11d05a6.dirty

[PATCH v2 2/9] index_has_changes(): avoid assuming operating on the_index

[Elijah Newren]

Modify index_has_changes() to take a struct istate* instead of just
operating on the_index.  This is only a partial conversion, though,
because we call do_diff_cache() which implicitly assumes work is to be
done on the_index.  Ongoing work is being done elsewhere to do the
remainder of the conversion, and thus is not duplicated here.  Instead,
a simple check is put in place until that work is complete.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/am.c      |  6 +++---
 cache.h           | 11 ++++++-----
 merge-recursive.c |  2 +-
 read-cache.c      | 11 +++++++----
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6273ea519..24ad3e435 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)
 
 	refresh_and_write_cache();
 
-	if (index_has_changes(&sb)) {
+	if (index_has_changes(&the_index, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
@@ -1820,7 +1820,7 @@ static void am_run(struct am_state *state, int resume)
 			 * Applying the patch to an earlier tree and merging
 			 * the result may have produced the same tree as ours.
 			 */
-			if (!apply_status && !index_has_changes(NULL)) {
+			if (!apply_status && !index_has_changes(&the_index, NULL)) {
 				say(state, stdout, _("No changes -- Patch already applied."));
 				goto next;
 			}
@@ -1874,7 +1874,7 @@ static void am_resolve(struct am_state *state)
 
 	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
 
-	if (!index_has_changes(NULL)) {
+	if (!index_has_changes(&the_index, NULL)) {
 		printf_ln(_("No changes - did you forget to use 'git add'?\n"
 			"If there is nothing left to stage, chances are that something else\n"
 			"already introduced the same changes; you might want to skip this patch."));
diff --git a/cache.h b/cache.h
index d49092d94..125f2767a 100644
--- a/cache.h
+++ b/cache.h
@@ -635,12 +635,13 @@ extern void move_index_extensions(struct index_state *dst, struct index_state *s
 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.
+ * Returns 1 if istate differs from HEAD, 0 otherwise.  When on an unborn
+ * branch, returns 1 if there are entries in istate, 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 index_has_changes(const struct index_state *istate,
+			     struct strbuf *sb);
 
 extern int verify_path(const char *path, unsigned mode);
 extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);
diff --git a/merge-recursive.c b/merge-recursive.c
index bed4a5be0..f9384fabf 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3266,7 +3266,7 @@ int merge_trees(struct merge_options *o,
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
 		struct strbuf sb = STRBUF_INIT;
 
-		if (!o->call_depth && index_has_changes(&sb)) {
+		if (!o->call_depth && index_has_changes(&the_index, &sb)) {
 			err(o, _("Dirty index: cannot merge (dirty: %s)"),
 			    sb.buf);
 			return 0;
diff --git a/read-cache.c b/read-cache.c
index 5a9a93da3..ad3102083 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1986,11 +1986,14 @@ int unmerged_index(const struct index_state *istate)
 	return 0;
 }
 
-int index_has_changes(struct strbuf *sb)
+int index_has_changes(const struct index_state *istate, struct strbuf *sb)
 {
 	struct object_id head;
 	int i;
 
+	if (istate != &the_index) {
+		BUG("index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.");
+	}
 	if (!get_oid_tree("HEAD", &head)) {
 		struct diff_options opt;
 
@@ -2008,12 +2011,12 @@ int index_has_changes(struct strbuf *sb)
 		diff_flush(&opt);
 		return opt.flags.has_changes != 0;
 	} else {
-		for (i = 0; sb && i < the_index.cache_nr; i++) {
+		for (i = 0; sb && i < istate->cache_nr; i++) {
 			if (i)
 				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, the_index.cache[i]->name);
+			strbuf_addstr(sb, istate->cache[i]->name);
 		}
-		return !!the_index.cache_nr;
+		return !!istate->cache_nr;
 	}
 }
 
-- 
2.18.0.137.g2a11d05a6.dirty

Re: [PATCH v2 2/9] index_has_changes(): avoid assuming operating on the_index

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Modify index_has_changes() to take a struct istate* instead of just
> operating on the_index.  This is only a partial conversion, though,
> because we call do_diff_cache() which implicitly assumes work is to be
> done on the_index.  Ongoing work is being done elsewhere to do the
> remainder of the conversion, and thus is not duplicated here.  Instead,
> a simple check is put in place until that work is complete.

Yeah, that is an unfortunate but necessary compromise until we
create do_diff_index() that can take an istate, and optionally turn
do_diff_cache() into

    #define do_diff_cache(...) do_diff_index(&the_index, ...)

if there are many callers that want to work on the default in-core
istate.

[PATCH v2 3/9] t6044: verify that merges expected to abort actually abort

[Elijah Newren]

t6044 has lots of tests for verifying that merge will abort as expected
when there are changes staged before the merge starts.  However, it only
checked for non-zero exit code, which could mean that the merge ran to
completion with conflicts.  Check that the merge was actually correctly
aborted, i.e. that .git/MERGE_HEAD is not present.

This changes one of the tests from expect_success to expect_failure.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6044-merge-unrelated-index-changes.sh | 32 ++++++++++++++++--------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 23b86fb97..f9c2f8179 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -82,7 +82,8 @@ test_expect_success 'ff update, important file modified' '
 	touch subdir/e &&
 	git add subdir/e &&
 
-	test_must_fail git merge E^0
+	test_must_fail git merge E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, trivial' '
@@ -91,7 +92,8 @@ test_expect_success 'resolve, trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve C^0
+	test_must_fail git merge -s resolve C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, non-trivial' '
@@ -100,7 +102,8 @@ test_expect_success 'resolve, non-trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve D^0
+	test_must_fail git merge -s resolve D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'recursive' '
@@ -109,16 +112,18 @@ test_expect_success 'recursive' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s recursive C^0
+	test_must_fail git merge -s recursive C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
-test_expect_success 'recursive, when merge branch matches merge base' '
+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_must_fail git merge -s recursive F^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, unrelated file touched' '
@@ -127,7 +132,8 @@ test_expect_success 'octopus, unrelated file touched' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file removed' '
@@ -136,7 +142,8 @@ test_expect_success 'octopus, related file removed' '
 
 	git rm b &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file modified' '
@@ -145,7 +152,8 @@ test_expect_success 'octopus, related file modified' '
 
 	echo 12 >>a && git add a &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'ours' '
@@ -154,7 +162,8 @@ test_expect_success 'ours' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s ours C^0
+	test_must_fail git merge -s ours C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'subtree' '
@@ -163,7 +172,8 @@ test_expect_success 'subtree' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s subtree E^0
+	test_must_fail git merge -s subtree E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_done
-- 
2.18.0.137.g2a11d05a6.dirty

[PATCH v2 4/9] t6044: add a testcase for index matching head, when head doesn't match HEAD

[Elijah Newren]

The `git merge-recursive` command allows the user to directly specify
three commits to merge -- base, head, and remote.  (More than three can be
specified in the case of multiple merge bases.)  Note that since the user
is allowed to specify head, it need not match HEAD.

Virtually every test and script in the current git.git codebase calls `git
merge-recursive` with head=HEAD, and likely external callers do as well,
which is why this has gone unnoticed.  There is one notable
counter-example: git-stash.sh.  However, git-stash called `git
merge-recursive` with an index that matches the expected merge result,
which happens to be a currently allowed exception to the "index must match
head" rule, so this never triggered an error previously.

Since we would like to tighten up the "index must match head" rule, we
need to make sure we are comparing to the correct head.  Add a testcase
that demonstrates the failure when we check the wrong HEAD.

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

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index f9c2f8179..92ec55255 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
+	git reset --hard &&
+	git checkout C^0 &&
+
+	# Make index match B
+	git diff C B | git apply --cached &&
+	# Merge B & F, with B as "head"
+	git merge-recursive A -- B F > out &&
+	test_i18ngrep "Already up to date" out
+'
+
 test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
-- 
2.18.0.137.g2a11d05a6.dirty

Re: [PATCH v2 4/9] t6044: add a testcase for index matching head, when head doesn't match HEAD

[SZEDER Gábor]

> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
> index f9c2f8179..92ec55255 100755
> --- a/t/t6044-merge-unrelated-index-changes.sh
> +++ b/t/t6044-merge-unrelated-index-changes.sh
> @@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '
>  	test_path_is_missing .git/MERGE_HEAD
>  '
>  
> +test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
> +	git reset --hard &&
> +	git checkout C^0 &&
> +
> +	# Make index match B
> +	git diff C B | git apply --cached &&

Branch 'C' contains the file 'c', which creates ambiguity on
case-insensitive file systems and makes the test fail:

  ++git reset --hard
  HEAD is now at ed5d5a6 B
  ++git checkout 'C^0'
  Previous HEAD position was ed5d5a6 B
  HEAD is now at 4fa59ce C
  ++git diff C B
  ++git apply --cached
  fatal: ambiguous argument 'C': both revision and filename
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'
  error: unrecognized input
  error: last command exited with $?=128
  not ok 8 - merge-recursive, when index==head but head!=HEAD

> +	# Merge B & F, with B as "head"
> +	git merge-recursive A -- B F > out &&
> +	test_i18ngrep "Already up to date" out
> +'
> +
>  test_expect_success 'octopus, unrelated file touched' '
>  	git reset --hard &&
>  	git checkout B^0 &&
> -- 
> 2.18.0.137.g2a11d05a6.dirty
> 
> 

Re: [RFC PATCH 2/7] t6044: add a testcase for index matching head, when head doesn't match HEAD

[Elijah Newren]

On Tue, Jul 10, 2018 at 1:39 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
>> index f9c2f8179..92ec55255 100755
>> --- a/t/t6044-merge-unrelated-index-changes.sh
>> +++ b/t/t6044-merge-unrelated-index-changes.sh
>> @@ -126,6 +126,17 @@ test_expect_failure 'recursive, when merge branch matches merge base' '
>>       test_path_is_missing .git/MERGE_HEAD
>>  '
>>
>> +test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
>> +     git reset --hard &&
>> +     git checkout C^0 &&
>> +
>> +     # Make index match B
>> +     git diff C B | git apply --cached &&
>
> Branch 'C' contains the file 'c', which creates ambiguity on
> case-insensitive file systems and makes the test fail:
>
>   ++git reset --hard
>   HEAD is now at ed5d5a6 B
>   ++git checkout 'C^0'
>   Previous HEAD position was ed5d5a6 B
>   HEAD is now at 4fa59ce C
>   ++git diff C B
>   ++git apply --cached
>   fatal: ambiguous argument 'C': both revision and filename
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
>   error: unrecognized input
>   error: last command exited with $?=128
>   not ok 8 - merge-recursive, when index==head but head!=HEAD
>

Good catch; thanks for pointing it out.  Here's a fix:

-- 8< --

Subject: [PATCH] fixup! t6044: add a testcase for index matching head, when
 head doesn't match HEAD

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6044-merge-unrelated-index-changes.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 6e0ecab9c..5e3779ebc 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -131,7 +131,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	git checkout C^0 &&
 
 	# Make index match B
-	git diff C B | git apply --cached &&
+	git diff C B -- | git apply --cached &&
 	# Merge B & F, with B as "head"
 	git merge-recursive A -- B F > out &&
 	test_i18ngrep "Already up to date" out
-- 
2.18.0.132.g6e63b23f4

[PATCH v2 5/9] merge-recursive: make sure when we say we abort that we actually abort

[Elijah Newren]

In commit 65170c07d4 ("merge-recursive: avoid incorporating uncommitted
changes in a merge", 2017-12-21), it was noted that there was a special
case when merge-recursive didn't rely on unpack_trees() to enforce the
index == HEAD requirement, and thus that it needed to do that enforcement
itself.  Unfortunately, it returned the wrong exit status, signalling that
the merge completed but had conflicts, rather than that it was aborted.
Fix the return code, and while we're at it, change the error message to
match what unpack_trees() would have printed.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index f9384fabf..9c0699be5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3267,9 +3267,9 @@ int merge_trees(struct merge_options *o,
 		struct strbuf sb = STRBUF_INIT;
 
 		if (!o->call_depth && index_has_changes(&the_index, &sb)) {
-			err(o, _("Dirty index: cannot merge (dirty: %s)"),
+			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 			    sb.buf);
-			return 0;
+			return -1;
 		}
 		output(o, 0, _("Already up to date!"));
 		*result = head;
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 92ec55255..3876cfa4f 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -116,7 +116,7 @@ test_expect_success 'recursive' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
-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.18.0.137.g2a11d05a6.dirty

[PATCH v2 6/9] merge-recursive: fix assumption that head tree being merged is HEAD

[Elijah Newren]

`git merge-recursive` does a three-way merge between user-specified trees
base, head, and remote.  Since the user is allowed to specify head, we can
not necesarily assume that head == HEAD.

Modify index_has_changes() to take an extra argument specifying the tree
to compare against.  If NULL, it will compare to HEAD.  We then use this
from merge-recursive to make sure we compare to the user-specified head.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/am.c                             |  7 ++++---
 cache.h                                  | 11 +++++++----
 merge-recursive.c                        |  2 +-
 read-cache.c                             | 12 ++++++++----
 t/t6044-merge-unrelated-index-changes.sh |  2 +-
 5 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 24ad3e435..cb8e2b960 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1763,7 +1763,7 @@ static void am_run(struct am_state *state, int resume)
 
 	refresh_and_write_cache();
 
-	if (index_has_changes(&the_index, &sb)) {
+	if (index_has_changes(&the_index, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
@@ -1820,7 +1820,8 @@ static void am_run(struct am_state *state, int resume)
 			 * Applying the patch to an earlier tree and merging
 			 * the result may have produced the same tree as ours.
 			 */
-			if (!apply_status && !index_has_changes(&the_index, NULL)) {
+			if (!apply_status &&
+			    !index_has_changes(&the_index, NULL, NULL)) {
 				say(state, stdout, _("No changes -- Patch already applied."));
 				goto next;
 			}
@@ -1874,7 +1875,7 @@ static void am_resolve(struct am_state *state)
 
 	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
 
-	if (!index_has_changes(&the_index, NULL)) {
+	if (!index_has_changes(&the_index, NULL, NULL)) {
 		printf_ln(_("No changes - did you forget to use 'git add'?\n"
 			"If there is nothing left to stage, chances are that something else\n"
 			"already introduced the same changes; you might want to skip this patch."));
diff --git a/cache.h b/cache.h
index 125f2767a..b5a75806e 100644
--- a/cache.h
+++ b/cache.h
@@ -218,6 +218,7 @@ struct cache_entry {
 /* Forward structure decls */
 struct pathspec;
 struct child_process;
+struct tree;
 
 /*
  * Copy the sha1 and stat state of a cache entry from one to
@@ -635,12 +636,14 @@ extern void move_index_extensions(struct index_state *dst, struct index_state *s
 extern int unmerged_index(const struct index_state *);
 
 /**
- * Returns 1 if istate differs from HEAD, 0 otherwise.  When on an unborn
- * branch, returns 1 if there are entries in istate, 0 otherwise.  If an
- * strbuf is provided, the space-separated list of files that differ will
- * be appended to it.
+ * Returns 1 if istate differs from tree, 0 otherwise.  If tree is NULL,
+ * compares istate to HEAD.  If tree is NULL and on an unborn branch,
+ * returns 1 if there are entries in istate, 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(const struct index_state *istate,
+			     struct tree *tree,
 			     struct strbuf *sb);
 
 extern int verify_path(const char *path, unsigned mode);
diff --git a/merge-recursive.c b/merge-recursive.c
index 9c0699be5..855ca39ca 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3266,7 +3266,7 @@ int merge_trees(struct merge_options *o,
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
 		struct strbuf sb = STRBUF_INIT;
 
-		if (!o->call_depth && index_has_changes(&the_index, &sb)) {
+		if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
 			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 			    sb.buf);
 			return -1;
diff --git a/read-cache.c b/read-cache.c
index ad3102083..acc8c390a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1986,22 +1986,26 @@ int unmerged_index(const struct index_state *istate)
 	return 0;
 }
 
-int index_has_changes(const struct index_state *istate, struct strbuf *sb)
+int index_has_changes(const struct index_state *istate,
+		      struct tree *tree,
+		      struct strbuf *sb)
 {
-	struct object_id head;
+	struct object_id cmp;
 	int i;
 
 	if (istate != &the_index) {
 		BUG("index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.");
 	}
-	if (!get_oid_tree("HEAD", &head)) {
+	if (tree)
+		cmp = tree->object.oid;
+	if (tree || !get_oid_tree("HEAD", &cmp)) {
 		struct diff_options opt;
 
 		diff_setup(&opt);
 		opt.flags.exit_with_status = 1;
 		if (!sb)
 			opt.flags.quick = 1;
-		do_diff_cache(&head, &opt);
+		do_diff_cache(&cmp, &opt);
 		diffcore_std(&opt);
 		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
 			if (i)
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 3876cfa4f..1d43712c5 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -126,7 +126,7 @@ test_expect_success 'recursive, when merge branch matches merge base' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
-test_expect_failure 'merge-recursive, when index==head but head!=HEAD' '
+test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	git reset --hard &&
 	git checkout C^0 &&
 
-- 
2.18.0.137.g2a11d05a6.dirty

Re: [PATCH v2 6/9] merge-recursive: fix assumption that head tree being merged is HEAD

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> `git merge-recursive` does a three-way merge between user-specified trees
> base, head, and remote.  Since the user is allowed to specify head, we can
> not necesarily assume that head == HEAD.

Makes sense.

[PATCH v2 7/9] t6044: add more testcases with staged changes before a merge is invoked

[Elijah Newren]

According to Documentation/git-merge.txt,

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed
    index entries are in the state that would result from the merge
    already.)

Add some tests showing that this exception, while it does accurately state
what would be a safe condition under which we could allow the merge to
proceed, is not what is actually implemented.

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

diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 1d43712c5..e609f14f8 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -137,6 +137,35 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	test_i18ngrep "Already up to date" out
 '
 
+test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 10 >subdir/a &&
+	git add subdir/a &&
+
+	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
+	# Since subdir/a matches neither HEAD nor what the merge would write
+	# to that file, the merge should fail to avoid overwriting what is
+	# currently found in subdir/a
+	test_must_fail git merge -s recursive E^0
+'
+
+test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 11 >subdir/a &&
+	git add subdir/a &&
+
+	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
+	# Since subdir/a matches what the merge would write to that file,
+	# the merge should be safe to proceed
+	git merge -s recursive E^0
+'
+
 test_expect_success 'octopus, unrelated file touched' '
 	git reset --hard &&
 	git checkout B^0 &&
-- 
2.18.0.137.g2a11d05a6.dirty

[PATCH v2 8/9] merge-recursive: enforce rule that index matches head before merging

[Elijah Newren]

builtin/merge.c says that when we are about to perform a merge:

    ...the index must be in sync with the head commit.  The strategies are
    responsible to ensure this.

merge-recursive has always relied on unpack_trees() to enforce this
requirement, except in the case of an "Already up to date!" merge.
unpack-trees.c does not actually enforce this requirement, though.  It
allows for a pair of exceptions, in cases which it refers to as #14(ALT)
and #2ALT.  Documentation/technical/trivial-merge.txt can be consulted for
the precise meanings of the various case numbers and their meanings for
unpack-trees.c, but we have a high-level description of the intent behind
these two exceptions in a combined and summarized form in
Documentation/git-merge.txt:

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed index
    entries are in the state that would result from the merge already.)

While this high-level description does describe conditions under which it
would be safe to allow the index to diverge from HEAD, it does not match
what is actually implemented.  In particular, unpack-trees.c has no
knowledge of renames, and these two exceptions were written assuming that
no renames take place.  Once renames get into the mix, it is no longer
safe to allow the index to not match for #2ALT.  We could modify
unpack-trees to only allow #14(ALT) as an exception, but that would be
more strict than required for the resolve strategy (since the resolve
strategy doesn't handle renames at all).  Therefore, unpack_trees.c seems
like the wrong place to fix this.

Further, if someone fixes the combination of break and rename detection
and modifies merge-recursive to take advantage of the combination, then it
will also no longer be safe to allow the index to not match for #14(ALT)
when the recursive strategy is in use.  Therefore, leaving one of the
exceptions in place with the recursive merge strategy feels like we are
just leaving a latent bug in the code for folks in the future to stumble
across.

It may be possible to fix both unpack-trees and merge-recursive in a way
that implements the exception as stated in Documentation/git-merge.txt,
but it would be somewhat complex, possibly also buggy at first, and
ultimately, not all that valuable.  Instead, just enforce the requirement
stated in builtin/merge.c; error out if the index does not match the HEAD
commit, just like the 'ours' and 'octopus' strategies do.

Some testcase fixups were in order:
  t7611: had many tests designed to show that `git merge --abort` could
	 not always restore the index and working tree to the state they
	 were in before the merge started.  The tests that were associated
	 with having changes in the index before the merge started are no
         longer applicable, so they have been removed.
  t7504: had a few tests that had stray staged changes that were not
         actually part of the test under consideration
  t6044: We no longer expect stray staged changes to sometimes result
         in the merge continuing.  Also, fix a case where a merge
         didn't abort but should have.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c                        |  14 +--
 t/t6044-merge-unrelated-index-changes.sh |  19 ++--
 t/t7504-commit-msg-hook.sh               |   4 +-
 t/t7611-merge-abort.sh                   | 118 -----------------------
 4 files changed, 18 insertions(+), 137 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 855ca39ca..8f7a8e828 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3257,6 +3257,13 @@ int merge_trees(struct merge_options *o,
 		struct tree **result)
 {
 	int code, clean;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
+		err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		    sb.buf);
+		return -1;
+	}
 
 	if (o->subtree_shift) {
 		merge = shift_tree_object(head, merge, o->subtree_shift);
@@ -3264,13 +3271,6 @@ int merge_trees(struct merge_options *o,
 	}
 
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
-		struct strbuf sb = STRBUF_INIT;
-
-		if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
-			err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
-			    sb.buf);
-			return -1;
-		}
 		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 e609f14f8..6e0ecab9c 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -137,7 +137,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
 	test_i18ngrep "Already up to date" out
 '
 
-test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
+test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -145,14 +145,12 @@ test_expect_failure 'recursive, when file has staged changes not matching HEAD n
 	test_seq 1 10 >subdir/a &&
 	git add subdir/a &&
 
-	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
-	# Since subdir/a matches neither HEAD nor what the merge would write
-	# to that file, the merge should fail to avoid overwriting what is
-	# currently found in subdir/a
-	test_must_fail git merge -s recursive E^0
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
 '
 
-test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '
+test_expect_success 'recursive, when file has staged changes matching what a merge would give' '
 	git reset --hard &&
 	git checkout B^0 &&
 
@@ -160,10 +158,9 @@ test_expect_failure 'recursive, when file has staged changes matching what a mer
 	test_seq 1 11 >subdir/a &&
 	git add subdir/a &&
 
-	# HEAD has no subdir/a; merge would write 1..11 to subdir/a;
-	# Since subdir/a matches what the merge would write to that file,
-	# the merge should be safe to proceed
-	git merge -s recursive E^0
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
 '
 
 test_expect_success 'octopus, unrelated file touched' '
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 302a3a208..31b9c6a2c 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&
@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
-	echo "content" >> file &&
+	echo "content" >file &&
 	git add file &&
 	git commit -m "content"
 
@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&
diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index 7b4798e8e..7c84a518a 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -187,31 +187,6 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '
 	test_cmp expect actual
 '
 
-test_expect_success 'Abort clean merge with matching dirty index' '
-	git add bar &&
-	git diff --staged > expect &&
-	git merge --no-commit clean_branch &&
-	test -f .git/MERGE_HEAD &&
-	### When aborting the merge, git will discard all staged changes,
-	### including those that were staged pre-merge. In other words,
-	### --abort will LOSE any staged changes (the staged changes that
-	### are lost must match the merge result, or the merge would not
-	### have been allowed to start). Change expectations accordingly:
-	rm expect &&
-	touch expect &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff --staged > actual &&
-	test_cmp expect actual &&
-	test -z "$(git diff)"
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
 test_expect_success 'Fail conflicting merge with matching dirty worktree' '
 	echo barf > bar &&
 	git diff > expect &&
@@ -223,97 +198,4 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '
 	test_cmp expect actual
 '
 
-test_expect_success 'Abort conflicting merge with matching dirty index' '
-	git add bar &&
-	git diff --staged > expect &&
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	### When aborting the merge, git will discard all staged changes,
-	### including those that were staged pre-merge. In other words,
-	### --abort will LOSE any staged changes (the staged changes that
-	### are lost must match the merge result, or the merge would not
-	### have been allowed to start). Change expectations accordingly:
-	rm expect &&
-	touch expect &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff --staged > actual &&
-	test_cmp expect actual &&
-	test -z "$(git diff)"
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
-test_expect_success 'Abort merge with pre- and post-merge worktree changes' '
-	# Pre-merge worktree changes
-	echo xyzzy > foo &&
-	echo barf > bar &&
-	git add bar &&
-	git diff > expect &&
-	git diff --staged > expect-staged &&
-	# Perform merge
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	# Post-merge worktree changes
-	echo yzxxz > foo &&
-	echo blech > baz &&
-	### When aborting the merge, git will discard staged changes (bar)
-	### and unmerged changes (baz). Other changes that are neither
-	### staged nor marked as unmerged (foo), will be preserved. For
-	### these changed, git cannot tell pre-merge changes apart from
-	### post-merge changes, so the post-merge changes will be
-	### preserved. Change expectations accordingly:
-	git diff -- foo > expect &&
-	rm expect-staged &&
-	touch expect-staged &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff > actual &&
-	test_cmp expect actual &&
-	git diff --staged > actual-staged &&
-	test_cmp expect-staged actual-staged
-'
-
-test_expect_success 'Reset worktree changes' '
-	git reset --hard "$pre_merge_head"
-'
-
-test_expect_success 'Abort merge with pre- and post-merge index changes' '
-	# Pre-merge worktree changes
-	echo xyzzy > foo &&
-	echo barf > bar &&
-	git add bar &&
-	git diff > expect &&
-	git diff --staged > expect-staged &&
-	# Perform merge
-	test_must_fail git merge conflict_branch &&
-	test -f .git/MERGE_HEAD &&
-	# Post-merge worktree changes
-	echo yzxxz > foo &&
-	echo blech > baz &&
-	git add foo bar &&
-	### When aborting the merge, git will discard all staged changes
-	### (foo, bar and baz), and no changes will be preserved. Whether
-	### the changes were staged pre- or post-merge does not matter
-	### (except for not preventing starting the merge).
-	### Change expectations accordingly:
-	rm expect expect-staged &&
-	touch expect &&
-	touch expect-staged &&
-	# Abort merge
-	git merge --abort &&
-	test ! -f .git/MERGE_HEAD &&
-	test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
-	git diff > actual &&
-	test_cmp expect actual &&
-	git diff --staged > actual-staged &&
-	test_cmp expect-staged actual-staged
-'
-
 test_done
-- 
2.18.0.137.g2a11d05a6.dirty

Re: [PATCH v2 8/9] merge-recursive: enforce rule that index matches head before merging

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>     ...[merge will] abort if there are any changes registered in the index
>     relative to the `HEAD` commit.  (One exception is when the changed index
>     entries are in the state that would result from the merge already.)
>
> While this high-level description does describe conditions under which it
> would be safe to allow the index to diverge from HEAD, it does not match
> what is actually implemented.  In particular, unpack-trees.c has no
> knowledge of renames, and these two exceptions were written assuming that
> no renames take place.

I think the above analysis is quite correct (I even suspect that the
rule was outlined long before any renaming merge was designed).

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 855ca39ca..8f7a8e828 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3257,6 +3257,13 @@ int merge_trees(struct merge_options *o,
>  		struct tree **result)
>  {
>  	int code, clean;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
> +		err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> +		    sb.buf);
> +		return -1;
> +	}

This change to ensure the index is pristine upfront makes sense,
too.

[PATCH v2 9/9] merge: fix misleading pre-merge check documentation

[Elijah Newren]

builtin/merge.c contains this important requirement for merge strategies:

    ...the index must be in sync with the head commit.  The strategies are
    responsible to ensure this.

However, Documentation/git-merge.txt says:

    ...[merge will] abort if there are any changes registered in the index
    relative to the `HEAD` commit.  (One exception is when the changed
    index entries are in the state that would result from the merge
    already.)

Interestingly, prior to commit c0be8aa06b85 ("Documentation/git-merge.txt:
Partial rewrite of How Merge Works", 2008-07-19),
Documentation/git-merge.txt said much more:

    ...the index file must match the tree of `HEAD` commit...
    [NOTE]
    This is a bit of a lie.  In certain special cases [explained
    in detail]...
    Otherwise, merge will refuse to do any harm to your repository
    (that is...your working tree...and index are left intact).

So, this suggests that the exceptions existed because there were special
cases where it would case no harm, and potentially be slightly more
convenient for the user.  While the current text in git-merge.txt does
list a condition under which it would be safe to proceed despite the index
not matching HEAD, it does not match what is actually implemented, in
three different ways:

    * The exception is written to describe what unpack-trees allows.  Not
      all merge strategies allow such an exception, though, making this
      description misleading.  'ours' and 'octopus' merges have strictly
      enforced index==HEAD for a while, and the commit previous to this
      one made 'recursive' do so as well.

    * If someone did a three-way content merge on a specific file using
      versions from the relevant commits and staged it prior to running
      merge, then that path would technically satisfy the exception listed
      in git-merge.txt.  unpack-trees.c would still error out on the path,
      though, because it defers the three-way content merge logic to other
      parts of the code (resolve, octopus, or recursive) and has no way of
      checking whether the index entry from before the merge will match
      the end result of the merge.

    * The exception as implemented in unpack-trees actually only checked
      that the index matched the MERGE_HEAD version of the file and that
      HEAD matched the merge base.  Assuming no renames, that would indeed
      provide cases where the index matches the end result we'd get from a
      merge.  But renames means unpack-trees is checking that it instead
      matches something other than what the final result will be, risking
      either erroring out when we shouldn't need to, or not erroring out
      when we should and overwriting the user's staged changes.

In addition to the wording behind this exception being misleading, it is
also somewhat surprising to see how many times the code for the special
cases were wrong or the check to make sure the index matched head was
forgotten altogether:

* Prior to commit ee6566e8d70d ("[PATCH] Rewrite read-tree", 2005-09-05),
  there were many cases where an unclean index entry was allowed (look for
  merged_entry_allow_dirty()); it appears that in those cases, the merge
  would have simply overwritten staged changes with the result of the
  merge.  Thus, the merge result would have been correct, but the user's
  uncommitted changes could be thrown away without warning.

* Prior to commit 160252f81626 ("git-merge-ours: make sure our index
  matches HEAD", 2005-11-03), the 'ours' merge strategy did not check
  whether the index matched HEAD.  If it didn't, the resulting merge
  would include all the staged changes, and thus wasn't really an 'ours'
  strategy.

* Prior to commit 3ec62ad9ffba ("merge-octopus: abort if index does not
  match HEAD", 2016-04-09), 'octopus' merges did not check whether the
  index matched HEAD, also resulting in any staged changes from before
  the commit silently being folded into the resulting merge.  commit
  a6ee883b8eb5 ("t6044: new merge testcases for when index doesn't match
  HEAD", 2016-04-09) was also added at the same time to try to test to
  make sure all strategies did the necessary checking for the requirement
  that the index match HEAD.  Sadly, it didn't catch all the cases, as
  evidenced by the remainder of this list...

* Prior to commit 65170c07d466 ("merge-recursive: avoid incorporating
  uncommitted changes in a merge", 2017-12-21), merge-recursive simply
  relied on unpack_trees() to do the necessary check, but in one special
  case it avoided calling unpack_trees() entirely and accidentally ended
  up silently including any staged changes from before the merge in the
  resulting merge commit.

* The commit immediately before this one in this series noted that the
  exceptions were written in a way that assumed no renames, making it
  unsafe for merge-recursive to use.  merge-recursive was modified to
  use its own check to enforce that index==HEAD.

This history makes it very tempting to go into builtin/merge.c and replace
the comment that strategies must enforce that index matches HEAD with code
that just enforces it.  At this point, that would only affect the
'resolve' strategy; all other strategies have each been modified to
manually enforce it.  (However, note that index==HEAD is not strictly
enforced for fast-forward merges, as those are not considered a merge
strategy and they trigger in builtin/merge.c before the section in the
code where the relevant comment is found.)

But, even if we don't take the step of just fixing these problems by
enforcing index==HEAD for all strategies, we at least need to update this
misleading documentation in git-merge.txt.  For now, just modify the claim
in Documentation/git-merge.txt to fix the error.  The precise details
around combination of merges strategies and special cases probably is not
relevant to most users, so simply state that exceptions may exist but are
narrow and vary depending upon which merge strategy is in use.

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

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6a5c00e2c..b050c8c2e 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -122,9 +122,9 @@ merge' may need to update.
 
 To avoid recording unrelated changes in the merge commit,
 'git pull' and 'git merge' will also abort if there are any changes
-registered in the index relative to the `HEAD` commit.  (One
-exception is when the changed index entries are in the state that
-would result from the merge already.)
+registered in the index relative to the `HEAD` commit.  (Special
+narrow exceptions to this rule may exist depending on which merge
+strategy is in use, but generally, the index must match HEAD.)
 
 If all named commits are already ancestors of `HEAD`, 'git merge'
 will exit early with the message "Already up to date."
-- 
2.18.0.137.g2a11d05a6.dirty