merge-recursive: apply collision handling unification to recursive case
PATCH 1/1 merge-recursive: apply collision handling unification to recursive case
From: Elijah Newren <[email protected]>
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.
---
merge-recursive: apply collision handling unification to recursive case
This commit ties up a loose end that I was unaware of with the en/merge-path-collision topic from very early 2019.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-715%2Fnewren%2Frecursive-collision-handling-redux-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-715/newren/recursive-collision-handling-redux-v1
Pull-Request: https://github.com/git/git/pull/715
merge-recursive.c | 152 +++++++++--------------------- t/t6036-recursive-corner-cases.sh | 39 ++++++-- 2 files changed, 77 insertions(+), 114 deletions(-)
merge-recursive.c +47 -105
@@ -1557,35 +1557,6 @@ static int handle_file_collision(struct merge_options *opt, b, a); } -/* -* In the recursive case, we just opt to undo renames -*/ -if (opt->priv->call_depth && (prev_path1 || prev_path2)) { -/* Put first file (a->oid, a->mode) in its original spot */ -if (prev_path1) { -if (update_file (opt,1 , a, prev_path1)) -return -1 ; - }else { -if (update_file (opt,1 , a, collide_path)) -return -1 ; - } - -/* Put second file (b->oid, b->mode) in its original spot */ -if (prev_path2) { -if (update_file (opt,1 , b, prev_path2)) -return -1 ; - }else { -if (update_file (opt,1 , b, collide_path)) -return -1 ; - } - -/* Don't leave something at collision path if unrenaming both */ -if (prev_path1 && prev_path2) -remove_file (opt,1 , collide_path,0 ); - -return 0 ; - } -/* Remove rename sources if rename/add or rename/rename(2to1) */ if (prev_path1) remove_file(opt, 1, prev_path1,
@@ -1746,85 +1717,56 @@ static int handle_rename_rename_1to2(struct merge_options *opt,return -1 ;free (path_desc); -if (opt->priv->call_depth) { -/* -* FIXME: For rename/add-source conflicts (if we could detect -* such), this is wrong. We should instead find a unique -* pathname and then either rename the add-source file to that -* unique path, or use that unique path instead of src here. -*/ -if (update_file (opt,0 , &mfi.blob, o->path)) -return -1 ; +if (opt->priv->call_depth) +remove_file_from_index (opt->repo->index, o->path); -/* -* Above, we put the merged content at the merge-base's -* path. Now we usually need to delete both a->path and -* b->path. However, the rename on each side of the merge -* could also be involved in a rename/add conflict. In -* such cases, we should keep the added file around, -* resolving the conflict at that path in its favor. -*/ - add = &ci->ren1->dst_entry->stages[flip_stage (2 )]; -if (is_valid (add)) { -if (update_file (opt,0 , add, a->path)) -return -1 ; - } -else -remove_file_from_index (opt->repo->index, a->path); - add = &ci->ren2->dst_entry->stages[flip_stage (3 )]; -if (is_valid (add)) { -if (update_file (opt,0 , add, b->path)) -return -1 ; - } -else -remove_file_from_index (opt->repo->index, b->path); +/* +* For each destination path, we need to see if there is a +* rename/add collision. If not, we can write the file out +* to the specified location. +*/ + add = &ci->ren1->dst_entry->stages[flip_stage (2 )]; +if (is_valid (add)) { + add->path = mfi.blob.path = a->path; +if (handle_file_collision (opt, a->path, +NULL ,NULL , + ci->ren1->branch, + ci->ren2->branch, + &mfi.blob, add) <0 ) +return -1 ; }else { -/* -* For each destination path, we need to see if there is a -* rename/add collision. If not, we can write the file out -* to the specified location. -*/ - add = &ci->ren1->dst_entry->stages[flip_stage (2 )]; -if (is_valid (add)) { - add->path = mfi.blob.path = a->path; -if (handle_file_collision (opt, a->path, -NULL ,NULL , - ci->ren1->branch, - ci->ren2->branch, - &mfi.blob, add) <0 ) -return -1 ; - }else { -char *new_path =find_path_for_conflict (opt, a->path, - ci->ren1->branch, - ci->ren2->branch); -if (update_file (opt,0 , &mfi.blob, - new_path ? new_path : a->path)) -return -1 ; -free (new_path); -if (update_stages (opt, a->path,NULL , a,NULL )) -return -1 ; - } +char *new_path =find_path_for_conflict (opt, a->path, + ci->ren1->branch, + ci->ren2->branch); +if (update_file (opt,0 , &mfi.blob, + new_path ? new_path : a->path)) +return -1 ; +free (new_path); +if (!opt->priv->call_depth && +update_stages (opt, a->path,NULL , a,NULL )) +return -1 ; + } - add = &ci->ren2->dst_entry->stages[flip_stage (3 )]; -if (is_valid (add)) { - add->path = mfi.blob.path = b->path; -if (handle_file_collision (opt, b->path, -NULL ,NULL , - ci->ren1->branch, - ci->ren2->branch, - add, &mfi.blob) <0 ) -return -1 ; - }else { -char *new_path =find_path_for_conflict (opt, b->path, - ci->ren2->branch, - ci->ren1->branch); -if (update_file (opt,0 , &mfi.blob, - new_path ? new_path : b->path)) -return -1 ; -free (new_path); -if (update_stages (opt, b->path,NULL ,NULL , b)) -return -1 ; - } + add = &ci->ren2->dst_entry->stages[flip_stage (3 )]; +if (is_valid (add)) { + add->path = mfi.blob.path = b->path; +if (handle_file_collision (opt, b->path, +NULL ,NULL , + ci->ren1->branch, + ci->ren2->branch, + add, &mfi.blob) <0 ) +return -1 ; + }else { +char *new_path =find_path_for_conflict (opt, b->path, + ci->ren2->branch, + ci->ren1->branch); +if (update_file (opt,0 , &mfi.blob, + new_path ? new_path : b->path)) +return -1 ; +free (new_path); +if (!opt->priv->call_depth && +update_stages (opt, b->path,NULL ,NULL , b)) +return -1 ; }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' ' test_must_fail git merge -s recursive R2^0 && git ls-files -s >out && - test_line_count = 2 out && + test_line_count = 5 out && git ls-files -u >out && - test_line_count = 2 out && + test_line_count = 3 out && git ls-files -o >out && test_line_count = 1 out &&
@@ -133,9 +133,9 @@ test_expect_success 'merge criss-cross + rename merges with basic modification' test_must_fail git merge -s recursive R2^0 && git ls-files -s >out && - test_line_count = 2 out && + test_line_count = 5 out && git ls-files -u >out && - test_line_count = 2 out && + test_line_count = 3 out && git ls-files -o >out && test_line_count = 1 out &&
@@ -218,8 +218,18 @@ test_expect_success 'git detects differently handled merges conflict' ' git ls-files -o >out && test_line_count = 1 out && - git rev-parse >expect \ - C:new_a D:new_a E:new_a && + git cat-file -p C:new_a >ours && + git cat-file -p C:a >theirs && + >empty && + test_must_fail git merge-file \ + -L "Temporary merge branch 1" \ + -L "" \ + -L "Temporary merge branch 2" \ + ours empty theirs && + sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked && + git hash-object ours-tweaked >expect && + git rev-parse >>expect \ + D:new_a E:new_a && git rev-parse >actual \ :1:new_a :2:new_a :3:new_a && test_cmp expect actual &&
@@ -257,7 +267,8 @@ test_expect_success 'git detects differently handled merges conflict, swapped' ' ctime=$(git log --no-walk --date=raw --format=%cd C | awk "{print \$1}") && newctime=$(($btime+1)) && git fast-export --no-data --all | sed -e s/$ctime/$newctime/ | git fast-import --force --quiet && - # End of differences; rest is copy-paste of last test + # End of most differences; rest is copy-paste of last test, + # other than swapping C:a and C:new_a due to order switch git checkout D^0 && test_must_fail git merge -s recursive E^0 &&
@@ -269,8 +280,18 @@ test_expect_success 'git detects differently handled merges conflict, swapped' ' git ls-files -o >out && test_line_count = 1 out && - git rev-parse >expect \ - C:new_a D:new_a E:new_a && + git cat-file -p C:a >ours && + git cat-file -p C:new_a >theirs && + >empty && + test_must_fail git merge-file \ + -L "Temporary merge branch 1" \ + -L "" \ + -L "Temporary merge branch 2" \ + ours empty theirs && + sed -e "s/^\([<=>]\)/\1\1\1/" ours >ours-tweaked && + git hash-object ours-tweaked >expect && + git rev-parse >>expect \ + D:new_a E:new_a && git rev-parse >actual \ :1:new_a :2:new_a :3:new_a && test_cmp expect actual &&
base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc
Signature · 1 line
gitgitgadget