
Check-in before code review is an antipattern - infodroid
https://jamesmckay.net/2015/07/check-in-before-code-review-is-an-antipattern/
======
greglindahl
What a misleading title! He's speaking approvingly of pull-request-based
workflow, which is not what anyone is going to think the title of the article
means.

~~~
rhpistole
Furthermore it's mostly a rant against subversion rather than going making any
kind of argument about his assertions.

Plus anybody who advocates the use of git rebase --interactive as a sensible
response to code review feedback is clearly insane.

~~~
jsolson
> Plus anybody who advocates the use of git rebase --interactive as a sensible
> response to code review feedback is clearly insane.

Why? Prior to a series of changes being pulled into someone else's history you
have all the incentive in the world to present the cleanest view of history
possible. Moreover, if you can rebase -i on top of master you can turn the
merge into a fast-forward rather than an actual merge. A more linear history
with fewer merges is much easier to bisect as well as to read, generally.

~~~
ghusbands
You present a clean history, yes, but not a correct one. The context in which
you made your changes, often noted as important, has been lost.

~~~
deathanatos
In the general case, I agree; rebases do lose a bit of context. As a code
reviewer, however, I find the reason I most often ask for a rebase & squash is
that the "context" that I'm asking to lose is simply the author isn't taking
the proper time to think about what he or she is doing, and it shows, in
commit messages like,

    
    
      * I can't spell
      * fix tests, too, lol
      * fixed for real
      * fixed
      * minor touchup
      * Implement feature foobar from issue #123
    

Once the code review has completed, this history is not going to ever be
useful again; worse, it impedes someone looking through the logs for useful
history, and other operations like cherry-picking this feature to a branch.

~~~
klodolph
I'd argue that what you're describing shouldn't even be called a rebase, even
though you could use "rebase -i" to accomplish it. For example, I just use
"reset --soft ; commit" instead.

------
artagnon
Git has some serious disadvantages when compared to a centralized VCS like
Perforce. It just can't handle anything more than 10~15 million lines of code
(this is the size of linux.git).

In a way, Perforce is a cute middle ground. It has "pending changelists" that
ReviewBoard or Phabricator can slurp up and present to reviewers. You can
easily give another person your changes by "shelving" them. Mutable history is
only really a "feature" in tiny projects; in massive ones, immutability
simplifies life greatly and restores peace of mind. In many ways, the way
Perforce handles conflicts is nicer: it doesn't pollute the actual files with
conflict markers, and has an option to resolve conflicts in a nice 3-pane UI.
Although it does get merges wrong every now and then.

Non-linear history is considered an "anti-feature" by many, and the LLVM
community runs a primary SVN server; users submit via git-svn.

Having cheap and lightweight branching is what leads to merge headaches. In
fact, (from one Tech Talk), I believe that Google runs a single branch to
evade merge problems altogether.

I think git's greatest asset is the excellent local development experience.
You can muck around as much as you like, making local branches/commits as
checkpoints, and squash them in to submit a final patch. The command-line UI
is fantastic, and everything is snappy (provided you have a small checkout). I
think having a single linear "trunk" on the server-side is a good idea.

~~~
pmiller2
> Non-linear history is considered an "anti-feature" by many, and the LLVM
> community runs a primary SVN server; users submit via git-svn.

> Having cheap and lightweight branching is what leads to merge headaches. In
> fact, (from one Tech Talk), I believe that Google runs a single branch to
> evade merge problems altogether.

Running a single branch also gets rid of the nonlinear history problem. We do
it where I work and it works great. Every developer submits PRs from feature
branches on their forked copy of the main repo, which get merged in after
review and testing. At any time, we can roll back to the previously deployed
version of prod if we have to.

Mutable history is also nice for these forked repos, since it allows us to
squash commits and eliminate any wrong turns we take in the development
process, leaving the main repo's history cleaner.

------
nradov
There's no reason you can't do code reviews on SVN branches prior to merging
to the trunk. My teams do this every day. It helps to have a dedicated code
review tool such as Atlassian Crucible.

And I wouldn't necessarily characterize SVN branching as "clunky, user-
unfriendly and error-prone". It might be slightly worse than Git, but with
modern tools like Eclipse there's hardly a difference. I've never found SVN
branching difficult at all.

