Skip to content

Commit 9df56a5

Browse files
committed
rn-88: add some clarifications on the merge bug(s)
1 parent 7f19dce commit 9df56a5

File tree

1 file changed

+41
-26
lines changed

1 file changed

+41
-26
lines changed

‎rev_news/drafts/edition-88.md‎

Lines changed: 41 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ This edition covers what happened during the month of May and June 2022.
3939

4040
Glen noticed that the bug seemed specific to the "ort" merge
4141
strategy, which became the default merge strategy in Git 2.34.0
42-
released last November, as when using the "recursive" strategy,
42+
released last November. When using the "recursive" strategy,
4343
which used to be the default merge strategy before "ort" took over,
4444
the merge seemed to work as expected.
4545

@@ -62,42 +62,57 @@ This edition covers what happened during the month of May and June 2022.
6262
# B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2
6363
```
6464

65-
He then explained that both the "ort" and "recursive" merge
66-
strategies have code to avoid "doubly transitive renames". Such
67-
renames happen when, for example, on one side of the merge a
68-
directory named "A" is renamed to "B", while on the other side "B"
69-
is renamed "C".
65+
He noted, though, that he can also trigger a different fatal error
66+
in the "ort" strategy with a small tweak to the test setup, and can
67+
also trigger that same other fatal error in the "recursive" strategy
68+
with his test cases.
7069

71-
The code to avoid "doubly transitive renames" is fooled when a
72-
leading directory of a directory is renamed though. For example if
73-
on one side a directory named "A" is renamed to "B", while on the
74-
other side a leading directory of "B" is renamed to "C". That's what
75-
triggers the bug.
70+
He then explained that both the "ort" and "recursive" merge
71+
strategies have code to avoid "doubly transitive [directory]
72+
renames". Such renames happen when, for example, on one side of the
73+
merge a directory named "A" is renamed to "B", while on the other
74+
side "B" is renamed "C".
75+
76+
The code to avoid "doubly transitive [directory] renames" is fooled
77+
when a leading directory of a directory is renamed though. For
78+
example if on one side a directory named "A" is renamed to "B",
79+
while on the other side a leading directory of "B" is renamed to
80+
"C". That still wouldn't have quite been enough to trigger this
81+
bug, though. It also required adding a file into directory A on one
82+
side and a file with the same name into directory B on the other.
7683

7784
Junio Hamano, the Git maintainer, thanked Elijah for his continued
7885
support of the merge strategy, and noticed that at least the code is
79-
not "making a silent mismerge" in this special case and the
80-
recursive strategy is working.
86+
not "making a silent mismerge" in this special case, and that the
87+
recursive strategy can be used as a fallback.
8188

8289
Elijah replied that he was glad the recursive strategy worked for
83-
Glen because it didn't work either with his minimal reproduction
84-
test case.
90+
Glen but noted that it didn't work with his minimal reproduction
91+
test case, which suggests it's less reliable as a fallback than one
92+
might hope.
8593

8694
Glen then wondered if turning off rename detection could help in
87-
case of merges with complex renames like this, but Elijah suggested
88-
using the 'resolve' strategy, which "is roughly the recursive
89-
strategy minus the renames and the multiple merge base handling",
90-
instead.
91-
92-
Elijah also posted
93-
[a small patch series](https://lore.kernel.org/git/[email protected]/)
94-
that adds test cases demonstrating the bug Glen found and fixing it
95-
in the ort strategy code.
95+
case of merges with complex renames like this, but Elijah pointed
96+
out [that might be more problematic than
97+
helpful](https://lore.kernel.org/git/CABPp-BGN0DoSr3bcjTmGZkcoj_dSVzOgFUQ++R=_z8v=nAJsTg@mail.gmail.com/),
98+
particularly since this case had a very large number of renames and
99+
users tend to have difficulty correctly resolving the conflicts that
100+
result from a lack of rename detection. However, he suggested that
101+
if turning off rename detection was really wanted that one could use
102+
the 'resolve' strategy, which "is roughly the recursive strategy
103+
minus the renames and the multiple merge base handling", instead.
104+
105+
Elijah also posted [a small patch
106+
series](https://lore.kernel.org/git/[email protected]/)
107+
that adds test cases demonstrating the bug Glen found and the
108+
related ones he found based on it, and fixes the bugs in the ort
109+
strategy. (The recursive strategy is deprecated, and the bugs noted
110+
here are not security critical.)
96111

97112
Jonathan Tan reviewed the series and verified that it indeed fixes
98-
Glen's test cases. Calvin Wan also commented on the patch series. So
113+
Glen's test case. Calvin Wan also commented on the patch series. So
99114
there is good hope that after a few iterations to polish the series
100-
the bug will be fixed soon.
115+
the bugs will be fixed soon.
101116

102117
<!---
103118
## Developer Spotlight:

0 commit comments

Comments
(0)