Merge branch 'en/ort-rename-fixes'

Various bugs about rename handling in "ort" merge strategy have
been fixed.

* en/ort-rename-fixes:
  merge-ort: fix directory rename on top of source of other rename/delete
  merge-ort: fix incorrect file handling
  merge-ort: clarify the interning of strings in opt->priv->path
  t6423: fix missed staging of file in testcases 12i,12j,12k
  t6423: document two bugs with rename-to-self testcases
  merge-ort: drop unnecessary temporary in check_for_directory_rename()
  merge-ort: update comments to modern testfile location
main
Junio C Hamano 2025-08-21 13:47:02 -07:00
commit d1123cd810
2 changed files with 542 additions and 28 deletions

View File

@ -316,9 +316,14 @@ struct merge_options_internal {
* (e.g. "drivers/firmware/raspberrypi.c").
* * store all relevant paths in the repo, both directories and
* files (e.g. drivers, drivers/firmware would also be included)
* * these keys serve to intern all the path strings, which allows
* us to do pointer comparison on directory names instead of
* strcmp; we just have to be careful to use the interned strings.
* * these keys serve to intern *all* path strings, which allows us
* to do pointer comparisons on file & directory names instead of
* using strcmp; however, for this pointer-comparison optimization
* to work, any code path that independently computes a path needs
* to check for it existing in this strmap, and if so, point to
* the path in this strmap instead of their computed copy. See
* the "reuse known pointer" comment in
* apply_directory_rename_modifications() for an example.
*
* The values of paths:
* * either a pointer to a merged_info, or a conflict_info struct
@ -2163,7 +2168,7 @@ static int handle_content_merge(struct merge_options *opt,
/*
* FIXME: If opt->priv->call_depth && !clean, then we really
* should not make result->mode match either a->mode or
* b->mode; that causes t6036 "check conflicting mode for
* b->mode; that causes t6416 "check conflicting mode for
* regular file" to fail. It would be best to use some other
* mode, but we'll confuse all kinds of stuff if we use one
* where S_ISREG(result->mode) isn't true, and if we use
@ -2313,14 +2318,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 12[npq] of t6423 for this next condition */
|| ((ci->filemask & 0x01) &&
strcmp(p->one->path, path));
}

/*
@ -2332,6 +2343,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 +2378,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);
@ -2520,7 +2532,7 @@ static void compute_collisions(struct strmap *collisions,
* happening, and fall back to no-directory-rename detection
* behavior for those paths.
*
* See testcases 9e and all of section 5 from t6043 for examples.
* See testcases 9e and all of section 5 from t6423 for examples.
*/
for (i = 0; i < pairs->nr; ++i) {
struct strmap_entry *rename_info;
@ -2573,6 +2585,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,
@ -2580,7 +2593,6 @@ static char *check_for_directory_rename(struct merge_options *opt,
{
char *new_path;
struct strmap_entry *rename_info;
struct strmap_entry *otherinfo;
const char *new_dir;
int other_side = 3 - side_index;

@ -2615,14 +2627,13 @@ static char *check_for_directory_rename(struct merge_options *opt,
* to not let Side1 do the rename to dumbdir, since we know that is
* the source of one of our directory renames.
*
* That's why otherinfo and dir_rename_exclusions is here.
* That's why dir_rename_exclusions is here.
*
* As it turns out, this also prevents N-way transient rename
* confusion; See testcases 9c and 9d of t6043.
* confusion; See testcases 9c and 9d of t6423.
*/
new_dir = rename_info->value; /* old_dir = rename_info->key; */
otherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);
if (otherinfo) {
if (strmap_contains(dir_rename_exclusions, new_dir)) {
path_msg(opt, INFO_DIR_RENAME_SKIPPED_DUE_TO_RERENAME, 1,
rename_info->key, path, new_dir, NULL,
_("WARNING: Avoiding applying %s -> %s rename "
@ -2631,7 +2642,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);
@ -2875,6 +2886,20 @@ static int process_renames(struct merge_options *opt,
newinfo = new_ent->value;
}

/*
* Directory renames can result in rename-to-self; the code
* below assumes we have A->B with different A & B, and tries
* to move all entries to path B. If A & B are the same path,
* the logic can get confused, so skip further processing when
* A & B are already the same path.
*
* As a reminder, we can avoid strcmp here because all paths
* are interned in opt->priv->paths; see the comment above
* "paths" in struct merge_options_internal.
*/
if (oldpath == newpath)
continue;

/*
* If pair->one->path isn't in opt->priv->paths, that means
* that either directory rename detection removed that
@ -3419,7 +3444,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,

View File

@ -4731,7 +4731,7 @@ test_setup_12i () {

mkdir -p source/subdir &&
echo foo >source/subdir/foo &&
echo bar >source/bar &&
printf "%d\n" 1 2 3 4 5 6 7 >source/bar &&
echo baz >source/baz &&
git add source &&
git commit -m orig &&
@ -4747,6 +4747,7 @@ test_setup_12i () {
git switch B &&
git mv source/bar source/subdir/bar &&
echo more baz >>source/baz &&
git add source/baz &&
git commit -m B
)
}
@ -4758,6 +4759,88 @@ test_expect_success '12i: Directory rename causes rename-to-self' '

git checkout A^0 &&

# 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 &&
uniq <actual >tracked &&
test_line_count = 3 tracked &&

git status --porcelain -uno >actual &&
cat >expect <<-\EOF &&
M source/baz
R source/bar -> source/subdir/bar
EOF
test_cmp expect actual
)
'

# Testcase 12i2, Identical to 12i except that source/subdir/bar modified on unrenamed side
# Commit O: source/{subdir/foo, bar, baz_1}
# Commit A: source/{foo, bar_2, baz_1}
# Commit B: source/{subdir/{foo, bar}, baz_2}
# Expected: source/{foo, bar, baz_2}, with conflicts on
# source/bar vs. source/subdir/bar

test_setup_12i2 () {
git init 12i2 &&
(
cd 12i2 &&

mkdir -p source/subdir &&
echo foo >source/subdir/foo &&
printf "%d\n" 1 2 3 4 5 6 7 >source/bar &&
echo baz >source/baz &&
git add source &&
git commit -m orig &&

git branch O &&
git branch A &&
git branch B &&

git switch A &&
git mv source/subdir/foo source/foo &&
echo 8 >> source/bar &&
git add source/bar &&
git commit -m A &&

git switch B &&
git mv source/bar source/subdir/bar &&
echo more baz >>source/baz &&
git add source/baz &&
git commit -m B
)
}

test_expect_success '12i2: Directory rename causes rename-to-self' '
test_setup_12i2 &&
(
cd 12i2 &&

git checkout A^0 &&

test_must_fail git -c merge.directoryRenames=conflict merge -s recursive B^0 &&

test_path_is_missing source/subdir &&
@ -4771,7 +4854,7 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
git status --porcelain -uno >actual &&
cat >expect <<-\EOF &&
UU source/bar
M source/baz
M source/baz
EOF
test_cmp expect actual
)
@ -4806,6 +4889,7 @@ test_setup_12j () {
git switch B &&
git mv bar subdir/bar &&
echo more baz >>baz &&
git add baz &&
git commit -m B
)
}
@ -4817,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 &&
# 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_path_is_missing subdir &&
test_path_is_file bar &&
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 &&
@ -4829,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
M baz
R bar -> subdir/bar
EOF
test_cmp expect actual
)
@ -4865,6 +4968,7 @@ test_setup_12k () {
git switch B &&
git mv dirA/bar dirB/bar &&
echo more baz >>dirA/baz &&
git add dirA/baz &&
git commit -m B
)
}
@ -4876,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 &&
# 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_path_is_missing dirB &&
test_path_is_file dirA/bar &&
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 &&
@ -4888,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
M dirA/baz
R dirA/bar -> dirB/bar
EOF
test_cmp expect actual
)
@ -5056,6 +5179,25 @@ test_expect_success '12m: Change parent of renamed-dir to symlink on other side'
)
'

# Testcase 12n, Directory rename transitively makes rename back to self
#
# (Since this is a cherry-pick instead of merge, the labels are a bit weird.
# O, the original commit, is A~1 rather than what branch O points to.)
#
# Commit O: tools/hello
# world
# Commit A: tools/hello
# tools/world
# Commit B: hello
# In words:
# A: world -> tools/world
# B: tools/ -> /, i.e. rename all of tools to toplevel directory
# delete world
#
# Expected:
# CONFLICT (file location): tools/world vs. world
#

test_setup_12n () {
git init 12n &&
(
@ -5092,10 +5234,357 @@ test_expect_success '12n: Directory rename transitively makes rename back to sel
git checkout -q B^0 &&

test_must_fail git cherry-pick A^0 >out &&
grep "CONFLICT (file location).*should perhaps be moved" out
test_grep "CONFLICT (file location).*should perhaps be moved" out &&

# Should have 1 entry for hello, and 2 for world
test_stdout_line_count = 3 git ls-files -s &&
test_stdout_line_count = 1 git ls-files -s hello &&
test_stdout_line_count = 2 git ls-files -s world
)
'

# Testcase 12n2, Directory rename transitively makes rename back to self
#
# Commit O: tools/hello
# world
# Commit A: tools/hello
# tools/world
# Commit B: hello
# In words:
# A: world -> tools/world
# B: tools/ -> /, i.e. rename all of tools to toplevel directory
# delete world
#
# Expected:
# CONFLICT (file location): tools/world vs. world
#

test_setup_12n2 () {
git init 12n2 &&
(
cd 12n2 &&

mkdir tools &&
echo hello >tools/hello &&
git add tools/hello &&
echo world >world &&
git add world &&
git commit -m "O" &&

git branch O &&
git branch A &&
git branch B &&

git switch A &&
git mv world tools/world &&
git commit -m "Move world into tools/" &&

git switch B &&
git mv tools/hello hello &&
git rm world &&
git commit -m "Move hello from tools/ to toplevel"
)
}

test_expect_success '12n2: Directory rename transitively makes rename back to self' '
test_setup_12n2 &&
(
cd 12n2 &&

git checkout -q B^0 &&

test_might_fail git -c merge.directoryRenames=true merge A^0 >out &&

# Should have 1 entry for hello, and either 0 or 2 for world
#
# NOTE: Since merge.directoryRenames=true, there is no path
# conflict for world vs. tools/world; it should end up at
# world. The fact that world was unmodified on side A, means
# there was no content conflict; we should just take the
# content from side B -- i.e. delete the file. So merging
# could just delete world.
#
# However, rename-to-self-via-directory-rename is a bit more
# challenging. Relax this test to allow world to be treated
# as a modify/delete conflict as well, meaning it will have
# two higher order stages, that just so happen to match.
#
test_stdout_line_count = 1 git ls-files -s hello &&
test_stdout_line_count = 2 git ls-files -s world &&
test_grep "CONFLICT (modify/delete).*world deleted in HEAD" out
)
'

# 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