On 2015-12-09 09:43:19 -0500, Noah Misch wrote: > Aware of that, I have avoided[1] saying that I was shocked to see that > commit's defects. Despite a committer-author and _two_ committer > reviewers, the patch was rife with wrong new comments, omitted updates > to comments it caused to become wrong,
It's not like that patch wasn't posted for review for months... > and unsolicited whitespace churn. Whitespace churn? The commit includes a pgindent run, because Alvaro asked me to do that, but that just affected a handful of lines. If you mean the variable ordering: given several variables were renamed anyway, additionally putting them in a easier to understand order, seems rather painless. If you mean 'pgindent immune' long lines - multixact.c is far from the only one with those, and they're prett harmless. > You call this thread's latest code a patch > submission, but I call it bandaging the tree after a recent commit > that never should have reached the tree. Oh, for christs sake. > Hey, if you'd like me to > post the traditional patch files, that's easy. It would have been > easier for me. You've been asked that, repeatedly. At least if you take 'traditional patch files' to include traditional, iterative, patches ontop of the current tree. > I hope those who have not already read commit 4f627f8 will not waste time > reading it. We have to, who knows what's hiding in there. Your git log even shows that you had conflicts in your approach (83cb04 Conflicts: src/backend/access/transam/multixact.c). > They should instead ignore multixact changes from commit 4f627f8 > through its revert. The 2015-09-26 commits have not appeared in a supported > release, and no other work has built upon them. > They have no tenure. Man. > (I am glad you talked the author out of back-patching; otherwise, > 9.4.5 and 9.3.10 would have introduced a data loss bug.) Isn't that a bug in a, as far as we know, impossible scenario? Unless I miss something there's no known case where it's "expected" that find_multixact_start() fails after initially succeeding? Sure, it sucks that the bug survived review and that it was written in the first place. But it not showing up during testing isn't meaningful, given it's a should-never-happen scenario. I'm actually kinda inclined to rip out the whole "previous pass" logic out alltogether, and replace it with a PANIC. It's a hard to test, should never happen, scenario. If it happens, things have already seriously gone sour. > > That's not a one-time problem for Andres during the course of review; > > that is a problem for every single person who looks at the commit > > history from now until the end of time. > > It's a service to future readers that no line of "git blame master <...>" will > refer to a 2015-09-26 multixact commit. And a disservice for everyone doing git log, or git blame for intermediate states of the tree. The benefit for git blame, are almost nonexistant, not seing a couple newlines changed, or not seing some intermediate commits isn't really important. > Blame reports will instead refer to > replacement commits designed to be meaningful for study in isolation. If I > instead structured the repairs as you ask, the blame would find one of 4f627f8 > or its various repair commits, none of which would be a self-contained unit of > development. So what? That's how development in general works. And how it actually happened in this specific case. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
