* [PATCH v9 29.25/30] merge-recursive: improve output precision around skipping updates
2018-04-13 19:56 ` [RFC PATCH v9 0/30] Add directory rename detection to git Elijah Newren
@ 2018-04-13 19:56 ` Elijah Newren
2018-04-13 19:56 ` [PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge Elijah Newren
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Elijah Newren @ 2018-04-13 19:56 UTC (permalink / raw)
To: git; +Cc: torvalds, gitster, sbeller, Elijah Newren
When a merge results in contents that already existed in HEAD (because the
changes on a side branch were a subset of what was changed by HEAD), and
those contents were already at the right path, the working directory
updates can be skipped. However, the relevant code would sometimes claim
the working directory update could be skipped despite the fact that the
contents were at the wrong path. Make the output reflect the situation
more precisely, including an additional message that tests can check for
to make sure we are getting correct behavior.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 11 ++++++-----
t/t6022-merge-rename.sh | 2 +-
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 4a1ecdea03..05250939c7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2761,21 +2761,22 @@ static int merge_content(struct merge_options *o,
o->branch2, path2, &mfi))
return -1;
- if (mfi.clean && !df_conflict_remains &&
- oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {
+ if (mfi.clean && oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {
int path_renamed_outside_HEAD;
- output(o, 3, _("Skipped %s (merged same as existing)"), path);
/*
* The content merge resulted in the same file contents we
* already had. We can return early if those file contents
* are recorded at the correct path (which may not be true
- * if the merge involves a rename).
+ * if the merge involves a rename or there's a D/F conflict).
*/
path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
- if (!path_renamed_outside_HEAD) {
+ if (!df_conflict_remains && !path_renamed_outside_HEAD) {
+ output(o, 3, _("Skipped %s (merged same as existing)"), path);
add_cacheinfo(o, mfi.mode, &mfi.oid, path,
0, (!o->call_depth), 0);
return mfi.clean;
+ } else {
+ output(o, 3, _("Had correct contents for %s, but not at right path"), path);
}
} else
output(o, 2, _("Auto-merging %s"), path);
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 05ebba7afa..574088bfaf 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -245,7 +245,7 @@ test_expect_success 'merge of identical changes in a renamed file' '
GIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep "^Skipped B" &&
git reset --hard HEAD^ &&
git checkout change &&
- GIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep "^Skipped B"
+ GIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep "^Had correct contents for B, but not at right path"
'
test_expect_success 'setup for rename + d/f conflicts' '
--
2.16.0.35.g6dd7ede834
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge
2018-04-13 19:56 ` [RFC PATCH v9 0/30] Add directory rename detection to git Elijah Newren
2018-04-13 19:56 ` [PATCH v9 29.25/30] merge-recursive: improve output precision around skipping updates Elijah Newren
@ 2018-04-13 19:56 ` Elijah Newren
2018-04-13 21:03 ` Stefan Beller
2018-04-13 19:56 ` [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths Elijah Newren
2018-04-13 19:56 ` [PATCH v9 30/30] merge-recursive: fix check for skipability of working tree updates Elijah Newren
3 siblings, 1 reply; 13+ messages in thread
From: Elijah Newren @ 2018-04-13 19:56 UTC (permalink / raw)
To: git; +Cc: torvalds, gitster, sbeller, Elijah Newren
Add several tests checking whether updates can be skipped in a merge.
Also add several similar testcases for where updates cannot be skipped in
a merge to make sure that we skip if and only if we should.
In particular:
* Testcase 1a (particularly 1a-check-L) would have pointed out the
problem Linus has been dealing with for year with his merges[1].
* Testcase 2a (particularly 2a-check-L) would have pointed out the
problem with my directory-rename-series before it broke master[2].
* Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase
than 12b of t6043 making that one easier to understand.
* There are several complementary testcases to make sure we're not just
fixing those particular issues while regressing in the opposite
direction.
[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/
[2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t6046-merge-skip-unneeded-updates.sh | 497 +++++++++++++++++++++++++++++++++
1 file changed, 497 insertions(+)
create mode 100755 t/t6046-merge-skip-unneeded-updates.sh
diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh
new file mode 100755
index 0000000000..89c3a953ae
--- /dev/null
+++ b/t/t6046-merge-skip-unneeded-updates.sh
@@ -0,0 +1,497 @@
+#!/bin/sh
+
+test_description="merge cases"
+
+# The setup for all of them, pictorially, is:
+#
+# A
+# o
+# / \
+# O o ?
+# \ /
+# o
+# B
+#
+# To help make it easier to follow the flow of tests, they have been
+# divided into sections and each test will start with a quick explanation
+# of what commits O, A, and B contain.
+#
+# Notation:
+# z/{b,c} means files z/b and z/c both exist
+# x/d_1 means file x/d exists with content d1. (Purpose of the
+# underscore notation is to differentiate different
+# files that might be renamed into each other's paths.)
+
+. ./test-lib.sh
+
+
+###########################################################################
+# SECTION 1: Cases involving no renames (one side has subset of changes of
+# the other side)
+###########################################################################
+
+# Testcase 1a, Changes on A, subset of changes on B
+# Commit O: b_1
+# Commit A: b_2
+# Commit B: b_3
+# Expected: b_2
+
+test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '
+ test_create_repo 1a &&
+ (
+ cd 1a &&
+
+ test_write_lines 1 2 3 4 5 6 7 8 9 10 >b
+ git add b &&
+ test_tick &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git checkout A &&
+ test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 10.5 >b &&
+ git add b &&
+ test_tick &&
+ git commit -m "A" &&
+
+ git checkout B &&
+ test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 >b &&
+ git add b &&
+ test_tick &&
+ git commit -m "B"
+ )
+'
+
+test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
+ test_when_finished "git -C 1a reset --hard" &&
+ (
+ cd 1a &&
+
+ git checkout A^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&
+
+ test_i18ngrep "Skipped" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:b &&
+ git rev-parse >expect A:b &&
+ test_cmp expect actual &&
+
+ git hash-object b >actual &&
+ git rev-parse A:b >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '1a-check-R: Modify(A)/Modify(B), change on B subset of A' '
+ test_when_finished "git -C 1a reset --hard" &&
+ (
+ cd 1a &&
+
+ git checkout B^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&
+
+ test_i18ngrep "Auto-merging" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:b &&
+ git rev-parse >expect A:b &&
+ test_cmp expect actual &&
+
+ git hash-object b >actual &&
+ git rev-parse A:b >expect &&
+ test_cmp expect actual
+ )
+'
+
+###########################################################################
+# SECTION 2: Cases involving basic renames
+###########################################################################
+
+# Testcase 2a, Changes on A, rename on B
+# Commit O: b_1
+# Commit A: b_2
+# Commit B: c_1
+# Expected: c_2
+
+test_expect_success '2a-setup: Modify(A)/rename(B)' '
+ test_create_repo 2a &&
+ (
+ cd 2a &&
+
+ test_seq 1 10 >b
+ git add b &&
+ test_tick &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git checkout A &&
+ test_seq 1 11 > b &&
+ git add b &&
+ test_tick &&
+ git commit -m "A" &&
+
+ git checkout B &&
+ git mv b c &&
+ test_tick &&
+ git commit -m "B"
+ )
+'
+
+test_expect_success '2a-check-L: Modify/rename, merge into modify side' '
+ test_when_finished "git -C 2a reset --hard" &&
+ (
+ cd 2a &&
+
+ git checkout A^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&
+
+ test_i18ngrep "Had correct contents" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:c &&
+ git rev-parse >expect A:b &&
+ test_cmp expect actual &&
+
+ git hash-object c >actual &&
+ git rev-parse A:b >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:b &&
+ test_path_is_missing b
+ )
+'
+
+test_expect_success '2a-check-R: Modify/rename, merge into rename side' '
+ test_when_finished "git -C 2a reset --hard" &&
+ (
+ cd 2a &&
+
+ git checkout B^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&
+
+ test_i18ngrep "Auto-merging" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:c &&
+ git rev-parse >expect A:b &&
+ test_cmp expect actual &&
+
+ git hash-object c >actual &&
+ git rev-parse A:b >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:b &&
+ test_path_is_missing b
+ )
+'
+
+# Testcase 2b, Changed and renamed on A, subset of changes on B
+# Commit O: b_1
+# Commit A: c_2
+# Commit B: b_3
+# Expected: c_2
+
+test_expect_success '2b-setup: Modify(A)/Modify(B), change on B subset of A' '
+ test_create_repo 2b &&
+ (
+ cd 2b &&
+
+ test_write_lines 1 2 3 4 5 6 7 8 9 10 >b
+ git add b &&
+ test_tick &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git checkout A &&
+ test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 10.5 >b &&
+ git add b &&
+ git mv b c &&
+ test_tick &&
+ git commit -m "A" &&
+
+ git checkout B &&
+ test_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 >b &&
+ git add b &&
+ test_tick &&
+ git commit -m "B"
+ )
+'
+
+test_expect_success '2b-check-L: Modify(A)/Modify(B), change on B subset of A' '
+ test_when_finished "git -C 2b reset --hard" &&
+ (
+ cd 2b &&
+
+ git checkout A^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&
+
+ test_i18ngrep "Skipped" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:c &&
+ git rev-parse >expect A:c &&
+ test_cmp expect actual &&
+
+ git hash-object c >actual &&
+ git rev-parse A:c >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:b &&
+ test_path_is_missing b
+ )
+'
+
+test_expect_success '2b-check-R: Modify(A)/Modify(B), change on B subset of A' '
+ test_when_finished "git -C 2b reset --hard" &&
+ (
+ cd 2b &&
+
+ git checkout B^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&
+
+ test_i18ngrep "Auto-merging" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 1 index_files &&
+
+ git rev-parse >actual HEAD:c &&
+ git rev-parse >expect A:c &&
+ test_cmp expect actual &&
+
+ git hash-object c >actual &&
+ git rev-parse A:c >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:b &&
+ test_path_is_missing b
+ )
+'
+
+###########################################################################
+# SECTION 3: Cases involving directory renames
+#
+# NOTE:
+# Directory renames only apply when one side renames a directory, and the
+# other side adds or renames a path into that directory. Applying the
+# directory rename to that new path creates a new pathname that didn't
+# exist on either side of history. Thus, it is impossible for the
+# merge contents to already be at the right path, so all of these checks
+# exist just to make sure that updates are not skipped.
+###########################################################################
+
+# Testcase 3a, Change + rename into dir foo on A, dir rename foo->bar on B
+# Commit O: bq_1, foo/whatever
+# Commit A: foo/{bq_2, whatever}
+# Commit B: bq_1, bar/whatever
+# Expected: bar/{bq_2, whatever}
+
+test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_create_repo 3a &&
+ (
+ cd 3a &&
+
+ mkdir foo &&
+ test_seq 1 10 >bq &&
+ test_write_lines a b c d e f g h i j k >foo/whatever &&
+ git add bq foo/whatever &&
+ test_tick &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git checkout A &&
+ test_seq 1 11 > bq &&
+ git add bq &&
+ git mv bq foo/ &&
+ test_tick &&
+ git commit -m "A" &&
+
+ git checkout B &&
+ git mv foo/ bar/ &&
+ test_tick &&
+ git commit -m "B"
+ )
+'
+
+test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_when_finished "git -C 3a reset --hard" &&
+ (
+ cd 3a &&
+
+ git checkout A^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&
+
+ test_i18ngrep "Had correct contents for bar/bq" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 2 index_files &&
+
+ git rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&
+ git rev-parse >expect A:foo/bq A:foo/whatever &&
+ test_cmp expect actual &&
+
+ git hash-object bar/bq bar/whatever >actual &&
+ git rev-parse A:foo/bq A:foo/whatever >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&
+ test_path_is_missing bq foo/bq foo/whatever
+ )
+'
+
+test_expect_success '3a-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_when_finished "git -C 3a reset --hard" &&
+ (
+ cd 3a &&
+
+ git checkout B^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&
+
+ test_i18ngrep "Auto-merging bar/bq" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 2 index_files &&
+
+ git rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&
+ git rev-parse >expect A:foo/bq A:foo/whatever &&
+ test_cmp expect actual &&
+
+ git hash-object bar/bq bar/whatever >actual &&
+ git rev-parse A:foo/bq A:foo/whatever >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&
+ test_path_is_missing bq foo/bq foo/whatever
+ )
+'
+
+# Testcase 3b, rename into dir foo on A, dir rename foo->bar + change on B
+# Commit O: bq_1, foo/whatever
+# Commit A: foo/{bq_1, whatever}
+# Commit B: bq_2, bar/whatever
+# Expected: bar/{bq_2, whatever}
+
+test_expect_success '3b-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_create_repo 3b &&
+ (
+ cd 3b &&
+
+ mkdir foo &&
+ test_seq 1 10 >bq &&
+ test_write_lines a b c d e f g h i j k >foo/whatever &&
+ git add bq foo/whatever &&
+ test_tick &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git checkout A &&
+ git mv bq foo/ &&
+ test_tick &&
+ git commit -m "A" &&
+
+ git checkout B &&
+ test_seq 1 11 > bq &&
+ git add bq &&
+ git mv foo/ bar/ &&
+ test_tick &&
+ git commit -m "B"
+ )
+'
+
+test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_when_finished "git -C 3b reset --hard" &&
+ (
+ cd 3b &&
+
+ git checkout A^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&
+
+ test_i18ngrep "Auto-merging bar/bq" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 2 index_files &&
+
+ git rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&
+ git rev-parse >expect B:bq A:foo/whatever &&
+ test_cmp expect actual &&
+
+ git hash-object bar/bq bar/whatever >actual &&
+ git rev-parse B:bq A:foo/whatever >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&
+ test_path_is_missing bq foo/bq foo/whatever
+ )
+'
+
+test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+ test_when_finished "git -C 3b reset --hard" &&
+ (
+ cd 3b &&
+
+ git checkout B^0 &&
+
+ GIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&
+
+ test_i18ngrep "Had correct contents for bar/bq" out &&
+ test_must_be_empty err &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 2 index_files &&
+
+ git rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&
+ git rev-parse >expect B:bq A:foo/whatever &&
+ test_cmp expect actual &&
+
+ git hash-object bar/bq bar/whatever >actual &&
+ git rev-parse B:bq A:foo/whatever >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&
+ test_path_is_missing bq foo/bq foo/whatever
+ )
+'
+
+test_done
--
2.16.0.35.g6dd7ede834
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths
2018-04-13 19:56 ` [RFC PATCH v9 0/30] Add directory rename detection to git Elijah Newren
2018-04-13 19:56 ` [PATCH v9 29.25/30] merge-recursive: improve output precision around skipping updates Elijah Newren
2018-04-13 19:56 ` [PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge Elijah Newren
@ 2018-04-13 19:56 ` Elijah Newren
2018-04-16 0:37 ` Junio C Hamano
2018-04-13 19:56 ` [PATCH v9 30/30] merge-recursive: fix check for skipability of working tree updates Elijah Newren
3 siblings, 1 reply; 13+ messages in thread
From: Elijah Newren @ 2018-04-13 19:56 UTC (permalink / raw)
To: git; +Cc: torvalds, gitster, sbeller, Elijah Newren
In commit aacb82de3ff8 ("merge-recursive: Split was_tracked() out of
would_lose_untracked()", 2011-08-11), was_tracked() was split out of
would_lose_untracked() with the intent to provide a function that could
answer whether a path was tracked in the index before the merge. Sadly,
it instead returned whether the path was in the working tree due to having
been tracked in the index before the merge OR having been written there by
unpack_trees(). The distinction is important when renames are involved,
e.g. for a merge where:
HEAD: modifies path b
other: renames b->c
In this case, c was not tracked in the index before the merge, but would
have been added to the index at stage 0 and written to the working tree by
unpack_trees(). would_lose_untracked() is more interested in the
in-working-copy-for-either-reason behavior, while all other uses of
was_tracked() want just was-it-tracked-in-index-before-merge behavior.
Unsplit would_lose_untracked() and write a new was_tracked() function
which answers whether a path was tracked in the index before the merge
started.
This will also make was_dirty() return better results, as it will be based
off the original index rather than an index that possibly only copied over
some of the stat information.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 82 +++++++++++++++++++++++++++++++++++++++----------------
merge-recursive.h | 1 +
2 files changed, 59 insertions(+), 24 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 05250939c7..adc800f188 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -344,6 +344,7 @@ static int git_merge_trees(struct merge_options *o,
{
int rc;
struct tree_desc t[3];
+ struct index_state tmp_index = { NULL };
memset(&o->unpack_opts, 0, sizeof(o->unpack_opts));
if (o->call_depth)
@@ -354,7 +355,7 @@ static int git_merge_trees(struct merge_options *o,
o->unpack_opts.head_idx = 2;
o->unpack_opts.fn = threeway_merge;
o->unpack_opts.src_index = &the_index;
- o->unpack_opts.dst_index = &the_index;
+ o->unpack_opts.dst_index = &tmp_index;
setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
init_tree_desc_from_tree(t+0, common);
@@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,
init_tree_desc_from_tree(t+2, merge);
rc = unpack_trees(3, t, &o->unpack_opts);
+ cache_tree_free(&active_cache_tree);
+
+ o->orig_index = the_index;
+ the_index = tmp_index;
+
/*
- * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
- * so set to the new index which will usually have modification
- * timestamp info copied over.
+ * src_index is used in verify_uptodate, but was NULLified in
+ * unpack_trees, so we need to set it back to the original index.
*/
- o->unpack_opts.src_index = &the_index;
- cache_tree_free(&active_cache_tree);
+ o->unpack_opts.src_index = &o->orig_index;
+
return rc;
}
@@ -773,31 +778,51 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok)
!(empty_ok && is_empty_dir(path));
}
-static int was_tracked(const char *path)
+/*
+ * Returns whether path was tracked in the index before the merge started
+ */
+static int was_tracked(struct merge_options *o, const char *path)
{
- int pos = cache_name_pos(path, strlen(path));
+ int pos = index_name_pos(&o->orig_index, path, strlen(path));
if (0 <= pos)
- /* we have been tracking this path */
+ /* we were tracking this path before the merge */
return 1;
- /*
- * Look for an unmerged entry for the path,
- * specifically stage #2, which would indicate
- * that "our" side before the merge started
- * had the path tracked (and resulted in a conflict).
- */
- for (pos = -1 - pos;
- pos < active_nr && !strcmp(path, active_cache[pos]->name);
- pos++)
- if (ce_stage(active_cache[pos]) == 2)
- return 1;
return 0;
}
static int would_lose_untracked(const char *path)
{
- return !was_tracked(path) && file_exists(path);
+ /*
+ * This may look like it can be simplified to:
+ * return !was_tracked(o, path) && file_exists(path)
+ * but it can't. This function needs to know whether path was
+ * in the working tree due to EITHER having been tracked in the
+ * index before the merge OR having been put into the working copy
+ * and index by unpack_trees(). Due to that either-or requirement,
+ * we check the current index instead of the original one.
+ */
+ int pos = cache_name_pos(path, strlen(path));
+
+ if (pos < 0)
+ pos = -1 - pos;
+ while (pos < active_nr &&
+ !strcmp(path, active_cache[pos]->name)) {
+ /*
+ * If stage #0, it is definitely tracked.
+ * If it has stage #2 then it was tracked
+ * before this merge started. All other
+ * cases the path was not tracked.
+ */
+ switch (ce_stage(active_cache[pos])) {
+ case 0:
+ case 2:
+ return 0;
+ }
+ pos++;
+ }
+ return file_exists(path);
}
static int was_dirty(struct merge_options *o, const char *path)
@@ -805,7 +830,7 @@ static int was_dirty(struct merge_options *o, const char *path)
struct cache_entry *ce;
int dirty = 1;
- if (o->call_depth || !was_tracked(path))
+ if (o->call_depth || !was_tracked(o, path))
return !dirty;
ce = cache_file_exists(path, strlen(path), ignore_case);
@@ -2407,7 +2432,7 @@ static int process_renames(struct merge_options *o,
* add-source case).
*/
remove_file(o, 1, ren1_src,
- renamed_stage == 2 || !was_tracked(ren1_src));
+ renamed_stage == 2 || !was_tracked(o, ren1_src));
oidcpy(&src_other.oid,
&ren1->src_entry->stages[other_stage].oid);
@@ -2800,7 +2825,7 @@ static int merge_content(struct merge_options *o,
if (update_stages(o, path, &one, &a, &b))
return -1;
} else {
- int file_from_stage2 = was_tracked(path);
+ int file_from_stage2 = was_tracked(o, path);
struct diff_filespec merged;
oidcpy(&merged.oid, &mfi.oid);
merged.mode = mfi.mode;
@@ -3075,6 +3100,15 @@ int merge_trees(struct merge_options *o,
else
clean = 1;
+ /* Free the extra index left from git_merge_trees() */
+ /*
+ * FIXME: Need to also data allocated by setup_unpack_trees_porcelain()
+ * tucked away in o->unpack_opts.msgs, but the problem is that only
+ * half of it refers to dynamically allocated data, while the other
+ * half points at static strings.
+ */
+ discard_index(&o->orig_index);
+
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index d863cf8867..248093e407 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -29,6 +29,7 @@ struct merge_options {
struct hashmap current_file_dir_set;
struct string_list df_conflict_file_set;
struct unpack_trees_options unpack_opts;
+ struct index_state orig_index;
};
/*
--
2.16.0.35.g6dd7ede834
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths
2018-04-13 19:56 ` [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths Elijah Newren
@ 2018-04-16 0:37 ` Junio C Hamano
2018-04-16 21:21 ` Elijah Newren
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2018-04-16 0:37 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, torvalds, sbeller
Elijah Newren <newren@gmail.com> writes:
> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,
> init_tree_desc_from_tree(t+2, merge);
>
> rc = unpack_trees(3, t, &o->unpack_opts);
> + cache_tree_free(&active_cache_tree);
> +
> + o->orig_index = the_index;
> + the_index = tmp_index;
> +
> /*
> - * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
> - * so set to the new index which will usually have modification
> - * timestamp info copied over.
> + * src_index is used in verify_uptodate, but was NULLified in
> + * unpack_trees, so we need to set it back to the original index.
> */
Was NULLified? I thought that the point of src/dst distinction
Linus introduced long time ago at 34110cd4 ("Make 'unpack_trees()'
have a separate source and destination index", 2008-03-06) was that
we can then keep the source side of the traversal unmodified.
> - o->unpack_opts.src_index = &the_index;
> - cache_tree_free(&active_cache_tree);
> + o->unpack_opts.src_index = &o->orig_index;
> -static int was_tracked(const char *path)
> +/*
> + * Returns whether path was tracked in the index before the merge started
> + */
> +static int was_tracked(struct merge_options *o, const char *path)
> {
> - int pos = cache_name_pos(path, strlen(path));
> + int pos = index_name_pos(&o->orig_index, path, strlen(path));
>
> if (0 <= pos)
> - /* we have been tracking this path */
> + /* we were tracking this path before the merge */
> return 1;
>
> - /*
> - * Look for an unmerged entry for the path,
> - * specifically stage #2, which would indicate
> - * that "our" side before the merge started
> - * had the path tracked (and resulted in a conflict).
> - */
> - for (pos = -1 - pos;
> - pos < active_nr && !strcmp(path, active_cache[pos]->name);
> - pos++)
> - if (ce_stage(active_cache[pos]) == 2)
> - return 1;
> return 0;
> }
I do agree with the simplicity of the new code that directly asks
exactly what we want to ask. However, there is one thing that is
puzzling below...
> static int would_lose_untracked(const char *path)
> {
> - return !was_tracked(path) && file_exists(path);
> + /*
> + * This may look like it can be simplified to:
> + * return !was_tracked(o, path) && file_exists(path)
> + * but it can't. This function needs to know whether path was
> + * in the working tree due to EITHER having been tracked in the
> + * index before the merge OR having been put into the working copy
> + * and index by unpack_trees(). Due to that either-or requirement,
> + * we check the current index instead of the original one.
> + */
If this path was created by merge-recursive, not by unpack_trees(),
what does this function want to say? Say, we are looking at path P,
the other branch we are merging moved some other path Q to P (while
our side modified contents at path Q). Then path P we are looking
at has contents of Q at the merge base at stage #1, the contents of
Q from our HEAD at stage #2 and the contents of P from the other
branch at stage #3. The code below says "path P is OK, we won't
lose it" in such a case, but it is unclear if the above comment
wants to also cover that case.
> + int pos = cache_name_pos(path, strlen(path));
> +
> + if (pos < 0)
> + pos = -1 - pos;
> + while (pos < active_nr &&
> + !strcmp(path, active_cache[pos]->name)) {
> + /*
> + * If stage #0, it is definitely tracked.
> + * If it has stage #2 then it was tracked
> + * before this merge started. All other
> + * cases the path was not tracked.
> + */
> + switch (ce_stage(active_cache[pos])) {
> + case 0:
> + case 2:
> + return 0;
> + }
> + pos++;
> + }
> + return file_exists(path);
> }
>
> static int was_dirty(struct merge_options *o, const char *path)
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths
2018-04-16 0:37 ` Junio C Hamano
@ 2018-04-16 21:21 ` Elijah Newren
0 siblings, 0 replies; 13+ messages in thread
From: Elijah Newren @ 2018-04-16 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Linus Torvalds, Stefan Beller
On Sun, Apr 15, 2018 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,
>> init_tree_desc_from_tree(t+2, merge);
>>
>> rc = unpack_trees(3, t, &o->unpack_opts);
>> + cache_tree_free(&active_cache_tree);
>> +
>> + o->orig_index = the_index;
>> + the_index = tmp_index;
>> +
>> /*
>> - * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
>> - * so set to the new index which will usually have modification
>> - * timestamp info copied over.
>> + * src_index is used in verify_uptodate, but was NULLified in
>> + * unpack_trees, so we need to set it back to the original index.
>> */
>
> Was NULLified? I thought that the point of src/dst distinction
> Linus introduced long time ago at 34110cd4 ("Make 'unpack_trees()'
> have a separate source and destination index", 2008-03-06) was that
> we can then keep the source side of the traversal unmodified.
That comment is messed up; maybe I edited and re-edited the comment
multiple times and then didn't notice the big problems when
re-reading?
Anyway, I should move the comment a few lines up, and make the code
instead read:
/*
* Update the_index to match the new results, AFTER saving a copy
* in o->orig_index. Update src_index to point to the saved copy.
* (verify_uptodate() checks src_index, and the original index is
* the one that had the necessary modification timestamps.)
*/
o->orig_index = the_index;
the_index = tmp_index;
o->unpack_opts.src_index = &o->orig_index;
>> static int would_lose_untracked(const char *path)
>> {
>> - return !was_tracked(path) && file_exists(path);
>> + /*
>> + * This may look like it can be simplified to:
>> + * return !was_tracked(o, path) && file_exists(path)
>> + * but it can't. This function needs to know whether path was
>> + * in the working tree due to EITHER having been tracked in the
>> + * index before the merge OR having been put into the working copy
>> + * and index by unpack_trees(). Due to that either-or requirement,
>> + * we check the current index instead of the original one.
>> + */
>
> If this path was created by merge-recursive, not by unpack_trees(),
> what does this function want to say? Say, we are looking at path P,
> the other branch we are merging moved some other path Q to P (while
> our side modified contents at path Q). Then path P we are looking
> at has contents of Q at the merge base at stage #1, the contents of
> Q from our HEAD at stage #2 and the contents of P from the other
> branch at stage #3. The code below says "path P is OK, we won't
> lose it" in such a case, but it is unclear if the above comment
> wants to also cover that case.
Oh, boy, here be dragons...
The comment as-is actually does cover your example case with Q and P:
unpack_trees(), which is unaware of renames, will see that P only
exists on one side of history and thus load it into the index at stage
0 rather than stage 3.
But your general comment about whether something else in
merge-recursive could create a path in the current index after
unpack_trees() is interesting...it touches on a pitfall that has bit
me multiple times. There is a required ordering in merge-recursive.c
that for any given path, the working directory must be updated before
the index is -- otherwise, would_lose_untracked() will return faulty
information. update_file_flags() has this ordering builtin,
update_stages() has a big obnoxious comment at the beginning about how
it should not be called until after update_file() is,
apply_directory_rename_modifications() has a big comment about this
~80% of the way through the function (look for
"would_lose_untracked"), and conflict_rename_rename_2to1() has a big
obnoxious comment near the end painstakingly pointing out that some
code that feels like it would make more sense being combined with a
previous function cannot be due to the
update-working-directory-before-index requirement.
I should probably add to this comment something about this annoying
(and error-prone) ordering restriction, since this function is the
source of those particular pains. Your suggested ideal-world rewrite
(run unpack_trees() with unpack_opts.index_only=1, do merge in memory,
then update working tree), would make this whole problem go away.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 30/30] merge-recursive: fix check for skipability of working tree updates
2018-04-13 19:56 ` [RFC PATCH v9 0/30] Add directory rename detection to git Elijah Newren
` (2 preceding siblings ...)
2018-04-13 19:56 ` [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths Elijah Newren
@ 2018-04-13 19:56 ` Elijah Newren
3 siblings, 0 replies; 13+ messages in thread
From: Elijah Newren @ 2018-04-13 19:56 UTC (permalink / raw)
To: git; +Cc: torvalds, gitster, sbeller, Elijah Newren
The can-working-tree-updates-be-skipped check has had a long and blemished
history. The update can be skipped iff:
a) The merged contents match what was in HEAD
b) The merged mode matches what was in HEAD
c) The target path is usable and matches what was in HEAD
Steps a & b are easy to check; we have always gotten those right. Step c
is just:
c1) Nothing else is in the way of putting the content at the target path
(i.e. it isn't involved in any D/F conflicts)
c2) target path was tracked in the index before the merge
While it is easy to overlook c1, this was fixed seven years ago with
commit 4ab9a157d069 ("merge_content(): Check whether D/F conflicts are
still present", 2010-09-20). merge-recursive didn't have a readily
available way to directly check c2, so various approximations were
used:
* In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip
an update, actually skip it", 2011-02-28), it was noted that although
the code claimed it was skipping the update, it did not actually skip
the update. The code was made to skip it, but used lstat(path, ...)
as an approximation to path-was-tracked-in-index-before-merge.
* In commit 5b448b853030 ("merge-recursive: When we detect we can skip
an update, actually skip it", 2011-08-11), the problem with using
lstat was noted. It was changed to the approximation
path2 && strcmp(path, path2)
which is also wrong. !path2 || strcmp(path, path2) would have been
better, but would have fallen short with directory renames.
* In c5b761fb2711 ("merge-recursive: ensure we write updates for
directory-renamed file", 2018-02-14), the problem with the previous
approximation was noted and changed to
was_tracked(path)
That looks like exactly what we were trying to answer, and is what
was_tracked() was *intended* for, but not what was_tracked()
actually returned.
Since the previous commit made was_tracked(path) actually mean "path was
tracked in the index before the merge", we can now use it instead of
other approximations to answer the question "was path tracked in the
index before the merge?" So, although the code change in this commit is
the same one made in c5b761fb2711, it now safe and correct due to the
prior fix to was_tracked().
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 20 +++++++++++++-------
t/t6043-merge-rename-directories.sh | 2 +-
t/t6046-merge-skip-unneeded-updates.sh | 4 ++--
3 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index adc800f188..1b71d00fdb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2786,16 +2786,22 @@ static int merge_content(struct merge_options *o,
o->branch2, path2, &mfi))
return -1;
+ /*
+ * We can skip updating the working tree file iff:
+ * a) The merged contents match what was in HEAD
+ * b) The merged mode matches what was in HEAD
+ * c) The target path is usable and matches what was in HEAD
+ * We test (a) & (b) here.
+ */
if (mfi.clean && oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {
- int path_renamed_outside_HEAD;
/*
- * The content merge resulted in the same file contents we
- * already had. We can return early if those file contents
- * are recorded at the correct path (which may not be true
- * if the merge involves a rename or there's a D/F conflict).
+ * Case c has two pieces:
+ * c1) Nothing else is in the way of writing the merged
+ * results to path (i.e. it isn't involved in any
+ * D/F conflict)
+ * c2) path was tracked in the index before the merge
*/
- path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
- if (!df_conflict_remains && !path_renamed_outside_HEAD) {
+ if (!df_conflict_remains && was_tracked(o, path)) {
output(o, 3, _("Skipped %s (merged same as existing)"), path);
add_cacheinfo(o, mfi.mode, &mfi.oid, path,
0, (!o->call_depth), 0);
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 45f620633f..2e28f2908d 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory hierarchy into another' '
)
'
-test_expect_failure '12b-check: Moving one directory hierarchy into another' '
+test_expect_success '12b-check: Moving one directory hierarchy into another' '
(
cd 12b &&
diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh
index 89c3a953ae..4dcec7226c 100755
--- a/t/t6046-merge-skip-unneeded-updates.sh
+++ b/t/t6046-merge-skip-unneeded-updates.sh
@@ -64,7 +64,7 @@ test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '
)
'
-test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
+test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
test_when_finished "git -C 1a reset --hard" &&
(
cd 1a &&
@@ -346,7 +346,7 @@ test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
)
'
-test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
test_when_finished "git -C 3a reset --hard" &&
(
cd 3a &&
--
2.16.0.35.g6dd7ede834
^ permalink raw reply related [flat|nested] 13+ messages in thread