4 ms·
Check-in before code review is an antipattern
- greglindahl 11y agoWhat 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.
- zeckalpha 11y agoThat's not how I read it. I read it as, don't push directly to master.
- greglindahl 11y agoDon't push directly to master isn't what anyone is going to think the title of the article means, either.
- ghusbands 11y agoYou're saying that in reply to someone who clearly did, which seems disingenuous. I also read it the way the author intended.
- klodolph 11y agoI don't think "disingenuous" is necessarily appropriate here. When people say "anyone" they don't generally mean "anyone, without exception".
- AznHisoka 11y agoNever assume or generalize. I thought that's what the title meant as well.
- mikeash 11y agoI assumed the title couldn't possibly mean that, but I also had no idea what else it could mean.
- zeckalpha 11y agoWhy couldn't the title mean that? It's not an unreasonable restriction.
- mikeash 11y agoSorry, 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.
- deleted 11y ago[deleted]
- koko775 11y ago(oops! sorry for the accidental flag. I was trying to reply and misclicked) That's the very first thing that occurred to me, actually.
- rhpistole 11y agoFurthermore 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.
- MrRadar 11y ago> 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.
- hyperpape 11y agoWe 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.
- jsolson 11y ago> 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 11y agoYou 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.
- joshschreuder 11y agoI agree, I thought the argument was going to be against code reviewing branches, not for it.
- ajdlinux 11y agoMy 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...
- SilasX 11y agoYeah 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?
- mooreds 11y agoSubversion 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.
- Grue3 11y agoOn 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.
- klodolph 11y agoYes. 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.
- bmm6o 11y agoI 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 11y agoGit 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.
- yincrash 11y agoThis stackoverflow post is just for you! http://stackoverflow.com/questions/19333088/how-is-svn-v-1-8-branching-merging-compared-to-git http://stackoverflow.com/questions/19333088/how-is-svn-v-1-8...
- jsolson 11y agoIt 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.
- SofDroid 11y agogood
- mentos 11y agoThere's probably a very good reason why there is no subversionhub.com
- badmadrad 11y agoThe word antipattern is an antipattern.
- JadeNB 11y ago"'Considered harmful' considered harmful" (http://meyerweb.com/eric/comment/chech.html http://meyerweb.com/eric/comment/chech.html)?
- asolove 11y agoShould be titled "Check-in to trunk/master before code review is an antipattern".
- artagnon 11y agoGit 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 11y ago> 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.
- setpatchaddress 11y agoPerforce is expensive garbage designed by mainframe programmers. The command line tool had the worst UI of any VCS I've ever used.
- bitL 11y agoHonestly, 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.
- tyre 11y agoHow 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.
- bitL 11y agoI 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.
- 13of40 11y agoWell, for one, it would give your coworkers a chance to learn what's inside the black box, with you there to answer any questions.
- bitL 11y agoUnfortunately, 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.
- tyre 11y agoNote 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 11y agoOr how about this: If you have a social problem, don't start with a technological solution.
- nradov 11y agoThere'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 11y agoWhat's your workflow? Do developers check in bug fixes in trunk? TIA.
- abstractbeliefs 11y agoNot 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.
- nradov 11y agoDevelopers 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.
- bottled_poe 11y agoThe 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 11y agoWe 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 11y agocalling 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.