63 comments

[ 3.0 ms ] story [ 124 ms ] thread
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.
That's not how I read it. I read it as, don't push directly to master.
Don't push directly to master isn't what anyone is going to think the title of the article means, either.
You're saying that in reply to someone who clearly did, which seems disingenuous. I also read it the way the author intended.
I don't think "disingenuous" is necessarily appropriate here. When people say "anyone" they don't generally mean "anyone, without exception".
Never assume or generalize.

I thought that's what the title meant as well.

I assumed the title couldn't possibly mean that, but I also had no idea what else it could mean.
Why couldn't the title mean that? It's not an unreasonable restriction.
Sorry, I'm referring a bit too far up the chain with "that." The only meaning I could imagine for the title was that one shouldn't place code into source control at all before code review, which is too absurd to possibly be what was meant.
(comment deleted)
(oops! sorry for the accidental flag. I was trying to reply and misclicked)

That's the very first thing that occurred to me, actually.

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.

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

A completely misguided rant at that. The author even admits it: "To be fair, you can adopt this workflow with Subversion, using either task branches or patches..." This is exactly how my team uses Subversion (and used CVS before it; yes, we used this exact "branch per issue, code review before merge" workflow with CVS up through 2013) and we don't find the overhead of branching and merging onerous. Granted, I haven't ever tried contributing to an open source project using SVN, but arguably Git is the more-correct tool for that kind of development anyways.

We use branches with SVN, but it's slow and creates a lot of overhead on quick tasks. People who can get away with it work out of trunk and commit directly to trunk when they can.
> 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.

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.
I often just use `rebase -i` as a way to squash + fixup commit messages.
Presumably each change in the history will be reviewed, though, right? One assumes the point of doing rebase -i to reorganize commits during review is to present a history with MORE and BETTER context -- one which can be more easily reviewed and understood and where changes can be accepted in-order and piecemeal.
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.
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.
> 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.

Au contraire, rebase can break bisect by creating commits that were never actually tested or compiled. And if your bisection algorithm cannot handle merges, then that's a problem with a bad bisection algorithm.

Nor does it make history more readable; it makes history more readable in the same sense that inlining all procedures makes a program more readable (i.e. not really).

Merge commits are an abstraction mechanism. You can view a branch's history as a purely linear history (git log --first-parent) of normal and merge commits, where merge commits represent the merged branches (and where the commit message should summarize the branch changes properly). You can then unfold merge commits and view the branches they represent in such a quasi-linear fashion, too. Repeat as needed.

This can also guide a bisection algorithm (bisect along git log --first-parent, descending down the merge hierarchy as necessary) and is generally more readable if presented properly (that Git doesn't have a tool other than git log --first-parent for this is a different story). Conversely, a fully linearized history is longer and lacks any structure (as I said, it's like manually inlining all procedures in a piece of code).

> Au contraire, rebase can break bisect by creating commits that were never actually tested or compiled. And if your bisection algorithm cannot handle merges, then that's a problem with a bad bisection algorithm.

This is only true if you allow commits that haven't individually passed your CI suite to be pushed. That's insane.

> Nor does it make history more readable; it makes history more readable in the same sense that inlining all procedures makes a program more readable (i.e. not really).

Working at an organization (Google) that for the most part doesn't use branches and where other merges are impossible (every file must have previously been resolved against whatever is at HEAD prior to submitting), I respectfully disagree[0]. A linear history of individually reviewed and validated changes is MUCH easier to deal with than the regularly branched/merged history of my previous gig (which did use git, along with pre-push change review for each change).

> Merge commits are an abstraction mechanism. You can view a branch's history as a purely linear history (git log --first-parent) of normal and merge commits, where merge commits represent the merged branches (and where the commit message should summarize the branch changes properly). You can then unfold merge commits and view the branches they represent in such a quasi-linear fashion, too. Repeat as needed.

They're a leaky abstraction, though. Unless you mutate published history (which I do think is insane) it's inevitable that your nice bundle of merged changes will one day be incomplete: additional changes to make the bundle actually function as intended will be required. These will end up detached from it as some other commit series on HEAD or merged into HEAD. I find it far easier to tie together a series of changes via things like issue tags in the commit than expecting a merge commit to properly bundle things up.

[0]: Mind you, Google also for the most part does not use git -- we do our own thing, and with that history is necessarily linear, but I've found I really do prefer it that way. See: http://www.wired.com/2015/09/google-2-billion-lines-codeand-...

> This is only true if you allow commits that haven't individually passed your CI suite to be pushed. That's insane.

It's also pretty common in open source projects (due to limited CI resources or other reasons). I also note that this is not the only problem: the problem is that these commits never existed in the wild. This can also lead to subtle problems that CI simply can't catch.

> Working at an organization (Google) that for the most part doesn't use branches and where other merges are impossible (every file must have previously been resolved against whatever is at HEAD prior to submitting), I respectfully disagree[0]. A linear history of individually reviewed and validated changes is MUCH easier to deal with than the regularly branched/merged history of my previous gig (which did use git, along with pre-push change review for each change).

Your problem here is that Git does not have a good way of displaying structured history and just throws the raw DAG or a topologically or chronologically sorted list at the user; lack of named branches means that you're largely looking at an anonymous soup of objects with no tools to filter them. Contrast with Bazaar's hierarchical logs (augmented with branch nicknames), for example, if you want to make a serious argument.

> They're a leaky abstraction, though. Unless you mutate published history (which I do think is insane)

Published mutable history is not per se insane. Please familiarize yourself with Darcs/Pijul/Mercurial's changeset evolution. Note that I am not saying this is necessarily a good idea, it's just not per se insane. Not all projects have the same requirements/workflow/number of contributors/etc.

