From 1b9fbefbe08ff2bbcbb8a6089867b32fca603eac Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:24:56 -0700 Subject: index_has_changes(): avoid assuming operating on the_index 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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 869092f7b9..32529ca5b6 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1983,7 +1983,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; -- cgit v1.2.1 From 92702392cefdbd66ca593fa909540230ef9e005e Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:24:59 -0700 Subject: merge-recursive: make sure when we say we abort that we actually abort 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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 32529ca5b6..2fb1bdb010 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1984,9 +1984,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; -- cgit v1.2.1 From e1f8694f3394caf3d3cd57c6c7593f0b0cdb1f9e Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:25:00 -0700 Subject: merge-recursive: fix assumption that head tree being merged is HEAD `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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 2fb1bdb010..171587bc7a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1983,7 +1983,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; -- cgit v1.2.1 From eddd1a411d935420e1746108ecfee564114a129f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:25:02 -0700 Subject: merge-recursive: enforce rule that index matches head before merging 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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'merge-recursive.c') diff --git a/merge-recursive.c b/merge-recursive.c index 171587bc7a..352aa50fba 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1974,6 +1974,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); @@ -1981,13 +1988,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; -- cgit v1.2.1