Phabricator -> GitHub PRs?

classic Classic list List threaded Threaded
142 messages Options
1234 ... 8
Reply | Threaded
Open this post in threaded view
|

Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.

-bw

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev


On 1/7/20 6:03 PM, Bill Wendling via llvm-dev wrote:
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.


FYI: We did have a thread on this a couple of months ago: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html

 -Hal



-bw

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
What was the verdict? Any plans to move? I hate coding anything knowing that I'll have to use Phabricator. It's like nails on a chalkboard.

-bw

On Tue, Jan 7, 2020 at 4:13 PM Finkel, Hal J. <[hidden email]> wrote:


On 1/7/20 6:03 PM, Bill Wendling via llvm-dev wrote:
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.


FYI: We did have a thread on this a couple of months ago: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html

 -Hal



-bw

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev


On 1/7/20 6:17 PM, Bill Wendling wrote:
What was the verdict? Any plans to move? I hate coding anything knowing that I'll have to use Phabricator. It's like nails on a chalkboard.


As you might imagine, not everyone agrees with you. My thoughts on how to move forward were here: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136591.html - and I do think that we should move forward. It will take some work, however.

 -Hal



-bw

On Tue, Jan 7, 2020 at 4:13 PM Finkel, Hal J. <[hidden email]> wrote:


On 1/7/20 6:03 PM, Bill Wendling via llvm-dev wrote:
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.


FYI: We did have a thread on this a couple of months ago: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html

 -Hal



-bw

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
Then perhaps those opposed could suggest how to use Phabricator/Arcanist so that I don't throw my keyboard through my monitor?

-bw

On Tue, Jan 7, 2020 at 4:33 PM Finkel, Hal J. <[hidden email]> wrote:


On 1/7/20 6:17 PM, Bill Wendling wrote:
What was the verdict? Any plans to move? I hate coding anything knowing that I'll have to use Phabricator. It's like nails on a chalkboard.


As you might imagine, not everyone agrees with you. My thoughts on how to move forward were here: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136591.html - and I do think that we should move forward. It will take some work, however.

 -Hal



-bw

On Tue, Jan 7, 2020 at 4:13 PM Finkel, Hal J. <[hidden email]> wrote:


On 1/7/20 6:03 PM, Bill Wendling via llvm-dev wrote:
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.


FYI: We did have a thread on this a couple of months ago: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html

 -Hal



-bw

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
Hi Bill,

On 01/07, Bill Wendling via llvm-dev wrote:
> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
> that I don't throw my keyboard through my monitor?

Please explain your problems, w/o the hyperbole, so people can actually do that.

