I wonder how fossil fanboys do review?
Say I'm supposed to review a feature to be merged into master. In the usual git-liar workflow, I get a pull request with a bunch of commits which implement the feature in logical steps. I can understand what each commit does, analyze it in isolation, verify that the software builds and works between the commits so no breakage will occur if somebody runs bisect in the future.
And without lying? You guys want me to review the whole branch, including commits like "fix a typo which broke the build" and "address the remarks of Johny Grammar Nazi"? Or maybe look at the Big Diff and try to make sense of all of it? I think I would rather be lied to ![Tongue :P](https://www.eevblog.com/forum/Smileys/default/tongue.gif)
Code review is a human issue. No tool is any good there despite the marketing. I know that because I’ve watched people hand jobbing each other’s PR’s through because they share a pint after work occasionally. I’ve watched people miss some pretty obvious shit. My favourite being the one where there was a non terminal loop iterating through dates which took out 45 production machines running 50 threads each in under twenty seconds from deployment and caused a massive SLA pay out. All because a dingbat was a bit hung over that day.
And then there’s the issue of the size or scope of a change. When that rebased and squashed PR for a feature branch comes in at 20,000 LOC no one is going to give a shit about it. Flag it through like the chequered flag at the end of a Grand Prix.
And on top of that does anyone know enough about what they’re doing to rationalise every decision made? They need to know the business domain and the programming language and the entire system well for this to be productive.
Ergo we have to ask should we even bother?
Yes we should but don’t formalise it. Drag a random colleague over and sit down and go through it together in person on your branch. Push it to someone else to get feedback on the other side of the planet. Get a QA guy to have a look at your branch. Get the build system and full integration test feedback in your branch (you are building your branches in your CI/CD platform aren’t you?). Once confidence is attained, then merge it and deploy it.
None of that is VCS or product specific.
What is the value of a PR? Near nothing.
I find it’s better to review the humans. Good programmers do the above implicitly and self organise.
Edit: for ref I was the dingbat who left the non terminal loop and it was flagged through by someone who actually accepted the review 11 seconds after I opened the PR...