16
7
Fork
You've already forked design
4

WIP: "Stacked Pull Request" design document for Forgejo #48

Closed
mfenniak wants to merge 10 commits from mfenniak/forgejo-design:stacked-prs into main
pull from: mfenniak/forgejo-design:stacked-prs
merge into: forgejo:main
forgejo:main
forgejo:15-new-pr
forgejo:private-issues
forgejo:file-editor

I've been following the discussions on forgejo/discussions#325 with quite a bit of interest, and I'm intrigued at the possibilities for a drastically better large-scale code review that could be enabled by improvements in this area. Over the past two days I've started a design document around the concept, which is attached.

This isn't complete, but as I'm starting work on UI mock-ups I thought it was time to surface the design for the earliest feedback possible.

I'm planning on implementing this feature end-to-end, if there is enough contributor interest to support the related work (e.g. design reviews, code reviews, etc.) and we can collaborate on a design that reaches mutual satisfaction.

I've been following the discussions on https://codeberg.org/forgejo/discussions/issues/325 with quite a bit of interest, and I'm intrigued at the possibilities for a drastically better large-scale code review that could be enabled by improvements in this area. Over the past two days I've started a design document around the concept, which is attached. This isn't complete, but as I'm starting work on UI mock-ups I thought it was time to surface the design for the earliest feedback possible. I'm planning on implementing this feature end-to-end, if there is enough contributor interest to support the related work (e.g. design reviews, code reviews, etc.) and we can collaborate on a design that reaches mutual satisfaction.
@ -0,0 +31,4 @@
***`p5) Change after Review`** Adapting to review feedback becomes more complex when using this strategy; for example, assuming that a review comment was made on one of the earlier changes to add a schema table for "Feature X", typical tooling today would require either:
* Using commits:
* Creating new commits with changes associated with the code review, and then using `git rebase` to squash into the earlier commit – which will cause rebase conflicts if the changes affect later commits. This can be automated (to some extent) by creating new commits with `git commit --fixup ...`, and then using `git rebase --autosquash`.
* A force push is required for the modified branch, which rewrites commit identifiers and prevents a reviewer from seeing *only what has changed* since the last review (a feature of GitHub; but not Forgejo presently)
Contributor
Copy link

Here is how I deal with that particular problem in Forgejo.

  • As a PR author, when force-pushing changes to apply review suggestions, I link the compare diff that I obtain from the conversation page.
    image
  • As reviewer, I look at the conversation panel looking for new commits and/or force push and go over them to see the differences if the author does not point at them explicitly.
