<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 27, 2015 at 11:24 PM, Dan Liew via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<br>
I recently tried reviewing/committing some of my code on<br>
Phabricator/Arcanist for the first time and I noticed that the docs<br>
[1] ask for feedback, so here it is!<br>
<br>
Phabricator functions reasonably well and it is a lot easier to write<br>
comments and respond to comments on particular parts of code as<br>
opposed to the old way of copy and pasting patches into e-mails sent<br>
to llvm-commits. Two minors annoyances:<br>
<br>
* IIRC you have to scroll to the bottom of the page and click a submit<br>
button to have comments that have just been written accepted. This is<br>
really annoying and unintuitive, especially if the patch is large<br>
which means lots and lots and lots of scrolling.<br>
<br>
* I also have a personal preference for writing review comments in<br>
markdown. Phabricator seems to use some other syntax [3] which is not<br>
compatible. I'm very used to writing reviews on GitHub which uses<br>
GitHub flavoured markdown so I kept accidentaly typing that into the<br>
comment boxes.<br>
<br>
I came up against a number of issues when it came to actually<br>
committing code once it had been reviewed.<br>
<br>
# TL;DR<br>
<br>
I don't like the arcanist tool at all and we should have a documented<br>
manual way of committing code reviewed on Phabricator.<br>
<br>
# Long version<br>
<br>
The docs [2] to seem to suggest the **only correct way** (i.e.<br>
properly closes the phabriacator review) is to commit your change is<br>
via the arcanist command line tool.<br>
<br>
I had/have several problems with the arcanist tool.<br>
<br>
* It is written in PHP  (provokes a knee-jerk "kill it with fire"<br>
reaction in me and likely others). IMHO not the most suitable language<br>
for writing a command line tool. As the rest of the points below<br>
demonstrate<br>
<br>
* I suspect that most people don't have (and don't want to have) the<br>
PHP runtime installed just to run a command line tool.<br>
<br>
* The tool does not work out of box due PHP's default configuration<br>
(5.6.16 on Arch Linux). I see warnings like this.<br>
<br>
```<br>
PHP Warning:  ini_set(): open_basedir restriction in effect. File() is<br>
not within the allowed path(s):<br>
(/srv/http/:/home/:/tmp/:/usr/share/pear/:/usr/share/webapps/) in<br>
/home/dan/dev/libphutil/scripts/__init_scr<br>
ipt__.php on line 52<br>
```<br>
<br>
See the attached ``arc.txt`` for the full output.<br>
<br>
I had to remove the ``open_basedir`` setting (making my PHP install<br>
less secure) in ``php.ini`` to get the tool to work.<br>
<br>
IMHO we should have other documented method(s) of committing the code<br>
(that still correctly closes the review in phabricator) without using<br>
the arcanist tool. Perhaps the Phabricator web interface should have<br>
an option to generate the correctly formatted commit message text<br>
which can then be used when committed manually?<br>
<br>
I'm aware phabricator is an external project so we are at the mercy of<br>
the Phabricator devs but it seems reasonable to me request a command<br>
line tool not written in PHP, e.g. written in something more sensible<br>
for a lightweight client to a server application (e.g. Python, Perl,<br>
Go, ...).<br>
<br>
I also have issue with the commands shown in the docs for committing<br>
the code, i.e.<br>
<br>
```<br>
arc patch D<Revision><br>
arc commit --revision D<Revision><br>
```<br>
<br>
This has some implicit assumptions that aren't mentioned.<br>
<br>
* ``arc commit`` only works if you're using svn. If you're working<br>
with LLVM via "git svn" it doesn't work. You can to commit the usual<br>
way (i.e. ``git svn dcommit``)<br></blockquote><div><br></div><div>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: <a href="https://secure.phabricator.com/book/phabricator/article/arcanist_diff/">https://secure.phabricator.com/book/phabricator/article/arcanist_diff/</a></div><div><br></div><div>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`).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
* Running the ``arc patch`` command assumes you are on a clean master<br>
branch and want to apply a patch uploaded Phabricator. This is<br>
pointless (and won't work) if you are already on a local branch that<br>
already has the commit you want to commit to trunk.<br></blockquote><div><br></div><div>To clarify, is the local branch the patch you've already created, been approved, and is ready to land?</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I'm happy to write patches to improve the documentation here but I<br>
would like some guidance on what the correct method of committing<br>
without arcanist is.<br>
<br>
[1] <a href="http://llvm.org/docs/Phabricator.html#status" rel="noreferrer" target="_blank">http://llvm.org/docs/Phabricator.html#status</a><br>
[2] <a href="http://llvm.org/docs/Phabricator.html#committing-a-change" rel="noreferrer" target="_blank">http://llvm.org/docs/Phabricator.html#committing-a-change</a><br>
[3] <a href="https://secure.phabricator.com/book/phabricator/article/remarkup/" rel="noreferrer" target="_blank">https://secure.phabricator.com/book/phabricator/article/remarkup/</a><br>
<br>
HTH,<br>
Dan<br>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br></blockquote></div><br></div></div>