Skip to content

Commit 8020504

Browse files
newrengitster
newren
authored and
gitster
committed
merge-recursive: apply collision handling unification to recursive case
In the en/merge-path-collision topic (see commit ac193e0, "Merge branch 'en/merge-path-collision'", 2019-01-04), all the "file collision" conflict types were modified for consistency. In particular, rename/add, rename/rename(2to1) and each rename/add piece of a rename/rename(1to2)/add[/add] conflict were made to behave like add/add conflicts have always been handled. However, this consistency was not enforced when opt->priv->call_depth > 0 for rename/rename conflicts. Update rename/rename(1to2) and rename/rename(2to1) conflicts in the recursive case to also be consistent. As an added bonus, this simplifies the code considerably. Signed-off-by: Elijah Newren <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 2d2118b commit 8020504

File tree

2 files changed

+77
-114
lines changed

2 files changed

+77
-114
lines changed

‎merge-recursive.c

Lines changed: 47 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,35 +1557,6 @@ static int handle_file_collision(struct merge_options *opt,
15571557
b, a);
15581558
}
15591559

1560-
/*
1561-
* In the recursive case, we just opt to undo renames
1562-
*/
1563-
if (opt->priv->call_depth && (prev_path1 || prev_path2)) {
1564-
/* Put first file (a->oid, a->mode) in its original spot */
1565-
if (prev_path1) {
1566-
if (update_file(opt, 1, a, prev_path1))
1567-
return -1;
1568-
} else {
1569-
if (update_file(opt, 1, a, collide_path))
1570-
return -1;
1571-
}
1572-
1573-
/* Put second file (b->oid, b->mode) in its original spot */
1574-
if (prev_path2) {
1575-
if (update_file(opt, 1, b, prev_path2))
1576-
return -1;
1577-
} else {
1578-
if (update_file(opt, 1, b, collide_path))
1579-
return -1;
1580-
}
1581-
1582-
/* Don't leave something at collision path if unrenaming both */
1583-
if (prev_path1 && prev_path2)
1584-
remove_file(opt, 1, collide_path, 0);
1585-
1586-
return 0;
1587-
}
1588-
15891560
/* Remove rename sources if rename/add or rename/rename(2to1) */
15901561
if (prev_path1)
15911562
remove_file(opt, 1, prev_path1,
@@ -1746,85 +1717,56 @@ static int handle_rename_rename_1to2(struct merge_options *opt,
17461717
return -1;
17471718
free(path_desc);
17481719

1749-
if (opt->priv->call_depth) {
1750-
/*
1751-
* FIXME: For rename/add-source conflicts (if we could detect
1752-
* such), this is wrong. We should instead find a unique
1753-
* pathname and then either rename the add-source file to that
1754-
* unique path, or use that unique path instead of src here.
1755-
*/
1756-
if (update_file(opt, 0, &mfi.blob, o->path))
1757-
return -1;
1720+
if (opt->priv->call_depth)
1721+
remove_file_from_index(opt->repo->index, o->path);
17581722

1759-
/*
1760-
* Above, we put the merged content at the merge-base's
1761-
* path. Now we usually need to delete both a->path and
1762-
* b->path. However, the rename on each side of the merge
1763-
* could also be involved in a rename/add conflict. In
1764-
* such cases, we should keep the added file around,
1765-
* resolving the conflict at that path in its favor.
1766-
*/
1767-
add = &ci->ren1->dst_entry->stages[flip_stage(2)];
1768-
if (is_valid(add)) {
1769-
if (update_file(opt, 0, add, a->path))
1770-
return -1;
1771-
}
1772-
else
1773-
remove_file_from_index(opt->repo->index, a->path);
1774-
add = &ci->ren2->dst_entry->stages[flip_stage(3)];
1775-
if (is_valid(add)) {
1776-
if (update_file(opt, 0, add, b->path))
1777-
return -1;
1778-
}
1779-
else
1780-
remove_file_from_index(opt->repo->index, b->path);
1723+
/*
1724+
* For each destination path, we need to see if there is a
1725+
* rename/add collision. If not, we can write the file out
1726+
* to the specified location.
1727+
*/
1728+
add = &ci->ren1->dst_entry->stages[flip_stage(2)];
1729+
if (is_valid(add)) {
1730+
add->path = mfi.blob.path = a->path;
1731+
if (handle_file_collision(opt, a->path,
1732+
NULL, NULL,
1733+
ci->ren1->branch,
1734+
ci->ren2->branch,
1735+
&mfi.blob, add) < 0)
1736+
return -1;
17811737
} else {
1782-
/*
1783-
* For each destination path, we need to see if there is a
1784-
* rename/add collision. If not, we can write the file out
1785-
* to the specified location.
1786-
*/
1787-
add = &ci->ren1->dst_entry->stages[flip_stage(2)];
1788-
if (is_valid(add)) {
1789-
add->path = mfi.blob.path = a->path;
1790-
if (handle_file_collision(opt, a->path,
1791-
NULL, NULL,
1792-
ci->ren1->branch,
1793-
ci->ren2->branch,
1794-
&mfi.blob, add) < 0)
1795-
return -1;
1796-
} else {
1797-
char *new_path = find_path_for_conflict(opt, a->path,
1798-
ci->ren1->branch,
1799-
ci->ren2->branch);
1800-
if (update_file(opt, 0, &mfi.blob,
1801-
new_path ? new_path : a->path))
1802-
return -1;
1803-
free(new_path);
1804-
if (update_stages(opt, a->path, NULL, a, NULL))
1805-
return -1;
1806-
}
1738+
char *new_path = find_path_for_conflict(opt, a->path,
1739+
ci->ren1->branch,
1740+
ci->ren2->branch);
1741+
if (update_file(opt, 0, &mfi.blob,
1742+
new_path ? new_path : a->path))
1743+
return -1;
1744+
free(new_path);
1745+
if (!opt->priv->call_depth &&
1746+
update_stages(opt, a->path, NULL, a, NULL))
1747+
return -1;
1748+
}
18071749

1808-
add = &ci->ren2->dst_entry->stages[flip_stage(3)];
1809-
if (is_valid(add)) {
1810-
add->path = mfi.blob.path = b->path;
1811-
if (handle_file_collision(opt, b->path,
1812-
NULL, NULL,
1813-
ci->ren1->branch,
1814-
ci->ren2->branch,
1815-
add, &mfi.blob) < 0)
1816-
return -1;
1817-
} else {
1818-
char *new_path = find_path_for_conflict(opt, b->path,
1819-
ci->ren2->branch,
1820-
ci->ren1->branch);
1821-
if (update_file(opt, 0, &mfi.blob,
1822-
new_path ? new_path : b->path))
1823-
return -1;
1824-
free(new_path);
1825-
if (update_stages(opt, b->path, NULL, NULL, b))
1826-
return -1;
1827-
}
1750+
add = &ci->ren2->dst_entry->stages[flip_stage(3)];
1751+
if (is_valid(add)) {
1752+
add->path = mfi.blob.path = b->path;
1753+
if (handle_file_collision(opt, b->path,
1754+
NULL, NULL,
1755+
ci->ren1->branch,
1756+
ci->ren2->branch,
1757+
add, &mfi.blob) < 0)
1758+
return -1;
1759+
} else {
1760+
char *new_path = find_path_for_conflict(opt, b->path,
1761+
ci->ren2->branch,
1762+
ci->ren1->branch);
1763+
if (update_file(opt, 0, &mfi.blob,
1764+
new_path ? new_path : b->path))
1765+
return -1;
1766+
free(new_path);
1767+
if (!opt->priv->call_depth &&
1768+
update_stages(opt, b->path, NULL, NULL, b))
1769+
return -1;
18281770
}
18291771

18301772
return 0;

‎t/t6036-recursive-corner-cases.sh

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,9 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '
6060
test_must_fail git merge -s recursive R2^0 &&
6161
6262
git ls-files -s >out &&
63-
test_line_count = 2 out &&
63+
test_line_count = 5 out &&
6464
git ls-files -u >out &&
65-
test_line_count = 2 out &&
65+
test_line_count = 3 out &&
6666
git ls-files -o >out &&
6767
test_line_count = 1 out &&
6868
@@ -133,9 +133,9 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'
133133
test_must_fail git merge -s recursive R2^0 &&
134134
135135
git ls-files -s >out &&
136-
test_line_count = 2 out &&
136+
test_line_count = 5 out &&
137137
git ls-files -u >out &&
138-
test_line_count = 2 out &&
138+
test_line_count = 3 out &&
139139
git ls-files -o >out &&
140140
test_line_count = 1 out &&
141141
@@ -218,8 +218,18 @@ test_expect_success 'git detects differently handled merges conflict' '
218218
git ls-files -o >out &&
219219
test_line_count = 1 out &&
220220
221-
git rev-parse >expect \
222-
C:new_a D:new_a E:new_a &&
221+
git cat-file -p C:new_a >ours &&
222+
git cat-file -p C:a >theirs &&
223+
>empty &&
224+
test_must_fail git merge-file \
225+
-L "Temporary merge branch 1" \
226+
-L "" \
227+
-L "Temporary merge branch 2" \
228+
ours empty theirs &&
229+
sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
230+
git hash-object ours-tweaked >expect &&
231+
git rev-parse >>expect \
232+
D:new_a E:new_a &&
223233
git rev-parse >actual \
224234
:1:new_a :2:new_a :3:new_a &&
225235
test_cmp expect actual &&
@@ -257,7 +267,8 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
257267
ctime=$(git log --no-walk --date=raw --format=%cd C | awk "{print \$1}") &&
258268
newctime=$(($btime+1)) &&
259269
git fast-export --no-data --all | sed -e s/$ctime/$newctime/ | git fast-import --force --quiet &&
260-
# End of differences; rest is copy-paste of last test
270+
# End of most differences; rest is copy-paste of last test,
271+
# other than swapping C:a and C:new_a due to order switch
261272
262273
git checkout D^0 &&
263274
test_must_fail git merge -s recursive E^0 &&
@@ -269,8 +280,18 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
269280
git ls-files -o >out &&
270281
test_line_count = 1 out &&
271282
272-
git rev-parse >expect \
273-
C:new_a D:new_a E:new_a &&
283+
git cat-file -p C:a >ours &&
284+
git cat-file -p C:new_a >theirs &&
285+
>empty &&
286+
test_must_fail git merge-file \
287+
-L "Temporary merge branch 1" \
288+
-L "" \
289+
-L "Temporary merge branch 2" \
290+
ours empty theirs &&
291+
sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked &&
292+
git hash-object ours-tweaked >expect &&
293+
git rev-parse >>expect \
294+
D:new_a E:new_a &&
274295
git rev-parse >actual \
275296
:1:new_a :2:new_a :3:new_a &&
276297
test_cmp expect actual &&

0 commit comments

Comments
 (0)