5 ms·

The patch-based workflow is very similar to how the Linux kernel review workflow works. Arguably, this is the way Git is supposed to work - submit patches, disc

by lima 8y ago

The patch-based workflow is very similar to how the Linux kernel review workflow works. Arguably, this is the way Git is supposed to work - submit patches, discuss them, then merge them once they're ready. Every idea is one commit, no checkpoints or "fix typo". Pull requests are used to merge other maintainer's trees that have many commits in them. It also ensures that there's a permanent link between a commit and its review, since there's a 1:1 mapping. Gerrit works similarly, but Phabricator's UI is much more friendly. For small projects or teams, the overhead of introducing a new tool may not be worth it, for for large projects with many contributors, the workflow is - in my opinion - strongly superior. If you set Github to "squash and rebase" and enforce a "one PR per idea" rule, you will get a pretty similar workflow. The tooling isn't quite as polished, but it's sufficient for many projects. Quote: > As far as code reviews go, this is pretty spot on. I was part of a a startup that was using GitHub pull requests for code reviews. As the team grew, it became more and more intractable, although not simply because of notifications. Side-by-side diffs and checkpointed diffs (so that you can see what changed since the last round of review and whether/how your comments were addressed) are handled very poorly by GitHub. We ultimately switched to Phabricator, and while there was a little friction as folks got acquainted with the new tool, it made code reviews a much more pleasant process. Recently, I had to go through a full code review back on GH pull requests, and it felt like pulling teeth in comparison. They're fine for interacting with contributors to an open source project, but compared to working with a tool like Phabricator that's built for a code reviewer's workflow (and for teams of engineers working together on a project), they just don't hold a candle, in my opinion. (from a previous discussion: https://news.ycombinator.com/item?id=7697132 https://news.ycombinator.com/item?id=7697132)

zbentley 8y ago
> Side-by-side diffs and checkpointed diffs (so that you can see what changed since the last round of review and whether/how your comments were addressed) are handled very poorly by GitHub. I've heard that complaint a lot. How are these things handled poorly by GitHub? It's an honest question; I've only used GH and a bad, ad-hoc, poorly-thought-through Perforce workflow, so I may not know what I'm missing, but it has always seemed quite simple for me to scroll through a pull request and see feedback (either tagged to lines of diff or general), and commits that address pieces of feedback. Then you can click on each commit to view it, or open the full hairball diff to see how those changes end up in the final result. What's missing there? It sucks when someone puts a bunch of unrelated changes in one commit, or when their commit message is bad ("code review"), but those seem like they'd be problems regardless of GitHub. I've looked at patch-submit-discuss/mailing-list-style workflows, and they seem fine. The discussion is definitely more front and center. However, when people submit updated patches it seems like I'd want to be able to easily tell what changed between their first iteration and second iteration of the patch--i.e. did they actually address the feedback they got? This can be hard to see, especially if a patch is substantial or to confusing/repetitive code. I can download all the submissions and run diff on each pair in sequence, but that seems like needless hassle when, unless the feedback was "throw it all out and do it a totally different way" (which, in a branching workflow, is often a good reason to start on a fresh branch), subsequent iterations could be submitted as a "patch to a patch", with an easy way to view the cumulative diff. At this point, it would basically be a branch workflow. Are there solutions for this in the patch-submit-discuss model?
- u801e 8y ago
  > I've heard that complaint a lot. How are these things handled poorly by GitHub? Basically, when changes are requested, the patch based work flow is to submit a revised set of commits up for review. In Github, one can do that by either force-pushing to the branch that's currently attached to a pull request, or one can push up a new branch and open up a new pull request associated with that branch. But there isn't a way to view the "interdiff" between the version of the branch before review and the version of the branch after review. Review tools like Phabricator, Gerrit, and email lists allow one do view that diff.
  - WhyNotHugo 8y ago
    > But there isn't a way to view the "interdiff" between the version of the branch before review and the version of the branch after review. Review tools like Phabricator, Gerrit, and email lists allow one do view that diff. Yes there is. You even get a link to view that exact diff whenever someone updates a PR. It shows changes since you last viewed the PR, up to its current state.
    - u801e 8y ago
      That's only the case if someone pushes up a commit like "Addressing comments" or "Fixed bug". If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments, Github does not provide a way to see the diff between the previous branch head commit and the current head commit.
      - zbentley 8y ago
        > If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments Why would you do that, in general? That seems needlessly hostile to reviewers, since then they have to go through your diff and make sure you caught every place their comment applied (e.g. "did they actually handle the else-case everywhere they're testing on $var?"). Is this common practice in situations* where it harms diff readability? If so, why? * There are totally cases where that's necessary, but they don't seem like more than a small minority of comment/feedback/amendment cases. These are times like "you put all of this code in the totally wrong place" or "this is missing a fundamental assumption that would require you to totally rearchitect every change you wrote". In those cases, a rebase (plus clearly calling out "everything was rebased starting here please start reading from here on!") or a brand new branch is in order.
nh2 8y ago
> The patch-based workflow is very similar to how the Linux kernel review workflow works. I don't think that is accurate. Or there kernel its very common to submit whole patch series (branches), and to rebase them on the latest kernel. Phabricator makes multi commit reviews possible but a total pain.
- lima 8y ago
  It works exactly the same with Phabricator - you submit a series of individual patches and have them depend on each other.
