2 ms·

While recent PR improvements have made it a lot better, there are still a few things missing that I know of. For example, if a contributor force pushes a new c

While recent PR improvements have made it a lot better, there are still a few things missing that I know of. For example, if a contributor force pushes a new commit to a PR branch, as a reviewer, I can't see the old version and compare against it. I'd have to tell the contributor to not do that (doesn't work for first time contributors), or always try to manually backup each stage of a PR and compare with locally. This doesn't happen on Gerrit because it tracks the entire history of the change. Second thing. Once a PR gets large and lasts a while, it's hard to see what is going on, what review comments still need to be addressed, and where new replies are showing up. Consider this PR [0] as an example, can you tell where my "latest reply" was? It's not at the bottom because it was a reply to one of the early inline conversations, and you have to expand all the "Show outdated" links manually to find it. Those are just a few things off the top of my head, as someone who used PRs and Gerrit. I prefer PRs but I don't think they're viable for a project that takes code quality and code review as seriously as Go does. I do hope PRs get the remaining missing pieces to be able to match Gerrit. [0] https://github.com/google/go-github/pull/497 https://github.com/google/go-github/pull/497

avar 10y ago
> For example, if a contributor force pushes a new commit to a PR > branch, as a reviewer, I can't see the old version and compare > against it. This is not fully true. While you can't "git fetch" that branch anymore, you can see comments against those old diffs in the pull request UI, which are the things likely to have changed. For example, I submitted this PR against uWSGI a few months ago: https://github.com/unbit/uwsgi/pull/1392 https://github.com/unbit/uwsgi/pull/1392 Initially I screwed it up and inserted a field in the middle of a struct, whereas within a stable release it should of course be added to the end. If you click on "show outdated" you can see that old diff where I'm adding the field to the middle of the struct, but click on "files changed" and it's the current change where I fixed the issue. I do think GitHub could improve here, they could expose forced pushes via refs/pull/NUM-COUNTER instead of just refs/pull/NUM as they do now, but it's important to point out that they're handling the common case. When someone force-pushes a pull request they're usually doing so in response to comments on the request, and those comments will still make sense because GitHub saves away the old diff, which you can compare with the current state.

avar 10y ago

> For example, if a contributor force pushes a new commit to a PR > branch, as a reviewer, I can't see the old version and compare > against it. This is not fully true. While you can't "git fetch" that branch anymore, you can see comments against those old diffs in the pull request UI, which are the things likely to have changed. For example, I submitted this PR against uWSGI a few months ago: https://github.com/unbit/uwsgi/pull/1392 https://github.com/unbit/uwsgi/pull/1392 Initially I screwed it up and inserted a field in the middle of a struct, whereas within a stable release it should of course be added to the end. If you click on "show outdated" you can see that old diff where I'm adding the field to the middle of the struct, but click on "files changed" and it's the current change where I fixed the issue. I do think GitHub could improve here, they could expose forced pushes via refs/pull/NUM-COUNTER instead of just refs/pull/NUM as they do now, but it's important to point out that they're handling the common case. When someone force-pushes a pull request they're usually doing so in response to comments on the request, and those comments will still make sense because GitHub saves away the old diff, which you can compare with the current state.