forgejo/discussions
49
43

Patch-based code review UI #325

Open
opened 2025年04月01日 19:47:37 +02:00 by senekor · 27 comments

I'm a contributor of Jujutsu, a git-compatible VCS. I'm also a happy Forgejo self-hoster! 😄 Jujutsu is currently engaged in an effort that would benefit from collaboration across the Git ecosystem, including Forgejo. I will explain as concisely as possible.

Many VCS-enthousiasts are excited about patch-based code review / stacked PRs. Code review tools that descend from the GitHub tradition, including those of GitLab and Forgejo, don't support these workflows well. That's because it's hard to track what has and hasn't been reviewed between force-pushes. This incentivizes continuously adding meaningless "fixup commits" on top of a branch, to make iterative code review easier. At the end, PRs are often squashed to clean up the history, losing valuable intermediary information in the process.

Unfortunately, there is no trivial solution to this. After a force-push, it's not generally possible for Forgejo & Co. to associate old and new versions of a commit. Content- and metadata-based heuristics could probably go a long way, but they won't be perfect. Jujutsu and other projects have the answer to this: In addition to the SHA hash, a commit is associated with a change-id that doesn't change after a commit --amend or rebase -i. Jujutsu, GitButler and Gerrit have just agreed to standardize on a common format for a change-id commit header. That means Jujutsu users will soon start pushing commits to Forgejo instances that contain this header. It could be used by Forgejo to provide a reliable, patch-based review experience.

My question to Forgejo contributors is: Would you be interested in adding a patch-based review UI to Forgejo that uses this commit header? I'm actually interested in contributing / helping with this UI myself. I have many ideas about how that could work and what benefits it will bring. The important part is that commits (and commit messages!) should be at the center and reviewers can easily track what they haven't reviewed yet, even across a force-push. By default, reviewers should only be presented with the interdiff of the current version and the one they previously reviewed. Feel free to ask about / discuss details, just trying to keep this first message short.

We are also planning to convince the Git project itself to support this new standardized header. This would mean that every Forgejo user would benefit from reliable patch-based code review, not just Jujutsu users. We assume the Git project will be the hardest to convince, so we're gathering all the social momentum we can before we talk to them 😉

I'm looking forward to discuss with you how to improve Forgejo and at the same time uplift the entire Git ecosystem.