u801e 8y ago
> Every idea is one commit Ideally, a commit should be self contained and change only one thing. This means that a feature would consist of multiple commits. Since later commits may depend one or more earlier commits (some of which may be in the same feature branch), how do tools that work on a commit based workflow handle those dependencies?
- drewg123 8y ago
  Dependent changes are a nightmare in phabricator. I tried it once using arcanist and git, and it seemed to be confused about which patchset I wanted to change. After this experience, I talked to a few colleagues, and they basically said "yeah, just don't use dependent reviews"
- lima 8y ago
  Most large teams avoid long-lived feature branches and instead try to get things reviewed and merged into master as soon as possible. It's better to have many small reviews than a few massive ones. You usually try to avoid dependencies. If you can't avoid them, have a series of patches depend on each other. Phabricator supports stacking patches. Here's a random patchset: https://secure.phabricator.com/D19429 https://secure.phabricator.com/D19429 Scroll down to "Stack" - it will display the list of patches depending on each other: https://i.imgur.com/WTl4B8x.png https://i.imgur.com/WTl4B8x.png

by lima 8y ago

zbentley 8y ago

> Side-by-side diffs and checkpointed diffs (so that you can see what changed since the last round of review and whether/how your comments were addressed) are handled very poorly by GitHub. I've heard that complaint a lot. How are these things handled poorly by GitHub? It's an honest question; I've only used GH and a bad, ad-hoc, poorly-thought-through Perforce workflow, so I may not know what I'm missing, but it has always seemed quite simple for me to scroll through a pull request and see feedback (either tagged to lines of diff or general), and commits that address pieces of feedback. Then you can click on each commit to view it, or open the full hairball diff to see how those changes end up in the final result. What's missing there? It sucks when someone puts a bunch of unrelated changes in one commit, or when their commit message is bad ("code review"), but those seem like they'd be problems regardless of GitHub. I've looked at patch-submit-discuss/mailing-list-style workflows, and they seem fine. The discussion is definitely more front and center. However, when people submit updated patches it seems like I'd want to be able to easily tell what changed between their first iteration and second iteration of the patch--i.e. did they actually address the feedback they got? This can be hard to see, especially if a patch is substantial or to confusing/repetitive code. I can download all the submissions and run diff on each pair in sequence, but that seems like needless hassle when, unless the feedback was "throw it all out and do it a totally different way" (which, in a branching workflow, is often a good reason to start on a fresh branch), subsequent iterations could be submitted as a "patch to a patch", with an easy way to view the cumulative diff. At this point, it would basically be a branch workflow. Are there solutions for this in the patch-submit-discuss model?

u801e 8y ago
> I've heard that complaint a lot. How are these things handled poorly by GitHub? Basically, when changes are requested, the patch based work flow is to submit a revised set of commits up for review. In Github, one can do that by either force-pushing to the branch that's currently attached to a pull request, or one can push up a new branch and open up a new pull request associated with that branch. But there isn't a way to view the "interdiff" between the version of the branch before review and the version of the branch after review. Review tools like Phabricator, Gerrit, and email lists allow one do view that diff.
- WhyNotHugo 8y ago
  > But there isn't a way to view the "interdiff" between the version of the branch before review and the version of the branch after review. Review tools like Phabricator, Gerrit, and email lists allow one do view that diff. Yes there is. You even get a link to view that exact diff whenever someone updates a PR. It shows changes since you last viewed the PR, up to its current state.
  - u801e 8y ago
    That's only the case if someone pushes up a commit like "Addressing comments" or "Fixed bug". If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments, Github does not provide a way to see the diff between the previous branch head commit and the current head commit.
    - zbentley 8y ago
      > If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments Why would you do that, in general? That seems needlessly hostile to reviewers, since then they have to go through your diff and make sure you caught every place their comment applied (e.g. "did they actually handle the else-case everywhere they're testing on $var?"). Is this common practice in situations* where it harms diff readability? If so, why? * There are totally cases where that's necessary, but they don't seem like more than a small minority of comment/feedback/amendment cases. These are times like "you put all of this code in the totally wrong place" or "this is missing a fundamental assumption that would require you to totally rearchitect every change you wrote". In those cases, a rebase (plus clearly calling out "everything was rebased starting here please start reading from here on!") or a brand new branch is in order.

nh2 8y ago

> The patch-based workflow is very similar to how the Linux kernel review workflow works. I don't think that is accurate. Or there kernel its very common to submit whole patch series (branches), and to rebase them on the latest kernel. Phabricator makes multi commit reviews possible but a total pain.

lima 8y ago
It works exactly the same with Phabricator - you submit a series of individual patches and have them depend on each other.

u801e 8y ago

> Every idea is one commit Ideally, a commit should be self contained and change only one thing. This means that a feature would consist of multiple commits. Since later commits may depend one or more earlier commits (some of which may be in the same feature branch), how do tools that work on a commit based workflow handle those dependencies?

drewg123 8y ago
Dependent changes are a nightmare in phabricator. I tried it once using arcanist and git, and it seemed to be confused about which patchset I wanted to change. After this experience, I talked to a few colleagues, and they basically said "yeah, just don't use dependent reviews"
lima 8y ago
Most large teams avoid long-lived feature branches and instead try to get things reviewed and merged into master as soon as possible. It's better to have many small reviews than a few massive ones. You usually try to avoid dependencies. If you can't avoid them, have a series of patches depend on each other. Phabricator supports stacking patches. Here's a random patchset: https://secure.phabricator.com/D19429 https://secure.phabricator.com/D19429 Scroll down to "Stack" - it will display the list of patches depending on each other: https://i.imgur.com/WTl4B8x.png https://i.imgur.com/WTl4B8x.png