Re: [PATCH 3/3] merge-ort: fix issue with dual rename and add/add conflict
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jun 27, 2022, 18:47 UTC
- Message-ID
- <20220627184744.1361309-1-jonathantanmy@google.com>
- In-Reply-To
- <bb2badccb71d76efe0e47431246376b1e7016b05.1655871652.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> The testcases added in t6423 a couple commits ago are slightly different > but similar in principle. They involve a similar case of paired > renaming but instead of A/ -> B/ and B/ -> C/, the second side renames > a leading directory of B/ to C/.
Hmm...one side moved sub1 -> sub3 and the other moved sub2 from the root to under sub1, right? So it's more like A/ -> B/ and C/ -> A/C/.
> And both sides add a new file > somewhere under the directory that the other side will rename.
Rename or move, I think.
Show 6 quoted lines
> While > the new files added start within different directories and thus could > logically end up within different directories, it is weird for a file > on one side to end up where the other one started and not move along > with it. So, let's just turn off directory rename detection in this > case as well.
Makes sense.
Show 11 quoted lines
> diff --git a/merge-ort.c b/merge-ort.c > index fa6667de18c..5bcb9a4980b 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -2292,9 +2292,13 @@ static char *check_for_directory_rename(struct merge_options *opt, > struct strmap_entry *rename_info; > struct strmap_entry *otherinfo = NULL; > const char *new_dir; > + int other_side = 3 - side_index; > > + /* Cases where there is no new path, so we return NULL */
What do you mean by "no new path"?
> if (strmap_empty(dir_renames)) > return new_path; > + if (strmap_get(&collisions[other_side], path)) > + return new_path;
So as far as I understand, collisions here, for a given side, is a map. The map's keys are all the filenames of added and renamed files (I'm assuming that's what 'A' and 'R' are) from that side after any directory moves on the other side are applied. So, for example, if we add "foo/a" on side A and rename "foo/" to "bar/" on side B, then side A's collision map would have a key "bar/a". So I'm not sure if "collision" is the right name here, but the existing code already uses it so I'll leave it be.
It makes sense that this situation (of side A having "bar/a" because side B renamed "foo/" to "bar/", and at the same time, side B adds its own "bar/a") is weird, as stated in the commit message, so I don't mind disabling checking for directory rename in this case. However, in theory, I don't see how disabling this check would fix anything, since the bug seems to be about two different files on different sides being renamed to the same filename through some convoluted means. (Unless this is the only convoluted means to do it, and disabling it means that the bug wouldn't occur.)