I'm a contributor of [Jujutsu](https://github.com/jj-vcs/jj?tab=readme-ov-file#jujutsua-version-control-system), a git-compatible VCS. I'm also a happy Forgejo self-hoster! 😄 Jujutsu is currently engaged in an effort that would benefit from collaboration across the Git ecosystem, including Forgejo. I will explain as concisely as possible. Many VCS-enthousiasts are excited about [patch-based code review](https://blog.gitbutler.com/interdiff-review-with-git-range-diff/) / [stacked PRs](https://graphite.dev/blog/stacked-prs). Code review tools that descend from the GitHub tradition, including those of GitLab and Forgejo, don't support these workflows well. That's because it's hard to track what has and hasn't been reviewed between force-pushes. This incentivizes continuously adding meaningless "fixup commits" on top of a branch, to make iterative code review easier. At the end, PRs are often squashed to clean up the history, losing valuable intermediary information in the process. Unfortunately, there is no trivial solution to this. After a force-push, it's not generally possible for Forgejo & Co. to associate old and new versions of a commit. Content- and metadata-based heuristics could probably go a long way, but they won't be perfect. Jujutsu and other projects have the answer to this: In addition to the SHA hash, a commit is associated with a change-id that doesn't change after a `commit --amend` or `rebase -i`. Jujutsu, [GitButler](https://gitbutler.com/) and [Gerrit](https://www.gerritcodereview.com/) have just agreed to standardize on a common format for a _change-id commit header_. That means Jujutsu users will soon start pushing commits to Forgejo instances that contain this header. It could be used by Forgejo to provide a reliable, patch-based review experience. My question to Forgejo contributors is: Would you be interested in adding a patch-based review UI to Forgejo that uses this commit header? I'm actually interested in contributing / helping with this UI myself. I have many ideas about how that could work and what benefits it will bring. The important part is that commits (and commit messages!) should be at the center and reviewers can easily track what they haven't reviewed yet, even across a force-push. By default, reviewers should only be presented with the interdiff of the current version and the one they previously reviewed. Feel free to ask about / discuss details, just trying to keep this first message short. We are also planning to convince the Git project itself to support this new standardized header. This would mean that every Forgejo user would benefit from reliable patch-based code review, not just Jujutsu users. We assume the Git project will be the hardest to convince, so we're gathering all the social momentum we can before we talk to them 😉 I'm looking forward to discuss with you how to improve Forgejo and at the same time uplift the entire Git ecosystem.
Author
Copy link

For context, discussion to support the change-id header in Git itself has already started on the Git mailing list.

For context, discussion to support the change-id header in Git itself has already started on the [Git mailing list](https://lore.kernel.org/git/CAESOdVAspxUJKGAA58i0tvks4ZOfoGf1Aa5gPr0FXzdcywqUUw@mail.gmail.com/T/#u).
Author
Copy link

The implementation in Jujutsu is done, commit objects with the change-id header set are already being sent over the wire. It's ready for Forgejo to take advantage of.

The implementation in Jujutsu is done, commit objects with the change-id header set are already being sent over the wire. It's ready for Forgejo to take advantage of.

A good first step to show jujustu can be used as a drop-in replacement for Git, even if added features are not taken advantage of, would be to run the test suite with it instead of Git. What do you think?

A good first step to show jujustu can be used as a drop-in replacement for Git, even if added features are not taken advantage of, would be to run the test suite with it instead of Git. What do you think?
Author
Copy link

I'm not sure what you mean. Jujutsu is not a drop-in replacement for Git, but it shells out to Git to send commits to a remote for compatibility.

I'm not sure what you mean. Jujutsu is not a drop-in replacement for Git, but it shells out to Git to send commits to a remote for compatibility.

Assuming Jujutsu is compatible with Git ("A Git-compatible VCS that is both simple and powerful "), it could be used instead of Git. Maybe it is not 100% compatible? I am missing something apparently 😁

Assuming Jujutsu is compatible with Git ("A Git-compatible VCS that is both simple and powerful "), it could be used instead of Git. Maybe it is not 100% compatible? I am missing something apparently 😁
Author
Copy link

Well, the CLI is not the same as git. It's compatible in the sense that you can pull and push to a git remote without anyone noticing the difference. Maybe I'm misunderstanding how the tests work, but if the git CLI is being called to simulate a client, then that can't easily be replaced by jj. Not without rewriting all the commands anyway. But I'm also not sure what value that would bring, we'd basically be testing jj and not forgejo. (I've never hear of any bugs with the git compatibility.)

Well, the CLI is not the same as git. It's compatible in the sense that you can pull and push to a git remote without anyone noticing the difference. Maybe I'm misunderstanding how the tests work, but if the git CLI is being called to simulate a client, then that can't easily be replaced by jj. Not without rewriting all the commands anyway. But I'm also not sure what value that would bring, we'd basically be testing jj and not forgejo. (I've never hear of any bugs with the git compatibility.)
Author
Copy link

Even for the additional change-id header, it's not that relevant if jj or git are the clients. Git will happily construct new commits with the same header with commit --amend and replay. So while not very likely at the moment, forgejo can receive commit objects with that header from any client.

Even for the additional change-id header, it's not that relevant if jj or git are the clients. Git will happily construct new commits with the same header with `commit --amend` and `replay`. So while not very likely at the moment, forgejo can receive commit objects with that header from any client.

It is worth clarifying what is compatible and what is not, IMHO. But indeed, if the CLI is not compatible my suggestion is pointless 😊

It is worth clarifying what is compatible and what is not, IMHO. But indeed, if the CLI is not compatible my suggestion is pointless 😊
Author
Copy link

Here's a great piece that goes in more detail about the review workflow that's being proposed (vs the GitHub model):

Why some of us like "interdiff" code review

