Elijah Newren <newren@xxxxxxxxx> writes: > Sorry for the long delay. I haven't gotten much Git time lately... > > On Mon, Jun 13, 2022 at 9:52 AM Glen Choo <chooglen@xxxxxxxxxx> wrote: >> > [...] >> Elijah Newren <newren@xxxxxxxxx> writes: >> >> > On Fri, Jun 10, 2022 at 9:53 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> >> >> Elijah Newren <newren@xxxxxxxxx> writes: >> >> >> >> > On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@xxxxxxxxxx> wrote: >> >> >> >> >> >> (I'm not 100% what the bug _is_, only that there is one.) >> >> >> >> >> >> = Report >> >> >> >> >> >> At $DAYJOB, there was a report that "git merge" was failing on certain >> >> >> branches. Fortunately, the repo is publicly accessible, so I can share >> >> >> the full reproduction recipe: >> >> >> ... >> >> > Thanks for the detailed report; very cool. Interestingly, if you >> >> > reverse the direction of the merge (checkout origin/upstream-master >> >> > and merge origin/master) then you get a different error: >> >> > ... >> >> > Anyway, long story short...I don't have a fix yet, but just thought >> >> > I'd mention I saw the email and spent some hours digging in. >> >> >> >> Thanks for continued support for the ort strategy. From the very >> >> beginning, I was hesitant to make our tools try to be too clever >> >> with excessive heuristics, but at least we are not making a silent >> >> mismerge in this case, so it is probably OK, especially with "-s >> >> recursive" still left as an escape hatch. >> > >> > In fact, the more general problem area here appears to affect the >> > recursive strategy as well. I'm glad the specific testcase reported >> > works under recursive and gave Glen (or his user) a workaround, but >> > that feels like luck rather than design because my minimal >> > reproduction testcase not only triggers the same issue he saw with the >> > ort strategy, but also triggers a previously unknown fatal bug in the >> > recursive strategy too. >> >> Yeah, hm. I was surprised that we encountered this bug at all, but it's >> not so surprising after seeing how many edge conditions this bug >> contains. > > To be fair, I've dug into cases with more. :-) :) >> I wonder if there other rename detection bugs lurking beyond the >> horizon, > > I was trying to dig around for related issues so I can fix the class > of problems rather than just the instance. Reversing the direction of > the merge was just one component of that (and I reported that > particular tweak since it triggered something a little different). > > The original motivation for writing merge-ort was to address bugs I > couldn't otherwise fix within merge-recursive's implementation. I've > put a lot of time into corner cases, many of which (perhaps even the > majority) were not actually motivated by real-life testcases but me > just having an obsession with making Git's merge machinery handle > weird inputs. Junio even commented on some of my testcases with 'I am > not sure if there is a single "correct" answer everybody can agree on > for each of these "insane" cases, though.'. Now, obviously I can miss > some inputs, as evidenced by the issue you reported, so there is > always a chance there are more. However... > >> whether we already assume that these bugs exist, and if so, >> whether we should officially document "merge without rename detection" >> as a workaround [1]. >> >> [1] Assuming that the workaround works of course. I tried to disable >> rename detection several times, but I couldn't really figure out whether >> I did it correctly or whether it fixed the bug (which is why I didn't >> include it in the initial report.) > > Turning off renames and relying on users to correct merge issues may > be reasonable when there are only a few. When there are more than a > few, my experience in the past with turning off rename detection (or > there being too many renames that rename detection turns itself off) > is that users often: > > * don't match up renamed files and do a three-way merge, but just > pick one of the two conflicting sides, unknowingly discarding changes > made on the other side > * sometimes notice the files that should have been renames, and > manually hand apply the subset of changes they remember from one file > to the other, and unknowingly discarding the remaining subset of > changes (which were often changes made by people other than the one > doing the merges). > > In the particular repository in question, you've got 600+ renames on > one side, and 200+ on the other -- including multiple different entire > directories. (Also, since lack of rename detection makes renames get > reported as modify/delete conflicts, and you've got 400+ actual > modify/delete conflicts on top of all the renames, users would have > lots of "fun" attempting to sort things all out without tool support.) > So, I'm worried the "fallback"/"workaround" is likely to put users in > a worse situation rather than a better one. Ah, that's a good point. I hadn't given too much thought to the complexity of manually resolving a merge without rename detection, but, like you said, it's probably not a useful fallback anyway. > But, even if your goal really is to have a fallback, why not just use > the `resolve` strategy? Your testcase doesn't have multiple merge > bases, and the resolve strategy is roughly the recursive strategy > minus the renames and the multiple merge base handling. Oh, today I learned about the `resolve` strategy.. > (Also, I'm not just avoiding work. I have already written patches to > turn off rename detection in the ort strategy. I pointed these out to > Stolee and Dscho for a special internal usecase of theirs, and at > least one of those emails cc'ed the mailing list. so you should be > able to find those patches with a search. I'm just not convinced of > the need to merge those patches.) > > > > Anyway, that all said, I posted a fix for this issue over at > https://lore.kernel.org/git/pull.1268.git.1655871651.gitgitgadget@xxxxxxxxx/. > With it, I can repeat the tensorflow merge you highlighted, in either > direction, without issue. That's fantastic. Thanks for keeping me in the loop :)