~~~
nova22033
What's your workflow? Do developers check in bug fixes in trunk? TIA.

~~~
abstractbeliefs
Not him, but where I last worked once you had your dirty working directory,
you'd run rbtool to upload the changes to reviewboard, where they could get
looked at.

Once it was approved, you committed. A hook in the svn server would check that
you had a link to the reviewboard entry, the entry was approved, and that your
changes matched the diff that was approved.

------
bmm6o
I agree with the main thesis. But I wonder about:

> _Subversion makes branching and merging so clunky, user-unfriendly and
> error-prone that it simply isn’t practical_

Is this really a technical problem, or is it more cultural? I'm not very
familiar with Git... is there really something there that makes this
fundamentally easier?

~~~
Pfhreak
Git branching is fast and very simple. A 'git checkout -b FOO' and you've got
one made. You can flip back and forth between branches with a single command
as well, which makes very easy to set up experiments (say you wanted to
profile a couple different prototypes), or you are working on three different
new features at the same time.

Branches are also very easy to move around the revision history. If I created
a branch 3 months ago, and I wanted to go back and pull that back onto
mainline, I can easy 'rebase' those changes as if I created the branch off of
the most recent commit. Again, it's a single command that, barring merge
conflicts, just works.

Branches can also be pushed and pulled from one machine to another. Got
started working on a feature, but can't finish it in this sprint? Have a
coworker pull that branch to their box and pick it up. Made an experimental
change? Push that branch back to the server so others can take a look.

Git branching absolutely changed the way I write code. I create branches all
the time, because they are so cheap I can use them to mess around with small
changes without worrying about messing up mainline or my current feature
branches.

------
tyre
Note that the author's point about git (the technology) is what it enables
users to do.

Teams can still use git or subversion or mercurial and have terrible code
review processes, just as they can use Slack or IRC and have terrible
communication, or use Ruby, C, Haskell, or ECMAScript 2 and have beautiful or
nightmarish code.

Takeaways:

1) Technology is a tool, not an end in itself. 2) People are the hard part.
Technology can open the door, but that doesn't mean someone will walk through
it.

~~~
klodolph
Or how about this: If you have a social problem, don't start with a
technological solution.

------
asolove
Should be titled "Check-in to trunk/master before code review is an
antipattern".

------
bottled_poe
The recommendation seems to be to lock down the code base to the elite few.
How about instead, we implement regression testing, employ a competent
development team and communicate the software design before coding. Code
reviews address the effects, not the cause of the problem.

~~~
madeofpalk
We do peer code reviews in pull requests which require at least n people to
approve, as well has automated tests which run and report their status back
(blocking the PR if tests fail).

------
metaphorm
calling things "anti-patterns" might itself be an "anti-pattern"

I don't even disagree with the thesis of the article. its just that the choice
of language here obscures the point and makes it harder to focus on what the
author is actually trying to say.

------
badmadrad
The word antipattern is an antipattern.

~~~
JadeNB
"'Considered harmful' considered harmful"
([http://meyerweb.com/eric/comment/chech.html](http://meyerweb.com/eric/comment/chech.html))?

------
mentos
There's probably a very good reason why there is no subversionhub.com

------
bitL
Honestly, if you are working on a standalone feature with complicated
math/distributed algorithm/transactional concurrency etc. only a handful
people in the world understand, how is a code review going to help you beside
trivial matters like code formatting/naming? How do you approach this
situation in order not to upset the few people that can get it done?

I remember SUN's application servers had a licensed code inside their
transactional core nobody was able to understand nor touch; it just worked as
it was supposed to, prepared by a single genius who got it right.

~~~
detaro
You still can make dumb mistakes in genius code. And if your teammates can't
follow the code at all, then it should either be better documented or they
better educated. Having code that is untouchable because the lone genius left
the company/got hit by a bus isn't something to be celebrated.

~~~
bitL
Sure, but companies like SUN have huge automated testing suites that run after
each commit, as well as gazillion of branches. And the author of the code is
most likely contributing plenty of those tests, if only in philosophical sense
(by the hands of accompanying SDET).

And whether you like it or not, the best companies rise and fall with
geniuses. You can't automate them away. You can only accommodate them or they
go elsewhere and you are done.

