Skip to content

Commit 3f29dd6

Browse files
gitstergitster
committed
Merge branch 'en/d-f-conflict-fix'
* en/d-f-conflict-fix: merge-recursive: Avoid excessive output for and reprocessing of renames merge-recursive: Fix multiple file rename across D/F conflict t6031: Add a testcase covering multiple renames across a D/F conflict merge-recursive: Fix typo Mark tests that use symlinks as needing SYMLINKS prerequisite t/t6035-merge-dir-to-symlink.sh: Remove TODO on passing test fast-import: Improve robustness when D->F changes provided in wrong order fast-export: Fix output order of D/F changes merge_recursive: Fix renames across paths below D/F conflicts merge-recursive: Fix D/F conflicts Add a rename + D/F conflict testcase Add additional testcases for D/F conflicts Conflicts: merge-recursive.c
2 parents aca3550 + 96ecac6 commit 3f29dd6

8 files changed

Lines changed: 276 additions & 24 deletions

‎builtin/fast-export.c‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,10 +148,39 @@ static void handle_object(const unsigned char *sha1)
148148
free(buf);
149149
}
150150

151+
static int depth_first(const void *a_, const void *b_)
152+
{
153+
const struct diff_filepair *a = *((const struct diff_filepair **)a_);
154+
const struct diff_filepair *b = *((const struct diff_filepair **)b_);
155+
const char *name_a, *name_b;
156+
int len_a, len_b, len;
157+
int cmp;
158+
159+
name_a = a->one ? a->one->path : a->two->path;
160+
name_b = b->one ? b->one->path : b->two->path;
161+
162+
len_a = strlen(name_a);
163+
len_b = strlen(name_b);
164+
len = (len_a < len_b) ? len_a : len_b;
165+
166+
/* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
167+
cmp = memcmp(name_a, name_b, len);
168+
if (cmp)
169+
return cmp;
170+
return (len_b - len_a);
171+
}
172+
151173
static void show_filemodify(struct diff_queue_struct *q,
152174
struct diff_options *options, void *data)
153175
{
154176
int i;
177+
178+
/*
179+
* Handle files below a directory first, in case they are all deleted
180+
* and the directory changes to a file or symlink.
181+
*/
182+
qsort(q->queue, q->nr, sizeof(q->queue[0]), depth_first);
183+
155184
for (i = 0; i < q->nr; i++) {
156185
struct diff_filespec *ospec = q->queue[i]->one;
157186
struct diff_filespec *spec = q->queue[i]->two;

‎fast-import.c‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1528,6 +1528,14 @@ static int tree_content_remove(
15281528
for (i = 0; i < t->entry_count; i++) {
15291529
e = t->entries[i];
15301530
if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
1531+
if (slash1 && !S_ISDIR(e->versions[1].mode))
1532+
/*
1533+
* If p names a file in some subdirectory, and a
1534+
* file or symlink matching the name of the
1535+
* parent directory of p exists, then p cannot
1536+
* exist and need not be deleted.
1537+
*/
1538+
return 1;
15311539
if (!slash1 || !S_ISDIR(e->versions[1].mode))
15321540
goto del_entry;
15331541
if (!e->tree)

‎merge-recursive.c‎

Lines changed: 88 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,14 +1017,22 @@ static int process_renames(struct merge_options *o,
10171017

10181018
if (mfi.clean &&
10191019
sha_eq(mfi.sha, ren1->pair->two->sha1) &&
1020-
mfi.mode == ren1->pair->two->mode)
1020+
mfi.mode == ren1->pair->two->mode) {
10211021
/*
1022-
* This messaged is part of
1022+
* This message is part of
10231023
* t6022 test. If you change
10241024
* it update the test too.
10251025
*/
10261026
output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
1027-
else {
1027+
1028+
/* There may be higher stage entries left
1029+
* in the index (e.g. due to a D/F
1030+
* conflict) that need to be resolved.
1031+
*/
1032+
if (!ren1->dst_entry->stages[2].mode !=
1033+
!ren1->dst_entry->stages[3].mode)
1034+
ren1->dst_entry->processed = 0;
1035+
} else {
10281036
if (mfi.merge || !mfi.clean)
10291037
output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
10301038
if (mfi.merge)
@@ -1070,6 +1078,7 @@ static int process_entry(struct merge_options *o,
10701078
unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
10711079
unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
10721080

1081+
entry->processed = 1;
10731082
if (o_sha && (!a_sha || !b_sha)) {
10741083
/* Case A: Deleted in one */
10751084
if ((!a_sha && !b_sha) ||
@@ -1102,33 +1111,28 @@ static int process_entry(struct merge_options *o,
11021111
} else if ((!o_sha && a_sha && !b_sha) ||
11031112
(!o_sha && !a_sha && b_sha)) {
11041113
/* Case B: Added in one. */
1105-
const char *add_branch;
1106-
const char *other_branch;
11071114
unsigned mode;
11081115
const unsigned char *sha;
1109-
const char *conf;
11101116

11111117
if (a_sha) {
1112-
add_branch = o->branch1;
1113-
other_branch = o->branch2;
11141118
mode = a_mode;
11151119
sha = a_sha;
1116-
conf = "file/directory";
11171120
} else {
1118-
add_branch = o->branch2;
1119-
other_branch = o->branch1;
11201121
mode = b_mode;
11211122
sha = b_sha;
1122-
conf = "directory/file";
11231123
}
11241124
if (string_list_has_string(&o->current_directory_set, path)) {
1125-
const char *new_path = unique_path(o, path, add_branch);
1126-
clean_merge = 0;
1127-
output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
1128-
"Adding %s as %s",
1129-
conf, path, other_branch, path, new_path);
1130-
remove_file(o, 0, path, 0);
1131-
update_file(o, 0, sha, mode, new_path);
1125+
/* Handle D->F conflicts after all subfiles */
1126+
entry->processed = 0;
1127+
/* But get any file out of the way now, so conflicted
1128+
* entries below the directory of the same name can
1129+
* be put in the working directory.
1130+
*/
1131+
if (a_sha)
1132+
output(o, 2, "Removing %s", path);
1133+
/* do not touch working file if it did not exist */
1134+
remove_file(o, 0, path, !a_sha);
1135+
return 1; /* Assume clean till processed */
11321136
} else {
11331137
output(o, 2, "Adding %s", path);
11341138
update_file(o, 1, sha, mode, path);
@@ -1176,6 +1180,64 @@ static int process_entry(struct merge_options *o,
11761180
return clean_merge;
11771181
}
11781182

1183+
/*
1184+
* Per entry merge function for D/F conflicts, to be called only after
1185+
* all files below dir have been processed. We do this because in the
1186+
* cases we can cleanly resolve D/F conflicts, process_entry() can clean
1187+
* out all the files below the directory for us.
1188+
*/
1189+
static int process_df_entry(struct merge_options *o,
1190+
const char *path, struct stage_data *entry)
1191+
{
1192+
int clean_merge = 1;
1193+
unsigned o_mode = entry->stages[1].mode;
1194+
unsigned a_mode = entry->stages[2].mode;
1195+
unsigned b_mode = entry->stages[3].mode;
1196+
unsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode);
1197+
unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
1198+
unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
1199+
const char *add_branch;
1200+
const char *other_branch;
1201+
unsigned mode;
1202+
const unsigned char *sha;
1203+
const char *conf;
1204+
struct stat st;
1205+
1206+
/* We currently only handle D->F cases */
1207+
assert((!o_sha && a_sha && !b_sha) ||
1208+
(!o_sha && !a_sha && b_sha));
1209+
1210+
entry->processed = 1;
1211+
1212+
if (a_sha) {
1213+
add_branch = o->branch1;
1214+
other_branch = o->branch2;
1215+
mode = a_mode;
1216+
sha = a_sha;
1217+
conf = "file/directory";
1218+
} else {
1219+
add_branch = o->branch2;
1220+
other_branch = o->branch1;
1221+
mode = b_mode;
1222+
sha = b_sha;
1223+
conf = "directory/file";
1224+
}
1225+
if (lstat(path, &st) == 0 && S_ISDIR(st.st_mode)) {
1226+
const char *new_path = unique_path(o, path, add_branch);
1227+
clean_merge = 0;
1228+
output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
1229+
"Adding %s as %s",
1230+
conf, path, other_branch, path, new_path);
1231+
remove_file(o, 0, path, 0);
1232+
update_file(o, 0, sha, mode, new_path);
1233+
} else {
1234+
output(o, 2, "Adding %s", path);
1235+
update_file(o, 1, sha, mode, path);
1236+
}
1237+
1238+
return clean_merge;
1239+
}
1240+
11791241
void set_porcelain_error_msgs(const char **msgs, const char *cmd)
11801242
{
11811243
const char *msg;
@@ -1269,6 +1331,13 @@ int merge_trees(struct merge_options *o,
12691331
&& !process_entry(o, path, e))
12701332
clean = 0;
12711333
}
1334+
for (i = 0; i < entries->nr; i++) {
1335+
const char *path = entries->items[i].string;
1336+
struct stage_data *e = entries->items[i].util;
1337+
if (!e->processed
1338+
&& !process_df_entry(o, path, e))
1339+
clean = 0;
1340+
}
12721341

12731342
string_list_clear(re_merge, 0);
12741343
string_list_clear(re_head, 0);

‎t/t3509-cherry-pick-merge-df.sh‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#!/bin/sh
2+
3+
test_description='Test cherry-pick with directory/file conflicts'
4+
. ./test-lib.sh
5+
6+
test_expect_success SYMLINKS 'Setup rename across paths each below D/F conflicts' '
7+
mkdir a &&
8+
>a/f &&
9+
git add a &&
10+
git commit -m a &&
11+
12+
mkdir b &&
13+
ln -s ../a b/a &&
14+
git add b &&
15+
git commit -m b &&
16+
17+
git checkout -b branch &&
18+
rm b/a &&
19+
mv a b/a &&
20+
ln -s b/a a &&
21+
git add . &&
22+
git commit -m swap &&
23+
24+
>f1 &&
25+
git add f1 &&
26+
git commit -m f1
27+
'
28+
29+
test_expect_success SYMLINKS 'Cherry-pick succeeds with rename across D/F conflicts' '
30+
git reset --hard &&
31+
git checkout master^0 &&
32+
git cherry-pick branch
33+
'
34+
35+
test_done

‎t/t6020-merge-df.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ git commit -m "File: dir"'
2222

2323
test_expect_code 1 'Merge with d/f conflicts' 'git merge "merge msg" B master'
2424

25-
test_expect_failure 'F/D conflict' '
25+
test_expect_success 'F/D conflict' '
2626
git reset --hard &&
2727
git checkout master &&
2828
rm .git/index &&

‎t/t6031-merge-recursive.sh‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,35 @@ test_expect_success FILEMODE 'verify executable bit on file' '
5757
test -x file2
5858
'
5959

60+
test_expect_success 'merging with triple rename across D/F conflict' '
61+
git reset --hard HEAD &&
62+
git checkout -b main &&
63+
git rm -rf . &&
64+
65+
echo "just a file" >sub1 &&
66+
mkdir -p sub2 &&
67+
echo content1 >sub2/file1 &&
68+
echo content2 >sub2/file2 &&
69+
echo content3 >sub2/file3 &&
70+
mkdir simple &&
71+
echo base >simple/bar &&
72+
git add -A &&
73+
test_tick &&
74+
git commit -m base &&
75+
76+
git checkout -b other &&
77+
echo more >>simple/bar &&
78+
test_tick &&
79+
git commit -a -m changesimplefile &&
80+
81+
git checkout main &&
82+
git rm sub1 &&
83+
git mv sub2 sub1 &&
84+
test_tick &&
85+
git commit -m changefiletodir &&
86+
87+
test_tick &&
88+
git merge other
89+
'
90+
6091
test_done

‎t/t6035-merge-dir-to-symlink.sh‎

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,22 +48,70 @@ test_expect_success 'setup for merge test' '
4848
git tag baseline
4949
'
5050

51-
test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
51+
test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (resolve)' '
5252
git reset --hard &&
5353
git checkout baseline^0 &&
5454
git merge -s resolve master &&
5555
test -h a/b &&
5656
test -f a/b-2/c/d
5757
'
5858

59-
test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
59+
test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '
6060
git reset --hard &&
6161
git checkout baseline^0 &&
6262
git merge -s recursive master &&
6363
test -h a/b &&
6464
test -f a/b-2/c/d
6565
'
6666

67+
test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (resolve)' '
68+
git reset --hard &&
69+
git checkout master^0 &&
70+
git merge -s resolve baseline^0 &&
71+
test -h a/b &&
72+
test -f a/b-2/c/d
73+
'
74+
75+
test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '
76+
git reset --hard &&
77+
git checkout master^0 &&
78+
git merge -s recursive baseline^0 &&
79+
test -h a/b &&
80+
test -f a/b-2/c/d
81+
'
82+
83+
test_expect_failure 'do not lose untracked in merge (resolve)' '
84+
git reset --hard &&
85+
git checkout baseline^0 &&
86+
>a/b/c/e &&
87+
test_must_fail git merge -s resolve master &&
88+
test -f a/b/c/e &&
89+
test -f a/b-2/c/d
90+
'
91+
92+
test_expect_success 'do not lose untracked in merge (recursive)' '
93+
git reset --hard &&
94+
git checkout baseline^0 &&
95+
>a/b/c/e &&
96+
test_must_fail git merge -s recursive master &&
97+
test -f a/b/c/e &&
98+
test -f a/b-2/c/d
99+
'
100+
101+
test_expect_success 'do not lose modifications in merge (resolve)' '
102+
git reset --hard &&
103+
git checkout baseline^0 &&
104+
echo more content >>a/b/c/d &&
105+
test_must_fail git merge -s resolve master
106+
'
107+
108+
test_expect_success 'do not lose modifications in merge (recursive)' '
109+
git reset --hard &&
110+
git checkout baseline^0 &&
111+
echo more content >>a/b/c/d &&
112+
test_must_fail git merge -s recursive master
113+
'
114+
67115
test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
68116
git reset --hard &&
69117
git checkout start^0 &&
@@ -74,20 +122,28 @@ test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
74122
git tag test2
75123
'
76124

77-
test_expect_success 'merge should not have conflicts (resolve)' '
125+
test_expect_success 'merge should not have D/F conflicts (resolve)' '
78126
git reset --hard &&
79127
git checkout baseline^0 &&
80128
git merge -s resolve test2 &&
81129
test -h a/b-2 &&
82130
test -f a/b/c/d
83131
'
84132

85-
test_expect_failure 'merge should not have conflicts (recursive)' '
133+
test_expect_success 'merge should not have D/F conflicts (recursive)' '
86134
git reset --hard &&
87135
git checkout baseline^0 &&
88136
git merge -s recursive test2 &&
89137
test -h a/b-2 &&
90138
test -f a/b/c/d
91139
'
92140

141+
test_expect_success 'merge should not have F/D conflicts (recursive)' '
142+
git reset --hard &&
143+
git checkout -b foo test2 &&
144+
git merge -s recursive baseline^0 &&
145+
test -h a/b-2 &&
146+
test -f a/b/c/d
147+
'
148+
93149
test_done

0 commit comments

Comments
 (0)