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 - 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 > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160107/b00a355a/attachment-0001.html>
> 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 <mailto: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 <mailto: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 likeThis 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? — 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 <http://llvm.org/docs/Phabricator.html#status> > [2] http://llvm.org/docs/Phabricator.html#committing-a-change <http://llvm.org/docs/Phabricator.html#committing-a-change> > [3] https://secure.phabricator.com/book/phabricator/article/remarkup/ <https://secure.phabricator.com/book/phabricator/article/remarkup/> > > HTH, > Dan > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <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/2a21a3c8/attachment.html>
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>
Matthias Braun via llvm-dev
2016-Jan-07 19:08 UTC
[llvm-dev] Phabricator/Arcanist feedback
> On Jan 7, 2016, at 9:57 AM, Mehdi Amini via llvm-dev <llvm-dev at lists.llvm.org> wrote: > >> >> On Jan 7, 2016, at 9:45 AM, David Blaikie via llvm-dev <llvm-dev at lists.llvm.org <mailto: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 <mailto: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 <mailto: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?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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160107/9e328e53/attachment.html>