> it's inevitable that your nice bundle of merged changes will one day be incomplete: additional changes to make the bundle actually function as intended will be required. These will end up detached from it as some other commit series on HEAD or merged into HEAD.

Note that when I say that this is an abstraction mechanism, it's not the only abstraction mechanism. This, again, is a Git problem [1], not a VCS problem. This is, for example, where named branches come into play; see in particular Fossil's tagging mechanism (which has little to do with Git-style tags) or Veracity's stamps.

[1] Git is a solid workhorse for a DVCS, but it is a very basic version control system, sort of the lowest common denominator in the DVCS world. This makes it a poor basis for arguments as to what can be done in a VCS.

I thought the -i option was irrelevant to whether the merge commit is present. It's the "putting your commits directly downstream of master" that avoids the merge commit, not the use of the interactive rebase features. Right?
"git rebase -i" is essentially how the entire Linux kernel process works - and IMO it works pretty beautifully. The only real downside I've seen is that it can lead to developers submitting code which breaks bisection due to not testing each individual rebased commit - in the kernel, this is somewhat alleviated by the Intel 0day bot which gives us automated build failure feedback, but it's still an issue. The benefits of a nice clean changelog though should not be underestimated.
I agree, I thought the argument was going to be against code reviewing branches, not for it.
My first thought was pull-request or patch-based workflow, but then again my day job involves the Linux kernel, and all the other projects at my workplace have adopted the kernel workflow and code review process as standard...
Yeah I read it and am still confused: Subversion doesn't let you have branches like git, where it's just some version of the code people can toy around with on their machines without affecting anyone? Any sharing of code to the central repo blocks further work?

If true, that seems like Stone Age code workflow. You might as well argue that code should be tested before deployment rather than edited on production directly, or that it's a really neat idea to keep track of who changed what code when. Duh?

Subversion does let you have branches, but every shop I have worked in that used subversion used branches rarely, if at all.

The branch-merge default of git is really remarkable, and is baked into the tool and the community.

On the other hand, in one shop where I worked we not only used Subversion branches, but also a modified GitFlow adapted for Subversion. It worked pretty well.
Yes. I currently use Subversion at work, and the only way I keep things working smoothly is by using Git locally, and then checking changes from Git into Subversion when they're fully tested (at least developer-tested, not QA-tested). A coworker recently asked permission to branch, and our manager denied the request. The last time my workflow was like this was back in the 1990s, and it certainly feels like the stone age.
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?

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.

It was, historically, definitely a technical problem. You had to know what the base changelist for a branch was in order to integrate into it from trunk or to merge it back into trunk cleanly. I believe these problems may now have been fixed, but I haven't used svn in so long that I can't say for sure.

Regardless, branching and merging in git were designed to be straightforward from the outset. Due to the nature of git's commit graph it is straightforward to tell (with appropriate tools, and this is somewhere that git suffered initially but has improved dramatically as its popularity has waxed) where in history a branch was created and which commits are being merged.

If you want a branch and merge workflow where branches are used frequently and merges are the mechanism by which changes arrive in 'trunk' (which is basically how the Linux kernel is maintained with Linus being the final integrator -- he merges branches from what are effectively lieutenants who act as integrators for their respective subsystems) then git makes this workflow very straightforward.

Some of this is historical. I started using subversion at 1.3 and merge tracking didn't exist at all until 1.5; we just upgraded to 1.8 at work and it's much smarter, though it does still lag behind git a bit.
There's probably a very good reason why there is no subversionhub.com
Should be titled "Check-in to trunk/master before code review is an antipattern".
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.

> 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.

Perforce is expensive garbage designed by mainframe programmers. The command line tool had the worst UI of any VCS I've ever used.
Correction: Google runs a single repo with everything from Search to Gmail, not a single branch.
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.

How many teams in the world have this problem?

My guess is not many. The 99% usecase in version control revolves around teams that need to enforce standards.

I was on two such teams. So for me it's like 50% so far. But your point is taken. I am more hinting at bothering your geniuses as little with distracting tasks as possible, even at cost of hiring a "cleaner" that would make sure their work is smoothly integrated with whatever flavor of version control/issue system/project management you are running. You can completely obliterate their abilities if you force them to change their workflow instead of building a workflow around them.
Well, for one, it would give your coworkers a chance to learn what's inside the black box, with you there to answer any questions.
Unfortunately, the coworkers might not be intellectually capable of understanding it at all, not even in infinite time :-( I understand this could be a management nightmare, but some things are just "given from above" and you can either turn a blind eye on them and lose the race, or use them for the benefit of your people. I would choose the latter even if I can't control it.
Huh - can you share a piece of source code that the average developer wouldn't be intellectually capable of understanding, not even in infinite time? Or point to one in the open source corpus?
For example code that computes robotic motion via non-linear optimization of non-linear differential equations? Code that computes CFD of Formula 1 car aerodynamics? Code using advanced computer vision allowing autonomous drones? How many of your developers would understand what is going on? Do you think their code review input would be different than a handful of useless comments such as "here is an extra space", "the name of this class has lowercase letters" etc.?
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.
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.

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.

Or how about this: If you have a social problem, don't start with a technological solution.
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.

What's your workflow? Do developers check in bug fixes in trunk? TIA.
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.

Developers check in all changes (whether bug fixes or new features) on task branches first, and use those branches to run tests and conduct code reviews prior to merging to the trunk. That way the trunk build almost always works, and at any point in time we can deliver the trunk code to customers.
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.
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).
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.