merge-recursive: fix rename/add conflict handling
This makes the rename/add conflict handling make use of the new handle_file_collision() function, which fixes several bugs and improves things for the rename/add case significantly. Previously, rename/add would: * Not leave any higher order stage entries in the index, making it appear as if there were no conflict. * Would place the rename file at the colliding path, and move the added file elsewhere, which combined with the lack of higher order stage entries felt really odd. It's not clear to me why the rename should take precedence over the add; if one should be moved out of the way, they both probably should. * In the recursive case, it would do a two way merge of the added file and the version of the renamed file on the renamed side, completely excluding modifications to the renamed file on the unrenamed side of history. Use the new handle_file_collision() to fix all of these issues. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren committed
Nov 7, 2018 at 20:40 UTC
7f8671656f31f5d1cc9d9748b7dc62b76a428cc5
3 files changed
+101
-64
merge-recursive.c
+86
-51
@@ -186,6 +186,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
186
enum rename_type {
187
RENAME_NORMAL = 0,
188
RENAME_VIA_DIR,
189
+ RENAME_ADD,
190
RENAME_DELETE,
191
RENAME_ONE_FILE_TO_ONE,
192
RENAME_ONE_FILE_TO_TWO,
@@ -228,6 +229,7 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
229
struct stage_data *src_entry1,
230
struct stage_data *src_entry2)
231
{
232
+ int ostage1 = 0, ostage2;
233
struct rename_conflict_info *ci;
234
235
/*
@@ -264,18 +266,22 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
266
dst_entry2->rename_conflict_info = ci;
267
}
268
267
- if (rename_type == RENAME_TWO_FILES_TO_ONE) {
268
- /*
269
- * For each rename, there could have been
270
- * modifications on the side of history where that
271
- * file was not renamed.
272
- */
273
- int ostage1 = o->branch1 == branch1 ? 3 : 2;
274
- int ostage2 = ostage1 ^ 1;
269
+ /*
270
+ * For each rename, there could have been
271
+ * modifications on the side of history where that
272
+ * file was not renamed.
273
+ */
274
+ if (rename_type == RENAME_ADD ||
275
+ rename_type == RENAME_TWO_FILES_TO_ONE) {
276
+ ostage1 = o->branch1 == branch1 ? 3 : 2;
277
278
ci->ren1_other.path = pair1->one->path;
279
oidcpy(&ci->ren1_other.oid, &src_entry1->stages[ostage1].oid);
280
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
281
+ }
282
+
283
+ if (rename_type == RENAME_TWO_FILES_TO_ONE) {
284
+ ostage2 = ostage1 ^ 1;
285
286
ci->ren2_other.path = pair2->one->path;
287
oidcpy(&ci->ren2_other.oid, &src_entry2->stages[ostage2].oid);
@@ -1559,7 +1565,6 @@ static struct diff_filespec *filespec_from_entry(struct diff_filespec *target,
1565
return target;
1566
}
1567
1562
-#if 0 // #if-0-ing avoids unused function warning; will make live in next commit
1568
static int handle_file_collision(struct merge_options *o,
1569
const char *collide_path,
1570
const char *prev_path1,
@@ -1575,6 +1580,20 @@ static int handle_file_collision(struct merge_options *o,
1580
char *alt_path = NULL;
1581
const char *update_path = collide_path;
1582
1583
+ /*
1584
+ * It's easiest to get the correct things into stage 2 and 3, and
1585
+ * to make sure that the content merge puts HEAD before the other
1586
+ * branch if we just ensure that branch1 == o->branch1. So, simply
1587
+ * flip arguments around if we don't have that.
1588
+ */
1589
+ if (branch1 != o->branch1) {
1590
+ return handle_file_collision(o, collide_path,
1591
+ prev_path2, prev_path1,
1592
+ branch2, branch1,
1593
+ b_oid, b_mode,
1594
+ a_oid, a_mode);
1595
+ }
1596
+
1597
/*
1598
* In the recursive case, we just opt to undo renames
1599
*/
@@ -1678,7 +1697,38 @@ static int handle_file_collision(struct merge_options *o,
1697
*/
1698
return mfi.clean;
1699
}
1681
-#endif
1700
+
1701
+static int handle_rename_add(struct merge_options *o,
1702
+ struct rename_conflict_info *ci)
1703
+{
1704
+ /* a was renamed to c, and a separate c was added. */
1705
+ struct diff_filespec *a = ci->pair1->one;
1706
+ struct diff_filespec *c = ci->pair1->two;
1707
+ char *path = c->path;
1708
+ char *prev_path_desc;
1709
+ struct merge_file_info mfi;
1710
+
1711
+ int other_stage = (ci->branch1 == o->branch1 ? 3 : 2);
1712
+
1713
+ output(o, 1, _("CONFLICT (rename/add): "
1714
+ "Rename %s->%s in %s. Added %s in %s"),
1715
+ a->path, c->path, ci->branch1,
1716
+ c->path, ci->branch2);
1717
+
1718
+ prev_path_desc = xstrfmt("version of %s from %s", path, a->path);
1719
+ if (merge_mode_and_contents(o, a, c, &ci->ren1_other, prev_path_desc,
1720
+ o->branch1, o->branch2,
1721
+ 1 + o->call_depth * 2, &mfi))
1722
+ return -1;
1723
+ free(prev_path_desc);
1724
+
1725
+ return handle_file_collision(o,
1726
+ c->path, a->path, NULL,
1727
+ ci->branch1, ci->branch2,
1728
+ &mfi.oid, mfi.mode,
1729
+ &ci->dst_entry1->stages[other_stage].oid,
1730
+ ci->dst_entry1->stages[other_stage].mode);
1731
+}
1732
1733
static int handle_file(struct merge_options *o,
1734
struct diff_filespec *rename,
@@ -2860,47 +2910,23 @@ static int process_renames(struct merge_options *o,
2910
0 /* update_wd */))
2911
clean_merge = -1;
2912
} else if (!oid_eq(&dst_other.oid, &null_oid)) {
2863
- clean_merge = 0;
2864
- try_merge = 1;
2865
- output(o, 1, _("CONFLICT (rename/add): Rename %s->%s in %s. "
2866
- "%s added in %s"),
2867
- ren1_src, ren1_dst, branch1,
2868
- ren1_dst, branch2);
2869
- if (o->call_depth) {
2870
- struct merge_file_info mfi;
2871
- struct diff_filespec one, a, b;
2872
-
2873
- oidcpy(&one.oid, &null_oid);
2874
- one.mode = 0;
2875
- one.path = ren1->pair->two->path;
2876
-
2877
- oidcpy(&a.oid, &ren1->pair->two->oid);
2878
- a.mode = ren1->pair->two->mode;
2879
- a.path = one.path;
2880
-
2881
- oidcpy(&b.oid, &dst_other.oid);
2882
- b.mode = dst_other.mode;
2883
- b.path = one.path;
2884
-
2885
- if (merge_mode_and_contents(o, &one, &a, &b, ren1_dst,
2886
- branch1, branch2,
2887
- o->call_depth * 2, &mfi)) {
2888
- clean_merge = -1;
2889
- goto cleanup_and_return;
2890
- }
2891
- output(o, 1, _("Adding merged %s"), ren1_dst);
2892
- if (update_file(o, 0, &mfi.oid,
2893
- mfi.mode, ren1_dst))
2894
- clean_merge = -1;
2895
- try_merge = 0;
2896
- } else {
2897
- char *new_path = unique_path(o, ren1_dst, branch2);
2898
- output(o, 1, _("Adding as %s instead"), new_path);
2899
- if (update_file(o, 0, &dst_other.oid,
2900
- dst_other.mode, new_path))
2901
- clean_merge = -1;
2902
- free(new_path);
2903
- }
2913
+ /*
2914
+ * Probably not a clean merge, but it's
2915
+ * premature to set clean_merge to 0 here,
2916
+ * because if the rename merges cleanly and
2917
+ * the merge exactly matches the newly added
2918
+ * file, then the merge will be clean.
2919
+ */
2920
+ setup_rename_conflict_info(RENAME_ADD,
2921
+ ren1->pair,
2922
+ NULL,
2923
+ branch1,
2924
+ branch2,
2925
+ ren1->dst_entry,
2926
+ NULL,
2927
+ o,
2928
+ ren1->src_entry,
2929
+ NULL);
2930
} else
2931
try_merge = 1;
2932
@@ -3312,6 +3338,15 @@ static int process_entry(struct merge_options *o,
3338
conflict_info->branch2))
3339
clean_merge = -1;
3340
break;
3341
+ case RENAME_ADD:
3342
+ /*
3343
+ * Probably unclean merge, but if the renamed file
3344
+ * merges cleanly and the result can then be
3345
+ * two-way merged cleanly with the added file, I
3346
+ * guess it's a clean merge?
3347
+ */
3348
+ clean_merge = handle_rename_add(o, conflict_info);
3349
+ break;
3350
case RENAME_DELETE:
3351
clean_merge = 0;
3352
if (handle_rename_delete(o,
t/t6036-recursive-corner-cases.sh
+13
-11
@@ -185,7 +185,7 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
185
git branch B &&
186
git checkout -b C &&
187
echo 10 >>a &&
188
- echo "other content" >>new_a &&
188
+ test_write_lines 0 1 2 3 4 5 6 7 foobar >new_a &&
189
git add a new_a &&
190
test_tick && git commit -m C &&
191
@@ -195,14 +195,14 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
195
196
git checkout B^0 &&
197
test_must_fail git merge C &&
198
- git clean -f &&
198
+ git show :2:new_a >new_a &&
199
+ git add new_a &&
200
test_tick && git commit -m D &&
201
git tag D &&
202
203
git checkout C^0 &&
204
test_must_fail git merge B &&
204
- rm new_a~HEAD new_a &&
205
- printf "Incorrectly merged content" >>new_a &&
205
+ test_write_lines 0 1 2 3 4 5 6 7 bad_merge >new_a &&
206
git add -u &&
207
test_tick && git commit -m E &&
208
git tag E
@@ -225,21 +225,23 @@ test_expect_success 'git detects differently handled merges conflict' '
225
test_line_count = 1 out &&
226
227
git rev-parse >expect \
228
- D:new_a E:new_a &&
228
+ C:new_a D:new_a E:new_a &&
229
git rev-parse >actual \
230
- :2:new_a :3:new_a &&
230
+ :1:new_a :2:new_a :3:new_a &&
231
test_cmp expect actual &&
232
233
- git cat-file -p C:new_a >ours &&
234
- git cat-file -p B:new_a >theirs &&
233
+ # Test that the two-way merge in new_a is as expected
234
+ git cat-file -p D:new_a >ours &&
235
+ git cat-file -p E:new_a >theirs &&
236
>empty &&
237
test_must_fail git merge-file \
237
- -L "Temporary merge branch 1" \
238
+ -L "HEAD" \
239
-L "" \
239
- -L "Temporary merge branch 2" \
240
+ -L "E^0" \
241
ours empty theirs &&
242
sed -e "s/^\([<=>]\)/\1\1\1/" ours >expect &&
242
- git cat-file -p :1:new_a >actual &&
243
+ git hash-object new_a >actual &&
244
+ git hash-object ours >expect &&
245
test_cmp expect actual
246
)
247
'
t/t6042-merge-rename-corner-cases.sh
+2
-2
@@ -1095,8 +1095,8 @@ test_conflicts_with_adds_and_renames() {
1095
}
1096
1097
test_conflicts_with_adds_and_renames rename rename failure
1098
-test_conflicts_with_adds_and_renames rename add failure
1099
-test_conflicts_with_adds_and_renames add rename failure
1098
+test_conflicts_with_adds_and_renames rename add success
1099
+test_conflicts_with_adds_and_renames add rename success
1100
test_conflicts_with_adds_and_renames add add success
1101
1102
# Setup: