Skip to content

Commit e3349f2

Browse files
gitstergitster
committed
Merge branch 'en/merge-dual-dir-renames-fix'
Fixes a long-standing corner case bug around directory renames in the merge-ort strategy. * en/merge-dual-dir-renames-fix: merge-ort: fix issue with dual rename and add/add conflict merge-ort: shuffle the computation and cleanup of potential collisions merge-ort: make a separate function for freeing struct collisions merge-ort: small cleanups of check_for_directory_rename t6423: add tests of dual directory rename plus add/add conflict
2 parents 3d3874d + 751e165 commit e3349f2

File tree

2 files changed

+153
-26
lines changed

2 files changed

+153
-26
lines changed

‎merge-ort.c

Lines changed: 48 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2398,6 +2398,27 @@ static void compute_collisions(struct strmap *collisions,
23982398
}
23992399
}
24002400

2401+
static void free_collisions(struct strmap *collisions)
2402+
{
2403+
struct hashmap_iter iter;
2404+
struct strmap_entry *entry;
2405+
2406+
/* Free each value in the collisions map */
2407+
strmap_for_each_entry(collisions, &iter, entry) {
2408+
struct collision_info *info = entry->value;
2409+
string_list_clear(&info->source_files, 0);
2410+
}
2411+
/*
2412+
* In compute_collisions(), we set collisions.strdup_strings to 0
2413+
* so that we wouldn't have to make another copy of the new_path
2414+
* allocated by apply_dir_rename(). But now that we've used them
2415+
* and have no other references to these strings, it is time to
2416+
* deallocate them.
2417+
*/
2418+
free_strmap_strings(collisions);
2419+
strmap_clear(collisions, 1);
2420+
}
2421+
24012422
static char *check_for_directory_rename(struct merge_options *opt,
24022423
const char *path,
24032424
unsigned side_index,
@@ -2406,18 +2427,23 @@ static char *check_for_directory_rename(struct merge_options *opt,
24062427
struct strmap *collisions,
24072428
int *clean_merge)
24082429
{
2409-
char *new_path = NULL;
2430+
char *new_path;
24102431
struct strmap_entry *rename_info;
2411-
struct strmap_entry *otherinfo = NULL;
2432+
struct strmap_entry *otherinfo;
24122433
const char *new_dir;
2434+
int other_side = 3 - side_index;
24132435

2436+
/*
2437+
* Cases where we don't have or don't want a directory rename for
2438+
* this path.
2439+
*/
24142440
if (strmap_empty(dir_renames))
2415-
return new_path;
2441+
return NULL;
2442+
if (strmap_get(&collisions[other_side], path))
2443+
return NULL;
24162444
rename_info = check_dir_renamed(path, dir_renames);
24172445
if (!rename_info)
2418-
return new_path;
2419-
/* old_dir = rename_info->key; */
2420-
new_dir = rename_info->value;
2446+
return NULL;
24212447

24222448
/*
24232449
* This next part is a little weird. We do not want to do an
@@ -2443,6 +2469,7 @@ static char *check_for_directory_rename(struct merge_options *opt,
24432469
* As it turns out, this also prevents N-way transient rename
24442470
* confusion; See testcases 9c and 9d of t6043.
24452471
*/
2472+
new_dir = rename_info->value; /* old_dir = rename_info->key; */
24462473
otherinfo = strmap_get_entry(dir_rename_exclusions, new_dir);
24472474
if (otherinfo) {
24482475
path_msg(opt, INFO_DIR_RENAME_SKIPPED_DUE_TO_RERENAME, 1,
@@ -2454,7 +2481,8 @@ static char *check_for_directory_rename(struct merge_options *opt,
24542481
}
24552482

24562483
new_path = handle_path_level_conflicts(opt, path, side_index,
2457-
rename_info, collisions);
2484+
rename_info,
2485+
&collisions[side_index]);
24582486
*clean_merge &= (new_path != NULL);
24592487

24602488
return new_path;
@@ -3171,18 +3199,15 @@ static int detect_regular_renames(struct merge_options *opt,
31713199
static int collect_renames(struct merge_options *opt,
31723200
struct diff_queue_struct *result,
31733201
unsigned side_index,
3202+
struct strmap *collisions,
31743203
struct strmap *dir_renames_for_side,
31753204
struct strmap *rename_exclusions)
31763205
{
31773206
int i, clean = 1;
3178-
struct strmap collisions;
31793207
struct diff_queue_struct *side_pairs;
3180-
struct hashmap_iter iter;
3181-
struct strmap_entry *entry;
31823208
struct rename_info *renames = &opt->priv->renames;
31833209

31843210
side_pairs = &renames->pairs[side_index];
3185-
compute_collisions(&collisions, dir_renames_for_side, side_pairs);
31863211

31873212
for (i = 0; i < side_pairs->nr; ++i) {
31883213
struct diff_filepair *p = side_pairs->queue[i];
@@ -3198,7 +3223,7 @@ static int collect_renames(struct merge_options *opt,
31983223
side_index,
31993224
dir_renames_for_side,
32003225
rename_exclusions,
3201-
&collisions,
3226+
collisions,
32023227
&clean);
32033228

32043229
possibly_cache_new_pair(renames, p, side_index, new_path);
@@ -3224,20 +3249,6 @@ static int collect_renames(struct merge_options *opt,
32243249
result->queue[result->nr++] = p;
32253250
}
32263251

3227-
/* Free each value in the collisions map */
3228-
strmap_for_each_entry(&collisions, &iter, entry) {
3229-
struct collision_info *info = entry->value;
3230-
string_list_clear(&info->source_files, 0);
3231-
}
3232-
/*
3233-
* In compute_collisions(), we set collisions.strdup_strings to 0
3234-
* so that we wouldn't have to make another copy of the new_path
3235-
* allocated by apply_dir_rename(). But now that we've used them
3236-
* and have no other references to these strings, it is time to
3237-
* deallocate them.
3238-
*/
3239-
free_strmap_strings(&collisions);
3240-
strmap_clear(&collisions, 1);
32413252
return clean;
32423253
}
32433254

@@ -3248,6 +3259,7 @@ static int detect_and_process_renames(struct merge_options *opt,
32483259
{
32493260
struct diff_queue_struct combined = { 0 };
32503261
struct rename_info *renames = &opt->priv->renames;
3262+
struct strmap collisions[3];
32513263
int need_dir_renames, s, i, clean = 1;
32523264
unsigned detection_run = 0;
32533265

@@ -3297,12 +3309,22 @@ static int detect_and_process_renames(struct merge_options *opt,
32973309
ALLOC_GROW(combined.queue,
32983310
renames->pairs[1].nr + renames->pairs[2].nr,
32993311
combined.alloc);
3312+
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
3313+
int other_side = 3 - i;
3314+
compute_collisions(&collisions[i],
3315+
&renames->dir_renames[other_side],
3316+
&renames->pairs[i]);
3317+
}
33003318
clean &= collect_renames(opt, &combined, MERGE_SIDE1,
3319+
collisions,
33013320
&renames->dir_renames[2],
33023321
&renames->dir_renames[1]);
33033322
clean &= collect_renames(opt, &combined, MERGE_SIDE2,
3323+
collisions,
33043324
&renames->dir_renames[1],
33053325
&renames->dir_renames[2]);
3326+
for (i = MERGE_SIDE1; i <= MERGE_SIDE2; i++)
3327+
free_collisions(&collisions[i]);
33063328
STABLE_QSORT(combined.queue, combined.nr, compare_pairs);
33073329
trace2_region_leave("merge", "directory renames", opt->repo);
33083330

‎t/t6423-merge-rename-directories.sh

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5199,6 +5199,111 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
51995199
)
52005200
'
52015201

5202+
# Testcase 12l, Both sides rename a directory into the other side, both add
5203+
# a file which after directory renames are the same filename
5204+
# Commit O: sub1/file, sub2/other
5205+
# Commit A: sub3/file, sub2/{other, new_add_add_file_1}
5206+
# Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}
5207+
#
5208+
# In words:
5209+
# A: sub1/ -> sub3/, add sub2/new_add_add_file_1
5210+
# B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
5211+
#
5212+
# Expected: sub3/{file, newfile, sub2/other}
5213+
# CONFLICT (add/add): sub1/sub2/new_add_add_file
5214+
#
5215+
# Note that sub1/newfile is not extraneous. Directory renames are only
5216+
# detected if they are needed, and they are only needed if the old directory
5217+
# had a new file added on the opposite side of history. So sub1/newfile
5218+
# is needed for there to be a sub1/ -> sub3/ rename.
5219+
5220+
test_setup_12l () {
5221+
test_create_repo 12l_$1 &&
5222+
(
5223+
cd 12l_$1 &&
5224+
5225+
mkdir sub1 sub2
5226+
echo file >sub1/file &&
5227+
echo other >sub2/other &&
5228+
git add sub1 sub2 &&
5229+
git commit -m "O" &&
5230+
5231+
git branch O &&
5232+
git branch A &&
5233+
git branch B &&
5234+
5235+
git checkout A &&
5236+
git mv sub1 sub3 &&
5237+
echo conflicting >sub2/new_add_add_file &&
5238+
git add sub2 &&
5239+
test_tick &&
5240+
git add -u &&
5241+
git commit -m "A" &&
5242+
5243+
git checkout B &&
5244+
echo dissimilar >sub2/new_add_add_file &&
5245+
echo brand >sub1/newfile &&
5246+
git add sub1 sub2 &&
5247+
git mv sub2 sub1 &&
5248+
test_tick &&
5249+
git commit -m "B"
5250+
)
5251+
}
5252+
5253+
test_expect_merge_algorithm failure success '12l (B into A): Rename into each other + add/add conflict' '
5254+
test_setup_12l BintoA &&
5255+
(
5256+
cd 12l_BintoA &&
5257+
5258+
git checkout -q A^0 &&
5259+
5260+
test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 &&
5261+
5262+
test_stdout_line_count = 5 git ls-files -s &&
5263+
5264+
git rev-parse >actual \
5265+
:0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
5266+
:2:sub1/sub2/new_add_add_file \
5267+
:3:sub1/sub2/new_add_add_file &&
5268+
git rev-parse >expect \
5269+
O:sub1/file B:sub1/newfile O:sub2/other \
5270+
A:sub2/new_add_add_file \
5271+
B:sub1/sub2/new_add_add_file &&
5272+
test_cmp expect actual &&
5273+
5274+
git ls-files -o >actual &&
5275+
test_write_lines actual expect >expect &&
5276+
test_cmp expect actual
5277+
)
5278+
'
5279+
5280+
test_expect_merge_algorithm failure success '12l (A into B): Rename into each other + add/add conflict' '
5281+
test_setup_12l AintoB &&
5282+
(
5283+
cd 12l_AintoB &&
5284+
5285+
git checkout -q B^0 &&
5286+
5287+
test_must_fail git -c merge.directoryRenames=true merge -s recursive A^0 &&
5288+
5289+
test_stdout_line_count = 5 git ls-files -s &&
5290+
5291+
git rev-parse >actual \
5292+
:0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
5293+
:2:sub1/sub2/new_add_add_file \
5294+
:3:sub1/sub2/new_add_add_file &&
5295+
git rev-parse >expect \
5296+
O:sub1/file B:sub1/newfile O:sub2/other \
5297+
B:sub1/sub2/new_add_add_file \
5298+
A:sub2/new_add_add_file &&
5299+
test_cmp expect actual &&
5300+
5301+
git ls-files -o >actual &&
5302+
test_write_lines actual expect >expect &&
5303+
test_cmp expect actual
5304+
)
5305+
'
5306+
52025307
###########################################################################
52035308
# SECTION 13: Checking informational and conflict messages
52045309
#

0 commit comments

Comments
 (0)