Here is how I deal with that particular problem in Forgejo. - As a PR author, when force-pushing changes to apply review suggestions, I [link the compare diff](https://code.forgejo.org/forgejo/runner/compare/4e8d0a3fad9e098eaf60c843072300bead4fd062..c17d65711b9ca7a36443b98214f85097f0a4be40) that I obtain from the conversation page. ![image](/attachments/4da81393-e559-4444-9b5e-5ba43f297d60) - As reviewer, I look at the conversation panel looking for new commits and/or force push and go over them to see the differences if the author does not point at them explicitly.
mfenniak marked this conversation as resolved
@ -0,0 +51,4 @@
* **Gerrit** - A forge & code review tool which uses virtual branches and the git CLI in order to create commits (which can be iterated with new patchsets via future modifications to commits with the same `Change-Id`); a series of changes can be made as a sequence of commits with unique Change-Id that are pushed at the same time.
* **Phabricator** - Was an internal tool at Facebook, currently only maintained as a community fork named [Phorge](https://we.phorge.it/). Other than a quick view of the software (which, on Phorge itself, lacks a code review policy or other "audits"), I haven't explored its model much.
Contributor
Copy link

The Phabricator / Phorge UX for reviews has been a painful experience for me. IMHO it is not worth digging into.

The Phabricator / Phorge UX for reviews has been a painful experience for me. IMHO it is not worth digging into.
@ -0,0 +64,4 @@
* **[ghstack](https://github.com/ezyang/ghstack)** - A Python tool that will turn multiple commits on a branch into multiple pull requests in GitHub, one per commit; modifying each PR requires changing the commit that generated that PR, either through a rebase-and-squash into the commit, or an `--amend` if appropriate. However the tool is limited in scope to GitHub, doesn't allow PRs to be generated from forks, and "You will NOT be able to merge these commits using the normal GitHub UI" and must use a `ghstack land` CLI option.
* **[Stacked Git](https://stacked-git.github.io/)** - An application for managing Git commits as a stack of patches... although it provides an alternate CLI for editing and managing a queue of patches, it doesn't seem to touch extensively on the problems described in this document. It may be a useful "engine" to implement some patch manipulation options though.
Contributor
Copy link

IIRC Mercurial has a unique and conceptually more advanced model for dealing with commits and changeset. I can't recall where or how but when I looked at it there were enlightening notions. I did not come back to it in a very long time because Mercurial is unfortunately fading away and such models require a lot of energy to promote and implement.

I apologize: this is not much to go on, just a hint that there might be something good for inspiration in there.

IIRC Mercurial has a unique and conceptually more advanced model for dealing with commits and changeset. I can't recall where or how but when I looked at it there were enlightening notions. I did not come back to it in a very long time because Mercurial is unfortunately fading away and such models require a lot of energy to promote and implement. I apologize: this is not much to go on, just a hint that there might be something good for inspiration in there.
@ -0,0 +4,4 @@
* When faced with a pull request with a moderate-to-large scope, it is difficult for a code-reviewer to effectively review the work. For example, assume that a pull request "Add Feature X" involves changes to 100 files; this is beyond the capacity for a reviewer to hold the changes in their head, understand their interactions, deduce their reasonings, and maintain enough focus to perform an effective review.
* Developers can attempt to reduce this difficulty by splitting the work into either multiple well-formed cohesive commits, or multiple pull requests. "Add Feature X" can, instead, be created as a series of commits or pull requests such as "Add new schema table for Feature X", "Create new library methods for Feature X", "Refactor to use new library methods for Feature X", "Remove obsolete library methods post-'Feature X'".
Contributor
Copy link

A common way to deal with this is also by splitting part of the codebase into a separate repository and include it as a dependency. One way to look at it is that if you find yourself in need of entangled pull requests / branches on a given area of your codebase, it may be a sign that it needs its own repository. In that case the release / tag can be seen as an alternative to a merged pull request where acceptance in the codebase is by reading the release notes rather than reviewing the actual changes.

The difficulty in doing (and the motivation for having a "monorepo", IMHO) is the historical lack of tooling to deal with dependencies at the developper level. It has been the realm of people dealing with distributions and packages (in some cases with dedicated and complex tooling - Debian Developers). But only in the recent years were tools available in forges to enpower the developer:

A common way to deal with this is also by splitting part of the codebase into a separate repository and include it as a dependency. One way to look at it is that if you find yourself in need of entangled pull requests / branches on a given area of your codebase, it may be a sign that it needs its own repository. In that case the release / tag can be seen as an alternative to a merged pull request where acceptance in the codebase is by reading the release notes rather than reviewing the actual changes. The difficulty in doing (and the motivation for having a "monorepo", IMHO) is the historical lack of tooling to deal with dependencies at the developper level. It has been the realm of people dealing with distributions and packages (in some cases with dedicated and complex tooling - Debian Developers). But only in the recent years were tools available in forges to enpower the developer: - dependency management - (dependabot / renovate) - reverse-dependency management - (no tooling, Forgejo has [a custom tool to do that](https://code.forgejo.org/actions/cascading-pr/) used as explained roughly here https://code.forgejo.org/actions/cascading-pr/#forgejo-dependencies). For instance it is used to verify that a change in a repository ([ACT](https://code.forgejo.org/forgejo/act)) is not going to break the CI of other repositories that depend on it ([forgejo end-to-end testing](https://code.forgejo.org/forgejo/end-to-end)).
Author
Member
Copy link

Hm... I think this is an interesting thought... I'm not quite sure what to do with it at the moment in the scope of this design.

My experience is that when this occurs, it introduces additional review complexity in the area of "reviewing each change without the context of later incoming changes". But that would typically be managed pretty easily with a link between the related changes.

Hm... I think this is an interesting thought... I'm not quite sure what to do with it at the moment in the scope of this design. My experience is that when this occurs, it introduces additional review complexity in the area of "reviewing each change without the context of later incoming changes". But that would typically be managed pretty easily with a link between the related changes.
@ -0,0 +275,4 @@
## TODO
> **🚨 TODO Items:**
Contributor
Copy link
  • Collecting first hand user stories / facts dealing with pull requests as evidence of what people actually do nowadays, the problems they experience and the workaround / techniques they use to solve them (which tools or methods when there are no tools)

This is time consuming because reaching out to users and getting them to tell their own story instead of sharing theoretical ideas is not as easy as it seems. But I'm absolutely convinced that without those evidence the odds of identifying the problems that deserve solving and the chances of having such a solution adopted by users are not great.

- Collecting first hand user stories / facts dealing with pull requests as evidence of what people actually do nowadays, the problems they experience and the workaround / techniques they use to solve them (which tools or methods when there are no tools) This is time consuming because reaching out to users and getting them to tell their own story instead of sharing theoretical ideas is not as easy as it seems. But I'm absolutely convinced that without those evidence the odds of identifying the problems that deserve solving and the chances of having such a solution adopted by users are not great.
Author
Member
Copy link

I agree. Although it's very tempting for me to say that my own experience is enough to support many of the assertions and plans in this design... I'd prefer to put in the right effort to learn if there is a gem to be found here.

I've finished a first draft of a first user research session proposal: forgejo/user-research#65

Will add this to the TODO list on my next design revision as a clear blocker for moving forward. 👍

I agree. Although it's very tempting for me to say that my own experience is enough to support many of the assertions and plans in this design... I'd prefer to put in the right effort to learn if there is a gem to be found here. I've finished a first draft of a first user research session proposal: https://codeberg.org/forgejo/user-research/pulls/65 Will add this to the TODO list on my next design revision as a clear blocker for moving forward. 👍
Author
Member
Copy link

Some great user research data is trickling its way in (forgejo/user-research#65). My initial thoughts in reviewing the research are...

In the broadest scope of the feedback, I'm a little skeptical that the initial design work here may run contrary to the clear best-practices articulated by survey respondents. "Stacked Pull Requests" as initially designed may be a tool that aids a developer in not performing continuous integration, and not merging and managing small changes. While there are clearly cases that larger scoped changes are necessary, I begin to wonder whether making large changes less painful may make them more common, and therefore have a larger overall quantity of pain. 🤔

There have been problems raised which may be rich for enhancement in Forgejo's existing code review system, which would be applicable for a larger number of users. If there's ever a capability to make "Stacked Pull Requests", it would be built on the foundation of reviewing individual pull requests -- and reinforcing that foundation may be a rational starting point. Some specific problems noted in the research:

  • Merging of pull requests by one contributor while there were open unresolved review comments
  • No automated aid in resolving a review comment and telling the reviewer "here's the relevant change to resolve this"
    • Challenges in loss of context of comment and related change, especially when force pushes are involved
  • Unclear responsibility in "resolving" a review; does a dev do that when incorporating a suggested change, or does a reviewer do that when reviewing that the suggested change is complete?

I have a few outstanding user research volunteers that I intend to collect from and ruminate upon before making any decisions, but I'm currently leaning towards designing some review refinements and shelving the stacked concept design for the moment.

Some great user research data is trickling its way in (forgejo/user-research#65). My initial thoughts in reviewing the research are... In the broadest scope of the feedback, I'm a little skeptical that the initial design work here may run contrary to the clear best-practices articulated by survey respondents. "Stacked Pull Requests" as initially designed may be a tool that aids a developer in *not* performing continuous integration, and not merging and managing small changes. While there are clearly cases that larger scoped changes are necessary, I begin to wonder whether making large changes less painful may make them more common, and therefore have a larger overall quantity of pain. 🤔 There have been problems raised which may be rich for enhancement in Forgejo's existing code review system, which would be applicable for a larger number of users. If there's ever a capability to make "Stacked Pull Requests", it would be built on the foundation of reviewing individual pull requests -- and reinforcing that foundation may be a rational starting point. Some specific problems noted in the research: - Merging of pull requests by one contributor while there were open unresolved review comments - No automated aid in resolving a review comment and telling the reviewer "here's the relevant change to resolve this" - Challenges in loss of context of comment and related change, especially when force pushes are involved - Unclear responsibility in "resolving" a review; does a dev do that when incorporating a suggested change, or does a reviewer do that when reviewing that the suggested change is complete? I have a few outstanding user research volunteers that I intend to collect from and ruminate upon before making any decisions, but I'm currently leaning towards designing some review refinements and shelving the stacked concept design for the moment.
Author
Member
Copy link

Closing this out for the time being, as noted previously (I'm currently leaning towards designing some review refinements and shelving the stacked concept design for the moment) and to clarify that it isn't current and active work.

Closing this out for the time being, as noted previously (I'm currently leaning towards designing some review refinements and shelving the stacked concept design for the moment) and to clarify that it isn't current and active work.
mfenniak closed this pull request 2025年08月09日 04:20:56 +02:00
Contributor
Copy link

It was a very interesting exploration 👍

It was a very interesting exploration 👍

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
User research - Accessibility
Requires input about accessibility features, likely involves user testing.
User research - Blocked
Do not pick as-is! We are happy if you can help, but please coordinate with ongoing redesign in this area.
User research - Community
Community features, such as discovering other people's work or otherwise feeling welcome on a Forgejo instance.
User research - Config (instance)
Instance-wide configuration, authentication and other admin-only needs.
User research - Errors
How to deal with errors in the application and write helpful error messages.
User research - Filters
How filter and search is being worked with.
User research - Future backlog
The issue might be inspiring for future design work.
User research - Git workflow
AGit, fork-based and new Git workflow, PR creation etc
User research - Labels
Active research about Labels
User research - Moderation
Moderation Featuers for Admins are undergoing active User Research
User research - Needs input
Use this label to let the User Research team know their input is requested.
User research - Notifications/Dashboard
Research on how users should know what to do next.
User research - Rendering
Text rendering, markup languages etc
User research - Repo creation
Active research about the New Repo dialog.
User research - Repo units
The repo sections, disabling them and the "Add more" button.
User research - Security
User research - Settings (in-app)
How to structure in-app settings in the future?
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo/design!48
Reference in a new issue
forgejo/design
No description provided.
Delete branch "mfenniak/forgejo-design:stacked-prs"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?