In the same spirit as a code review, I’d ask “What could go wrong?” with this idea.
Ok, I'll bite. There are a lot of broad generalizations being thrown around. It all sounds great in principle. The reality is that there are different development scenarios, and not all work well with a mechanically enforced commit barrier that takes flexibility and judgment away from developers.
-
Incremental development is more important. New prototypes will be
developed on a feature branch, less incrementally, less
transparently, and less collaboratively. The author will avoid
surrounding cleanup because that causes merge overhead. Ideally,
someone would be free to review your prototype as you work on it. I
think that would lead to much better designs and sometimes save the
author a lot of time. Unfortunately, the reality is that often no
one is available to do this, but the developer is still on a
schedule. -
Synchronous reviews are disruptive and cause priority inversion
(similar to Swift evolution). For me, reviews often take precedence
over planned work because I know someone is being held up. This
means spending more time reviewing features that are lower priority
than important planned work that, at least in my case, I'm already
spending close to zero time on. There are simply too many
disruptions and inefficiencies as it is. -
Inefficiency. It takes me much longer to review a series of patches
than it does to simply review a section of code. If I'm really
reviewing the code, it can sometimes take much longer than just
writing the code myself--it takes more time to explain how to write
code than to write it. When it comes to developing new piece of
code, I would strongly prefer to collaborate via source editors and
commits rather then web forms. -
Disincentive to make partial improvements. (I see this all the
time). There's always a better way to write the code and more that
should be done. There are often serious problems with the original
code that are being perpetuated by a change. The reviewer is
obligated to point that out. The committer is already several levels
deep in cleanup only tangential to their real task. Faced with an
inevitable review battle, the partial improvement loses out to time
pressure. -
Disincentive to be online/available most of the time. I've noticed,
in past teams, that the much more productive, disciplined engineers
than myself stay out of chat rooms because they are too
disruptive. I had to do the same with LLVM, primarily because it
greatly increased the likelihood of being hit up for inopportune code
reviews (as opposed to important questions).
My understanding is that Swift has the same review policy as LLVM. It has always worked well for LLVM. It's just not working well for Swift. The reasons have nothing to do with the policy though. Real problems:
-
A very small number of people who understand the code well enough to
truly review changes, but are far too busy with critical,
time-sensitive work to review every PR. -
In reality, neither the person making the change nor the reviewer
will have a very deep understanding of the code. That puts both in a
bad position of declaring code "reviewed" when it's just being
rubber stamped. -
I'll bet every Swift developer would love to have their PR fully
reviewed precommit. In reality, almost every commit feels like an
emergency at the time. There are always branch deadlines, critical
features being irreversibly punted, outstanding bugs that aren't
getting fixed. Enforcing a review policy doesn't stand up to this
pressure.
Changing the policy won't change the reality, or fix anything by itself. But I do think it might help expose and force solutions to those real problems. So, for that reason I think a strict(er) review policy would improve the status quo. At the very least it might help initially establish a culture that would make the policy unnecessary.