Phabricator RFC

From Koha Wiki
Jump to navigation Jump to search

Doing code review with Phabricator/Phorge instead of Bugzilla (idea)

Status: unknown
Sponsored by: no-sponsor
Developed by: community effort probably
Expected for: who knows; [created at 2024-12-03]
Bug number: Bug {{{bug}}}
Work in progress repository: https://we.phorge.it/
Description: Replacing code review with Phabricator (actually Phorge, its fork), while keeping Bugzilla used for bugs, akin to what Mozilla does


I propose to migrate code review to Phorge and use Bugzilla only for bugs. This is what Mozilla itself does successfully (and they're the freaking original authors of Bugzilla itself!).

While Bugzilla is nice for issue tracking, and it has a bunch of history in Koha already populated, plus a bunch of nice tools around it like the Dashboard, it's not really a good tool for code review, even with `git bz`:

  • viewing changes in the Web UI is not nice:
    • cannot view all changes at once
    • cannot view the file around, such as open the full file, view more lines above/below, quickly open
    • cannot easily see git blame
    • cannot see darker code highlight in the places where the line was actually changed, in case the change encompasses a fix to existing line of code
    • cannot leave comments individually under a single line of code
    • cannot view diffs between replaced sets of patches, when patches were updated by someone (did they just sign off or changed something else?)
  • the process of manual sign-off is tiresome, even with Bugzilla, compared to making an approval review in a web UI
  • cannot easily run unit-tests after each code change (like [1] for the example link posted below); in Koha we wouldn't run full unit tests as they take long, but could run simplified ones that QA runs, including linting, saving a TON of time, the QA queue is always big
  • cannot format comments and insert images and other markup there (could change with new Bugzilla at least)

Advantages of Phorge:

  • ability to achieve smooth and much more painless flow of code patches, faster feedback loop and less pain means everyone is happier
  • ability to review the code much more intuitively, with all the features you could imagine (more advanced than what GitHub et al have), including stuff like inline comments!
  • hopefully ability to submit and amend changes more easily
  • running small unit tests on every change
  • has various optional mini-apps like mini-wiki, mini-blog (imagine stuff like updates to code style could be posted there, with a developer-focused feed, but not replacing the main wiki I imagine)
  • Herald rules can let people set up notifications to watch over changes to certain parts of codebase (quote: If you're interested in keeping track of changes to certain parts of a codebase (e.g., maybe changes to a feature or changes in a certain language, or there's just some intern who you don't trust) you can write a Herald rule to automatically CC you on any revisions which match rules (like content, author, files affected, etc.))
  • audits feature (post-merge) to raise issue with previously committed changes or trigger audit requests based on herald rules, if some important code was changed and we want its owner/manager to review it post-merge and decide whether to do anything or not (basically equivalent of comments under commits on GitHub, but a bit more powerful, of course you could also still just comment on original bugs or Differential probably)
  • aforementioned Herald rules can add blocking reviewers to certain parts of code during review phase too
  • it can apply metadata into merged commits automatically, including the Differential link
  • can mark as "Changes Planned", say what is planned, basically you can nicely show WIP stuff before it's fully ready yet
  • they claim it's fast and snappy, it seems to be true
  • supports extensions
  • probably much more I'll forget about
  • I mean just look at the thing and poke around and be amazed!!!

Useful links:

Example of how Mozilla integration works (worth a good look, since it fits Koha's case very well in terms of Bugzilla usage):

The Differential is linked to the Bug both ways around, the link to Phabricator is made in the form of a virtual attachment in Bugzilla of MIME type `text/x-phabricator-request`, which actually redirects to Phabricator after clicking it. This retains some familiarity of the workflow, and attachments can still be used to attach things like screenshots etc. during the discussion phase, until a patch set is proposed. The feature of Phabricator tasks is unused, since Bugzilla bugs take over their role.

`git bz` would be replaced by Arcanist (arc) command line tool.

From what I understand there's some flexibility in how exactly the workflow is set up, as expected. It can even auto-detect merged commits and then close the Differential etc.

I think the workflow of 

bug -> patchset -> [discussion+follow-up] -> sign-off -> QA -> [QA feedback+follow-up] -> ready to merge -> merge

could be replaced by something like: 

bug -> make Differential with patch and have it linked -> [discussion+follow-up] -> someone approves (this doesn't make it ready yet) -> QA group review is auto-requested -> QA sees it and looks at it -> [QA feedback means making review with change request etc.+follow-up] -> QA approves -> "This revision is now accepted and ready to land." -> gets auto-merged into some branch at some time probably

Basically the same familiar review workflow could be replicated nicely in an intuitive manner.

Further ideas

The opportunity of a change here could be simultaneously used to conduct some other changes like:

1. @ashimema's idea of introducing an intermediate branch between main and the merge queue, where changes could be merged periodically like to main now, tested by Jenkins, and if there's no regressions, main would be fast-forwarded to these commits, making it easier to revert a broken patchset and let its author fix it, before it was set in stone by being written to main (and avoid the time race to fix said regression before next release)

2. I had something more in mind, but forgot

Alternatives considered

Alternative is to do Pull Requests on a Git hosting site like GitHub or Forgejo. The advantage there would be that people would already be much more familiar with such workflow and be able to contribute with minimal learning and no need for installing third-party tools.

Apple has a very funky integration of Bugzilla, GitHub and their internal bug tracking system Radare: https://github.com/WebKit/WebKit/pull/37052 It seems that they automatically mirror the PRs from GitHub as bugs in Bugzilla? Or perhaps the authors manually create a dummy bug in Bugzilla and link it to PR? Doesn't seem like it's quite as polished as the main RFC here...?

I also considered Gerrit, but tbh by comparing the two I'm only amazed by Phabricator and disgusted by Gerrit in comparison, Phabricator seems to be much more intuitive. I feel already at home even though I never really used either, after some digging and poking around. Fun-fact: MediaWiki does the exact opposite, using Phabricator for bug tracking and Gerrit for code review...

Retrieved from "https://wiki.koha-community.org/w/index.php?title=Phabricator_RFC&oldid=35142"