Pull Request feature requests for GitHub

[tstellar]

We should create a list of Pull Request feature requests that we think would help the project and submit it to GitHub.

The goal is to submit this list by September 1, 2022.

Please comment below with your feature ideas.

Summary of ideas:

• Ability to comment anywhere in the file, not just on the changed lines.: Commenting on unchanged lines in a pull request github/roadmap#347
• We would like to have a way to automatically subscribe to pull requests based on which files are changed, who the author is, or keywords in the commit message.
• Make it possible to approve or request changes for individual commits in the same PR
• When visiting a PR from a notification email or github.com/notifications, always go to the oldest new event on the Conversations tab.
• Allow users to view a diff between arbitrary versions of a PR: https://github.com/github/roadmap/issues/211
• Allow users to easily fetch arbitrary versions of a PR.
• Allow creating a pull request from a branch in a fork against a pull request in the original repo.
• Ability to add a team as a blocking reviewer for a pull request.
• Add an option for unthreaded conversation view.
• A single diff + conversation view without needing to flick between tabs.
• No collapsed conversations (certainly not in the middle of conversations, possibly okay in an unthreaded view to have the oldest collapsed, like Phab does).
• A Stack view, allowing easy viewing and navigating of stacked PRs.
• "Outdated" comments not removed from the diff view, but instead kept near their original location.
• Should be able to search [ ... ] changed lines/nearly context with an alternate mode to search full content of affected files.
• Allow adding review comments on commit messages as if they were code.

[llvmbot]

@llvm/issue-subscribers-infrastructure

[h-vetinari]

Delivery of github/roadmap#347 would allow to not lose review comments due to rebases etc. (and presumably also to build the "done"/"not done" interface that reviewers value about phab). Note that this was originally scheduled for Q1 2022 and has slipped to Q4 2022 already - highlighting the importance of this to LLVM might help keep it from slipping further.

[h-vetinari]

Regarding stacked PRs: it's currently possible in github to open a PR against a PR (well, its branch), but the problem is that while the primary one would be open in this repo, the secondary ones would have to be opened on the fork of the contributor proposing the PR. That makes this approach wholly infeasible from a process POV (i.e. reviews split over several repos).

However, I think it would not be a big technological hurdle for github to implement the possibility to raise a PR in the same repo against another open PR. AFAIU, this would achieve 90+% of the stacked PR feature from phabricator.

PS. And this would benefit not just LLVM - many other projects are running into visibility issues for such cases (PRs in forks), resp. lacking good ways for someone try to pick up an old PR and build on top of it) - so the case for that becomes stronger.

[hubert-reinterpretcast]

Delivery of github/roadmap#347 would allow to not lose review comments due to rebases etc. (and presumably also to build the "done"/"not done" interface that reviewers value about phab).

Strictly from the referenced Issue itself (which deals with allowing comments on unchanged lines), the additional improvements being claimed appear to be speculative.

We should specifically request that full context (including the rest of the tree) is available for all comments.
We should specifically request that "outdated" in-line comments (the ones GitHub makes hard to find when coincidental changes occur near the line) can be attached to newer commits.
We should specifically request that comments can be forced to display in the conversation view (not collapsed or hidden).

[hubert-reinterpretcast]

We should specifically request that the conversion view can be displayed unthreaded (avoiding the new comment on older thread problem).

We should specifically request that full text search of the various views be possible without having to expand things (for the "Files changed" view, should be able to search--even if the file is not loaded--changed lines/nearby context or search full content of affected files).

[h-vetinari]

Delivery of github/roadmap#347 would allow to not lose review comments due to rebases etc. (and presumably also to build the "done"/"not done" interface that reviewers value about phab).

Strictly from the referenced Issue itself (which deals with allowing comments on unchanged lines), the additional improvements being claimed appear to be speculative.

That's fair, though it solves the issue at heart that causes a lot of other issues like lost comments - because once the comments aren't anchored to evanescent things like specific hunks anymore (which can get lost during a rebase), then there shouldn't be a reason not to keep them in the UI. So you're right that GH could conceivably implement this and still end up losing review comments (and not doing so should be underscored as important to them), but at least there won't be as strong a technical reason to do so anymore.

[tstellar]

From Issue #56638

We would like to have a way to automatically subscribe to pull requests based on which files are changed, who the author is, or keywords in the commit message.

[mshockwave]

