by aarohmankad on 8/23/17, 3:45 AM with 62 comments
by hasenj on 8/23/17, 6:02 AM
> This is how the Kernel team does it, so everyone should do it that way
99% of the time, your team has absolutely nothing in common with the Kernel team and it's foolish to adopt processes from other teams without knowing why.
If anything, I would say the right way to use git in a small team is to always merge and never rebase.
Whatever you decide to do, make sure to think about your problem first, then use the tools to solve your problem.
by rcthompson on 8/23/17, 5:59 AM
For example, maybe in one project most of the time everyone pushes and pulls from a centralized master and not to and from each other. But then two developers get together to try and solve a particularly troublesome bug. While they're doing this, they can push and pull directly from each other without worrying about master, and then once they're done, they can merge, clean up, etc., and then push the result to master. Git allows these kinds of pockets of decentralized development within an overall centralized workflow, and does so seamlessly.
Since Git allows any organizational structure, it's up to the people working on a project to choose what works best for them. That's a responsibility that they didn't have before DVCS, because only one structure was supported by the tools.
by agrona on 8/23/17, 5:32 AM
>Have you ever did a code review, and gave feedback that was longer than doing the fix yourself, while you’re at it?
But this seems to neglect the utility of teaching people to do things correctly. Sure, I could always just fix junior programmers' mistakes, but it seems better to educate them in the first place.
Sending the fixes back down doesn't seem like it would have the same impact.
by kazinator on 8/23/17, 5:36 AM
> * You have one online repository for every project (or even the entire code in case of a monorepo).
> * Your repository/repositories have one branch that everyone branch from and develop their changes against. (Release branches don’t count.)
The kernel has one main repo that integrates things from the others, the "torvalds/linux.git":
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...
This keeps Linux together as One Thing. This is Linux's CVSROOT, so to speek.
> You have one process that everyone have to follow to review the code; one review system and one place that runs all your tests.
No kidding; also: one payroll from which everyone gets paid and one product that customers understand: not seven versions of the product based on whose cubicle it came from. See the trend?
by lobster_johnson on 8/23/17, 5:49 AM
by godd2 on 8/23/17, 5:36 AM
Not everyone wants a distributed version control system, and not every team is ready for a more complicated process.
by matt4077 on 8/23/17, 5:35 AM
I always thought this process was sort-of intended. I feel it shows some respect to the author of the code under review to comment on it, and not to change it–especially not if that would result in changes that are then merged without first getting approval from the author. I'm having trouble finding the right words for this idea...
It's also a way to teach people about coding standards and expectations for little things, such as spelling in comments.
by colbyh on 8/23/17, 5:35 AM
by friendzis on 8/23/17, 6:05 AM
Lets pretend that architects are under team leads, seniors under architects, juniors under seniors, etc.. In this hierarchical model every layer forwards some responsibilities down the chain and trusts the task to be broken down, distributed further down and completed. In this model code reviews function merely as tools to roughly check for gaping mistakes and whether code fits "architecturally". People are trusted to do their job to the best of their ability and are allowed to have personal style.
In a model where seniors go over everything juniors made, fix according to their own view and then commit we have a problems that lower layers are not trusted to do their job correctly and in the end everything has to strictly conform to product owner's vision or will be "fixed" (rewritten). In this model there are no longer junior, mid-level, senior engineers, because the more minions one has under them, the less time they have to produce anything on their own. In this model there are junior, mid, senior code-monkeys and one mighty product owner who knows it all best. Not sure that environment where only career growth opportunity is ability to rewrite other's code to your own liking would be good for team morality.
by skrebbel on 8/23/17, 6:10 AM
Of course no such thing exists, so I use Git which has these features but combines them with 10x worse UX and 10x more condescension from the community. But I put up with that, only because it's the lesser of two evils.
I can't imagine I'm alone in this.
by ravitation on 8/23/17, 5:57 AM
Many of the suggested methods of code sharing do happen, especially on large projects with multiple teams.
But the “one master branch” problem is not a result of missuing git... It's a result of there being one version used in production... No git process change is going to change that.
His real issue seems to be with code reviews... Which isn't really something unique to git...
by nurettin on 8/23/17, 5:36 AM
This applies to pretty much any version control where you can create a remote branch.
by Walkman on 8/23/17, 8:34 AM
Seriously, the author probably never worked in teams with different sizes [1, 2], considered another workflows [3, 4] and skill levels of the developers (yes, that's a variable you have to consider also!)
Also if you use Git in the same manner as CVS, you should do it because it's much faster, more reliable and more flexible...
Edit: Ok, the author is saying things which I agree with, I was commenting on the click-bait title and the first paragraphs mostly, but still, there is more to the story.
[1]: https://m.signalvnoise.com/the-majestic-monolith-29166d02222...
[2]: https://blog.bradfieldcs.com/you-are-not-google-84912cf44afb
[3]: https://trunkbaseddevelopment.com/
[4]: http://nvie.com/posts/a-successful-git-branching-model/
by Sytten on 8/23/17, 5:38 AM
by chmike on 8/23/17, 5:40 AM
This is also where the continuous integration tests usually take place.
What is a good point of the OP is that people may not be sharing and testing code enough before pushing to the reference repo.
by bsimpson on 8/23/17, 5:48 AM
Code review often catches typos, or edge cases that should be tested. As the OP points out, you end up serializing the problem into a comment, which the other developer has to deserialize, implement, and send back. Worse, these sort of issues often come across as nags - I'd be happy to fix a typo myself, but feel like a jerk calling someone else's out. I rather like the idea of pulling down somebody's diff, amending it, and pushing it back up. I'll have to try that sometime.
by dasil003 on 8/23/17, 7:24 AM
Talk about throwing out the baby with the bathwater!
Just because you have a blessed master branch, and a central repo, does not mean imply you aren't making use of your DVCS. You can still do all development on topic branches, including CI, and you can do it all in an asynchronous manner.
This idea of pushing branches between individuals in an ad-hoc fashion with whatever tools everyone wants to use makes perfect sense for something like kernel development where A) there is no single product running on a single production environment, it's a freaking kernel deployed to billions of systems globally and B) you're dealing with a huge set of people who you could never hope to standardize anyway.
When you're shipping a complex product to a single production environment, it makes a great deal of sense to centralize certain things. Having everyone bike-shedding on a million different code review paths, and relying on each individual to pass their code around to other individuals with no single place where you can look to find out what someone was working on will lead a lot of chaotic inefficiency that will leave juniors confused, and all in the pursuit of some platonic ideal that is cargo-culted from a completely different project.
Don't get me wrong, I think most people have a lot to learn about how to use git better, and I am a huge proponent of the value of a well-curated VCS history, but this article is inappropriately prescriptive nonsense.
by alphaalpha101 on 8/23/17, 6:14 AM
Yes. Yes yes yes. I hate this so much. I want my code reviews to involve fixing little things. I don't even have suggested edits, I can only highlight comments and make comments, and even suggested edits wouldn't be enough. I want to just make the changes. It's all on a branch anyway.
by Everlag on 8/23/17, 5:40 AM
> open in IDE/text editor of their choice with or along diff viewer, jump around the new code, run tests and even write some new ones, toy with it
This doesn't scale. I know team leads in large projects where reviewing external contributions is their main responsibility; this style would be impossible.
Instead, I think the human, diff based approach with CI running against the reviewed code is enough. A reviewer going over the tests which are shown to be passing should be sufficient after the usual diff read-through.
by chiefalchemist on 8/23/17, 12:09 PM
Just because (autonomous) you can merge and push, doesn't mean you should. Right?
by sreya on 8/23/17, 5:55 AM
>It might seem like chaos, but it is not. It is just fluid synchronization, or to put differently: eventual consistency system.
Can anyone explain this? The example he used wasn't fleshed out enough for me to understand how this system wouldn't be chaos
by feketegy on 8/23/17, 7:52 AM
by cntlzw on 8/23/17, 6:08 AM