my Phabricator findings [was: Research Request]

[Laszlo Ersek <lersek@redhat.com>]

Hi,

I'd like to conclude my Phabricator investigation now. Indeed I have
spent a lot less time on it than on GitHub, however my reason for this
earlier finish is not bias or exhaustion. I believe to have found some
pretty critical functionality gaps in Phabricator, which make it
"secondary" how the emails it sends look and such.

(The URLs we used for investigation are:

https://code.bluestop.org/D1
https://code.bluestop.org/D2
https://code.bluestop.org/D3
https://code.bluestop.org/D4
)

Namely: Phabricator doesn't integrate natively with git topic branches,
or with "git" itself, for that matter.

Submitting a series of commits (a topic branch) for review seems to
require a tool called "arcanist" (or "arc" for short), which from my
perspective is a very unwelcome requirement. (Earlier I expressed my
hope that it would be an optional tool.) Rebecca pointed me to the
following article:

  https://smacleod.ca/posts/commit-series-with-phabricator/

and while the method described might technically work, I think it's a
large step back in usability if we need to dance around the system with
an extra tool (which isn't even packaged by several Linux distributions
currently), just for submitting a topic branch for review.

In addition, the way a commit series is represented in a Phabricator
(for the purposes of comprehensive review), AIUI, is a chain of
inter-dependent, but *separate* review requests. While the article above
describes a method for interleaving a local "git rebase -i" procedure
with re-stacking the patches / review requests on the Phabricator
server, it strongly feels like a workaround for a system for which a
*series* of patches is only an afterthought, not the core idea. (In git,
branches are the core idea -- creating them is very cheap, and merging
them is actually possible.)

In other words, even though setting up the local commit messages as
required by Phabricator, and running "arc diff" in a loop basically,
might obviate a manual struggle on the WebUI, for stacking multiple
review requests (one per patch) into a series, I don't see how it could
work in any sensible way with the size of the patch sets that we
regularly post. 10-20 patches in an edk2 series are totally normal, and
50+ patches in a series are not unheard of.

Yes, 50+ patches in a series are not frequent, but long series are
*exactly* when you need your tooling to support you the most. I can't
see myself integrating an external tool with "git rebase" (esp. one that
might call out over the network, such as "arc diff") when I'm trying to
move code hunks between patches in a 20+ part series.

When polishing a large series, it's not uncommon to rebase the whole
thing multiple tens of times.

Yet further, according to Rebecca's explanation in
<https://code.bluestop.org/D1>, fetching a patch under review for local
testing again requires "arc". This is not native to git, and I think it
causes more problems than it solves.

Also, what about fetching a series of patches, i.e. a series of
dependent review requests... The local git command line works *already*
(once we're given a remote URL and a branch or tag), so let's not mess
with that please.

To be honest, I'm stumped how Mozilla could adopt (according to the
article linked at the top) "Phabricator as the primary code review
system for Firefox".

Out of this exercise, I'd like to suggest a hard requirement for all
future candidates: native integration with "git" is a must, and the
system must not require additional command line tools for basic git
operations (such as push, fetch, rebase), and basic participation in
development.

Thanks,
Laszlo