Github PRs (at least on the enterprise version) still have the problem of collapsing comments where the associated line of code changed in some way that had nothing to do with the comment. They also require a lot of scrolling if the overall diff is rather long. And, they still don't provide a way to comment on commit messages themselves.
Collapsing comments when the code changes is the correct thing to do 90% of the time in my experience, and for that other 10% you can manually mark is as unresolved. It's the correct default IMO.
> Collapsing comments when the code changes is the correct thing to do 90% of the time in my experience
Why not just leave them uncollapsed and allow one to mark them as resolved when the change is actually resolved. The other problem is that it's difficult to see what actually changed when a line is marked out of date without having to scan through the entire updated diff and then jump back and forth between the diff and conversation view.
Collapsing them by default makes it difficult to find the comment by scanning. If they were uncollapsed until I marked them as resolved, then I could at least use the find feature in the browser and search for my username to find my comments.
Agreed on the comment collapse, about comment on the commit message I'd say it's a commit on the main PR (since by the time the PR is done the commit msgs are pretty much not changeable, except for the last one)
> about comment on the commit message I'd say it's a commit on the main PR
Except that if the overall diff line I choose to comment on about the commit message changes in some way, then the comment is collapsed, which makes it difficult to keep track of what needs to change.
> since by the time the PR is done the commit msgs are pretty much not changeable, except for the last one
Stacked reviews are the crucial thing missing from GitHub pull requests. Truly, once you get used to working wit hthose, going back is intolerable, to the point where I now have a hacked together workflow where I live in git rebase -i and have it automatically push specially named branches for each commit in a stack.
So a few months ago I actually found a tool for this! The tool makes it easy to manage stacked reviews on GitHub. It's working super well for me. I also showed it to a bunch of people I work with, and a lot of them have taken it up.
At my place of work, we've sort of implemented that feature by using fixup or squash commits. That is, if a comment is made on a diff line in a PR, use git blame to determine which commit was responsible for that line of code, update the line and commit it with a commit title like: fixup! <original commit title>. If we want to update the commit message, we use "squash! <original commit title>" and put the updated commit message in the commit message body (using the --allow-empty switch when committing so that we don't have to change any code to make a commit).
Then, we can run git rebase -i --autosquash --keep-empty to handle applying the changes requested to the correct commits at the end of the review process.
You have a task to build a Thing, which involves creating the basic "Thing framework" and then implementing subfeatures A, B and C of into/on top of it
Maybe you'll build the framework & subfeature A together, because you need some bit of meat in there to properly figure things out and be able to test things
On Gerrit you'd probably create two commits which get pushed as two CLs
* CL1 Create Thing framework
* CL2 Implement A for Thing
They're now pushed for people to review. While they're looking at them, you continue working on, making CL3 implementing B
* CL3 Implement B for Thing
One of your coworkers points out an issue in CL1, so you fix it (by amending the commit) and repush the stack. Now your stack is
* CL1 Create Thing framework [v2]
* CL2 Implement A for Thing
* CL3 Implement B for Thing
CL1 & CL2 get approved so you merge them (though typically with Gerrit they're cherry picked - CLs are individual Git commits). You push up an implementation of CL4, so your stack now looks like
* CL3 Implement B for Thing
* CL3 Implement C for Thing
The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). When you're working in Git you typically (unless for whatever reason you have multiple unrelated stacks on the go, which is relatively rare) just work off of the master branch, so all the commits between `origin/master` and `master` - i.e. the set that automatically show up in `git rebase -i` and similar tooling - are your stack. When you pull, you `git pull --rebase` (or rather set the config for your machine or repo to default to that). When you've revised something in your stack or want to add something to it, you just do "git push HEAD:origin/refs/for/master" update Gerrit with the latest version of your stack
It takes a little time to get used to (and you have to learn how to hold `git rebase -i` properly), but when you're used to it it's immensely more productive than the Github/clones PR flow (and doesn't involve manual branch juggling, etc). I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged)
When I'm in this situation when dealing with something which is PR based, I tend to end up merging CL1 & CL2 (because its easier to review things when you have an example) and hang on to CL3 & CL4 on my local machine until the first one gets merged
> The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack).
What prevents a situation like CL2 getting approved and merged prior to the resolution of CL1? Or more generally, what ensures that the ordering of commits in a set of CLs in a given stack is preserved prior to merging?
> I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged)
Rather than stacked PRs (which essentially doubles the number of commits since Github (and possibly Gitlab) introduces a merge commit for each PR even if it's a fast-forward merge), would it not be better to just combine CL1 through CL4 into a single feature branch where each commit corresponds to a CL? It would make for a large PR, but it can be reviewed on a per commit basis.
No, I'll take Github PRs any day. They could be better, of course.