Here's a great piece that goes in more detail about the review workflow that's being proposed (vs the GitHub model): [Why some of us like "interdiff" code review](https://gist.github.com/thoughtpolice/9c45287550a56b2047c6311fbadebed2)

To add a stone to your social momentum bucket, I'd be interested in using (and possibly working on) this feature in Gitea. Reviewing things in this system sounds a lot nicer than having to either look at full diff to see if the issue was fixed or individual commits and possibly missing something that came later.

To add a stone to your social momentum bucket, I'd be interested in using (and possibly working on) this feature in Gitea. Reviewing things in this system sounds a lot nicer than having to either look at full diff to see if the issue was fixed or individual commits and possibly missing something that came later.

@senekor Actually, would you mind posting a similar issue in Gitea repository and document what would be required for this? I don't believe similar suggestion has been made before in the tracker and while I could do it myself, I've never used this workflow before while you're familiar with it (or at least it seems so), so you'd know better how to implement it, how it works and what is needed for this.

I don't want to have to bring up Gitea in Forgejo's territory to look at this feature or discuss it. I'm not comfortable with this - nor I think it's welcome to do.

Also this isn't a request for you to make a port of your changes (though I guess no one there would mind provided they would comply with contribution guidelines there) - having an outline of how such workflow should function would be very helpful for someone to attempt it there. Especially since I cannot look at changes made here.

@senekor Actually, would you mind posting a similar issue in Gitea repository and document what would be required for this? I don't believe similar suggestion has been made before in the tracker and while I could do it myself, I've never used this workflow before while you're familiar with it (or at least it seems so), so you'd know better how to implement it, how it works and what is needed for this. I don't want to have to bring up Gitea in Forgejo's territory to look at this feature or discuss it. I'm not comfortable with this - nor I think it's welcome to do. Also this isn't a request for you to make a port of your changes (though I guess no one there would mind provided they would comply with contribution guidelines there) - having an outline of how such workflow should function would be very helpful for someone to attempt it there. Especially since I cannot look at changes made here.
Author
Copy link

I'm personally not interested in contributing to Gitea, but please feel free to drop by on the Jujutsu Discord server. I'm sure plenty of people there will be happy to discuss these ideas with you.

I'm personally not interested in contributing to Gitea, but please feel free to drop by on the [Jujutsu Discord server](https://discord.gg/dkmfj3aGQN). I'm sure plenty of people there will be happy to discuss these ideas with you.

The Gitea project keeps harassing Forgejo contributors in various ways, please ignore thefox messages.

The Gitea project keeps harassing Forgejo contributors in various ways, please ignore thefox messages.
Author
Copy link

Yeah, I've stumbled across some of it. It's impressive how well you guys are putting up with the harassment. Thanks for keeping this project going with a healthy community, it's the reason I want to contribute to Forgejo and not something else.

Yeah, I've stumbled across some of it. It's impressive how well you guys are putting up with the harassment. Thanks for keeping this project going with a healthy community, it's the reason I want to contribute to Forgejo and not something else.
Author
Copy link

The people from tangled have been talking to jujutsu folks as well, they're currently in the process of implementing a patch-based review UI. Here's their blog post about it, I think it's a great perspective on the topic.

The people from tangled have been talking to jujutsu folks as well, they're currently in the process of implementing a patch-based review UI. Here's their [blog post](https://blog.tangled.sh/pulls) about it, I think it's a great perspective on the topic.
Member
Copy link

Hey, I really like the appearance of it and as a person who has worked (at least on the documentation side) on AGit, I'd say that I'd love to have more than one workflow other than the typical GitHub-y one. Where one would go from here, I don't know - but I'm just sending positive vibes and that I'd want to see this. :)

Hey, I really like the appearance of it and as a person who has worked (at least on the documentation side) on AGit, I'd say that I'd love to have more than one workflow other than the typical GitHub-y one. Where one would go from here, I don't know - but I'm just sending positive vibes and that I'd want to see this. :)
Owner
Copy link

I am interested in considering this feature from a usability perspective, however my time budget is pretty tight. If someone is interested in working on this, feel free to ping me for a conversation on how things could be best presented to users.

I am interested in considering this feature from a usability perspective, however my time budget is pretty tight. If someone is interested in working on this, feel free to ping me for a conversation on how things could be best presented to users.

As a long-time Gerrit (and Gitea/ForgeJo) user, I would love to have this feature in ForgeJo. Has the development already started somewhere?

As a long-time Gerrit (and Gitea/ForgeJo) user, I would love to have this feature in ForgeJo. Has the development already started somewhere?
Member
Copy link

@kisg Hey there, are you interested in contributing to its development? :)

@kisg Hey there, are you interested in contributing to its development? :)
Author
Copy link

Small update from my side, I will soon start a project to build a standalone code review tool that implements the ideas here. It will be compatible with Forgejo and other code sharing platforms. While that's not a direct contribution to Forgejo, its users will still benefit from it. I still think having a direct implementation in Forgejo would be very nice and I'll be happy to help as time permits if anyone else wants to work on it. Once my standalone tool is mature enough, it may serve as inspiration for a direct implementation in Forgejo. Showing something that works is usually better than explaining ideas in words.

Small update from my side, I will soon start a project to build a _standalone_ code review tool that implements the ideas here. It will be compatible with Forgejo and other code sharing platforms. While that's not a direct contribution to Forgejo, its users will still benefit from it. I still think having a direct implementation in Forgejo would be very nice and I'll be happy to help as time permits if anyone else wants to work on it. Once my standalone tool is mature enough, it may serve as inspiration for a direct implementation in Forgejo. Showing something that works is usually better than explaining ideas in words.

