3 ms·

> No GitHub PR sync for stacks FWIW, I think the fact that you need client support for this is a bizarre shortcoming of GitHub. If GitHub just let you review

> No GitHub PR sync for stacks FWIW, I think the fact that you need client support for this is a bizarre shortcoming of GitHub. If GitHub just let you review individual commits like Gerrit does, the concept of "PR stacking" would be unnecessary (as it is with Gerrit). The model of "a PR is just a blob of changes" is a weird baby's toy version of a code review tool IMO!

mkingston 1y ago
You can review a PR commit by commit. But it's not obvious how in the UI. Navigate to the commits tab on the PR page and you can navigate through the commits in the PR and review them one by one. Although it's possible this isn't what you mean.
- baq 1y ago
  Github doesn’t understand that all commits can change for review purposes. This is what makes it a ‘toy’ (though I prefer stronger wording here.)
- rfoo 1y ago
  Right, the problem is when I amended one of my patches there's no way for the reviewer to look at a diff between current and previous versions.
  - nhaehnle 1y ago
    Indeed. I wrote a tool for myself to help with that in my reviews: https://github.com/nhaehnle/vctools/tree/main/diff-modulo-base https://github.com/nhaehnle/vctools/tree/main/diff-modulo-ba... It admittedly doesn't have a lot of polish, but I do use it regularly and I'd be happy to help anybody who is interested in using it.
  - w0m 1y ago
    ADO handles this by showing a dropdown of 'comment left on update X, currently update Y' with a toggle to show the difference. I ~hate how ADO handles individual commits (comment left on a single commit is hidden), but I do like the update tracking on a PR as it allows cleaning up the commit tree seamlessly without losing history or comment traceability (less you remove/move files).
IshKebab 1y ago
GitHub does let you review individual commits, at least you can leave comments on them. All we need is GitHub to support a `Depends on: #123` annotation which would hide commits already in #123 and not let you merge until #123 is merged.
- SkiFire13 1y ago
  > All we need is GitHub to support a `Depends on: #123` annotation which would hide commits already in #123 and not let you merge until #123 is merged. You can get an approximation by having the PR target the branch used by #123.
  - dietr1ch 1y ago
    This means multiple stacked branches that you need to maintain on your own, doesn't it? That's the annoying part IMO, even when a short script would do or the rebases take a few characters in some efficient git UI.
  - atq2119 1y ago
    This doesn't work across forks, though, and therefore doesn't compose with GitHub's model where you push your changes to your private fork and then open a PR from there. There's also no native UI support for it in GitHub -- I'd expect to have a navigation element for stacked PRs like in Gerrit.
- bjackman 1y ago
  You can review them but you can't look at the diff between them after they've been amended in response, so it's not actually a usable workflow. IIUC the typical model is that people just upload new commits like "respond to review comments" and then eventually squash the whole PR into one commit when it's ready. So basically you are just giving up on the idea that the commit history is part of the artifact you're working on.
endtime 1y ago
Yes, I agree, much of the blame lies with GitHub -- but realistically, GitHub isn't going to change to accommodate tools like jj.
- bjackman 1y ago
  Yeah but I just wanted to highlight that a better world exists! People should know that GitHub's toy model isn't the only one available. Still, you're right that it makes sense to build tools to work around GitHub since there's always gonna be cases where you can't avoid using it.
  - endtime 1y ago
    Yeah, effectively all my programming is at my job, where we use GH Enterprise, so I don't really have a choice. I prefer Gerrit but not up to me.
HALtheWise 1y ago
I highly recommend revup, it allows managing and uploading stacked (or arbitrary trees of) PRs to Github, including adding a comment that shows approximate revision-to-revision diffs if you want it to. I don't actually think that per-commit reviewing obviates the desire for stacked PRs, for example I often have some PRs in my stack that are not yet ready for review or merging. https://github.com/Skydio/revup https://github.com/Skydio/revup

mkingston 1y ago

You can review a PR commit by commit. But it's not obvious how in the UI. Navigate to the commits tab on the PR page and you can navigate through the commits in the PR and review them one by one. Although it's possible this isn't what you mean.

baq 1y ago
Github doesn’t understand that all commits can change for review purposes. This is what makes it a ‘toy’ (though I prefer stronger wording here.)
rfoo 1y ago
Right, the problem is when I amended one of my patches there's no way for the reviewer to look at a diff between current and previous versions.
- nhaehnle 1y ago
  Indeed. I wrote a tool for myself to help with that in my reviews: https://github.com/nhaehnle/vctools/tree/main/diff-modulo-base https://github.com/nhaehnle/vctools/tree/main/diff-modulo-ba... It admittedly doesn't have a lot of polish, but I do use it regularly and I'd be happy to help anybody who is interested in using it.
- w0m 1y ago
  ADO handles this by showing a dropdown of 'comment left on update X, currently update Y' with a toggle to show the difference. I ~hate how ADO handles individual commits (comment left on a single commit is hidden), but I do like the update tracking on a PR as it allows cleaning up the commit tree seamlessly without losing history or comment traceability (less you remove/move files).

IshKebab 1y ago

GitHub does let you review individual commits, at least you can leave comments on them. All we need is GitHub to support a `Depends on: #123` annotation which would hide commits already in #123 and not let you merge until #123 is merged.

SkiFire13 1y ago
> All we need is GitHub to support a `Depends on: #123` annotation which would hide commits already in #123 and not let you merge until #123 is merged. You can get an approximation by having the PR target the branch used by #123.
- dietr1ch 1y ago
  This means multiple stacked branches that you need to maintain on your own, doesn't it? That's the annoying part IMO, even when a short script would do or the rebases take a few characters in some efficient git UI.
- atq2119 1y ago
  This doesn't work across forks, though, and therefore doesn't compose with GitHub's model where you push your changes to your private fork and then open a PR from there. There's also no native UI support for it in GitHub -- I'd expect to have a navigation element for stacked PRs like in Gerrit.
bjackman 1y ago
You can review them but you can't look at the diff between them after they've been amended in response, so it's not actually a usable workflow. IIUC the typical model is that people just upload new commits like "respond to review comments" and then eventually squash the whole PR into one commit when it's ready. So basically you are just giving up on the idea that the commit history is part of the artifact you're working on.

endtime 1y ago

Yes, I agree, much of the blame lies with GitHub -- but realistically, GitHub isn't going to change to accommodate tools like jj.

bjackman 1y ago
Yeah but I just wanted to highlight that a better world exists! People should know that GitHub's toy model isn't the only one available. Still, you're right that it makes sense to build tools to work around GitHub since there's always gonna be cases where you can't avoid using it.
- endtime 1y ago
  Yeah, effectively all my programming is at my job, where we use GH Enterprise, so I don't really have a choice. I prefer Gerrit but not up to me.

HALtheWise 1y ago

I highly recommend revup, it allows managing and uploading stacked (or arbitrary trees of) PRs to Github, including adding a comment that shows approximate revision-to-revision diffs if you want it to. I don't actually think that per-commit reviewing obviates the desire for stacked PRs, for example I often have some PRs in my stack that are not yet ready for review or merging. https://github.com/Skydio/revup https://github.com/Skydio/revup