merge-recursive: apply collision handling unification to recursive case
In the en/merge-path-collision topic (see commit ac193e0e0aa5, "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 <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren committed
Feb 27, 2020 at 00:05 UTC
802050400a51270bd2b4a13e410412693975c766
2 files changed
+77
-114
merge-recursive.c
+47
-105
@@ -1557,35 +1557,6 @@ static int handle_file_collision(struct merge_options *opt,
1557
b, a);
1558
}
1559
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
-
1560
/* Remove rename sources if rename/add or rename/rename(2to1) */
1561
if (prev_path1)
1562
remove_file(opt, 1, prev_path1,
@@ -1746,85 +1717,56 @@ static int handle_rename_rename_1to2(struct merge_options *opt,
1717
return -1;
1718
free(path_desc);
1719
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);
1722
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;
1737
} 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
+ }
1749
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;
1770
}
1771
1772
return 0;
t/t6036-recursive-corner-cases.sh
+30
-9
@@ -60,9 +60,9 @@ test_expect_success 'merge simple rename+criss-cross with no modifications' '
60
test_must_fail git merge -s recursive R2^0 &&
61
62
git ls-files -s >out &&
63
- test_line_count = 2 out &&
63
+ test_line_count = 5 out &&
64
git ls-files -u >out &&
65
- test_line_count = 2 out &&
65
+ test_line_count = 3 out &&
66
git ls-files -o >out &&
67
test_line_count = 1 out &&
68
@@ -133,9 +133,9 @@ test_expect_success 'merge criss-cross + rename merges with basic modification'
133
test_must_fail git merge -s recursive R2^0 &&
134
135
git ls-files -s >out &&
136
- test_line_count = 2 out &&
136
+ test_line_count = 5 out &&
137
git ls-files -u >out &&
138
- test_line_count = 2 out &&
138
+ test_line_count = 3 out &&
139
git ls-files -o >out &&
140
test_line_count = 1 out &&
141
@@ -218,8 +218,18 @@ test_expect_success 'git detects differently handled merges conflict' '
218
git ls-files -o >out &&
219
test_line_count = 1 out &&
220
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 &&
233
git rev-parse >actual \
234
:1:new_a :2:new_a :3:new_a &&
235
test_cmp expect actual &&
@@ -257,7 +267,8 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
267
ctime=$(git log --no-walk --date=raw --format=%cd C | awk "{print \$1}") &&
268
newctime=$(($btime+1)) &&
269
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
272
273
git checkout D^0 &&
274
test_must_fail git merge -s recursive E^0 &&
@@ -269,8 +280,18 @@ test_expect_success 'git detects differently handled merges conflict, swapped' '
280
git ls-files -o >out &&
281
test_line_count = 1 out &&
282
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 &&
295
git rev-parse >actual \
296
:1:new_a :2:new_a :3:new_a &&
297
test_cmp expect actual &&