Post-Commit Reviews
medium.com
medium.com
It's a good fit for situations where code reviews are just a quick check that there's nothing seriously wrong with the commit, but if you're using your code reviews as the excellent teaching / mentoring tool that they can be, code reviews aren't a rubber stamp.
I still remember my first "real" job. The team was on TFS and my code was basically
if (condition) { if (same condition) { // do something } }
and a senior developer laughed and said mcny is testing whether we actually look at the code and not just rubber stamp his changes.
It was actually an honest mistake.
At the same place though, we had F5 load balancers and actual integration with AS/400 systems but development/test environments didn't have a load balancer in front of them :(
That sucks; I really wish people would make a better effort at staging-production parity. Like, it's one thing to run dev on a smaller cluster or older hardware (although still potentially risky!), but changing out things like how connections are passed to the environment seems like asking for incompatible changes to get pushed through and break prod.
I can't speak for everybody, but, honestly I've always fought for parity of dev and prod; usually it comes down to cost though. If infrastructure is 10% of the operating budget of something and you need a dev environment for every team (or, ever dev!) then that cost is going to explode.
I've actually put in some er, "brown M&Ms" in my code before sending it off to review, because I was sure the people reviewing my code were like "oh, he wrote it so it's probably good". I think what would've helped in that case would be anonymized code reviews & comments, where the names of the author / commenters only pops up after it has been approved & merged.
Actually I think that should be a feature everywhere, e.g. in Github issues. That way it becomes harder to judge e.g. comments / issues / commits based on the person who made it. Pseudonyms are nice, but you still build up a 'reputation' over time. But it'll probably have to be an opt-in option per user.
Often the issue is not that the DAG can't usefully hold all the alternates and piecemeal sub-solutions, in useful ways that we can come back to and revisit as necessary, but that our visualization tools on top of the DAG aren't great about letting us meaningfully interact with that (showing it when we need it, hiding it when it is noise to the signal we are looking for). (And we have the power to build better defaults for our visualization tools and/or better visualization tools.)
To use a DVCS in a distributed fashion, doesn't it make more sense to store that data in the DVCS itself?
You choose one of: (a) migrate the data, (b) leave the old system running, (c) archive the data. Same as if you need to change version control systems, or any other important system.
> What if your code review system becomes a central point of failure for information that you need about your process/workflow?
Could you elaborate? I thought we were talking about needing to answer questions like "what were they thinking when they decided to make a specific change, at a level of detail too fine for the change description?" What would be critical to your workflow, but only available in past review discussions?
> To use a DVCS in a distributed fashion, doesn't it make more sense to store that data in the DVCS itself?
Nowhere I've worked has used the version control system in a distributed way, beyond someone being able to work while they're on their laptop without wifi.
Those discussions are an "oral history" of the project. While we may hope for 100% documentation, it isn't uncommon that everything from implicit best practices to "hidden" decision processes to even customer complaints only ever get recorded in code reviews. "Why did we decide to do it this way?" is often the most common question for an archeology dig, and the answer sometimes is critical to a given workflow. If you want to fix the bug without breaking the user scenario that introduced the bug, that can be very important information to have.
> Nowhere I've worked has used the version control system in a distributed way, beyond someone being able to work while they're on their laptop without wifi.
So? Just because few do it today or think they "need" to do it other than what they believe to be rare offline scenarios, implies it isn't work striving for more distributed systems as an ideal?
That sounds like something to fix. If something is important enough that people will want to know it when working on the code later, it belongs in either a comment or a change description. When I have to reference old code review discussions it is extremely helpful to have them, but ideally you do need them only very rarely. We both recognize that these are valuable, where we differ is that I don't think they are so valuable that they need the kind of uptime you get from including them in a DVCS.
> striving for more distributed systems as an ideal?
Yes, I don't think this is very important. If it later becomes important it would be reasonable to integrate it into the DVCS.
Which is indeed what I'm arguing for. I think a lot of what gets done in code review should be directly reflected in the source control change comments. So much to the point that I think a good code review system should be not much more than a thin veneer over a commit log.
I agree it's probably a distinction of degree where we disagree. I think the goal of a source control system is to encode source changes in all their forms (including reviews/revisions). Whether or not that is for DVCS "uptime", I see pushing things to an exterior code review system, at least to me, seems a failure in your source control system (or your usage of a source control system), because I see it as such a fundamental part of what constitutes "source control".
Git's DAG as it exists already provides a lot of tools to give both "shallow visualizations" at a high level and "deep visualizations" into more complicated back-and-forth processes such as code reviews. Rather than solve these "visualization problems" by outsourcing components of it to external systems, I'd rather see that invested into the DVCS itself and/or the tools we use with the DVCS. (To some extent I think the tools are already there for git, they just have the wrong defaults to encourage more general usage of the DAG to communicate than a lot of projects tend to use.)
I want a human to read through it and ensure that the code is readable and written in a way that makes it hard to write bugs. Even if there are no bugs (well, no bugs that the tests picked up) right now, there are a ton of code structure errors I see all the time that will make it easy for someone to introduce bugs later.
I want a human to verify that the code change isn't spaghetti. Any new constructs or interfaces should be vetted by someone with an eye toward future maintenance and extensibility.
It's also possible that the entire approach taken by the committer is wrong, and someone with better context and domain knowledge will catch that. Ideally an issue like that is caught well before it gets to PR/code review time, but often that's not the case.
While I like the idea that issues can be fixed in future pull requests, that's a hard culture to push for. Many times people are pushed to get things out and then move onto the next thing. If a manager or product owner asks me why a particular thing isn't out to production yet, I can say "it's still in review, and there are issues we're working through to get it into shape to be merged" and that's usually sufficient. If they instead ask (after the feature has been merged), "why haven't we moved onto the next thing?", and I answer "we had some issues with the prior feature that need to be cleaned up post-merge", the response I will get is, "we don't have time for that; you already shipped the feature, so if there are cleanups, create tickets for them and we'll find time to deal with them later". And of course "later" never arrives, at least not until those unfixed issues inevitably cause production downtime, and then suddenly, big surprise, they're a huge priority (and the engineering team looks bad regardless).
You might say that much of what I described in the previous paragraph sounds like quite a dysfunctional organization, but, well... from my experience and from what I hear, most of us work within dysfunctional orgs, and we have to do things to work around some of the garbage... like requiring pre-merge reviews.
If you want to commit unreviewed (or even reviewed) code to a pre deployment branch, then make it just that, no? Have developers commit to a pre deployment branch and merge accepted PRs from there into master.
At some point you have to say “this code is going to trigger deployment”.
How is that not a problem in preventing logical bugs that tests didn't catch in the first developers code?
The pre-commit code review is our opportunity to stay in the loop and a bulwark against technical debt and silly mistakes before they ever touch a core branch.
The master had to stay green (or blue, actually) and all commits should have been reviewed before release. Each feature had its own short lived branch. Architectural issues and larger refactorings were of course discussed in advance. Because the code wasn't released that often, it didn't matter that much if the review was done in master or in the feature branch and this way minimized conflicts.
I wouldn't recommend it for a system with frequent deployments and more than a handful of developers.
> It also encourages the developer to keep pull requests small, ensuring a feature is developed in many small pull requests (or at the very least comprising of many atomic commits).
I am of the opinion that pre-commit reviews also encourage small pull requests, if only to make it easier on reviewers -> easy review hopefully leads to fast approvals.
> It also allows the reviewer to batch reviews, so they are reviewing a number of changes in one fell swoop, as opposed to being constantly interrupted for code reviews.
This I can see being a potential benefit depending on the workflow. You could technically do the same pre-commit by working on a longer branch and putting that up for review as well.
> In my own experience, this reduces nitpicking on the reviewer’s part, and in the event the reviewer does nitpick, it allows the developer to address the minor concerns at their leisure.
Nitpicks are what they are, and shouldn't block a PR in the first place, so not sure how much benefit this would be.
> If a design document has been clearly fleshed out and agreed upon before the code is implemented, the implementation will not comprise of any major surprises to the reviewer.
That depends - the design may have been agreed upon by someone, but not necessarily by the reviewer. There may still be major surprises, which will be harder to fix post merge than pre merge.
> This means that the reviewer can have a fair degree of confidence that the change is functionally correct and has undergone extensive testing before they review it.
This seems brittle when there are lots of changes going on at once. It may be hard to narrow down if something works or not.
> Building trust and learning how to work well as a team takes time.
I think this line makes it clear to me where my difference in opinion is. I don't even necessarily trust my own code!
I will often make a hedged critical comment, but approve the PR. I am capable of identifying my own "nitpicks" that are not functionally relevant. This allows me to provide my opinion, and track examples of where my opinion on a particular subject could have been applied, while not mucking up the process with a minority view.
Nor do I understand the argument that delaying reviews lets a developer iterate faster. If you are going to continue iterating on your work without waiting for feedback, why does it matter whether the work is merged in already or still on your side branch?
Put another way, problems are more likely to be found before release.
Actual example: Plenty of times things have passed code review on my previous teams, and only after merging to trunk and being forced to use the changes have the other devs encountered problems that needed to be dealt with.
It does slow down the other dev, but unless it was something really obvious the two of them would typically pair on it until everything was good.
This insider attack is clearly viable since something like this happened with the recent Twitter hack of verified accounts.
Even if I worked with a hall of fame team of developers, I don't want to integrate your crappy first draft that hasn't been peer reviewed or tested into my work. Generally, if you wouldn't deploy it to Prod as-is, I don't want to build on top of it.
When you allow merging of half-done work, it places additional cognitive overhead on each developer they now all have to keep up and be aware of the timeline of each other's changes and are potentially blocked from releasing until all the merged work that isn't yet released is done.
There is also the thinking that "developer velocity" = how fast you can merge stuff into a shared integration branch. This is dated, low-ownership thinking that I wouldn't want any team I am on to take part in.
The developer is not "done" until it's running in Prod. So you want process that makes that as frictionless as possible, while still having all the peer-review goodness.
The best flow I have experienced is dead simple and it's:
feature branch -> create PR -> peer-review/manual testing/business sign off -> merge to mainline -> CI/CD runs tests and deploys to Prod (from mainline)
All code that goes to the master branch must have been vetted (code reviews and tests).
It is perfectly possible to submit code to a review branch and then to merge it onto the master branch once reviewed. This can even be automated when used with code review tools, e.g. Gerrit.
Other than WIP reviews, I don't see any difference between pre-commit or post-commit reviewing - especially if they're both pre-deploy as described.