Not sure if this is already on the roadmap: review individual commits in the same PR. I think PR is a good way to deliver a series of patch (i.e. [PATCH 1/X] etc.) but the lacking of said feature is a deal breaker IMHO.

[joker-eph]

Commenting anywhere in the file instead of just in the 10 lines of context around a changed line.

[h-vetinari]

Commenting anywhere in the file instead of just in the 10 lines of context around a changed line.

See above, it's already roadmapped (currently for 22'Q4, but slipped already previously): github/roadmap#347

[jh7370]

Having a view ala Phab's UI where both the changes and the full conversation history (including out-of-line discussion) are visible (ideally with zero collapsed conversations) is desirable to me. I find flicking between tabs more of a pain than scrolling back and forth (especially when I often use the browser search to find keywords etc).

For stacked/chained PRs, I'd strongly prefer a PR per individual final commit for reasons discsussed elsewhere. However, for this to be easy to navigate, I'd also like to be able to view the tree relationship between all linked PRs (i.e. like Phabricator's "Stack" view).

I think the rest of my concerns with GitHub PRs are addressed upthread (specifically, being able to place comments anywhere in the full context, keeping "outdated" comments as near as possible to their original location, an unthreaded conversation view, and not collapsing the conversation view). As a bonus to the placing comments anywhere, there is the odd occasion where it would be nice to be able to place a comment even on a file not touched by the PR (e.g. to highlight something else that needs changing) - this isn't a Phabricator feature (that I know of), but would by a nice quality of life improvement under some cases.

[nhaehnle]

When visiting a PR from a notification email or github.com/notifications, always go to the oldest new event on the Conversations tab.

Background: It is far too often the case that GitHub sends you to the "Files Changed" tab that only shows an error message that says something like "content not found". This is probably mostly an issue when force pushes are used to get a clean commit history.

[nhaehnle]

Part 1: Allow users to view a diff between arbitrary versions of a PR.

For reference, in Phabricator the top UI for that looks like this, where the radio buttons on the right allow you to choose the versions to compare between:

Restrict the highlighted diff to difference relative to the merge base, so that there are no spurious changes shown when the PR is rebased on a later version of main.

For reference, here's a Git alias I'm using to get a somewhat similar effect:

rd = "!f() { lhsbase=$(git merge-base $1 $2); rhsbase=$(git merge-base $1 $3); git range-diff $lhsbase..$2 $rhsbase..$3; }; f"

git rd origin/main <old version of PR> <new version of PR>

But note that this isn't exactly what we should have. git range-diff produces a "diff of diffs", where what we really want is a diff where changes caused by a merge-base change are omitted entirely where possible and shown in a different style where showing them is unavoidable (because they're part of the context).

Part 2: Allow users to easily fetch arbitrary versions of a PR.

It is already possible to conveniently fetch PRs as

git fetch origin refs/pull/<id>/head   # or merge for a successful merge into the target branch

We should be able to fetch the entire history of PRs as, for example:

git fetch origin refs/pull/<id>/<nn>

... where nn is a plain PR version number.

[isbadawi]

Part 1: Allow users to view a diff between arbitrary versions of a PR.

There is something a bit like this in the roadmap https://github.com/github/roadmap/issues/211

[joker-eph]

Another request for GitHub could enable stacked PR without a branch is the destination repo: I think this should be possible for them to implement this by allowing to open a pull-request from my fork targeting an existing pull-requests in the destination repository.
Pull-requests already creates git references (you can fetch a pull-request locally git fetch origin pull/<ID>/head:<local branch name>), so if I could open a pull-request targeting pull/<ID>/head then we'd have stacked PR without branched in the main repo :)

[h-vetinari]

then we'd have stacked PR without branches in the main repo :)

Yup 🙃

[lenary]

I presume we will end up using llvmbot to replicate self-subscribing to PRs that hit files particular users are interested in, like we currently can with Herald?

One feature of Phabricator I’ve not seen in Github that LLVM has started using is that groups can be added as a blocking reviewer, requiring someone from that group to approve before the review is unblocked. I think this is used by libcxx and maybe other libraries now? Is this something we want to be able to do on Github? I’d presume yes.

[isbadawi]

One feature of Phabricator I’ve not seen in Github that LLVM has started using is that groups can be added as a blocking reviewer, requiring someone from that group to approve before the review is unblocked. I think this is used by libcxx and maybe other libraries now? Is this something we want to be able to do on Github? I’d presume yes.

