| Message ID | dea97c25e52c0c5dcba7698cdc2af742344fe36d.1624727122.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/ | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Directory rename detection can cause transitive renames, e.g. if the two > different sides of history each do one half of: > A/file -> B/file > B/ -> C/ > then directory rename detection transitively renames to give us > A/file -> C/file > > However, when C/ == A/, note that this gives us > A/file -> A/file. Heh, of course this is recent because the "guess that directory B has been moved to C in its entirety" heuristic is quite new. Nicely fixed and described. Will queue. Thanks. > > merge-recursive assumed that any rename D -> E would have D != E. While > that is almost always true, the above is a special case where it is not. > So we cannot do things like delete the rename source, we cannot assume > that a file existing at path E implies a rename/add conflict and we have > to be careful about what stages end up in the output. > > This change feels a bit hackish. It took me surprisingly many hours to > find, and given merge-recursive's design causing it to attempt to > enumerate all combinations of edge and corner cases with special code > for each combination, I'm worried there are other similar fixes needed > elsewhere if we can just come up with the right special testcase. > Perhaps an audit would rule it out, but I have not the energy. > merge-recursive deserves to die, and since it is on its way out anyway, > fixing this particular bug narrowly will have to be good enough. > > Reported-by: Anders Kaseorg <andersk@mit.edu> > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-recursive.c | 19 +++++++++++++------ > t/t6423-merge-rename-directories.sh | 4 ++-- > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index d146bb116f7..c895145a8f5 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt, > int renamed_stage = a_renames == renames1 ? 2 : 3; > int other_stage = a_renames == renames1 ? 3 : 2; > > + /* > + * Directory renames have a funny corner case... > + */ > + int renamed_to_self = !strcmp(ren1_src, ren1_dst); > + > /* BUG: We should only remove ren1_src in the base > * stage and in other_stage (think of rename + > * add-source case). > */ > - remove_file(opt, 1, ren1_src, > - renamed_stage == 2 || !was_tracked(opt, ren1_src)); > + if (!renamed_to_self) > + remove_file(opt, 1, ren1_src, > + renamed_stage == 2 || > + !was_tracked(opt, ren1_src)); > > oidcpy(&src_other.oid, > &ren1->src_entry->stages[other_stage].oid); > @@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt, > ren1->dir_rename_original_type == 'A') { > setup_rename_conflict_info(RENAME_VIA_DIR, > opt, ren1, NULL); > + } else if (renamed_to_self) { > + setup_rename_conflict_info(RENAME_NORMAL, > + opt, ren1, NULL); > } else if (oideq(&src_other.oid, null_oid())) { > setup_rename_conflict_info(RENAME_DELETE, > opt, ren1, NULL); > @@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt, > struct rename *ren = ci->ren1; > struct merge_file_info mfi; > int clean; > - int side = (ren->branch == opt->branch1 ? 2 : 3); > > /* Merge the content and write it out */ > clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path), > @@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt, > opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT && > ren->dir_rename_original_dest) { > if (update_stages(opt, path, > - NULL, > - side == 2 ? &mfi.blob : NULL, > - side == 2 ? NULL : &mfi.blob)) > + &mfi.blob, &mfi.blob, &mfi.blob)) > return -1; > clean = 0; /* not clean, but conflicted */ > } > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > index 7480daab46a..d67b5c28944 100755 > --- a/t/t6423-merge-rename-directories.sh > +++ b/t/t6423-merge-rename-directories.sh > @@ -5058,7 +5058,7 @@ test_setup_12i () { > ) > } > > -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' ' > +test_expect_success '12i: Directory rename causes rename-to-self' ' > test_setup_12i && > ( > cd 12i && > @@ -5116,7 +5116,7 @@ test_setup_12j () { > ) > } > > -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' ' > +test_expect_success '12j: Directory rename to root causes rename-to-self' ' > test_setup_12j && > ( > cd 12j &&
On 6/29/2021 12:02 AM, Junio C Hamano wrote: > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Elijah Newren <newren@gmail.com> >> >> Directory rename detection can cause transitive renames, e.g. if the two >> different sides of history each do one half of: >> A/file -> B/file >> B/ -> C/ >> then directory rename detection transitively renames to give us >> A/file -> C/file >> >> However, when C/ == A/, note that this gives us >> A/file -> A/file. > > Heh, of course this is recent because the "guess that directory B > has been moved to C in its entirety" heuristic is quite new. > > Nicely fixed and described. Will queue. Thanks. I agree that the fixes in patches 2 and 3 are rather clean. The tests added in patch 1 provide significant confidence in the result, though I had a minor question about a possible third test case. Seeing the solutions that make the two tests pass leaves me to consider that the third test case is likely unnecessary, because there is nothing special about how the directories are being renamed that must be handled by the fix. Thanks, -Stolee
diff --git a/merge-recursive.c b/merge-recursive.c index d146bb116f7..c895145a8f5 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt, int renamed_stage = a_renames == renames1 ? 2 : 3; int other_stage = a_renames == renames1 ? 3 : 2; + /* + * Directory renames have a funny corner case... + */ + int renamed_to_self = !strcmp(ren1_src, ren1_dst); + /* BUG: We should only remove ren1_src in the base * stage and in other_stage (think of rename + * add-source case). */ - remove_file(opt, 1, ren1_src, - renamed_stage == 2 || !was_tracked(opt, ren1_src)); + if (!renamed_to_self) + remove_file(opt, 1, ren1_src, + renamed_stage == 2 || + !was_tracked(opt, ren1_src)); oidcpy(&src_other.oid, &ren1->src_entry->stages[other_stage].oid); @@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt, ren1->dir_rename_original_type == 'A') { setup_rename_conflict_info(RENAME_VIA_DIR, opt, ren1, NULL); + } else if (renamed_to_self) { + setup_rename_conflict_info(RENAME_NORMAL, + opt, ren1, NULL); } else if (oideq(&src_other.oid, null_oid())) { setup_rename_conflict_info(RENAME_DELETE, opt, ren1, NULL); @@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt, struct rename *ren = ci->ren1; struct merge_file_info mfi; int clean; - int side = (ren->branch == opt->branch1 ? 2 : 3); /* Merge the content and write it out */ clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path), @@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt, opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT && ren->dir_rename_original_dest) { if (update_stages(opt, path, - NULL, - side == 2 ? &mfi.blob : NULL, - side == 2 ? NULL : &mfi.blob)) + &mfi.blob, &mfi.blob, &mfi.blob)) return -1; clean = 0; /* not clean, but conflicted */ } diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 7480daab46a..d67b5c28944 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -5058,7 +5058,7 @@ test_setup_12i () { ) } -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' ' +test_expect_success '12i: Directory rename causes rename-to-self' ' test_setup_12i && ( cd 12i && @@ -5116,7 +5116,7 @@ test_setup_12j () { ) } -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' ' +test_expect_success '12j: Directory rename to root causes rename-to-self' ' test_setup_12j && ( cd 12j &&