I'm interested in taking some of the ideas discussed here into an implementation in Forgejo; I've begun a design document to that end

My initial proposed approach differs a bit from some of the approaches discussed here by leaning more into branches and stacked PRs, rather than approaches that rely heavily on history-rewriting tools. I feel that this pathway is more accessible to most developers. But I'm very open for discussion on all aspects of this design if anyone wants to join me over on the design issue.

It's in early stages but you can expect to see updates quickly for the next couple days.

I'm interested in taking some of the ideas discussed here into an implementation in Forgejo; I've begun a design document to that end - [Latest Draft Direct Link](https://codeberg.org/mfenniak/forgejo-design/src/branch/stacked-prs/features/Stacked%20Pull%20Requests/README.md) - [Pull Request w/ any Design Discussion](https://codeberg.org/forgejo/design/pulls/48) - [Latest Draft User Research Link](https://codeberg.org/mfenniak/forgejo-user-research/src/branch/managing-large-change-session-1/managing-large-changes/README.md) - [Pull Request w/ any User Research Discussion](https://codeberg.org/forgejo/user-research/pulls/65) My initial proposed approach differs a bit from some of the approaches discussed here by leaning more into branches and stacked PRs, rather than approaches that rely heavily on history-rewriting tools. I feel that this pathway is more accessible to most developers. But I'm very open for discussion on all aspects of this design if anyone wants to join me over on the design issue. It's in early stages but you can expect to see updates quickly for the next couple days.

Thanks for your work @mfenniak

I found this interesting:

The primary insight for me is that most of the best practices and advice for managing large change is to avoid it. So, developing new tools in Forgejo to support those use-cases may not be overall advisable. I'm kinda on the fence over this, but I think for my future plans it would be more practical to target small tactical improvements that support all kinds of change management.

I think this gets the cauasal relationship backwards. It's not that you have a big change and then you want to adopt a patch-based workflow or stacked PRs to break it down. Rather a patch-based workflow reduces the cost of small changes, meaning that you don't structure your changes as a large on in the first place.

For instance, patch-based changes lead to a much easier experience when reviewing rebased changes. Without this feature, you are more likely to wait until a set of changes is completely ready before reviewing. This would mean you end up reviewing one larger change rather than a stream of smaller ones.

I think similar factors come into play when other parts of DX are difficult. For instance, if it takes hours to run a test suite, I am much more likely to structure my changes as monolithic commits since the cost of running the test suite is high.

So, the way I see it the point of the patch-based workflow is to lower the cost of small changes, rather than making big changes easier to manage.

The way I see it a patch-based workflow is about making it easy to:

  • review diffs of diffs (either from a rebase, or from a new version of a change)
  • visualize dependencies between review units
  • allow submitting/updating a stack of changes with one push
Thanks for your work @mfenniak I found [this](https://codeberg.org/forgejo/user-research/pulls/65#issuecomment-6188290) interesting: > The primary insight for me is that most of the best practices and advice for managing large change is to avoid it. So, developing new tools in Forgejo to support those use-cases may not be overall advisable. I'm kinda on the fence over this, but I think for my future plans it would be more practical to target small tactical improvements that support all kinds of change management. I think this gets the cauasal relationship backwards. It's not that you have a big change and then you want to adopt a patch-based workflow or stacked PRs to break it down. Rather a patch-based workflow reduces the cost of small changes, meaning that you don't structure your changes as a large on in the first place. For instance, patch-based changes lead to a much easier experience when reviewing rebased changes. Without this feature, you are more likely to wait until a set of changes is completely ready before reviewing. This would mean you end up reviewing one larger change rather than a stream of smaller ones. I think similar factors come into play when other parts of DX are difficult. For instance, if it takes hours to run a test suite, I am much more likely to structure my changes as monolithic commits since the cost of running the test suite is high. So, the way I see it the point of the patch-based workflow is to lower the cost of small changes, rather than making big changes easier to manage. The way I see it a patch-based workflow is about making it easy to: - review diffs of diffs (either from a rebase, or from a new version of a change) - visualize dependencies between review units - allow submitting/updating a stack of changes with one push

It seems this may have stalled? After reading more about this feature, I would definitely like this in Forgejo, as it definitely would help prevent large pull requests by adopting stacked pull requests with rebases or more incremental reviews of pull requests that use force pushes for a clean history.

It seems this may have stalled? After reading more about this feature, I would definitely like this in Forgejo, as it definitely would help prevent large pull requests by adopting stacked pull requests with rebases or more incremental reviews of pull requests that use force pushes for a clean history.

Here's a comment explaining what happened with the linked proposal: forgejo/design#48 (comment)

Here's a comment explaining what happened with the linked proposal: https://codeberg.org/forgejo/design/pulls/48#issuecomment-6188299

I've left this topic a little unresolved, as I think there's still plenty of room to talk about where it can go in the future.

I jumped into designing a Forgejo-based "stacked PR" concept after reading the ideas here. After being redirected into user research by other contributors, that research didn't lead me in the direction of what I had envisioned. And that's fine -- I think I had too big of a chunk of development in mind.

I think rolling this discussion back towards @senekor's original topic -- Forgjeo v12 included an improved UX for commit-based reviews. There is definitely room to continue to improve this in incremental changes towards the ideas presented:

The important part is that commits (and commit messages!) should be at the center and reviewers can easily track what they haven't reviewed yet, even across a force-push.

  • Present: UX can put commits & messages at the center of the experience, when reviewers choose to.
  • Missing: Reviewers can't track what they've reviewed by commit (only by file in the overall view).
  • Missing: You can diff a force push, but you can't split that view by commits, and obviously it doesn't integrate with commit tracking since that doesn't exist.

These seem like very practical changes. I think they're well supported by my user research and the input here. The "next step" is probably to create mock UIs of these changes, and I think if we take them in small chunks they're quite achievable.

(Aside: I've become a Jujutsu user over the past few months and I quite enjoy the experience. A+.)

I've left this topic a little unresolved, as I think there's still plenty of room to talk about where it can go in the future. I jumped into designing a Forgejo-based "stacked PR" concept after reading the ideas here. After being redirected into user research by other contributors, that research didn't lead me in the direction of what I had envisioned. And that's fine -- I think I had too big of a chunk of development in mind. I think rolling this discussion back towards @senekor's original topic -- Forgjeo v12 included [an improved UX for commit-based reviews](https://forgejo.org/2025-07-release-v12-0/#improved-ux-for-per-commit-reviews). There is definitely room to continue to improve this in incremental changes towards the ideas presented: > The important part is that commits (and commit messages!) should be at the center and reviewers can easily track what they haven't reviewed yet, even across a force-push. - Present: UX *can* put commits & messages at the center of the experience, when reviewers choose to. - Missing: Reviewers can't track what they've reviewed by commit (only by file in the overall view). - Missing: You can diff a force push, but you can't split that view by commits, and obviously it doesn't integrate with commit tracking since that doesn't exist. These seem like very practical changes. I think they're well supported by my user research and the input here. The "next step" is probably to create mock UIs of these changes, and I think if we take them in small chunks they're quite achievable. (Aside: I've become a Jujutsu user over the past few months and I quite enjoy the experience. A+.)

Hello there!

I am interested in contributing to this feature. I have opened an issue to discuss, but I didn't think there was a discussion about it already. I apologize for not searching here.

I'll study the proposed designs, conversations, and the repo for the necessary changes and come back with a new design (or just an adapted version of @mfenniak's one 😄)

Overall, right now, it looks like the changes are very straightforward, but only time will tell if I come back to eat my words.

Hello there! I am interested in contributing to this feature. I have opened an [issue](https://codeberg.org/forgejo/forgejo/issues/10641) to discuss, but I didn't think there was a discussion about it already. I apologize for not searching here. I'll study the proposed designs, conversations, and the repo for the necessary changes and come back with a new design (or just an adapted version of @mfenniak's one 😄) Overall, right now, it looks like the changes are very straightforward, but only time will tell if I come back to eat my words.

@senekor wrote in #325 (comment):

The people from tangled have been talking to jujutsu folks as well, they're currently in the process of implementing a patch-based review UI. Here's their blog post about it, I think it's a great perspective on the topic.

Tangled has native support for git change-id header now, see this blog post for its workflow and UI design.

@senekor wrote in https://codeberg.org/forgejo/discussions/issues/325#issuecomment-3844220: > The people from tangled have been talking to jujutsu folks as well, they're currently in the process of implementing a patch-based review UI. Here's their [blog post](https://blog.tangled.sh/pulls) about it, I think it's a great perspective on the topic. Tangled has native support for git change-id header now, see this [blog post](https://blog.tangled.org/stacking/) for its workflow and UI design.
Sign in to join this conversation.
No Branch/Tag specified
No results found.
No results found.
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
12 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
forgejo/discussions#325
Reference in a new issue
forgejo/discussions
No description provided.
Delete branch "%!s()"

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?