[llvm-dev] Phabricator/Arcanist feedback

Thu Jan 7 10:27:08 PST 2016

On Thu, Jan 7, 2016 at 9:57 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Jan 7, 2016, at 9:45 AM, David Blaikie via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
>
> On Sun, Dec 27, 2015 at 10:59 PM, Manuel Klimek via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi Dan, thanks for the feedback.
>>
>> On Mon, Dec 28, 2015 at 6:24 AM Dan Liew <dan at su-root.co.uk> 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.
>>>
>>
>> Interesting, I don't read it that way :) I personally don't use arc to
>> commit.
>>
>>
>>> 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?
>>>
>>
>> You can just write the commit message the way you would write it if you
>> didn't use phab. There really is no need for arc if you don't like it (I
>> personally use it for everything but the commit).
>>
>
> Trick is (as mentioned later in the thread) to leave the "Differential
> Revision: " tag in there so it'll autoclose the review (if you like
>
>
> This does not work for me, I use "arc diff” to create the patch and git
> svn dcommit, and none of my revision are closing automatically. Is there
> something else that need to be setup?
>

Nothing else that I'm aware of...

>
> —
> Mehdi
>
>
> - you can do this manually, of course). Like you, Manuel, I tend to use
> arc to upload (which rewrites my commit message to include the diff rev tag
> and other bits) then manually git svn commit (after editing the commit
> message down - usually dropping a few of the extra Differential headers,
> removing any reviewers who didn't end up reviewing the patch, etc).
>
>
> - Dave
>
>
>>
>>
>>> 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 personally don't think that would be helpful; it would have much of the
>> same problems, and would introduce a bunch of its own, as it couldn't
>> easily reuse the phabricator code that already exists.
>>
>> 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 heard of people who have made this work with git svn.
>>
>> * 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.
>>>
>>> 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.
>>>
>>
>> Again, there is no one correct way. You can use arc, you can just
>> manually put together your commit message. If your commit messages are bad,
>> somebody will eventually start complaining post-commit ;)
>>
>> Feel free to send a patch to make it explicit that using arc is
>> completely optional. We mainly put it there because some people hand't
>> heard of arc and were handling their patches manually and complaining about
>> the web workflow.
>>
>>
>>>
>>> [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
>>
>>
> _______________________________________________
> 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/20160107/933dcb39/attachment.html>