== Testcases 8+ ==
Another bonus bug, found via understanding our final solution (and the
failure of our first attempted solution)!
Let's tweak testcase 7 a bit:
Original: A/file exists
our side: delete A/file
add -> C/file
their side: delete A/file
rename C/ -> A/
Here, there doesn't seem to be a big problem. Sure C/file gets modified
via the directory rename of C/ -> A/ so that it becomes A/file, but
there's no file in the way, right? Actually, here we have a problem
that the stage 1 entry of A/file would be combined with the stage 2
entry of C/file, and make it look like a modify/delete conflict.
Perhaps there is some extra checking that could be added to the code to
make it attempt to clear out the stage 1 entry of A/file, but the
various rename-to-self-via-directory-rename testcases make that a bit
more difficult. For now, it's easier to just treat this as a
path-in-the-way situation and not allow the directory rename to modify
C/file.
That sounds all well and good, but it does have an interesting side
effect. Due to the "relevant renames" optimizations in merge-ort (i.e.
only detect the renames you need), 100% renames whose files weren't
modified on the other side often go undetected. This means that if we
modify this testcase slightly to:
Original: A/file exists
our side: A/file -> C/file
their side: rename C/ -> A/
Then although this looks like where the directory rename just moves
C/file back to A/file and there's no problem, we may not detect the
A/file -> C/file rename. Instead it will look like a deletion of A/file
and an addition of C/file. The directory rename then appears to be
moving C/file to A/file, which is on top of an "unrelated" file (or at
least a file it doesn't know is related). So, we will report
path-in-the-way conflicts now in cases where we didn't before. That's
better than silently and accidentally combining stages of unrelated
files and making them look like a modify/delete; users can investigate
the reported conflict and simply resolve it.
This means we tweak the expected solution for testcases 12i, 12j, and
12k. (Those three tests are basically the same test repeated three
times, but I was worried when I added those that subtle differences in
parent/child, sibling/sibling, and toplevel directories might mess up
how rename-to-self testcases actually get handled.)
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
---
merge-ort.c | 18 +-
t/t6423-merge-rename-directories.sh | 357 ++++++++++++++++++++++++++--
2 files changed, 355 insertions(+), 20 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index feb06720c7e1..f1ecccee940b 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2313,14 +2313,20 @@ static char *apply_dir_rename(struct strmap_entry *rename_info,
return strbuf_detach(&new_path, NULL);
}
-static int path_in_way(struct strmap *paths, const char *path, unsigned side_mask)
+static int path_in_way(struct strmap *paths,
+ const char *path,
+ unsigned side_mask,
+ struct diff_filepair *p)
{
struct merged_info *mi = strmap_get(paths, path);
struct conflict_info *ci;
if (!mi)
return 0;
INITIALIZE_CI(ci, mi);
- return mi->clean || (side_mask & (ci->filemask | ci->dirmask));
+ return mi->clean || (side_mask & (ci->filemask | ci->dirmask))
+ // See testcases 12n, 12p, 12q for more details on this next condition
+ || ((ci->filemask & 0x01) &&
+ strcmp(p->one->path, path));
}
/*
@@ -2332,6 +2338,7 @@ static int path_in_way(struct strmap *paths, const char *path, unsigned side_mas
static char *handle_path_level_conflicts(struct merge_options *opt,
const char *path,
unsigned side_index,
+ struct diff_filepair *p,
struct strmap_entry *rename_info,
struct strmap *collisions)
{
@@ -2366,7 +2373,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt,
*/
if (c_info->reported_already) {
clean = 0;
- } else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index)) {
+ } else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index, p)) {
c_info->reported_already = 1;
strbuf_add_separated_string_list(&collision_paths, ", ",
&c_info->source_files);
@@ -2573,6 +2580,7 @@ static void free_collisions(struct strmap *collisions)
static char *check_for_directory_rename(struct merge_options *opt,
const char *path,
unsigned side_index,
+ struct diff_filepair *p,
struct strmap *dir_renames,
struct strmap *dir_rename_exclusions,
struct strmap *collisions,
@@ -2629,7 +2637,7 @@ static char *check_for_directory_rename(struct merge_options *opt,
return NULL;
}
- new_path = handle_path_level_conflicts(opt, path, side_index,
+ new_path = handle_path_level_conflicts(opt, path, side_index, p,
rename_info,
&collisions[side_index]);
*clean_merge &= (new_path != NULL);
@@ -3428,7 +3436,7 @@ static int collect_renames(struct merge_options *opt,
}
new_path = check_for_directory_rename(opt, p->two->path,
- side_index,
+ side_index, p,
dir_renames_for_side,
rename_exclusions,
collisions,
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 6611331769e9..f4d92b97ff80 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -4759,10 +4759,29 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
git checkout A^0 &&
- test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
-
- test_path_is_missing source/subdir &&
- test_path_is_file source/bar &&
+ # NOTE: A potentially better resolution would be for
+ # source/bar -> source/subdir/bar
+ # to use the directory rename to become
+ # source/bar -> source/bar
+ # (a rename to self), and thus we end up with bar with
+ # a path conflict (given merge.directoryRenames=conflict).
+ # However, since the relevant renames optimization
+ # prevents us from noticing
+ # source/bar -> source/subdir/bar
+ # as a rename and looking at it just as
+ # delete source/bar
+ # add source/subdir/bar
+ # the directory rename of source/subdir/bar -> source/bar does
+ # not look like a rename-to-self situation but a
+ # rename-on-top-of-other-file situation. We do not want
+ # stage 1 entries from an unrelated file, so we expect an
+ # error about there being a file in the way.
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+
+ grep "CONFLICT (implicit dir rename).*source/bar in the way" out &&
+ test_path_is_missing source/bar &&
+ test_path_is_file source/subdir/bar &&
test_path_is_file source/baz &&
git ls-files >actual &&
@@ -4771,8 +4790,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
git status --porcelain -uno >actual &&
cat >expect <<-\EOF &&
- UU source/bar
M source/baz
+ R source/bar -> source/subdir/bar
EOF
test_cmp expect actual
)
@@ -4882,10 +4901,29 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '
git checkout A^0 &&
- test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
-
- test_path_is_missing subdir &&
- test_path_is_file bar &&
+ # NOTE: A potentially better resolution would be for
+ # bar -> subdir/bar
+ # to use the directory rename to become
+ # bar -> bar
+ # (a rename to self), and thus we end up with bar with
+ # a path conflict (given merge.directoryRenames=conflict).
+ # However, since the relevant renames optimization
+ # prevents us from noticing
+ # bar -> subdir/bar
+ # as a rename and looking at it just as
+ # delete bar
+ # add subdir/bar
+ # the directory rename of subdir/bar -> bar does not look
+ # like a rename-to-self situation but a
+ # rename-on-top-of-other-file situation. We do not want
+ # stage 1 entries from an unrelated file, so we expect an
+ # error about there being a file in the way.
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+ grep "CONFLICT (implicit dir rename).*bar in the way" out &&
+
+ test_path_is_missing bar &&
+ test_path_is_file subdir/bar &&
test_path_is_file baz &&
git ls-files >actual &&
@@ -4894,8 +4932,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '
git status --porcelain -uno >actual &&
cat >expect <<-\EOF &&
- UU bar
M baz
+ R bar -> subdir/bar
EOF
test_cmp expect actual
)
@@ -4942,10 +4980,29 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
git checkout A^0 &&
- test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&
-
- test_path_is_missing dirB &&
- test_path_is_file dirA/bar &&
+ # NOTE: A potentially better resolution would be for
+ # dirA/bar -> dirB/bar
+ # to use the directory rename (dirB/ -> dirA/) to become
+ # dirA/bar -> dirA/bar
+ # (a rename to self), and thus we end up with bar with
+ # a path conflict (given merge.directoryRenames=conflict).
+ # However, since the relevant renames optimization
+ # prevents us from noticing
+ # dirA/bar -> dirB/bar
+ # as a rename and looking at it just as
+ # delete dirA/bar
+ # add dirB/bar
+ # the directory rename of dirA/bar -> dirB/bar does
+ # not look like a rename-to-self situation but a
+ # rename-on-top-of-other-file situation. We do not want
+ # stage 1 entries from an unrelated file, so we expect an
+ # error about there being a file in the way.
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+ grep "CONFLICT (implicit dir rename).*dirA/bar in the way" out &&
+
+ test_path_is_missing dirA/bar &&
+ test_path_is_file dirB/bar &&
test_path_is_file dirA/baz &&
git ls-files >actual &&
@@ -4954,8 +5011,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
git status --porcelain -uno >actual &&
cat >expect <<-\EOF &&
- UU dirA/bar
M dirA/baz
+ R dirA/bar -> dirB/bar
EOF
test_cmp expect actual
)
@@ -5259,6 +5316,276 @@ test_expect_success '12n2: Directory rename transitively makes rename back to se
)
'
+# Testcase 12o, Directory rename hits other rename source; file still in way on same side
+# Commit O: A/file1_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# C/other
+# Commit A: A/file1_1
+# A/stuff
+# B/stuff
+# C/file1_2
+# C/other
+# Commit B: D/file2_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# A/other
+# In words:
+# A: rename B/file1_2 -> C/file1_2
+# B: rename C/ -> A/
+# rename A/file1_1 -> D/file2_1
+# Rationale:
+# A/stuff is unmodified, it shows up in final output
+# B/stuff is unmodified, it shows up in final output
+# C/other touched on one side (rename to A), so A/other shows up in output
+# A/file1 is renamed to D/file2
+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is
+# "in the way" so we don't do the directory rename
+# Expected: A/stuff
+# B/stuff
+# A/other
+# D/file2
+# C/file1
+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1
+#
+
+test_setup_12o () {
+ git init 12o &&
+ (
+ cd 12o &&
+
+ mkdir -p A B C &&
+ echo 1 >A/file1 &&
+ echo 2 >B/file1 &&
+ echo other >C/other &&
+ echo Astuff >A/stuff &&
+ echo Bstuff >B/stuff &&
+ git add . &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git switch A &&
+ git mv B/file1 C/ &&
+ git add . &&
+ git commit -m "A" &&
+
+ git switch B &&
+ mkdir -p D &&
+ git mv A/file1 D/file2 &&
+ git mv C/other A/other &&
+ git add . &&
+ git commit -m "B"
+ )
+}
+
+test_expect_success '12o: Directory rename hits other rename source; file still in way on same side' '
+ test_setup_12o &&
+ (
+ cd 12o &&
+
+ git checkout -q A^0 &&
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+
+ test_stdout_line_count = 5 git ls-files -s &&
+ test_stdout_line_count = 1 git ls-files -s A/other &&
+ test_stdout_line_count = 1 git ls-files -s A/stuff &&
+ test_stdout_line_count = 1 git ls-files -s B/stuff &&
+ test_stdout_line_count = 1 git ls-files -s D/file2 &&
+
+ grep "CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way" out &&
+ test_stdout_line_count = 1 git ls-files -s C/file1
+ )
+'
+
+# Testcase 12p, Directory rename hits other rename source; file still in way on other side
+# Commit O: A/file1_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# C/other
+# Commit A: D/file2_1
+# A/stuff
+# B/stuff
+# C/file1_2
+# C/other
+# Commit B: A/file1_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# A/other
+# Short version:
+# A: rename A/file1_1 -> D/file2_1
+# rename B/file1_2 -> C/file1_2
+# B: Rename C/ -> A/
+# Rationale:
+# A/stuff is unmodified, it shows up in final output
+# B/stuff is unmodified, it shows up in final output
+# C/other touched on one side (rename to A), so A/other shows up in output
+# A/file1 is renamed to D/file2
+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 is
+# "in the way" so we don't do the directory rename
+# Expected: A/stuff
+# B/stuff
+# A/other
+# D/file2
+# C/file1
+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1
+#
+
+test_setup_12p () {
+ git init 12p &&
+ (
+ cd 12p &&
+
+ mkdir -p A B C &&
+ echo 1 >A/file1 &&
+ echo 2 >B/file1 &&
+ echo other >C/other &&
+ echo Astuff >A/stuff &&
+ echo Bstuff >B/stuff &&
+ git add . &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git switch A &&
+ mkdir -p D &&
+ git mv A/file1 D/file2 &&
+ git mv B/file1 C/ &&
+ git add . &&
+ git commit -m "A" &&
+
+ git switch B &&
+ git mv C/other A/other &&
+ git add . &&
+ git commit -m "B"
+ )
+}
+
+test_expect_success '12p: Directory rename hits other rename source; file still in way on other side' '
+ test_setup_12p &&
+ (
+ cd 12p &&
+
+ git checkout -q A^0 &&
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+
+ test_stdout_line_count = 5 git ls-files -s &&
+ test_stdout_line_count = 1 git ls-files -s A/other &&
+ test_stdout_line_count = 1 git ls-files -s A/stuff &&
+ test_stdout_line_count = 1 git ls-files -s B/stuff &&
+ test_stdout_line_count = 1 git ls-files -s D/file2 &&
+
+ grep "CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way" out &&
+ test_stdout_line_count = 1 git ls-files -s C/file1
+ )
+'
+
+# Testcase 12q, Directory rename hits other rename source; file removed though
+# Commit O: A/file1_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# C/other
+# Commit A: A/stuff
+# B/stuff
+# C/file1_2
+# C/other
+# Commit B: D/file2_1
+# A/stuff
+# B/file1_2
+# B/stuff
+# A/other
+# In words:
+# A: delete A/file1_1, rename B/file1_2 -> C/file1_2
+# B: Rename C/ -> A/, rename A/file1_1 -> D/file2_1
+# Rationale:
+# A/stuff is unmodified, it shows up in final output
+# B/stuff is unmodified, it shows up in final output
+# C/other touched on one side (rename to A), so A/other shows up in output
+# A/file1 is rename/delete to D/file2, so two stages for D/file2
+# B/file1 -> C/file1 and even though C/ -> A/, A/file1 as a source was
+# "in the way" (ish) so we don't do the directory rename
+# Expected: A/stuff
+# B/stuff
+# A/other
+# D/file2 (two stages)
+# C/file1
+# + CONFLICT (implicit dir rename): A/file1 in way of C/file1
+# + CONFLICT (rename/delete): D/file2
+#
+
+test_setup_12q () {
+ git init 12q &&
+ (
+ cd 12q &&
+
+ mkdir -p A B C &&
+ echo 1 >A/file1 &&
+ echo 2 >B/file1 &&
+ echo other >C/other &&
+ echo Astuff >A/stuff &&
+ echo Bstuff >B/stuff &&
+ git add . &&
+ git commit -m "O" &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git switch A &&
+ git rm A/file1 &&
+ git mv B/file1 C/ &&
+ git add . &&
+ git commit -m "A" &&
+
+ git switch B &&
+ mkdir -p D &&
+ git mv A/file1 D/file2 &&
+ git mv C/other A/other &&
+ git add . &&
+ git commit -m "B"
+ )
+}
+
+test_expect_success '12q: Directory rename hits other rename source; file removed though' '
+ test_setup_12q &&
+ (
+ cd 12q &&
+
+ git checkout -q A^0 &&
+
+ test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 >out &&
+
+ grep "CONFLICT (rename/delete).*A/file1.*D/file2" out &&
+ grep "CONFLICT (implicit dir rename).*Existing file/dir at A/file1 in the way" out &&
+
+ test_stdout_line_count = 6 git ls-files -s &&
+ test_stdout_line_count = 1 git ls-files -s A/other &&
+ test_stdout_line_count = 1 git ls-files -s A/stuff &&
+ test_stdout_line_count = 1 git ls-files -s B/stuff &&
+ test_stdout_line_count = 2 git ls-files -s D/file2 &&
+
+ # This is a slightly suboptimal resolution; allowing the
+ # rename of C/ -> A/ to affect C/file1 and further rename
+ # it to A/file1 would probably be preferable, but since
+ # A/file1 existed as the source of another rename, allowing
+ # the dir rename of C/file1 -> A/file1 would mean modifying
+ # the code so that renames do not adjust both their source
+ # and target paths in all cases.
+ ! grep "CONFLICT (file location)" out &&
+ test_stdout_line_count = 1 git ls-files -s C/file1
+ )
+'
###########################################################################
# SECTION 13: Checking informational and conflict messages
--
gitgitgadget
