From cffbfad50d1f37aea03520dd8d8ea983bc57da02 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:24:55 -0700 Subject: read-cache.c: move index_has_changes() from merge.c 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 Signed-off-by: Junio C Hamano diff --git a/merge.c b/merge.c index f06a477..fb5eaf2 100644 --- a/merge.c +++ b/merge.c @@ -17,37 +17,6 @@ static const char *merge_argument(struct commit *commit) return EMPTY_TREE_SHA1_HEX; } -int index_has_changes(struct strbuf *sb) -{ - struct object_id head; - int i; - - if (!get_oid_tree("HEAD", &head)) { - struct diff_options opt; - - diff_setup(&opt); - opt.flags.exit_with_status = 1; - if (!sb) - opt.flags.quick = 1; - do_diff_cache(&head, &opt); - diffcore_std(&opt); - for (i = 0; sb && i < diff_queued_diff.nr; i++) { - if (i) - strbuf_addch(sb, ' '); - strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path); - } - diff_flush(&opt); - return opt.flags.has_changes != 0; - } else { - for (i = 0; sb && i < active_nr; i++) { - if (i) - strbuf_addch(sb, ' '); - strbuf_addstr(sb, active_cache[i]->name); - } - return !!active_nr; - } -} - int try_merge_command(const char *strategy, size_t xopts_nr, const char **xopts, struct commit_list *common, const char *head_arg, struct commit_list *remotes) diff --git a/read-cache.c b/read-cache.c index 4b35e87..ba094fd 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; -- cgit v0.10.2-6-g49f6 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 diff --git a/builtin/am.c b/builtin/am.c index 1151b5c..2d80056 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; } @@ -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(&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 0323853..169c809 100644 --- a/cache.h +++ b/cache.h @@ -627,12 +627,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 869092f..32529ca 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; diff --git a/read-cache.c b/read-cache.c index ba094fd..f333a51 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; } } -- cgit v0.10.2-6-g49f6 From 58f4d1b96152a4838eba5fd865c7fffab392b0de Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:24:57 -0700 Subject: t6044: verify that merges expected to abort actually abort 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 Signed-off-by: Junio C Hamano diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh index 23b86fb..f9c2f81 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 -- cgit v0.10.2-6-g49f6 From cf69f2af08f8dc092d57b987f26719fe0d43a40d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:24:58 -0700 Subject: t6044: add a testcase for index matching head, when head doesn't match HEAD 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 Signed-off-by: Junio C Hamano diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh index f9c2f81..97f7460 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 && -- cgit v0.10.2-6-g49f6 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 diff --git a/merge-recursive.c b/merge-recursive.c index 32529ca..2fb1bdb 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; diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh index 97f7460..d55f164 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 && -- cgit v0.10.2-6-g49f6 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 diff --git a/builtin/am.c b/builtin/am.c index 2d80056..3d827d1 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; } @@ -1878,7 +1879,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 169c809..fad10c3 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 @@ -627,12 +628,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 2fb1bdb..171587b 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; diff --git a/read-cache.c b/read-cache.c index f333a51..639c1ff 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 d55f164..ef8a502 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 && -- cgit v0.10.2-6-g49f6 From 7f5271fa1584d99fb3c83f308f35295ed78c8b8b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:25:01 -0700 Subject: t6044: add more testcases with staged changes before a merge is invoked 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 Signed-off-by: Junio C Hamano diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh index ef8a502..9601255 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 && -- cgit v0.10.2-6-g49f6 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 diff --git a/merge-recursive.c b/merge-recursive.c index 171587b..352aa50 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; diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh index 9601255..5e3779e 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 302a3a2..31b9c6a 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 7b4798e..7c84a51 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 -- cgit v0.10.2-6-g49f6 From 55f39cf7551bd468065306328efdb78933b36b69 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 30 Jun 2018 18:25:03 -0700 Subject: merge: fix misleading pre-merge check documentation 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 Signed-off-by: Junio C Hamano diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index d5dfd84..141bd72 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." -- cgit v0.10.2-6-g49f6