On 02/11/16 13:17, Laszlo Ersek wrote: > On 02/10/16 20:37, Ryan Harkin wrote:
>> I've just updated to the tip of tianocore and I see the same problem >> as before. I have made sure I'm running the binary I built from this >> point in the tree, after the fix has been merged: >> >> 62989e0 2016-02-10 Merge branch 'master' of >> https://github.com/tianocore/edk2 [Jaben Carsey] >> >> And it doesn't work. So I went back to this commit: >> >> d2a0d2e 2016-02-08 ShellPkg: Fix ASCII and UNICODE file pipes. >> [jaben carsey] >> >> And applied the patch myself from the original email in this thread >> and it works for me. >> >> So I've looked again. If I checkout the tree right where the fix went in: >> >> 2dda8a1 2016-02-10 ShellPkg: ShellFileHandleReadLine must return >> UCS2 lines. [Jim Dailey] >> >> It also works. >> >> If I then checkout the tree at the next commit, a strange merge commit: >> >> 3a01358 2016-02-10 Merge branch 'master' of >> https://github.com/tianocore/edk2 [Jaben Carsey] >> >> Then part of the fix vanishes and the code no longer works. >> >> If I do a "git show -1 -m 3a01358", then I can see that the merge >> commit does indeed appear to add some of the code back. >> >> A further merge commit in the tree: >> >> 62989e0 2016-02-10 Merge branch 'master' of >> https://github.com/tianocore/edk2 [Jaben Carsey] >> >> Has loads of other funky stuff in it and it adds part of the change >> back, but only the comment part, the code code hunk that fixes my >> problem. >> >> Whilst I was surprised to see these merge commits in the tree in the >> first place, I didn't think too much about it. But now I'm seeing the >> diffs they are introducing, I'm either going mad or I'm getting >> worried about the workflow in the tree :( > > You should be getting worried about the workflow. The forking and the > merges were unintended. It's growing pains after the move to git. > > I'll do my best to remedy the damage. I think we managed to reconstruct what happened here. I recommend that you open the git history in gitk, so you can jump on the commits I'm about to mention. (1) Commit 8de84d424221 ("ArmVirtPkg: implement ArmVirtQemuKernel") is the last linear commit in the history. At that point the history was forked into two branches. One of those is (apparently) Jaben's local master branch, and the other is the real upstream master branch. (2) The patch "ShellPkg: Fix ASCII and UNICODE file pipes." got applied to *both* branches, identically. I don't know how this happened, but it is present in what I think was Jaben's local master branch (commit 9ed21946c76e), and also on the real master branch (d2a0d2e6acea). (3) I think Jaben didn't notice that his local master diverged from remote master. While Leif and Ard went on to commit 12 patches in total on top of d2a0d2e6acea, on the real master branch, Jaben committed the patch "ShellPkg: ShellFileHandleReadLine must return UCS2 lines" to his *diverged* local master branch (2dda8a123297). I think Jaben then tried to push his diverged local master to upstream, as remote master. Github must have rejected it (correctly) -- it would have been a non-fast-forward push, losing Leif's commits. (4) So Jaben followed the instructions on the wiki, and issued "git pull", about two minutes after applying "ShellPkg: ShellFileHandleReadLine must return UCS2 lines" (2dda8a123297) to his diverged master. Unfortunately, since his branch had diverged, and he (unknowingly) didn't request a fetch+rebase, but a fetch+merge, this created an actual merge commit (3a01358bdb03). (5) Funnily enough, Jaben could *still* not push his diverged local master to remote master, because at the exact same (6 seconds after Jaben's merge in (4)) time Ard committed more patches to the *right* master branch, and pushed them too (first of those is a4626006bbf8). So github rejected Jaben's push again. (6) Therefore Jaben pulled again, about four minutes later, which created the *second* merge commit on his diverged local master branch (62989e0bd2bf). This action finally caught up with the right master branch, altough with a non-linear history, so github permitted Jaben to push his now-unified branch to github as the new master HEAD. (7) Unfortunately, Jaben's *first* pull in (4) must have resulted in a botched merge conflict resolution, which actually lost code from 2dda8a123297 -- step (3) -- on Jaben's local, diverged, master. When Ryan pulled the tree (about two and half hours after step (6)), he found out about this, and reported it. (8) About two hours later, Jaben committed and pushed the hunks that were lost in step (4) as a separate patch: 1095b432. The tree is now in a correct state (at 1095b432). I verified it by locally restoring the tree state to Ard's last commit on the real master branch (bbff41c11fd374c7818c96f69c28b51c9caba619), and applying the one patch that Jaben wanted to get in all along ("ShellPkg: ShellFileHandleReadLine must return UCS2 lines."). This state is identical to 1095b432. The moral is that all maintainers must be acutely aware of what "local branches that track remote branches" mean, what divergence means, and they must ensure they never create merge commits in their local history that they actually push to upstream later. Leif recommended a good practice for this: always use git pull --rebase or set the --rebase flag permanently in your git config, with git config --global pull.rebase true The effect of this would have been, for step (4) above: instead of a merge commit in Jaben's local branch, his branch would have been *rebased* on top of a fast-forwarded base (at that point: 7d0f92e8fd68). Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
