[llvm-dev] Phabricator/Arcanist feedback

[Nathan Wilson via llvm-dev]

On Sun, Dec 27, 2015 at 11:24 PM, Dan Liew via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Hi,
>
> I recently tried reviewing/committing some of my code on
> Phabricator/Arcanist for the first time and I noticed that the docs
> [1] ask for feedback, so here it is!
>
> Phabricator functions reasonably well and it is a lot easier to write
> comments and respond to comments on particular parts of code as
> opposed to the old way of copy and pasting patches into e-mails sent
> to llvm-commits. Two minors annoyances:
>
> * IIRC you have to scroll to the bottom of the page and click a submit
> button to have comments that have just been written accepted. This is
> really annoying and unintuitive, especially if the patch is large
> which means lots and lots and lots of scrolling.
>
> * I also have a personal preference for writing review comments in
> markdown. Phabricator seems to use some other syntax [3] which is not
> compatible. I'm very used to writing reviews on GitHub which uses
> GitHub flavoured markdown so I kept accidentaly typing that into the
> comment boxes.
>
> I came up against a number of issues when it came to actually
> committing code once it had been reviewed.
>
> # TL;DR
>
> I don't like the arcanist tool at all and we should have a documented
> manual way of committing code reviewed on Phabricator.
>
> # Long version
>
> The docs [2] to seem to suggest the **only correct way** (i.e.
> properly closes the phabriacator review) is to commit your change is
> via the arcanist command line tool.
>
> I had/have several problems with the arcanist tool.
>
> * It is written in PHP  (provokes a knee-jerk "kill it with fire"
> reaction in me and likely others). IMHO not the most suitable language
> for writing a command line tool. As the rest of the points below
> demonstrate
>
> * I suspect that most people don't have (and don't want to have) the
> PHP runtime installed just to run a command line tool.
>
> * The tool does not work out of box due PHP's default configuration
> (5.6.16 on Arch Linux). I see warnings like this.
>
> ```
> PHP Warning:  ini_set(): open_basedir restriction in effect. File() is
> not within the allowed path(s):
> (/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) in
> /home/dan/dev/libphutil/scripts/__init_scr
> ipt__.php on line 52
> ```
>
> See the attached ``arc.txt`` for the full output.
>
> I had to remove the ``open_basedir`` setting (making my PHP install
> less secure) in ``php.ini`` to get the tool to work.
>
> IMHO we should have other documented method(s) of committing the code
> (that still correctly closes the review in phabricator) without using
> the arcanist tool. Perhaps the Phabricator web interface should have
> an option to generate the correctly formatted commit message text
> which can then be used when committed manually?
>
> I'm aware phabricator is an external project so we are at the mercy of
> the Phabricator devs but it seems reasonable to me request a command
> line tool not written in PHP, e.g. written in something more sensible
> for a lightweight client to a server application (e.g. Python, Perl,
> Go, ...).
>
> I also have issue with the commands shown in the docs for committing
> the code, i.e.
>
> ```
> arc patch D<Revision>
> arc commit --revision D<Revision>
> ```
>
> This has some implicit assumptions that aren't mentioned.
>
> * ``arc commit`` only works if you're using svn. If you're working
> with LLVM via "git svn" it doesn't work. You can to commit the usual
> way (i.e. ``git svn dcommit``)
>

I think it makes sense that `arc commit` only works with svn since that's
where the repo is hosted and git is just a mirror. Phabricator's
documentation also says that `arc commit` is for subversion, whereas `arc
land` would be for git:
https://secure.phabricator.com/book/phabricator/article/arcanist_diff/

Anyhow, I generally create a local feature branch off of master, make the
local commit, and create the patch via arcanist (arc diff). If the patch
gets approved I'll merge and rebase/squash this back onto master and make
the commit using git svn (`git svn dcommit`).


>
> * Running the ``arc patch`` command assumes you are on a clean master
> branch and want to apply a patch uploaded Phabricator. This is
> pointless (and won't work) if you are already on a local branch that
> already has the commit you want to commit to trunk.
>

To clarify, is the local branch the patch you've already created, been
approved, and is ready to land?

You could just make the commit using `git svn dcommit` though I believe. I
try to commit on master though as noted above, so I could be wrong as I'm
not sure if there would be conflicts.


>
> I'm happy to write patches to improve the documentation here but I
> would like some guidance on what the correct method of committing
> without arcanist is.
>
> [1] http://llvm.org/docs/Phabricator.html#status
> [2] http://llvm.org/docs/Phabricator.html#committing-a-change
> [3] https://secure.phabricator.com/book/phabricator/article/remarkup/
>
> HTH,
> Dan
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151228/a2a21462/attachment-0001.html>