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. 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. 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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
