Phabricator/Arcanist feedback

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``)

* 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.

[1] http://llvm.org/docs/Phabricator.html#status
[2] http://llvm.org/docs/Phabricator.html#committing-a-change
[3] ◉ Remarkup Reference

HTH,
Dan

arc.txt (8.83 KB)

Hi Dan, thanks for the feedback.

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 :slight_smile: 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).

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 :wink:

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.

Hi,

Interesting, I don't read it that way :slight_smile: I personally don't use arc to
commit.

Sure the docs don't explicitly state that it's the only way. I just
took that to be implied because the instructions for requesting a
review provide multiple methods but the instructions for committing
only mention using a single method.

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 :wink:

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.

Sure. In order to document the manual alternative I need to know what
Phabricator looks for to work out it that it should close a review. Is
the

Differential Revision: http://reviews.llvm.org/<REVISION>

line in the commit message what it looks for?

Thanks,
Dan.

Hi,

Interesting, I don't read it that way :slight_smile: I personally don't use arc to
commit.

Sure the docs don't explicitly state that it's the only way. I just
took that to be implied because the instructions for requesting a
review provide multiple methods but the instructions for committing
only mention using a single method.

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 :wink:

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.

Sure. In order to document the manual alternative I need to know what
Phabricator looks for to work out it that it should close a review. Is
the

Differential Revision: http://reviews.llvm.org/<REVISION>

line in the commit message what it looks for?

Yes.

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.

Sure. In order to document the manual alternative I need to know what
Phabricator looks for to work out it that it should close a review. Is
the

Differential Revision: http://reviews.llvm.org/<REVISION>

line in the commit message what it looks for?

Yes.

Thanks for the clarification.

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:
◉ Arcanist User Guide: arc 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`).

I'll try to make sure this workflow is described in the patch for the
docs I will make.

* 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?

It has already landed. The commit was created by ``arc patch``. I
cherry-picked the commit it created on to my master branch, cleaned up
the commit message and did ``git svn dcommit``.

Thanks,
Dan.

I've created a patch on Phabricator to improve the documentation on
committing code reviewed on Phabricator

http://reviews.llvm.org/D15801

Hi Dan, thanks for the feedback.

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 :slight_smile: 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 - 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

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?

Hi Dan, thanks for the feedback.

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 :slight_smile: 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...

One thing that bit me a few times is the fact that the Differential Revision: line must be the last (non-empty) line in the commit message. If you add things like “This fixes …” behind it then it won’t get recognized.

  • Matthias

My biggest grievance with Phabricator is that you can't just close a revision manually. It seems that unless you use arcanist, you need to resort to some sort of trickery to get your revisions closed.

-Krzysztof

Huh? Under "Leap into action" on the bottom of the page, there is "Close
revision".

Joerg

Hmm. Indeed. I don't know why I didn't notice it before. Oh well, today I learned...

Thanks!

-Krzysztof

If it’s a revision authored by someone else (even if you are a reviewer), the “Close” doesn’t appear - it’s necessary to first “Comandeer Revision” before the close option shows up. I’ve been confused by that in the past on other projects using Phabricator.

Simon

I think you can close even if you are not the author (I did it multiple times), but the revision has to be “Accepted” first.

r257180.