GitHub can do this using the code owners feature: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners.

But there's no way to add a group as blocking for a particular PR in an ad-hoc way after it's opened, it's only driven by these path matches in the CODEOWNERS file.

[tstellar]

I've tried to summarize all the feature requests in the 1st comment. Please take a look and let me know if I missed anything or need to clarify something.

[h-vetinari]

I saw this recently in https://github.com/openxla/stablehlo, looks like a new feature that might address (parts of the) "let me see all open threads" / "make sure comments don't get lost" concerns. I haven't tried it myself yet, don't know what limitations / paper cuts exist.

[jh7370]

Having a view ala Phab's UI where both the changes and the full conversation history (including out-of-line discussion) are visible (ideally with zero collapsed conversations) is desirable to me. I find flicking between tabs more of a pain than scrolling back and forth (especially when I often use the browser search to find keywords etc).

For stacked/chained PRs, I'd strongly prefer a PR per individual final commit for reasons discsussed elsewhere. However, for this to be easy to navigate, I'd also like to be able to view the tree relationship between all linked PRs (i.e. like Phabricator's "Stack" view).

I think the rest of my concerns with GitHub PRs are addressed upthread (specifically, being able to place comments anywhere in the full context, keeping "outdated" comments as near as possible to their original location, an unthreaded conversation view, and not collapsing the conversation view). As a bonus to the placing comments anywhere, there is the odd occasion where it would be nice to be able to place a comment even on a file not touched by the PR (e.g. to highlight something else that needs changing) - this isn't a Phabricator feature (that I know of), but would by a nice quality of life improvement under some cases.

I've tried to summarize all the feature requests in the 1st comment. Please take a look and let me know if I missed anything or need to clarify something.

It looks like you may have missed my comment and the associated features noted in it? Summary version:

• A single diff + conversation view without needing to flick between tabs.
• No collapsed conversations (certainly not in the middle of conversations, possibly okay in an unthreaded view to have the oldest collapsed, like Phab does).
• A Stack view, allowing easy viewing and navigating of stacked PRs.
• "Outdated" comments not removed from the diff view, but instead kept near their original location.

[asb]

Assuming " Add an option for unthreaded conversation view." corresponds to @jh7370's request for a linear comment view (as supported on Phabricator), I think all my feature requests are covered. I think current GitHub behaviour is close to this, but the behaviour around sub-threads is different which can make it hard to follow. I believe is PersonA leaves comments and PersonB responds, it won't show a separate event+link for PersonB responding to those inline comments.

[tstellar]

I've tried to summarize all the feature requests in the 1st comment. Please take a look and let me know if I missed anything or need to clarify something.

It looks like you may have missed my comment and the associated features noted in it? Summary version:

* A single diff + conversation view without needing to flick between tabs.

* No collapsed conversations (certainly not in the middle of conversations, possibly okay in an unthreaded view to have the oldest collapsed, like Phab does).

* A Stack view, allowing easy viewing and navigating of stacked PRs.

* "Outdated" comments not removed from the diff view, but instead kept near their original location.

Thanks, I've pasted this summary into the first comment.

[hubert-reinterpretcast]

I've tried to summarize all the feature requests in the 1st comment. Please take a look and let me know if I missed anything or need to clarify something.

From #56635 (comment):

We should specifically request that full context (including the rest of the tree) is available for all comments.

(This means being able to comment on unchanged files.)

From #56635 (comment):

We should specifically request that full text search of the various views be possible without having to expand things (for the "Files changed" view, should be able to search--even if the file is not loaded--changed lines/nearby context or search full content of affected files).

(This can be provided by providing a UI interface that does a server-side search. I am not seeing anything in the current summary that covers this possibility.)

[tstellar]

I've tried to summarize all the feature requests in the 1st comment. Please take a look and let me know if I missed anything or need to clarify something.

From #56635 (comment):

We should specifically request that full context (including the rest of the tree) is available for all comments.

(This means being able to comment on unchanged files.)

Is this covered by the first item in the list?

From #56635 (comment):

We should specifically request that full text search of the various views be possible without having to expand things (for the "Files changed" view, should be able to search--even if the file is not loaded--changed lines/nearby context or search full content of affected files).

(This can be provided by providing a UI interface that does a server-side search. I am not seeing anything in the current summary that covers this possibility.)

I tried to summarize this and added it to the list.

[h-vetinari]

Review individual commits in the same PR

To reduce the back-and-forth with GH (once the list is sent), I think the above needs to be clarified - it's already possible to review individual commits in a PR, unless there's a very particular workflow that I don't see (the diff of an individual commit - with regular commenting functionality - is accessible by clicking on any given commit from either the conversation view or the commits tab; there's "previous" / "next" buttons for the commits in the PR, and it's also possible to select commit ranges)

[nickdesaulniers]

Review individual commits in the same PR

Isn't this already supported?

From a PR -> "Commits" tab -> -> ->

[hubert-reinterpretcast]

@tstellar:

We should specifically request that full context (including the rest of the tree) is available for all comments.

(This means being able to comment on unchanged files.)

Is this covered by the first item in the list?

It isn't covered exactly: The first item (including the issue it links to) only covers being able to comment on files that the PR author changed.

It should be possible to do a full text search of the various views without having to expand things (for the "Files changed" view, should be able to search--even if the file is not loaded--changed lines/nearby context or search full content of affected files).

Suggestion (to clarify my previous write-up):
should be able to search [ ... ] changed lines/nearly context with an alternate mode to search full content of affected files

[nhaehnle]

@tstellar It is really important that we have an iterative feedback process with GitHub, precisely because of stuff like this:

Review individual commits in the same PR

Isn't this already supported?

From a PR -> "Commits" tab -> -> ->

I don't think that's exactly what was asked for. One can look at an individual commit's diff and make comments there. But it is not possible (in the UI) to approve/request changes on individual commits. That state is per-PR, not per-commit.

Also:

Allow users to view a diff between arbitrary versions of a PR: https://github.com/github/roadmap/issues/211

I have a problem with characterizing this request as being in 1:1 relation with that roadmap issue. That roadmap issue does talk about what I was asking for (compare different versions of a PR), but it seems to tie that to a clear regression of the PR workflow, which is having to manually mark a PR as "for review" after pushing an update. The latter seems like an awful idea.

[nhaehnle]

Another request: Allow adding review comments on commit messages as if they were code.

Rationale: Commit messages matter for the legibility of a project's history, and so being able to point out mistakes or remark on some aspect of a commit message is important.

[RKSimon]

But should we make it clear that these are all blockers, not just requests? I don't see why we should consider making the move if all we're promised is jam tomorrow.......

[asb]

But should we make it clear that these are all blockers, not just requests? I don't see why we should consider making the move if all we're promised is jam tomorrow.......

My interpretation of this post is that the LLVM project will move to a GitHub PR based flow as of Oct 1 2023. I'm not sure there's any mechanism to flag issues as being a "blocker" - it would be difficult to do so without consensus on which changes represent regressions on which of those are serious enough to risk the success of the move. To me, not having a solution for retaining full history of the evolution of a PR is a major loss, but people could reasonably point out many other successful projects have used GitHub successfully despite lacking this...

[nhaehnle]

@tstellar It is really important that we have an iterative feedback process with GitHub, precisely because of stuff like this:

@nhaehnle Well, we cannot force them to this :) We will only send them our feedback / suggestions, but whether they will do something, when, what will be the process, etc. is beyond our control. To be honest, I will not expect things done immediately by request. Maybe they will include them into their roadmap.

Right, I overstated my point. What I do care about is that it should be clear that GitHub don't get to declare "mission accomplished" on our requests based on their own interpretation without another round of feedback. This is of course standard practice when dealing with tickets and issues, but since this seems to be a more informal process, I find it worth pointing out.

[tstellar]

I went through and did another round of updates, let me know how it looks.

[joker-eph]

But should we make it clear that these are all blockers, not just requests? I don't see why we should consider making the move if all we're promised is jam tomorrow.......

This is a good point about the overall migration: what are the things (if any) that are "deal breakers" for us? (that is: we will delay the move until we have a solution)

And for these, what is the alternative to GitHub proposing a builtin solution? (custom action and other kind of scripting, I think this how Tom has been looking at it here for example: #56638 ).

[rymiel]

Note that on the discourse topic for this , Aaron also brought up a fork of Phabricator that seems to have taken up the reigns of maintenance? https://we.phorge.it/phame/post/view/1/going_public/
in case GitHub just doesn't get their stuff together?

[tstellar]

This list of features was submitted to GitHub on Nov 7, 2022.