Pull requests are an industry-standard but what if the alternative is vastly better?
Ever held an opinion so strongly that you ignored anything th...
For further actions, you may consider blocking this person and/or reporting abuse
Alternatively, just create small PRs that always target master?
Creating and switching branches takes seconds so I'm struggling to understand how that can be considered a burden or a big overhead unless you were doing it thousands of times a day. I think the most PRs I've submitted in a single day was 10 and I can imagine a workflow where someone might need to do 20+. That's maybe 5 minutes of their day switching between branches? Seems like there are likely better opportunities to improve productivity to me.
Consider this scenario: you've filed a PR and moved on to the next one. Then, you realize there could be some improvement in the last PR. You would have to checkout the branch again and create a new commit and checkout the last branch again. If you're working on the same branch you would just have to amend the last commit. Another case could be where your PR is not merged and then, you're working on another which might have conflicting changes.
I agree that the cost of creating branches is not that high, but it's the context switching that can be a time sink. I think Google's workflow is about worrying mostly about your change and little else.
But in the commit review world you'll be constantly context-switching as your teammates push new commits and you have to drop everything and review them to unblock them from pushing their next commits. Especially for a smaller team and for teams with more juniors or newer teammates (who tend not to review other people's PRs), this will bog down everyone's commits in a big queue waiting to be reviewed.
Your first example has you making no changes locally after creating the PR. You would only be switching to a new branch if you had other work you needed to do. If we assume a model of "I'm going to stay on this work item until its in master", which it looks like to me you are, you would just stay on this branch until your PR is approved. If there is feedback that requires changes you make them and push to the remote PR branch. If your teams doesn't like the fact that there are multiple commits, can amend/squash locally and force push to your PR branch. I have some co-workers who feel strongly about re-writing history so that their merges are clean (single commit), which fits this case.
And if you are working under this model on work items in serial (one after the other, never moving on to the next item before the previous is finished) then you can accomplish that without local branches, only remote PR branches. You can do all of your work on your local master branch but when you push, instead of pushing to remote/origin/master you can go to remote/origin/shubham/JIRA-1337.
As to your second example, you can avoid conflicting with yourself by using PR chaining. I do this sometimes when I have a series of changes that depend upon each other. After the first change I create PR-A that points to master. Then the second is PR-B pointing to PR-A, and so one. This allows my reviewers to review my changes in bite-size chunks and unblocks me from having to wait for them to review each change before starting on the next. If I need to respond feedback on a particular PR I can easily switch to that branch and make changes. The downstream PRs can then get updated appropriately.
Now, obviously the above can easily get complicated but I consider that complexity 'worth it' because that's how I stay productive. My reviewers are not going to drop everything they are doing and review my work immediately - that wouldn't be good for their productivity with context switches outside of their control. Sometimes PRs will get reviewed within a few minutes of creation and that's great! But other times is could take a few hours before they have a chance.
So if you live in a world where you are making dozens of commits a day and getting immediate, useful feedback on each change I would get your position. I've just never lived in a world anything like that before :)
Hello, thanks for the post, I really enjoyed it and I think there are good points there. In general, I’m a fan of reducing ceremonies and boilerplate as much as possible (without losing control), so this approach fits well that way of thinking.
There’s something I didn’t understand, do you mind help me fill the gap?
The post says:
“You can't push your commit to master until your changes are approved.
If there are any suggestions, the developer would need to modify the commit (using amend or interactive rebasing) and push it again. This gives a clear view of what was changed and the reviewer understands if their suggestions were incorporated. When the reviewer approves the commit, it can be pushed to master.”
If we only use master, but we can’t push to master, where do we push to at the beginning? Where does the reviewer do its job? I didn’t understand either that we can’t push to master but then we need to amend the commit and push it
I got lost at some point but I don’t know exactly where.
Thanks again for the post!
The implementation may vary but the idea is there is temporary space where your commit is stored, and it's cherry-picked on the target branch when it's approved. This is very similar to how gerrit works. Commits can only be merged in the target branch once it has been reviewed.
The tool I am talking about can probably send a diff to a third party service or can create a temporary branch for your commit. . That'll be an abstraction.
We use Gerrit where I work, and I generally like it. The UI is tolerable with the newer beta design :)
The issue I have with it is that it only allows single commits to be reviewed. I find this to be limiting, as it requires one to amend a commit to address code review feedback. You also lose all the history of the changes in git. Gerrit does store the history of a review as patchsets whenever you push the amended commit with the same Change-Id and provides diffs which is nice, but this history is not stored in git itself (afaik).
This is what tools like Gerrit are for.
But the basic idea is that you don't actually push to master, you push to a review queue instead, where you can still amend your commit, and when it is finally approved, it is then merged with master.
And yes, this could easily mean that you've got merge conflicts again, so you're back to square one. Which is the Achilles heel of the whole idea, you now have to do the very thing you were supposed to avoid.
There is a difference though.
All this extra work is only needed when a commit fails the review. So if 99% of your commits pass review, all you're doing is pushing to one remote and pulling from another, which takes no extra effort in practice.
But if you're working with a "novice" team who are likely to fail reviews often, this workflow is a nightmare. And if you've only got two or three devs actively working on a codebase, it's probably an overkill.
So the important thing as always, is to think about what works for your specific circumstances, try your idea, then continuously evaluate and refine it.
Excellent article. It gives everyone the notion that exist many ways to do same thing. Solving a problem isn't a silver bullet thing. I see outstanding value on this approach as I see for PR/MR flow. it depends on the kind of problem you're trying solve and the project you're working on. Thanks to bring this to the light for me.
Overall, it depends on the code and how your team works on it.
In my organization, we have lots of individual projects. And, while each of us can contribute to a given project, there's usually a primary maintainer (or small set of primary maintainers). Further each project tends to be composed of discrete modules. Modifications – and associated PRs – tend to be restricted to a given module.
Typically, we use a fork-and-branch model for our activities. Because of the modular style of our coding and committing, merging tends to be at low risk of having conflicts to resolve. Basically, if you have even three developers simultaneously modifying a given project during a given time-period, it usually works out to: developer A was probably modifying module X; developer B was modifying module Y and developer C was modifying module Z. Which is to say, even though their forks' branches will be out of sync with each other, the merge is usually conflict-free (and while there may be module dependencies, so long as people focus on not altering their module's inputs and outputs in a way that breaks dependencies, those dependencies usually aren't endangered). Add in pre-commit linting and post-commit testing-tools and you can maintain a fairly high "branch → modify → commit → PR → merge" tempo (usually, there's teammates quickly available to do code-reviews because they're waiting for post-commit testing-tools to run ...and, because the commits tend to be small, the reviews tend to be fairly quick and short on pre-merge modification-request back-and-forth).
Obviously, much like not every project can be run like Chromium, not every organization's projects can be run like we (tend to) run ours. The parenthetical in my prior statement is there because, there are a non-trivial number of times where we have to deviate from preferred practices. However, those deviations are still infrequent enough as to not require us to globally change practices.
You are totally right - PR/MR is overkill in certain situations that are more common then the opposite.
I am looking forward for the tool you write since Gitlab code review is totally useless in that context. You can't even review entire file outside the commited lines.
Great article. I recently wrote an article on LinkedIn about how code reviews are greatly overrated, in my opinion. Your article summarizes the way I'd like my team to work. It takes time to switch a team's culture from a GitFlow-like branching strategy (horrible, in my opinion) to a trunk-based one, but it can be done.
inline.style
This is already exactly how Phabricator works. Have you given that a try?