Thanks,
  Johannes

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (235 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
Hi Bill,

On 01/07, Bill Wendling via llvm-dev wrote:
> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
> that I don't throw my keyboard through my monitor?

Please explain your problems, w/o the hyperbole, so people can actually do that.

It's not hyperbole, but fine. How do you use it to keep multiple, related changes in order? The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read.. How do you make it reasonably useful? Why can't I *easily* relate changes to each other? Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?

These are only off the top of my head. There are far more problems I've had with them.

-bw 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On 2020-01-08, Doerfert, Johannes via llvm-dev wrote:
>Hi Bill,
>
>On 01/07, Bill Wendling via llvm-dev wrote:
>> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>> that I don't throw my keyboard through my monitor?
>
>Please explain your problems, w/o the hyperbole, so people can actually do that.

+1

A list of arguments I extracted from previous discussions. I really hope
these points can be improved before we consider the migration.

* No diffs in mail - *super* inconvenient.
   One has to open each pr in browser (or fetch via git etc etc)
   (https://lists.llvm.org/pipermail/llvm-dev/2019-November/136583.html)

4) Phabricator's ability to see what has changed since the previous time you commented seems to be much more reliable than what Github provides.
   (https://lists.llvm.org/pipermail/llvm-dev/2019-November/136589.html)

In contrast, I regularly find github decides not to load comments, or marks them as resolved before they are (and hides them). I do not wish the quality of LLVM’s codebase to be beholden to the choices github makes around which parts of the discussion to show me. And the issues with subscribing to github issues carry over to subscribing to github pull requests.
   (https://lists.llvm.org/pipermail/llvm-dev/2019-November/136685.html)

The lack of a Herald replacement may be quite serious. There is no good
way to subscribe certain topics based on file paths or commit
descriptions.
   (https://lists.llvm.org/pipermail/llvm-dev/2019-November/136608.html)
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
<[hidden email]> wrote:

>
> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>>
>> Hi Bill,
>>
>> On 01/07, Bill Wendling via llvm-dev wrote:
>> > Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>> > that I don't throw my keyboard through my monitor?
>>
>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>>
> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?

You can use parent/child revisions. Phabricator encourages a
patches-based approach with small changes. For me that corresponds to
one commit per code review. When I address code review feedback in a
parent revision I use git's interactive rebase.

> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?

Inline comments are super useful, they can be marked as done and
hidden. I agree that sometimes there's a lot of context when quoting
text, but the format is very simple (similar to e-mail) so it's easy
to trim.

> Why can't I *easily* relate changes to each other?

What issues do you experience with parent/child revisions?

> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?

You can upload patches through
https://reviews.llvm.org/differential/diff/create/. I personally don't
use arcanist even though I found it pretty useful in the past.

>
> These are only off the top of my head. There are far more problems I've had with them.
>
> -bw
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <[hidden email]> wrote:
On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
<[hidden email]> wrote:
>
> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>>
>> Hi Bill,
>>
>> On 01/07, Bill Wendling via llvm-dev wrote:
>> > Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>> > that I don't throw my keyboard through my monitor?
>>
>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>>
> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?

You can use parent/child revisions. Phabricator encourages a
patches-based approach with small changes. For me that corresponds to
one commit per code review. When I address code review feedback in a
parent revision I use git's interactive rebase.

What's your workflow for doing this? My current workflow is:

$ vim ...
$ git commit -a -m f
$ git rebase -i
<move the commit I want to upload to the top of the change list>
$ arc diff --update <update ID> --head <sha1 of the commit>

Of course, that doesn't work if the patch relies upon other patches. However, if I keep the patches in order in git, then arc will upload all of the parent changes to that Phab revision.

> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?

Inline comments are super useful, they can be marked as done and
hidden. I agree that sometimes there's a lot of context when quoting
text, but the format is very simple (similar to e-mail) so it's easy
to trim.

FWIW, the inline comments are fine, but I don't see a way to quote text easily.

> Why can't I *easily* relate changes to each other?

What issues do you experience with parent/child revisions?

It was difficult to find out how to do it. I would expect it to have suggested revisions in the list, but it didn't. It would also be nice to do it through the "arc" tool, but there's no option to do that.
 
> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?

You can upload patches through
https://reviews.llvm.org/differential/diff/create/. I personally don't
use arcanist even though I found it pretty useful in the past.

This is part of my issue. The arcanist tool is meant to be used with Phabricator, especially if you develop in a terminal, but people seem to actively avoid using it. I personally find it far too easy to push the wrong changes to a revision.

-bw

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev


> On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
>
> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> <[hidden email]> wrote:
>>
>> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>>>
>>> Hi Bill,
>>>
>>> On 01/07, Bill Wendling via llvm-dev wrote:
>>>> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>>>> that I don't throw my keyboard through my monitor?
>>>
>>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>>>
>> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
>
> You can use parent/child revisions. Phabricator encourages a
> patches-based approach with small changes. For me that corresponds to
> one commit per code review. When I address code review feedback in a
> parent revision I use git's interactive rebase.

It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.

Also just because it's not easy to find unless you know it's there. You can view the parent/child relationships in the 'Revision Contents' section on the 'Stack' tab.

>> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
>
> Inline comments are super useful, they can be marked as done and
> hidden. I agree that sometimes there's a lot of context when quoting
> text, but the format is very simple (similar to e-mail) so it's easy
> to trim.
>
>> Why can't I *easily* relate changes to each other?
>
> What issues do you experience with parent/child revisions?
>
>> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
>
> You can upload patches through
> https://reviews.llvm.org/differential/diff/create/. I personally don't
> use arcanist even though I found it pretty useful in the past.
>
>>
>> These are only off the top of my head. There are far more problems I've had with them.
>>
>> -bw
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 5:56 PM Bill Wendling <[hidden email]> wrote:

>
> On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <[hidden email]> wrote:
>>
>> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
>> <[hidden email]> wrote:
>> >
>> > On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>> >>
>> >> Hi Bill,
>> >>
>> >> On 01/07, Bill Wendling via llvm-dev wrote:
>> >> > Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>> >> > that I don't throw my keyboard through my monitor?
>> >>
>> >> Please explain your problems, w/o the hyperbole, so people can actually do that.
>> >>
>> > It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
>>
>> You can use parent/child revisions. Phabricator encourages a
>> patches-based approach with small changes. For me that corresponds to
>> one commit per code review. When I address code review feedback in a
>> parent revision I use git's interactive rebase.
>>
> What's your workflow for doing this? My current workflow is:
>
> $ vim ...
> $ git commit -a -m f
> $ git rebase -i
> <move the commit I want to upload to the top of the change list>
> $ arc diff --update <update ID> --head <sha1 of the commit>
>
> Of course, that doesn't work if the patch relies upon other patches. However, if I keep the patches in order in git, then arc will upload all of the parent changes to that Phab revision.

My workflow is similar, although I upload the patches manually through
the web interface. If it's a single patch I use an alias for `show
HEAD -U999999 ` which I then copy/paste in Phabricator. If I'm working
on a say a set of 3 changes and I need to update the oldest one, I use
git rebase -i HEAD~3 on the feature branch, edit the last commit and
use `git show -U999999 <sha>`. Most of the time I don't bother
updating the child revisions unless it really matters. I land the
changes from the commits (by cherry-picking them to master) rather
than the phabricator diffs and then rebase the feature branch.

>> > The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
>>
>> Inline comments are super useful, they can be marked as done and
>> hidden. I agree that sometimes there's a lot of context when quoting
>> text, but the format is very simple (similar to e-mail) so it's easy
>> to trim.
> FWIW, the inline comments are fine, but I don't see a way to quote text easily.

Do you mean quoting text in inline comments? Non-inline comments you
can quote by using the dropdown at the top right of a comment and
selecting "Quote Comment" which just puts the plaintext in the form.
Like I said earlier I usually trim the history to keep things on
point.

>> > Why can't I *easily* relate changes to each other?
>>
>> What issues do you experience with parent/child revisions?
>>
> It was difficult to find out how to do it. I would expect it to have suggested revisions in the list, but it didn't. It would also be nice to do it through the "arc" tool, but there's no option to do that.
>
I'm not sure how many open revisions you have, but the Edit Child
Revision window is pretty easy to use imho. If you upload the patches
one by one, the most recent one is always at the top. I also really
like the open stack view. It's much more explicit than mentioning one
PR in another.
>>
>> > Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
>>
>> You can upload patches through
>> https://reviews.llvm.org/differential/diff/create/. I personally don't
>> use arcanist even though I found it pretty useful in the past.
>
>
> This is part of my issue. The arcanist tool is meant to be used with Phabricator, especially if you develop in a terminal, but people seem to actively avoid using it. I personally find it far too easy to push the wrong changes to a revision.

Like I said I don't use arcanist. I think it works great if you have a
one-off patch and know how to spell the name of your reviewers but
otherwise I prefer the web interface.

> -bw
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
Thanks for the tip.

I didn't know that the decision had already been made to stay with Phabricator. I find Phabricator hard to use and it makes me actively avoid contributing to LLVM knowing that I'll have to use it. I've read the documentation, but still have issues. And it's not like git where there's an obscure command not everyone knows about that will fix the problem. The advice I got so far has been to use Phabricator/Arcanist as little as possible within my workflow. I'll try doing that.

I apologize to anyone I may have offended.

-bw

On Tue, Jan 7, 2020 at 6:26 PM Daniel Sanders <[hidden email]> wrote:


> On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
>
> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> <[hidden email]> wrote:
>>
>> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>>>
>>> Hi Bill,
>>>
>>> On 01/07, Bill Wendling via llvm-dev wrote:
>>>> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>>>> that I don't throw my keyboard through my monitor?
>>>
>>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>>>
>> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
>
> You can use parent/child revisions. Phabricator encourages a
> patches-based approach with small changes. For me that corresponds to
> one commit per code review. When I address code review feedback in a
> parent revision I use git's interactive rebase.

It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.

Also just because it's not easy to find unless you know it's there. You can view the parent/child relationships in the 'Revision Contents' section on the 'Stack' tab.

>> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
>
> Inline comments are super useful, they can be marked as done and
> hidden. I agree that sometimes there's a lot of context when quoting
> text, but the format is very simple (similar to e-mail) so it's easy
> to trim.
>
>> Why can't I *easily* relate changes to each other?
>
> What issues do you experience with parent/child revisions?
>
>> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
>
> You can upload patches through
> https://reviews.llvm.org/differential/diff/create/. I personally don't
> use arcanist even though I found it pretty useful in the past.
>
>>
>> These are only off the top of my head. There are far more problems I've had with them.
>>
>> -bw
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
I'm not sure a decision was already made as such. I think it's more that there was a flurry of conversation last time with lots of conflicting opinions, and then the conversation just fizzled out.

FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we should take the same approach that eventually led to Phabricator being widely adopted: We should allow GitHub PR's and see if the community generally settles on one or the other.

On Jan 7, 2020, at 19:13, Bill Wendling <[hidden email]> wrote:

Thanks for the tip.

I didn't know that the decision had already been made to stay with Phabricator. I find Phabricator hard to use and it makes me actively avoid contributing to LLVM knowing that I'll have to use it. I've read the documentation, but still have issues. And it's not like git where there's an obscure command not everyone knows about that will fix the problem. The advice I got so far has been to use Phabricator/Arcanist as little as possible within my workflow. I'll try doing that.

I apologize to anyone I may have offended.

-bw

On Tue, Jan 7, 2020 at 6:26 PM Daniel Sanders <[hidden email]> wrote:


> On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
>
> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> <[hidden email]> wrote:
>>
>> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>>>
>>> Hi Bill,
>>>
>>> On 01/07, Bill Wendling via llvm-dev wrote:
>>>> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>>>> that I don't throw my keyboard through my monitor?
>>>
>>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>>>
>> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
>
> You can use parent/child revisions. Phabricator encourages a
> patches-based approach with small changes. For me that corresponds to
> one commit per code review. When I address code review feedback in a
> parent revision I use git's interactive rebase.

It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.

Also just because it's not easy to find unless you know it's there. You can view the parent/child relationships in the 'Revision Contents' section on the 'Stack' tab.

>> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
>
> Inline comments are super useful, they can be marked as done and
> hidden. I agree that sometimes there's a lot of context when quoting
> text, but the format is very simple (similar to e-mail) so it's easy
> to trim.
>
>> Why can't I *easily* relate changes to each other?
>
> What issues do you experience with parent/child revisions?
>
>> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
>
> You can upload patches through
> https://reviews.llvm.org/differential/diff/create/. I personally don't
> use arcanist even though I found it pretty useful in the past.
>
>>
>> These are only off the top of my head. There are far more problems I've had with them.
>>
>> -bw
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 7:32 PM Daniel Sanders
<[hidden email]> wrote:
>
> I'm not sure a decision was already made as such. I think it's more that there was a flurry of conversation last time with lots of conflicting opinions, and then the conversation just fizzled out.
>
> FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we should take the same approach that eventually led to Phabricator being widely adopted: We should allow GitHub PR's and see if the community generally settles on one or the other.

+1

>
> On Jan 7, 2020, at 19:13, Bill Wendling <[hidden email]> wrote:
>
> Thanks for the tip.
>
> I didn't know that the decision had already been made to stay with Phabricator. I find Phabricator hard to use and it makes me actively avoid contributing to LLVM knowing that I'll have to use it. I've read the documentation, but still have issues. And it's not like git where there's an obscure command not everyone knows about that will fix the problem. The advice I got so far has been to use Phabricator/Arcanist as little as possible within my workflow. I'll try doing that.
>

I believe that technically sending patches to the mailing list is
still a valid way to get your code reviewed. Not everyone monitors the
mailing list actively though so that might turn out to be more
frustrating than Phabricator.

> I apologize to anyone I may have offended.
>
> -bw
>
> On Tue, Jan 7, 2020 at 6:26 PM Daniel Sanders <[hidden email]> wrote:
>>
>>
>>
>> > On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
>> >
>> > On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
>> > <[hidden email]> wrote:
>> >>
>> >> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
>> >>>
>> >>> Hi Bill,
>> >>>
>> >>> On 01/07, Bill Wendling via llvm-dev wrote:
>> >>>> Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
>> >>>> that I don't throw my keyboard through my monitor?
>> >>>
>> >>> Please explain your problems, w/o the hyperbole, so people can actually do that.
>> >>>
>> >> It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
>> >
>> > You can use parent/child revisions. Phabricator encourages a
>> > patches-based approach with small changes. For me that corresponds to
>> > one commit per code review. When I address code review feedback in a
>> > parent revision I use git's interactive rebase.
>>
>> It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.
>>
>> Also just because it's not easy to find unless you know it's there. You can view the parent/child relationships in the 'Revision Contents' section on the 'Stack' tab.
>>
>> >> The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
>> >
>> > Inline comments are super useful, they can be marked as done and
>> > hidden. I agree that sometimes there's a lot of context when quoting
>> > text, but the format is very simple (similar to e-mail) so it's easy
>> > to trim.
>> >
>> >> Why can't I *easily* relate changes to each other?
>> >
>> > What issues do you experience with parent/child revisions?
>> >
>> >> Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
>> >
>> > You can upload patches through
>> > https://reviews.llvm.org/differential/diff/create/. I personally don't
>> > use arcanist even though I found it pretty useful in the past.
>> >
>> >>
>> >> These are only off the top of my head. There are far more problems I've had with them.
>> >>
>> >> -bw
>> >> _______________________________________________
>> >> cfe-dev mailing list
>> >> [hidden email]
>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020 at 10:36 PM Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
On Tue, Jan 7, 2020 at 7:32 PM Daniel Sanders
<[hidden email]> wrote:
>
> I'm not sure a decision was already made as such. I think it's more that there was a flurry of conversation last time with lots of conflicting opinions, and then the conversation just fizzled out.
>
> FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we should take the same approach that eventually led to Phabricator being widely adopted: We should allow GitHub PR's and see if the community generally settles on one or the other.
This means that people proposing patches control the apparent behaviour. How is someone that is primarily a reviewer meant to voice their opinion under such a system?

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On Tue, Jan 7, 2020, 19:33 Daniel Sanders via llvm-dev <[hidden email]> wrote:
I'm not sure a decision was already made as such. I think it's more that there was a flurry of conversation last time with lots of conflicting opinions, and then the conversation just fizzled out.

FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we should take the same approach that eventually led to Phabricator being widely adopted: We should allow GitHub PR's and see if the community generally settles on one or the other.

Sounds good to me as long as there is a way to contribute that doesn't require a GitHub account, since there are people who can't/won't use GitHub for various reasons.

Jacob

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev


On Jan 7, 2020, at 19:52, Hubert Tong <[hidden email]> wrote:

On Tue, Jan 7, 2020 at 10:36 PM Jonas Devlieghere via llvm-dev <[hidden email]> wrote:
On Tue, Jan 7, 2020 at 7:32 PM Daniel Sanders
<[hidden email]> wrote:
>
> I'm not sure a decision was already made as such. I think it's more that there was a flurry of conversation last time with lots of conflicting opinions, and then the conversation just fizzled out.
>
> FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we should take the same approach that eventually led to Phabricator being widely adopted: We should allow GitHub PR's and see if the community generally settles on one or the other.
This means that people proposing patches control the apparent behaviour. How is someone that is primarily a reviewer meant to voice their opinion under such a system?

When Phabricator was being introduced there were a few groups of reviewers who would ask for Phabricator patches to be re-sent by email and a few who would ask for emails to re-posted on Phabricator. To get my code reviewed I'd go along with whatever the reviewers in the area I wanted to change preferred. Over time, more reviewers asked for Phabricator and fewer asked for email.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On 01/07, Jonas Devlieghere wrote:

> On Tue, Jan 7, 2020 at 5:56 PM Bill Wendling <[hidden email]> wrote:
> >
> > On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <[hidden email]> wrote:
> >>
> >> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> >> <[hidden email]> wrote:
> >> >
> >> > On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <[hidden email]> wrote:
> >> >>
> >> >> Hi Bill,
> >> >>
> >> >> On 01/07, Bill Wendling via llvm-dev wrote:
> >> >> > Then perhaps those opposed could suggest how to use Phabricator/Arcanist so
> >> >> > that I don't throw my keyboard through my monitor?
> >> >>
> >> >> Please explain your problems, w/o the hyperbole, so people can actually do that.
> >> >>
> >> > It's not hyperbole, but fine.  How do you use it to keep multiple, related changes in order?
> >>
> >> You can use parent/child revisions. Phabricator encourages a
> >> patches-based approach with small changes. For me that corresponds to
> >> one commit per code review. When I address code review feedback in a
> >> parent revision I use git's interactive rebase.
> >>
> > What's your workflow for doing this? My current workflow is:
> >
> > $ vim ...
> > $ git commit -a -m f
> > $ git rebase -i
> > <move the commit I want to upload to the top of the change list>
> > $ arc diff --update <update ID> --head <sha1 of the commit>
> >
> > Of course, that doesn't work if the patch relies upon other patches. However, if I keep the patches in order in git, then arc will upload all of the parent changes to that Phab revision.
>
> My workflow is similar, although I upload the patches manually through
> the web interface. If it's a single patch I use an alias for `show
> HEAD -U999999 ` which I then copy/paste in Phabricator. If I'm working
> on a say a set of 3 changes and I need to update the oldest one, I use
> git rebase -i HEAD~3 on the feature branch, edit the last commit and
> use `git show -U999999 <sha>`. Most of the time I don't bother
> updating the child revisions unless it really matters. I land the
> changes from the commits (by cherry-picking them to master) rather
> than the phabricator diffs and then rebase the feature branch.
My workflow is similar but with arcanist. Most of the time I do an
interactive rebase and then `arc diff HEAD~`.

> >> > The interface for reviewing and responding to reviews is horrific, e.g. quoting text from a review is rather bad, the email it sends is badly formatted and hard to read. How do you make it reasonably useful?
> >>
> >> Inline comments are super useful, they can be marked as done and
> >> hidden. I agree that sometimes there's a lot of context when quoting
> >> text, but the format is very simple (similar to e-mail) so it's easy
> >> to trim.
> > FWIW, the inline comments are fine, but I don't see a way to quote text easily.
>
> Do you mean quoting text in inline comments? Non-inline comments you
> can quote by using the dropdown at the top right of a comment and
> selecting "Quote Comment" which just puts the plaintext in the form.
> Like I said earlier I usually trim the history to keep things on
> point.
Inline comments can be "quoted" by copy+paste and putting them after a
`>`


> >> > Why can't I *easily* relate changes to each other?
> >>
> >> What issues do you experience with parent/child revisions?
> >>
> > It was difficult to find out how to do it. I would expect it to have suggested revisions in the list, but it didn't. It would also be nice to do it through the "arc" tool, but there's no option to do that.
> >
> I'm not sure how many open revisions you have, but the Edit Child
> Revision window is pretty easy to use imho. If you upload the patches
> one by one, the most recent one is always at the top. I also really
> like the open stack view. It's much more explicit than mentioning one
> PR in another.
I have quite a few *active* open patches and interactions with patches
of other people in different parts of the compiler. The child/parent
thing needs to be clicked on once per patch manually but it has good
default suggestions and a search that works.

Click on the "Stack" tab on a patch like https://reviews.llvm.org/D72025
to see how multiple (>3) people managed to connect the patches in their
dependence order.


> >> > Why can't I submit through the Phabricator interface, but have to go to the command line, place the change in a new branch, pull to top-of-tree, rebase, and only then push while hoping it doesn't give fail because the tree became out of date? How can I do a rebase through Phabricator?
> >>
> >> You can upload patches through
> >> https://reviews.llvm.org/differential/diff/create/. I personally don't
> >> use arcanist even though I found it pretty useful in the past.
> >
> >
> > This is part of my issue. The arcanist tool is meant to be used with Phabricator, especially if you develop in a terminal, but people seem to actively avoid using it. I personally find it far too easy to push the wrong changes to a revision.
>
> Like I said I don't use arcanist. I think it works great if you have a
> one-off patch and know how to spell the name of your reviewers but
> otherwise I prefer the web interface.
I use arcanist all the time. While it has a lot of flaws it works for me
mostly fine. I like that I don't need to go to the web interface all the
time and while I use the browser to lookup usernames of people, it will
tell me if I misspelled one. Also, I can add reviewers later as well.
Usually I just look at the last commit if I create a new one to see what
reviewers I had there.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (235 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Phabricator -> GitHub PRs?

Valeriy Savchenko via cfe-dev
In reply to this post by Valeriy Savchenko via cfe-dev
On 08/01/2020 00:33, Finkel, Hal J. via cfe-dev wrote:
> As you might imagine, not everyone agrees with you. My thoughts on how
> to move forward were here:
> http://lists.llvm.org/pipermail/llvm-dev/2019-November/136591.html - and
> I do think that we should move forward. It will take some work, however.

I would disagree with your characterisation that Phabricator is a
superior tool:

Phabricator implements a large superset of the features that most users
want.  This extra complexity provides a barrier to entry for a lot of
users (in fact, this morning I had a colleague drop into my office for
help having done something wrong with Phabricator and mangled the diff,
and I was unable to help him).

GitHub PRs implement a subset of the functionality that some people
want.  This causes a problem for people wanting who rely on the other
features, most commonly reviewers.

Phabricator has the same problem as git: the learning curve is steep and
there are always new things to learn.  It also has the same advantage as
git: it probably can do anything that you want it to do, as long as
you're willing to invest the time to learn.

Favouring Phabricator over GitHub PRs is a decision to prioritise ease
of use for some reviewers' workflows over that of patch contributors.
That's fine, if it's a conscious decision (and not even one that I'd
necessarily disagree with: code reviewers are probably the most scarce
resource in the LLVM ecosystem and I'd be willing to lose some potential
contributors if it increased the likelihood of timely and thorough code
review), but it is misleading to claim that one tool is 'superior' in
the abstract, without defining the requirements.

David
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
1234 ... 8