Comment by zbentley | Hacker News Reader

Comment by zbentley | Hacker News Reader

Parent Full thread

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

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

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

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.

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

> Why would you do that, in general?

The main purpose is to maintain a clean set of commits. So rather than having a history like:

    Add method and unit tests
    Update version to 1.2.3
    Fixed issue in method
    Fixed syntax error
    Addressing review comments

you only have the first two commits.

> That seems needlessly hostile to reviewers

Why would doing something like this be considered hostile? The problem here is the tool used for reviewing code, not the submitter or reviewers. Other review tools have had the feature of viewing the diff of the branch across rebases for quite some time now.

The only other alternative is to rely on the submitter to make sure they squash the amendments they make into the correct commit. That in itself is more difficult after making multiple commits of that nature. What usually happens in that case is that those commits end up as part of the master branch. This makes it more difficult to revert certain changes because they're split across multiple commits and it also makes it more difficult to track down the source of a bug. It also makes it more difficult to see which commit was responsible for a given change when using git blame.

The problem I run into when reviewing PRs on Github is that many people rebase or otherwise mutate their history locally to address review comments, then force push to the branch being reviewed. Once they've done that, you've lost the original commits and can't see how the new ones differ.

https://github.com/isaacs/github/issues/997

Only if the branch has commits appended, not if it has modified commits.

Isn't modifying commits considered a poor practice in the general case?

I dislike when questions are worded like that, because it could be interpreted in two ways:

a) Given an arbitrary case, is there a chance that modifying commits is considered a poor practise in that case?

b) Is the belief that modifying commits is poor practise held by the population at large?

The answer to a) is yes, the answer to b) is no.

Some people prefer a clean history, where you have to explicitly request the steps to get to it if you want them. Others want the messy steps to be part of the history you show up-front.

In Gerrit you can diff against previous versions of the patch, right from the web-ui.

Reply on news.ycombinator.com