On 2016-03-08 04:12:32, David Woodhouse wrote: > As of yesterday, *all* my patches had been merged into OpenSSL HEAD, in > preparation for the OpenSSL 1.1 Beta 1 release this week. > > There is only one more patch outstanding in my OpenSSL git tree. > > Do those statements seem self-contradictory to you? Well, they're not > quite. The patches *were* merged, but one was then reverted because it > was utterly hosed and broke the build for fairly much *everyone* except > the UEFI target. Bad dwmw2. No biscuit. > > Why was it so broken? Well, it wasn't because I'm a *complete* muppet — > it was correct when I first committed it, and I *tested* it in a native > Linux build at that point too, as well as within the EDK2 build: > http://git.infradead.org/users/dwmw2/openssl.git/commitdiff/929ae044cf7 > > However, a subsequent change in the upstream tree affected my patch: > http://git.infradead.org/users/dwmw2/openssl.git/commitdiff/8731a4fcd#patch7 > > At some point I stupidly did a 'git pull --rebase' or something else > that you should never do. In rebasing, I then failed to correctly fix > up the changes, leaving it broken. I did a quick smoke-test rebuild > (under EDK2 only, it seems) of the rebased tree, but didn't spot the > error. >
It sounds like the issue was a lack or gap in testing after the rebase. I don't see that possibility going away just because you instead used merge. Especially if you consider resolving merge conflicts or other subtle errors that can creep in during a merge. (3a01358bdb03) > Now, you can point out that this involved user error on my part when I > did the rebase — but there are a number of problems with putting it > down to that alone: > > Firstly, it's a *predictable* error. You are *never* paying as much > attention (to each and every commit) when you rebase, as when you > create them in the first place. A process should not introduce > *predictable* user errors. > > In fact, although in *this* case it needed user error, there are cases > where the tools *will* handle the merge automatically, but the code is > still broken — you're not handling a new enum value that someone else > has added in the upstream you're rebasing onto, or something like that. > So the patch *applies* OK, but doesn't work. Or works most of the time > but has broken corner cases. > > And the most important thing is that when this happens during a rebase, > the history — which is the single most important thing that a version > control system exists to preserve! — is lost. When the breakage is > discovered, there's no way to go back and see "it works <here> and then > the merge <here> was broken". You just end up with a false history in > which a given commit *never* worked. > > Now, this one is a trivial (and recent) example, and it's easy enough > to sort out and even *find* the original commit in my local tree which > was subsequently discarded in the rebase. I'm using it because it was > *me* who screwed up, and I can rant at length about the *practice* of > rebasing, without making some other poor sod feel bad. We all screw up. > The point is to use the tools to help us *cope*. > > Although *this* one was simple, it *does* happen that more complex > submissions also end up broken even by the time they are merged. And > when that happens, if the tools are being used *properly*, you can > *see* that it actually worked on the commit just *before* it was > merged, and it was broken in the merge. And you can look for the cause > — what changed in the upstream, between the version that the work was > based on, and the tree it was merged into. > > This isn't a purely theoretical concern. I've *done* this. And I've > done it as a third party with little familiarity with the specific code > in question, a *long* time after it was merged. Because it was only a > relatively esoteric corner case that was broken, so nobody noticed — > the kind of thing you think about while you're *writing* the code, and > never again. Especially not while rebasing. > > (Which is also an argument for decent unit tests, mind you, but that's > a different rant and one on which I'm even *more* hypocritical.) > > So please, DO NOT REBASE submissions onto the latest master. Use the > tools properly as they were designed to be used — put any non-trivial > work into a tree and send it as a pull request. Sure, send the patches > to the list for review *too*, but that shouldn't be how they actually > get in. > I've worked on projects that generally rebase, and still regularly have patchsets larger than 50 patches contributed. EDK II has been forced to live in the rebase mindset for years due to svn, and rebasing has not been a problem for us either. At the subsystem level, I think even the kernel often relies on rebase along with format-patch/am to bring in patches rather than merges. So I disagree with the statement that rebase should *NEVER* be used. For EDK II, we decided to start with a rebase model as it was a smaller step (conceptually) from svn. I'm not sure what happened in your rebase, but I think issues with this model will be no more frequent than issues with merges. I don't think anyone has ruled out considering using merges once everyone is more comfortable with git. -- One other thing. Someone at Intel decided to make a commitment to bring all git based patches back into the old svn tree. For how long, I don't know. As you might imagine, this process is fairly delicate, and I don't think it would cope well at all with a git merge free-for-all. Taken all together, I think this means git merge and git pull (with merge) is a topic that we need to save to revisit sometime down the road. Anyway, I'll brace for your cluebat pummeling. :) -Jordan > Note: using 'git rebase --interactive' just to add Reviewed-by: and > similar tags is acceptable. But don't actually rebase onto a different > base. Keep it where it was, and *merge* it. > > -- > David Woodhouse Open Source Technology Centre > [email protected] Intel Corporation > > > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
