Fix bugs from interesting renaming pairs: one side renames A/file -> B/file, the other B/ -> A/ #1039
Conversation
|
/submit |
|
Submitted as pull.1039.git.git.1624727121.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
|
This branch is now known as |
|
This patch series was integrated into seen via be673f5. |
|
User |
|
This patch series was integrated into seen via 5e78701. |
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 C/file.
Since the default for merge.directoryRenames is conflict, this results
in an error message saying it is unclear whether the file should be
placed at B/file or C/file.
What if C/ is A/, though? In such a case, the transitive rename would
give us A/file, the original name we started with. Logically, having
an error message with B/file vs. A/file should be fine, as should
leaving the file where it started. But the logic in both
merge-recursive and merge-ort did not handle a case of a filename being
renamed to itself correctly; merge-recursive had two bugs, and merge-ort
had one. Add some testcases covering such a scenario.
Based-on-testcase-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Elijah Newren <newren@gmail.com>
Path conflicts (typically rename path conflicts, e.g. rename/rename(1to2) or rename/add/delete), and directory/file conflicts should obviously result in files not being marked as clean in the merge. We had a codepath where we missed consulting the path_conflict and df_conflict flags, based on match_mask. Granted, it requires an unusual setup to trigger this codepath (directory rename causing rename-to-self is the only case I can think of), but we still need to handle it. To make it clear that we have audited the other codepaths that do not explicitly mention these flags, add some assertions that the flags are not set. Reported-by: Anders Kaseorg <andersk@mit.edu> Signed-off-by: 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.
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>
newren
force-pushed
the
rename-plus-dir-rename-cancel
branch
from
dea97c2
to
7c96d6c
Compare
|
User |
|
/submit |
|
Submitted as pull.1039.v2.git.git.1625074200.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via d7ef8a8. |
|
The branch |
|
This patch series was integrated into seen via 7b60e3b. |
|
This patch series was integrated into seen via 026f7d0. |
|
This patch series was integrated into seen via 149c6e6. |
|
This patch series was integrated into seen via c549cf9. |
|
There was a status update in the "Cooking" section about the branch The merge code had funny interactions between content based rename detection and directory rename detection. Will merge to 'next'. |
|
This patch series was integrated into seen via 11bd56e. |
|
This patch series was integrated into seen via 9f1136e. |
|
This patch series was integrated into next via cf70875. |
|
There was a status update in the "Cooking" section about the branch The merge code had funny interactions between content based rename detection and directory rename detection. Will merge to 'master'. |
|
This patch series was integrated into seen via 0c118dc. |
|
There was a status update in the "Cooking" section about the branch The merge code had funny interactions between content based rename detection and directory rename detection. Will merge to 'master'. |
|
This patch series was integrated into seen via d3b88be. |
|
This patch series was integrated into next via d3b88be. |
|
This patch series was integrated into master via d3b88be. |
|
Closed via d3b88be. |
Anders Kaseorg recently reported a few issues in an interesting rename case[1]. I was able to duplicate and find multiple bugs from it; two in merge-recursive, and one in merge-ort. This series has some fixes.
Changes since v1:
[1] https://lore.kernel.org/git/CABPp-BGDfucqae=HNES_QmmsjpDbdHrR6CG=H3gtiDygHzquVg@mail.gmail.com/
cc: Derrick Stolee stolee@gmail.com
cc: Elijah Newren newren@gmail.com