[llvm-dev] Phabricator -> GitHub PRs?
Bill Wendling via llvm-dev
Finkel, Hal J. via llvm-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.
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 llvm...@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory
Bill Wendling via llvm-dev
Finkel, Hal J. via llvm-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.
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
Bill Wendling via llvm-dev
Doerfert, Johannes via llvm-dev
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?
Thanks,
Johannes
Bill Wendling via llvm-dev
Fangrui Song via llvm-dev
+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)
Jonas Devlieghere via llvm-dev
<cfe...@lists.llvm.org> wrote:
>
> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoe...@anl.gov> 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
> cfe...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Bill Wendling via llvm-dev
On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
<cfe...@lists.llvm.org> wrote:
>
> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoe...@anl.gov> 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.
Daniel Sanders via llvm-dev
> On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> <cfe...@lists.llvm.org> wrote:
>>
>> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoe...@anl.gov> 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.
Jonas Devlieghere via llvm-dev
>
> On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <jo...@devlieghere.com> wrote:
>>
>> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
>> <cfe...@lists.llvm.org> wrote:
>> >
>> > On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoe...@anl.gov> 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
Bill Wendling via llvm-dev
Daniel Sanders via llvm-dev
Jonas Devlieghere via llvm-dev
<daniel_l...@apple.com> 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 <isan...@gmail.com> 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.
Hubert Tong via llvm-dev
On Tue, Jan 7, 2020 at 7:32 PM Daniel Sanders
<daniel_l...@apple.com> 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.
Jacob Lifshay via llvm-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.
Daniel Sanders via llvm-dev
Doerfert, Johannes via llvm-dev
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.
`>`
> >> > 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.
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.
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.
David Chisnall via llvm-dev
> 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
Joerg Sonnenberger via llvm-dev
Let me give you (and others) a bit food for thoughts on this topic:
https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/
I think it covers a good chunk of the pros and cons of using GH PRs vs
Phabricator (or alternatives to Phabricator).
Joerg
Nicolai Hähnle via llvm-dev
<llvm...@lists.llvm.org> wrote:
> How do you use [Phabricator] to keep multiple, related changes in order?
It's worth pointing out that GitHub is not able to do this properly,
either. The problem on GitHub's side is that while a pull request can
contain multiple commits, one cannot properly review those commits
individually, and it is not at all possible to approve individual
commits in a pull request. And unlike Phabricator, GitHub does not
allow you to "stack" pull requests.
When I work with a series of changes, I use Phabricator's stack like
several others here mentioned, plus the little trick of:
$ git rebase -i master -x "arc diff @^"
to update the Phabricator reviews.
I then still have to manually edit the rebase script (to remove the
arc diff @^ line for those commits that did not actually change).
However, it does work reasonably well.
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
Konrad Kleine via llvm-dev
as a constructive addition to this topic I picked one thing from the long list of requests that seemed to pop up a lot and contacted the Github Support about this:
Diffs in emails.
To be fair, I'm not used to reading diffs in emails a lot but I hear that a lot of people here and also inside of Red Hat are doing so and always look for that feature in Github, Gerrit, Patchworks, or whatever tool they're using.
I hope nobody feels offended by this step to contact Github support but what else to do than to forward a request from the community to the one's that can implement it? Let's try to remain constructive.
Cheers
Konrad
Renato Golin via llvm-dev
<llvm...@lists.llvm.org> wrote:
> It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.
Side note: I find that there's too many unknown features of Phab that
require manual annotations on the commit message to work.
I don't think that's a good strategy. New contributors get lost in the
specifics and reviewers forget to do them.
> 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 parental relationship in Phab is not obvious. I can't easily see
it in the snapshot and often ask people to link the commits in order
when they already are.
I'd also like to see the whole series without having to navigate the
linked list.
Git allows multiple commits per pull request, so does GitHub's PR UI,
as well as showing all the other changes (force push, rebase, reorder,
additional fixups), which makes it much simpler to see what changed.
Phab is better at keeping track of old comments, but where GitHub
completely looses the comment (on history change), Phab moves the
comment to a random place in the file, which is equally broken.
Granted, GitHub's UI is much "simpler" than Phab, but to my view, this
is not a problem, but a benefit.
If we moved to GitHub PRs today, I wouldn't miss a thing.
cheers,
--renato
Renato Golin via llvm-dev
<llvm...@lists.llvm.org> wrote:
> It's worth pointing out that GitHub is not able to do this properly,
> either. The problem on GitHub's side is that while a pull request can
> contain multiple commits, one cannot properly review those commits
> individually, and it is not at all possible to approve individual
> commits in a pull request. And unlike Phabricator, GitHub does not
> allow you to "stack" pull requests.
That is true, but the stacking in Phab is less than obvious, and I
always ask people to add "[N/M]" in the series' titles anyway.
Honestly, the fact that I have to either user Arcanist or edit the
comments metadata by hand or use the UI to do a simple task is asking
a bit too much.
We rarely approve some patches and not others in a series, and when we
do, we ask people to create a new series without the approved patch,
or split them, so that we can continue reviewing the series.
Once the approved patches are committed, the series has already
changed and the representation in Phab is not always true anymore.
I think we need to move from "how apt is GitHub's PR UI in emulating
what Phab does" to "what do we really need from a review UI and how
simple it is the process for both contributors and reviewers".
My honest opinion, after using Phab for too many years, is that it is
too much hassle to both sides. Git (and GitHub) PR has the benefit
that most developers out there use it already, including LLVM
developers contributing to other projects.
The name of the arcane tool we have to use to automate our cul-de-sac
review technology is just the cherry on the cake. :)
--renato
Hubert Tong via llvm-dev
On Wed, 8 Jan 2020 at 02:26, Daniel Sanders via llvm-dev
<llvm...@lists.llvm.org> wrote:
> It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.
Side note: I find that there's too many unknown features of Phab that
require manual annotations on the commit message to work.
I don't think that's a good strategy. New contributors get lost in the
specifics and reviewers forget to do them.
> 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 parental relationship in Phab is not obvious. I can't easily see
it in the snapshot and often ask people to link the commits in order
when they already are.
I'd also like to see the whole series without having to navigate the
linked list.
Git allows multiple commits per pull request, so does GitHub's PR UI,
as well as showing all the other changes (force push, rebase, reorder,
additional fixups), which makes it much simpler to see what changed.
Phab is better at keeping track of old comments, but where GitHub
completely looses the comment (on history change),
Phab moves the
comment to a random place in the file, which is equally broken.
Granted, GitHub's UI is much "simpler" than Phab, but to my view, this
is not a problem, but a benefit.
David Greene via llvm-dev
> Granted, GitHub's UI is much "simpler" than Phab, but to my view, this
> is not a problem, but a benefit.
>
> If we moved to GitHub PRs today, I wouldn't miss a thing.
+1.
I still find Phab to be inscrutable. I don't use any of its advanced
features. I'm a long-time contributor. I can't imagine how difficult
it is for folks new to the project.
For all of GitHub's many flaws, its very strong advantage is that it is
a de facto standard. People understand it.
-David
David Greene via llvm-dev
> Honestly, the fact that I have to either user Arcanist or edit the
> comments metadata by hand or use the UI to do a simple task is asking
> a bit too much.
This brings something else to mind for me. Another advantage of GitHub
is that there are many tools out there that work with it. Not so much
for Phab. As an Emacs user, getting access to magit/forge's ability to
interact with GitHub PRs would be amazing.
> I think we need to move from "how apt is GitHub's PR UI in emulating
> what Phab does" to "what do we really need from a review UI and how
> simple it is the process for both contributors and reviewers".
Absolutely. Reviewing a patch series has always seemed a bit counter to
LLVM's development philosophy of small, incremental changes. I
understand the need to provide context for large changes (I have posted
mega-patches in Phab for that purpose), but the path to get to the large
change has always been small, incremental changes.
We have used one-commit-per-PR in our downstream for quite some time now
and it works well.
-David
Aaron Ballman via llvm-dev
<cfe...@lists.llvm.org> wrote:
>
> On Tue, Jan 14, 2020 at 11:32 AM Renato Golin via llvm-dev <llvm...@lists.llvm.org> wrote:
>>
>> On Wed, 8 Jan 2020 at 02:26, Daniel Sanders via llvm-dev
>> <llvm...@lists.llvm.org> wrote:
>> > It's worth mentioning that Phabricator can read strings of the format 'Depends on D1234' from commit messages and create those relationships for you.
>>
>> Side note: I find that there's too many unknown features of Phab that
>> require manual annotations on the commit message to work.
>>
>> I don't think that's a good strategy. New contributors get lost in the
>> specifics and reviewers forget to do them.
>>
>> > 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 parental relationship in Phab is not obvious. I can't easily see
>> it in the snapshot and often ask people to link the commits in order
>> when they already are.
>>
>> I'd also like to see the whole series without having to navigate the
>> linked list.
>>
>> Git allows multiple commits per pull request, so does GitHub's PR UI,
>> as well as showing all the other changes (force push, rebase, reorder,
>> additional fixups), which makes it much simpler to see what changed.
>
> Except the GitHub individual commit views are rather terrible for adding comments to. The individual commits are not treated as separable commits for approval purposes, and GitHub decides on its own that freshly added comments are outdated due to surrounding noise.
+1
Having used both Phab and GitHub for doing a lot of code reviews, I
*strongly* prefer the Phab workflow and Ux. It's not that Phab doesn't
have its issues, it's that the issues with GitHub are so much worse in
my experience.
~Aaron
>
>>
>>
>> Phab is better at keeping track of old comments, but where GitHub
>> completely looses the comment (on history change),
>
> That aspect of GitHub makes longer running reviews quickly unusable. It pretty much only works if all reviewers ensure that the PR originator addresses all comments before they "scroll off".
>
>>
>> Phab moves the
>> comment to a random place in the file, which is equally broken.
>
> Much less broken IMO.
>
>>
>>
>> Granted, GitHub's UI is much "simpler" than Phab, but to my view, this
>> is not a problem, but a benefit.
>
> GitHub's UI is a screen real-estate hog due to low information density even when collapsing things that you rather should see.
>
>>
>>
>> If we moved to GitHub PRs today, I wouldn't miss a thing.
>>
>> cheers,
>> --renato
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm...@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> cfe-dev mailing list
> cfe...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Mehdi AMINI via llvm-dev
Renato Golin via llvm-dev <llvm...@lists.llvm.org> writes:
> Honestly, the fact that I have to either user Arcanist or edit the
> comments metadata by hand or use the UI to do a simple task is asking
> a bit too much.
This brings something else to mind for me. Another advantage of GitHub
is that there are many tools out there that work with it. Not so much
for Phab. As an Emacs user, getting access to magit/forge's ability to
interact with GitHub PRs would be amazing.
> I think we need to move from "how apt is GitHub's PR UI in emulating
> what Phab does" to "what do we really need from a review UI and how
> simple it is the process for both contributors and reviewers".
Absolutely. Reviewing a patch series has always seemed a bit counter to
LLVM's development philosophy of small, incremental changes. I
understand the need to provide context for large changes (I have posted
mega-patches in Phab for that purpose), but the path to get to the large
change has always been small, incremental changes.
Renato Golin via llvm-dev
<hubert.rein...@gmail.com> wrote:
> Except the GitHub individual commit views are rather terrible for adding comments to. The individual commits are not treated as separable commits for approval purposes, and GitHub decides on its own that freshly added comments are outdated due to surrounding noise.
True, but that's not a bad thing. As I said earlier, I don't want to
approve some commits and not others without creating a whole new
series. I think that would be similarly clumsy on either.
> That aspect of GitHub makes longer running reviews quickly unusable. It pretty much only works if all reviewers ensure that the PR originator addresses all comments before they "scroll off".
Agree.
> GitHub's UI is a screen real-estate hog due to low information density even when collapsing things that you rather should see.
With Phab I spent most of the time scrolling to find the comment box,
or the link to reopen the history because search on the comments
doesn't work.
But I don't want to drag down the details of each flaw in each tool.
What we need to do is to weigh in the major pros and cons.
GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.
Phab has a very complex UI with a lot of features that people have
come to rely over the years. But it's far too complex for new people
and requires specially crafted tooling to work with.
Regardless of the shortcomings of both, I think those points speak
strongly for using GitHub PRs. We can adapt to a new simplified
process, and even build tools around, that will be used by other
projects. Win Win.
Joerg Sonnenberger via llvm-dev
> GitHub PR is the "de facto standard", everyone knows, the entry cost
> is practically zero. The UI is lean and missing features, but the
> large availability of tooling (either targeting GitHub directly or
> plain git) makes up for a lot of it.
Just like with the "Everyone knows git", I call bullshit on this. Sorry,
but the far majority of GitHub users know little more than the most barebone
functionality of Pull Requests and the typical use case in most projects
is to squash all changes. But at this point it seems clear that the real
goal is to just move everything to GitHub just for the sake of it.
> Phab has a very complex UI with a lot of features that people have
> come to rely over the years. But it's far too complex for new people
> and requires specially crafted tooling to work with.
Can you at least try to make reasonable arguments without ridiculous
hyperbole? Using phabricator for the casual user requires no special
tooling. Patches can be easily submitted via patch upload. There are
better ways to integrate it into a workflow, arcanist and git-phab just
to mention two.
Joerg
Fangrui Song via llvm-dev
>On Tue, Jan 14, 2020 at 09:56:53PM +0000, Renato Golin via cfe-dev wrote:
>> GitHub PR is the "de facto standard", everyone knows, the entry cost
>> is practically zero. The UI is lean and missing features, but the
>> large availability of tooling (either targeting GitHub directly or
>> plain git) makes up for a lot of it.
>
>Just like with the "Everyone knows git", I call bullshit on this. Sorry,
>but the far majority of GitHub users know little more than the most barebone
>functionality of Pull Requests and the typical use case in most projects
>is to squash all changes. But at this point it seems clear that the real
>goal is to just move everything to GitHub just for the sake of it.
Agree with Joerg here. And thanks for the recommendation https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/ It was a great reading!
We should find justifying reasons to migrate (it has a significant cost
for people used to the current system), not just for the sake of using
GitHub for everything.
Konrad Kleine's action is appreciated (contacted the Github Support
about a deficiency).
Emilio Cobos Álvarez via llvm-dev
> 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
> reasonably useful? Why can't I *easily* relate changes to each other?
> 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?
thing was a huge annoyance.
Nowadays we have tools to manage the stacks for us [1][2], and that as a
plus don't require arc/php to be installed on your system.
I don't do much LLVM / clang stuff, but submitting stuff with [1] just
works (with `moz-phab HEAD~N --no-bug`), and it should submit your last
N commits separately automatically, without having to submit them one-by
one and linking them via the web interface / annotate stuff in the
commit message.
Sorry if this is just noise, though maybe it helps.
Cheers,
-- Emilio
[1]: https://github.com/mozilla-conduit/review
[2]: https://github.com/mystor/phlay
Doerfert, Johannes via llvm-dev
> On 1/8/20 2:15 AM, Bill Wendling via cfe-dev wrote:
> > 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?
> FWIW, Mozilla moved to Phabricator not long ago and the revision stack thing
> was a huge annoyance.
>
> Nowadays we have tools to manage the stacks for us [1][2], and that as a
> plus don't require arc/php to be installed on your system.
>
> I don't do much LLVM / clang stuff, but submitting stuff with [1] just works
> (with `moz-phab HEAD~N --no-bug`), and it should submit your last N commits
> separately automatically, without having to submit them one-by one and
> linking them via the web interface / annotate stuff in the commit message.
>
> Sorry if this is just noise, though maybe it helps.
--
Johannes Doerfert
Researcher
Argonne National Laboratory
Lemont, IL 60439, USA
jdoe...@anl.gov
Doerfert, Johannes via llvm-dev
> Renato Golin via llvm-dev <llvm...@lists.llvm.org> writes:
>
> > Granted, GitHub's UI is much "simpler" than Phab, but to my view, this
> > is not a problem, but a benefit.
> >
> > If we moved to GitHub PRs today, I wouldn't miss a thing.
>
> +1.
>
> I still find Phab to be inscrutable. I don't use any of its advanced
> features. I'm a long-time contributor.
actual problems do you have with Phab? There might be usable solutions
out there already. The last time someone actually listed problems we got
a lot of good responses, some of which I will try out myself.
> I can't imagine how difficult it is for folks new to the project.
concrete problem descriptions though.
> For all of GitHub's many flaws, its very strong advantage is that it is
> a de facto standard. People understand it.
facto standard" is a weird argument for anything. People are advocating
to move away from mailing lists towards other system though mailing
lists are, or at least were, "de facto standard". Is the idea to keep up
with the "de facto standard" or to improve the status quo (for group X*)?
* Substitute X as you see fit.
Cheers,
Johannes
Renato Golin via llvm-dev
> > I still find Phab to be inscrutable. I don't use any of its advanced
> > features. I'm a long-time contributor.
>
> I asked a similar question in this thread in the very beginning: What
> actual problems do you have with Phab? There might be usable solutions
> out there already. The last time someone actually listed problems we got
> a lot of good responses, some of which I will try out myself.
This thread has fallen down to the following pattern:
1. I tell you what I don't like / can't stand
2. You tell me that's not a problem for you and why
3. You ask me to counter your argument
This is not a helpful way to conduct a fact checking exercise.
I respect the opinion of both sides, and I know some people have
gotten to like Phab and others to hate.
I ask that people refrain from attacking others for not engaging in
tit-for-tat "my fact is better than yours" discussion.
Phab is good for some things, Github is good for others. People are
allowed to like either.
> I am always in favor of improving the documentation. We need more
> concrete problem descriptions though.
More documentation or tooling won't fix the fact that much more people
know about GitHub PR than Phab.
It's the same reason why we moved to monorepo GitHub, because everyone
had their own tooling to handle multi-repo Git-SVN hybrid.
If we change the process to be more in tune with GitHub, then their PR
system will (obviously) be far more suitable.
What I'm asking is that we review both together. Current process with
Phab versus a GitHub process with GitHub PR.
> > For all of GitHub's many flaws, its very strong advantage is that it is
> > a de facto standard. People understand it.
>
> I do not. Arguably because I have not yet used it.
He said "most people". He is right, even if you don't, personally. Git
PR (GitHub, GitLab, Gerrit) is indeed the de facto standard.
> However, "it is a de facto standard" is a weird argument for anything. People are advocating
> to move away from mailing lists towards other system though mailing
> lists are, or at least were, "de facto standard". Is the idea to keep up
> with the "de facto standard" or to improve the status quo (for group X*)?
That's the very definition of "de facto". The vast majority of people
use Git, and of those, GitHub/GitLab, and of those, Git PRs.
Phab is niche compared to GitHub. It doesn't make it worse, but that is a fact.
Cranmer, Joshua via llvm-dev
> > GitHub PR is the "de facto standard", everyone knows, the entry cost
> > is practically zero. The UI is lean and missing features, but the
> > large availability of tooling (either targeting GitHub directly or
> > plain git) makes up for a lot of it.
>
> Just like with the "Everyone knows git", I call bullshit on this. Sorry, but the
> far majority of GitHub users know little more than the most barebone
> functionality of Pull Requests and the typical use case in most projects is to
> squash all changes. But at this point it seems clear that the real goal is to just
> move everything to GitHub just for the sake of it.
Is the Phabricator process any better in this regard, though? I can't say for certain. For most of the history of open source projects, contribution has generally progressed by means of the "get a diff of your change and email/upload it somewhere" (and note that Phabricator is a variant on the "upload it somewhere" portion of the spectrum). Contribution in this manner is still the norm for several large, complex open-source projects, such as Linux, Firefox, GCC, or OpenJDK. Indeed, looking at our peer projects (those that are of similar complexity and scale, or also tackle compiler-related problems), contribution via a pull request model appears to be accepted by only a small few.
My experience of using GitHub PRs in the past is that different projects have different expectations for how contributors should update their changes during the review process. This makes me skeptical that even moving to GitHub PRs would meaningfully reduce the friction for new contributors, instead changing what and where the friction lands. So, to me, it seems hard to justify moving to GitHub PRs.
David Greene via llvm-dev
>> GitHub PR is the "de facto standard", everyone knows, the entry cost
>> is practically zero. The UI is lean and missing features, but the
>> large availability of tooling (either targeting GitHub directly or
>> plain git) makes up for a lot of it.
>
> Just like with the "Everyone knows git", I call bullshit on this. Sorry,
> but the far majority of GitHub users know little more than the most barebone
> functionality of Pull Requests and the typical use case in most projects
> is to squash all changes. But at this point it seems clear that the real
> goal is to just move everything to GitHub just for the sake of it.
Let's try to assume good faith on the part of all parties.
>> Phab has a very complex UI with a lot of features that people have
>> come to rely over the years. But it's far too complex for new people
>> and requires specially crafted tooling to work with.
>
> Can you at least try to make reasonable arguments without ridiculous
> hyperbole? Using phabricator for the casual user requires no special
> tooling. Patches can be easily submitted via patch upload. There are
> better ways to integrate it into a workflow, arcanist and git-phab just
> to mention two.
I for one cannot get arcanist to work because of its reliance on PHP
which for whatever reason refuses to install on my machine. It it's
hard for me to do a simple install of a tool for code review, I can't
imagine the frustration of new people trying to get everything working.
I wasn't aware of git-phab and don't recall seeing it mentioned in the
LLVM documentation. I just checked and there is no mention of it. I'm
not sure how new people are supposed to know about it. It does look
nice from a cursory review of its front page. I'll give it a try!
I have never used Phab's more complex features. There's nothing in the
LLVM documentation that talks about patch series or patch parents or
anything more advanced than "submit a patch and pick reviewers." For
many people that is enough. But if I were to run into a situation where
one of the reviewers asked to link my change to some other change, o
present things as a patch series I would have no idea how to do that.
If I were new to the project I would just give up at that point.
There's this bit in the documentation:
Phabricator has many useful features, for example allowing you to
select diffs between different versions of the patch as it was
reviewed in the Revision Update History. Most features are self
descriptive - explore, and if you have a question, drop by on #llvm in
IRC to get help.
As has been noted in other threads, new users find IRC intimidating. I
haven't used IRC since I don't know when. I'm not even sure I *can*
given company policy. It's another hurdle to getting things done. And
important features are not "self descriptive" (see below).
The Phab instance has very little documentation and what there is is
difficult to find (I have to go to "More Applications" and click
"Guides" under "Utilities" -- really, does this organization make any
sense at all?). And then the guides seem limited to configuration, not
actual use-cases.
"More Applications" looks very intimidating with nonsensical names for
things I might want to do ("Harbormaster?" "Drydock?").
GitHub uses Markdown, which is another de facto standard. Phab uses
"Remarkup" which I guess is similar but different.
The front-page of Phab makes little sense to a new user. What is
"Differential?" What is "Diffusion?" Do they both have something to do
with diffs? Oh wait, "Diffusion" brings me to a list of "Active
Repositories." What?
Ok, let's try "Differential." Looks promising. There's a list of "LLVM
Needs Review." Looks like a code review tool. But I just want to
create a patch and submit it. Grumble, grumble, look back at the LLVM
documentation. Oh, it's the tiny inconspicuous button on the very top
right corner "+ Create Diff." The documentation told me about the
button but not where it's located. Took me some time to find it.
The above journey to get a patch submitted was my actual first
experience with Phab. It did not leave a good taste in my mouth.
-David
David Greene via llvm-dev
> Agree with Joerg here. And thanks for the recommendation
> https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/
> It was a great reading!
It's a good read (I did not get through all of it yet) but I lost of bit
of interest when he talks about reviewing pull requests with multiple
commits. Some of his arguments come down to, "people don't submit clean
histories for merging" which is going to be a problem for any review
tool. It isn't limited to GitHub.
I do agree that pull requests should group review comments by commit. I
certainly don't claim that GitHub is perfect.
I can't really speak to the scalability arguments against the pull
request model other than to say that we haven't had any issues
downstream with our repositories.
> We should find justifying reasons to migrate (it has a significant
> cost for people used to the current system), not just for the sake of
> using GitHub for everything.
People have posted multiple reasons so it's not fair to say that people
are pushing the move just for the sake of using GitHub. It is true that
current users of Phab would have to change workflows. That's the nature
of change. If the benefit is moving to something more universally
understood that is much easier and welcoming for new contributors, I am
all for it.
-David
David Greene via llvm-dev
>> I still find Phab to be inscrutable. I don't use any of its advanced
>> features. I'm a long-time contributor.
>
> I asked a similar question in this thread in the very beginning: What
> actual problems do you have with Phab? There might be usable solutions
> out there already. The last time someone actually listed problems we got
> a lot of good responses, some of which I will try out myself.
I just posted a few I've run into over the years.
>> I can't imagine how difficult it is for folks new to the project.
>
> I am always in favor of improving the documentation. We need more
> concrete problem descriptions though.
Hopefully my post will help.
>> For all of GitHub's many flaws, its very strong advantage is that it is
>> a de facto standard. People understand it.
>
> I do not. Arguably because I have not yet used it. However, "it is a de
> facto standard" is a weird argument for anything. People are advocating
> to move away from mailing lists towards other system though mailing
> lists are, or at least were, "de facto standard". Is the idea to keep up
> with the "de facto standard" or to improve the status quo (for group X*)?
>
> * Substitute X as you see fit.
I guess I see the goal being integrating new contributors as easy as
possible. Now I know that many people argue that keeping existing
contributors happy is more important and I can appreciate that
viewpoint. It is a difficult balance to strike. I am trying to think
about solutions that lower the barrier to new contributors while letting
existing contributors still get work done, though with workflow changes.
If moving away from mailing lists makes it significantly easier for new
contributors then I will very seriously consider that. I like e-mail
for a lot of reasons and would be sad to see it go but I'm willing to
make that sacrifice if needed to keep the project strong.
I feel like we are also discounting the possibility that GitHub can
improve. We have evidence that they will respond to requests for
features.
IME every project eventually dies because it is no longer useful or
because it becomes too difficult to attract/integrate new contributors.
gcc almost died in the '90's and it was only because the issue was
forced via a fork that the project finally opened up to new
contributors. I have seen projects in the private sector die because it
was too much work to teach new employees about them. So to me,
attracting new contributors to the LLVM project is vitally important.
Doerfert, Johannes via llvm-dev
I really did try to be constructive in these discussions. If my email
was conceived otherwise I'm sorry about that.
On 01/15, Renato Golin wrote:
> On Wed, 15 Jan 2020 at 10:47, Doerfert, Johannes <jdoe...@anl.gov> wrote:
> > > I still find Phab to be inscrutable. I don't use any of its advanced
> > > features. I'm a long-time contributor.
> >
> > I asked a similar question in this thread in the very beginning: What
> > actual problems do you have with Phab? There might be usable solutions
> > out there already. The last time someone actually listed problems we got
> > a lot of good responses, some of which I will try out myself.
>
> This thread has fallen down to the following pattern:
>
> 1. I tell you what I don't like / can't stand
> 2. You tell me that's not a problem for you and why
> 3. You ask me to counter your argument
People did offer solutions (tips, workflows, scripts, ...) as a response
to actual problems (on both sides of the argument).
> This is not a helpful way to conduct a fact checking exercise.
>
> I respect the opinion of both sides, and I know some people have
> gotten to like Phab and others to hate.
>
> I ask that people refrain from attacking others for not engaging in
> tit-for-tat "my fact is better than yours" discussion.
>
> Phab is good for some things, Github is good for others. People are
> allowed to like either.
forcing others to worsen theirs. Phab is, by many accounts, more
feature-rich, thus we might be able to actually work around the problems
people have reasonably if these are articulated.
I agree that there was communication of the kind "Phab is really bad, GH
are better because XXXX" with responses listing flaws in GH or saying
XXXX is not a problem. As I mentioned above, asking people to list
problems does actually result in solutions being offered, why is this
not good?
> > I am always in favor of improving the documentation. We need more
> > concrete problem descriptions though.
>
> More documentation or tooling won't fix the fact that much more people
> know about GitHub PR than Phab.
>
> It's the same reason why we moved to monorepo GitHub, because everyone
> had their own tooling to handle multi-repo Git-SVN hybrid.
>
> If we change the process to be more in tune with GitHub, then their PR
> system will (obviously) be far more suitable.
"maybe" we should make that the discussion. So far, we are mainly
discussion replacing Phab with PRs and keeping the system otherwise
intact.
> What I'm asking is that we review both together. Current process with
> Phab versus a GitHub process with GitHub PR.
Why not start with doing that.
> > > For all of GitHub's many flaws, its very strong advantage is that it is
> > > a de facto standard. People understand it.
> >
> > I do not. Arguably because I have not yet used it.
>
> He said "most people". He is right, even if you don't, personally. Git
> PR (GitHub, GitLab, Gerrit) is indeed the de facto standard.
I try to determine what they mean for us.
> > However, "it is a de facto standard" is a weird argument for anything. People are advocating
> > to move away from mailing lists towards other system though mailing
> > lists are, or at least were, "de facto standard". Is the idea to keep up
> > with the "de facto standard" or to improve the status quo (for group X*)?
>
> That's the very definition of "de facto". The vast majority of people
> use Git, and of those, GitHub/GitLab, and of those, Git PRs.
>
> Phab is niche compared to GitHub. It doesn't make it worse, but that is a fact.
the de facto standard for the sake of it, to improve the situation for a
certain group, or (implicitly) if the move would be generally speaking good.
Cheers,
Johannes
David Greene via llvm-dev
>> Nowadays we have tools to manage the stacks for us [1][2], and that as a
>> plus don't require arc/php to be installed on your system.
>>
>> I don't do much LLVM / clang stuff, but submitting stuff with [1] just works
>> (with `moz-phab HEAD~N --no-bug`), and it should submit your last N commits
>> separately automatically, without having to submit them one-by one and
>> linking them via the web interface / annotate stuff in the commit message.
>>
>> Sorry if this is just noise, though maybe it helps.
>
> This looks pretty cool, thanks! I'll for sure give it a try!
Agreed, this is interesting as is git-phab.
I would like to take a step back and talk about existing use-cases in
Phab. People have talked a lot about linking revisions, patch series,
parent/child relationships and so on and I have to confess I am
struggling to understand the use-cases for these things. Most of what I
do upstream is simple enough to accomplish with individual patch
submissions and reviews. In some cases I have posted "mega-patches" for
context purposes but I'll admit that isn't a very good workflow as
things quickly get out of date.
I would like to understand better how people use Phab's advanced
features and why. For example, what drives the need for patch series
and dependence relationships? Some walk-through examples would be very
helpful.
-David
Renato Golin via llvm-dev
> I'd say that helping people to improve their environment is better than
> forcing others to worsen theirs.
Note the difference: One side is trying to *help improve", while the
other is *forcing to worsen*.
This is really not helpful.
--renato
Doerfert, Johannes via llvm-dev
> > I'd say that helping people to improve their environment is better than
> > forcing others to worsen theirs.
>
> Note the difference: One side is trying to *help improve", while the
> other is *forcing to worsen*.
differently than I indented it to be read. In the part you cropped I
mention that *both sides* provide *helpful advice* to improve the setup
of people. I argue the problems need to be clarified for that. *Forcing
to worsen* happens when we say we *move* but also when we *do not move*.
Emails that just say we should move or not are therefore problematic.
Cheers,
Johannes
Emilio Cobos Álvarez via llvm-dev
On 1/15/20 6:50 PM, David Greene wrote:
> "Doerfert, Johannes via llvm-dev" <llvm...@lists.llvm.org> writes:
>
>>> Nowadays we have tools to manage the stacks for us [1][2], and that as a
>>> plus don't require arc/php to be installed on your system.
>>>
>>> I don't do much LLVM / clang stuff, but submitting stuff with [1] just works
>>> (with `moz-phab HEAD~N --no-bug`), and it should submit your last N commits
>>> separately automatically, without having to submit them one-by one and
>>> linking them via the web interface / annotate stuff in the commit message.
>>>
>>> Sorry if this is just noise, though maybe it helps.
>>
>> This looks pretty cool, thanks! I'll for sure give it a try!
>
> Agreed, this is interesting as is git-phab.
>
> I would like to take a step back and talk about existing use-cases in
> Phab. People have talked a lot about linking revisions, patch series,
> parent/child relationships and so on and I have to confess I am
> struggling to understand the use-cases for these things. Most of what I
> do upstream is simple enough to accomplish with individual patch
> submissions and reviews. In some cases I have posted "mega-patches" for
> context purposes but I'll admit that isn't a very good workflow as
> things quickly get out of date.
>
> I would like to understand better how people use Phab's advanced
> features and why. For example, what drives the need for patch series
> and dependence relationships? Some walk-through examples would be very
> helpful.
At least at Mozilla, it's good practice to split a given patch in
logical, reviewable pieces that are associated to the same bug, and that
the same or different people may need to review.
That generally makes the patches get reviewed much sooner. During
review, some of the initial parts may be good to land, while some others
may need changes.
[1] or [2] are recentish examples that come to mind, but it happens
fairly often. Of course for a bunch of simpler changes one revision is
enough.
The use cases are similar to the "I have one PR with multiple commits"
in GitHub, but with the advantage of being able to review them
individually, and thus they can land upstream sooner.
-- Emilio
Nicolai Hähnle via llvm-dev
> We rarely approve some patches and not others in a series, and when we
> do, we ask people to create a new series without the approved patch,
> or split them, so that we can continue reviewing the series.
This has simply not been true in my experience. Actually, not having
to re-send a new series is one of the main advantages that
Phabricator-based review has over the original review style for Git,
which is to send patch series via mailing lists.
It might be the case that you occasionally have series where major
redesigns are required, and then asking for a fresh start makes sense.
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
Nicolai Hähnle via llvm-dev
On Wed, Jan 15, 2020 at 12:51 PM David Greene via llvm-dev
<llvm...@lists.llvm.org> wrote:
> I would like to understand better how people use Phab's advanced
> features and why. For example, what drives the need for patch series
> and dependence relationships? Some walk-through examples would be very
> helpful.
Here's a somewhat more complex example of changes made by myself a
year and a half ago, starting with https://reviews.llvm.org/D48013
It's a series of changes across TableGen and the AMDGPU backend to
refactor how we represent a class of instructions. Some patterns that
occur:
- an NFC (no functional change) refactoring/rename change in an area
that is then later touched by a functional change.
- a change to common infrastructure (TableGen) that is motivated by
and used by a subsequent functional change in the AMDGPU backend, but
which can stand on its own and which may want to be reviewed by
different people
- a cleanup change to remove deprecated functionality from the backend
once the refactored functionality is available, separated out in order
to smoothen the upgrade path for frontends that are maintained outside
of the llvm-project repository
Reviewing all of this in a single amorphous diff is something that I
wouldn't wish on anybody. Conversely, having the linkage between
different commits provides context to reviewers.
It is also helpful to me: I can keep track of reviews to the series
while simultaneously working on other changes that happen to be
unrelated, and having the commits in separate stacks helps by
implicitly grouping them. Admittedly this advantage is comparatively
minor because the UI isn't perfect.
Cheers,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
Doerfert, Johannes via llvm-dev
> On Tue, Jan 14, 2020 at 11:41 AM Renato Golin <reng...@gmail.com> wrote:
> > We rarely approve some patches and not others in a series, and when we
> > do, we ask people to create a new series without the approved patch,
> > or split them, so that we can continue reviewing the series.
>
> This has simply not been true in my experience. Actually, not having
> to re-send a new series is one of the main advantages that
> Phabricator-based review has over the original review style for Git,
> which is to send patch series via mailing lists.
>
> It might be the case that you occasionally have series where major
> redesigns are required, and then asking for a fresh start makes sense.
multiple people with patches in all sorts of states, from merged to WIP:
The first was this one https://reviews.llvm.org/D69785, since then the
series grew in all directions (see the stack).
I have other (=smaller) patch series that evolve over time but this one
is the biggest and most complex.
David Greene via llvm-dev
> [1] or [2] are recentish examples that come to mind, but it happens
> fairly often. Of course for a bunch of simpler changes one revision is
> enough.
I think you forgot to include links. :)
> The use cases are similar to the "I have one PR with multiple commits"
> in GitHub, but with the advantage of being able to review them
> individually, and thus they can land upstream sooner.
Downstream we strongly suggest people use a one-commit-per-PR model.
That does incur some additional overhead but I personally haven't found
it to be an issue. Usually if I have a series of dependent patches,
then I need to merge the first one before I can do the second one, etc.
I can see how having multiple patches up at once for review might speed
things up. But what if a very first patch in a series needs major
changes and that affects every single following patch? The series has
to be re-posted anyway, right? I don't see how this is much better than
reviewing the first patch and getting that merged first, moving on to
the second patch, etc.
If the patches truly are independent, then why not open a separate PR
for each one? Yes, with GitHub that requires a separate branch for each
but if the changes are truly independent then multiple branches seems
entirely appropriate to me. IME it helps keep things organized.
Independent changes are independent lines of development. Changes
needed for one don't affect the state of the others.
If the patches are dependent, then we're back to the situation above
where review causes the need to re-post a new version of the series. If
review goes smoothly then I guess maybe there is some time saved but how
often does that really happen for any kind of large change that would be
needed to be broken up into multiple commits?
-David
Doerfert, Johannes via llvm-dev
> Joerg Sonnenberger via llvm-dev <llvm...@lists.llvm.org> writes:
>
> >> GitHub PR is the "de facto standard", everyone knows, the entry cost
> >> is practically zero. The UI is lean and missing features, but the
> >> large availability of tooling (either targeting GitHub directly or
> >> plain git) makes up for a lot of it.
> >
> > Just like with the "Everyone knows git", I call bullshit on this. Sorry,
> > but the far majority of GitHub users know little more than the most barebone
> > functionality of Pull Requests and the typical use case in most projects
> > is to squash all changes. But at this point it seems clear that the real
> > goal is to just move everything to GitHub just for the sake of it.
>
> Let's try to assume good faith on the part of all parties.
> >> Phab has a very complex UI with a lot of features that people have
> >> come to rely over the years. But it's far too complex for new people
> >> and requires specially crafted tooling to work with.
> >
> > Can you at least try to make reasonable arguments without ridiculous
> > hyperbole? Using phabricator for the casual user requires no special
> > tooling. Patches can be easily submitted via patch upload. There are
> > better ways to integrate it into a workflow, arcanist and git-phab just
> > to mention two.
>
> I for one cannot get arcanist to work because of its reliance on PHP
> which for whatever reason refuses to install on my machine. It it's
> hard for me to do a simple install of a tool for code review, I can't
> imagine the frustration of new people trying to get everything working.
try soon. (Came up in this thread somewhere).
> I wasn't aware of git-phab and don't recall seeing it mentioned in the
> LLVM documentation. I just checked and there is no mention of it. I'm
> not sure how new people are supposed to know about it. It does look
> nice from a cursory review of its front page. I'll give it a try!
We also need to openly discuss problems people face and look for ways to
remedy them.
> I have never used Phab's more complex features. There's nothing in the
> LLVM documentation that talks about patch series or patch parents or
> anything more advanced than "submit a patch and pick reviewers." For
> many people that is enough. But if I were to run into a situation where
> one of the reviewers asked to link my change to some other change, o
> present things as a patch series I would have no idea how to do that.
> If I were new to the project I would just give up at that point.
the reviewer explicitly requests it. This should not only be true for
Phab related questions but also for questions on code comments,
etiquette, process ...
If people do not ask questions, that is a different problem.
If questions are not answered, that would be the worst.
It should be "fairly simple" to simplify the interface by hiding less
often used apps.
While writing documentation is not easy, it would also help I guess.
Emilio Cobos Álvarez via llvm-dev
> Emilio Cobos Álvarez <emi...@crisal.io> writes:
>
>> [1] or [2] are recentish examples that come to mind, but it happens
>> fairly often. Of course for a bunch of simpler changes one revision is
>> enough.
>
> I think you forgot to include links. :)
Whoopsies :-)
* https://bugzilla.mozilla.org/show_bug.cgi?id=1449861
* https://bugzilla.mozilla.org/show_bug.cgi?id=1503656
>
>> The use cases are similar to the "I have one PR with multiple commits"
>> in GitHub, but with the advantage of being able to review them
>> individually, and thus they can land upstream sooner.
>
> Downstream we strongly suggest people use a one-commit-per-PR model.
> That does incur some additional overhead but I personally haven't found
> it to be an issue. Usually if I have a series of dependent patches,
> then I need to merge the first one before I can do the second one, etc.
>
> I can see how having multiple patches up at once for review might speed
> things up. But what if a very first patch in a series needs major
> changes and that affects every single following patch? The series has
> to be re-posted anyway, right? I don't see how this is much better than
> reviewing the first patch and getting that merged first, moving on to
> the second patch, etc.
>
> If the patches truly are independent, then why not open a separate PR
> for each one? Yes, with GitHub that requires a separate branch for each
> but if the changes are truly independent then multiple branches seems
> entirely appropriate to me. IME it helps keep things organized.
> Independent changes are independent lines of development. Changes
> needed for one don't affect the state of the others.
>
> If the patches are dependent, then we're back to the situation above
> where review causes the need to re-post a new version of the series. If
> review goes smoothly then I guess maybe there is some time saved but how
> often does that really happen for any kind of large change that would be
> needed to be broken up into multiple commits?
I don't think you necessarily need to post a second version of each
patch in the series. At least I update changes individually. Some of the
changes on the bottom of the stack may require changes on the following
patches of course, but...
-- Emilio
Hubert Tong via llvm-dev
Emilio Cobos Álvarez <emi...@crisal.io> writes:
> [1] or [2] are recentish examples that come to mind, but it happens
> fairly often. Of course for a bunch of simpler changes one revision is
> enough.
I think you forgot to include links. :)
> The use cases are similar to the "I have one PR with multiple commits"
> in GitHub, but with the advantage of being able to review them
> individually, and thus they can land upstream sooner.
Downstream we strongly suggest people use a one-commit-per-PR model.
That does incur some additional overhead but I personally haven't found
it to be an issue. Usually if I have a series of dependent patches,
then I need to merge the first one before I can do the second one, etc.
I can see how having multiple patches up at once for review might speed
things up. But what if a very first patch in a series needs major
changes and that affects every single following patch? The series has
to be re-posted anyway, right?
Joerg Sonnenberger via llvm-dev
> On 01/15, David Greene via llvm-dev wrote:
> > I for one cannot get arcanist to work because of its reliance on PHP
> > which for whatever reason refuses to install on my machine. It it's
> > hard for me to do a simple install of a tool for code review, I can't
> > imagine the frustration of new people trying to get everything working.
>
> Mozilla developed phab tools w/o PHP. I'll actually going to give them a
> try soon. (Came up in this thread somewhere).
I fully agree that installing PHP shouldn't really be a requirement.
Mercurial for example has the phabricator extension as standalone
Python code. It essentially provides two commands, phabsend and
phabread, to get changes from/to the Phabricitor server. It doesn't
cover the "discussion" part of a review, but that looks reasonable on
the web UI to me. I don't know what the status of git tooling is here. I
think git-phab still requires arcanist to be installed.
Joerg
Joerg Sonnenberger via llvm-dev
> Emilio Cobos Álvarez <emi...@crisal.io> writes:
>
> > [1] or [2] are recentish examples that come to mind, but it happens
> > fairly often. Of course for a bunch of simpler changes one revision is
> > enough.
>
> I think you forgot to include links. :)
>
> > The use cases are similar to the "I have one PR with multiple commits"
> > in GitHub, but with the advantage of being able to review them
> > individually, and thus they can land upstream sooner.
>
> Downstream we strongly suggest people use a one-commit-per-PR model.
> That does incur some additional overhead but I personally haven't found
> it to be an issue. Usually if I have a series of dependent patches,
> then I need to merge the first one before I can do the second one, etc.
One typical case for a patch series is if you need infrastructure in a
number of places in place first. Sending all changes at once allow
others to see where you are going, independent of whether the individual
pieces are acceptable immediately or not. Yes, this can mean later
patches need to be reworked for larger structural changes, but that
doesn't necessarily invalidate review remarks already made.
The other case is reworking a pass to handle a couple of similar, but not
identical cases. It might not be possible to take out patch 2 in a
series in this case, but most often the review changes here are smaller
"stylistic" issues without huge impact on later steps.
Joerg
Renato Golin via llvm-dev
> That is not what I am saying, or at least you seem to interpret it
> differently than I indented it to be read. In the part you cropped I
> mention that *both sides* provide *helpful advice* to improve the setup
> of people. I argue the problems need to be clarified for that. *Forcing
> to worsen* happens when we say we *move* but also when we *do not move*.
> Emails that just say we should move or not are therefore problematic.
So far I have seen good and bad arguments on both sides, but you keep
saying no one is providing good arguments on the other side.
It seems to me that the arguments against Phab are not a problem for
you, or you have a solution that works for you, and are therefore, not
good enough as an argument (or you need better ones).
What I'm saying is that the solutions you have may not be relevant or
good enough to them, and don't solve their problem.
In the end, both GitHub and Phab have benefits and problems, and this
will be more a choice of opinion than technical.
Trying to invalidate opinions just because they're not a big deal for
you is not helpful.
I spent many years doing code review on emails, then even more on
Phab, GitHub, Gerrit. My personal opinion is that GitHub is the least
bad in user experience, but also the simplest one, and not in the best
way. But the best part for me is that I can do almost everything from
the command line, using just plain git. That improves my productivity
immensely.
Phab is a major pain for me because I have trouble remembering all the
bells and whistles. I tried getting Arcanist to work, then it stops
working, then I have to do it all over again. The UI is super
counter-intuitive to me and even having used it for many years, it's
still alien. I make many mistakes, random ones, and that makes me
spend less time reviewing LLVM patches, not more.
It may be a problem just for me, maybe it's to do with how my brain is
wired, which granted, is a minority. That's why I have accepted it as
a fact of life to use Phab. But if there are people out there that
feel like me, then well, I'll support.
Regarding actual issues, we had enough already on both sides, I agree
with most of them, and I have already stated some myself. Tooling is
one of them, so proposing a tooling fix won't help me. i have to work
on multiple different environments, from Arch Linux, to Ubuntu, WSL
and Windows and no single tool, other than Git, can support the same
framework across the board (or at least it would be a massive job).
So, my vote is for *any* review process that I can do via Git, with
minimal use of web interfaces or specialised tools.
But I'm happy with whatever makes most people's lives easier. If
that's Phab, phab. I'll keep scratching my nails on the blackboard.
cheers,
Renato Golin via llvm-dev
> This has simply not been true in my experience. Actually, not having
> to re-send a new series is one of the main advantages that
> Phabricator-based review has over the original review style for Git,
> which is to send patch series via mailing lists.
Interesting. If they can be committed separately, why were they a
series in the first place?
Or was that independent, but related, patches? In this case, they
could have been different PRs with mention of each other, no?
> It might be the case that you occasionally have series where major
> redesigns are required, and then asking for a fresh start makes sense.
What I mean by patch series is exactly what you would have in a
sanitised git branch: a number of sequential patches that build on
each other. You cannot commit patch 3 before 2 as Git wouldn't let you
merge.
In those cases, if we want to have patch 3 before the series, the
author needs to rewrite history (rebase -i), push the patch up and
pull into another branch, so that we can have the old tree with -1
patches as the derived series, and the cherry-picked patch merged
sooner.
But in those cases, the patch would most certainly have to be
different, and that would also change the remaining series. So, new
patch, new series.
Makes sense?
--renato
David Blaikie via llvm-dev
(sometimes review might indicate the whole patch series direction is not preferred - in which case, yeah, maybe throwing it out and starting a new thread/set of threads is appropriate - but not always)
Renato Golin via llvm-dev
> I don't think creating a new thread (or series of N threads) because an early patch in a series necessitate some refactoring of later patches is ideal - while patch series can/should be avoided where reasonable (& if the series is small enough/the framing/design is obvious enough, you can hold on to the later patches rather than reviewing them all in parallel with the volatility that comes with that approach) - but there are certainly cases I've seen where it's helpful to send a dependent series together to help frame/justify the goals of the early refactoring patches. If the early refactoring requires changes - yeah, you get that done, signed off and committed (while possibly doing higher level design review of later patches that might be applicable regardless of the early refactoring - like if you decide to rename a newly created function in an early patch, that shouldn't hurt the review going on in later patches, for instance) & update later patches with the knock-on effects. No need to start new mail threads/lose review context/etc.
On Git, I normally would have done that on separate, but related,
branches and pull requests. II find it much easier to work with
rebases and cherry-picks across branches than on the same branch. A
rebase -i can turn dreadful if the same part of the code is touched by
more than one patch, and it can be almost impossible if that pattern
happens more than once.
In my use of the term, a patch series is a mandated order of directly
related commits, like you would have in a branch, if the patches are
logically separated into clear individual steps, for the clarity of
review.
Perhaps the name clash has caused more trouble than the original
discussion. If that's not what most people would have thought,
apologies for the confusion.
Doerfert, Johannes via llvm-dev
> > That is not what I am saying, or at least you seem to interpret it
> > differently than I indented it to be read. In the part you cropped I
> > mention that *both sides* provide *helpful advice* to improve the setup
> > of people. I argue the problems need to be clarified for that. *Forcing
> > to worsen* happens when we say we *move* but also when we *do not move*.
> > Emails that just say we should move or not are therefore problematic.
>
> So far I have seen good and bad arguments on both sides, but you keep
> saying no one is providing good arguments on the other side.
interpreted as such. I don't think the above contains such a claim, nor
could I find such a claim in the last email I send. If you interpret
what I wrote in such a way I can understand why you might be upset.
Please feel free to point out things I could formulate better to avoid
making this impression, either via the list or in a private mail.
> It seems to me that the arguments against Phab are not a problem for
> you, or you have a solution that works for you, and are therefore, not
> good enough as an argument (or you need better ones).
with Phab. I did questions the reason for a move and I asked for
problems to be described (by either side) so we can find solutions.
Whenever I said that something is not a problem for me I explained
myself, e.g., my setup. I also explained how I use Phab, and given that
I don't know enough about GH PRs, I did not attack it willingly.
> What I'm saying is that the solutions you have may not be relevant or
> good enough to them, and don't solve their problem.
constructive way nevertheless.
David Greene via llvm-dev
>> The above journey to get a patch submitted was my actual first
>> experience with Phab. It did not leave a good taste in my mouth.
>
> What if we take these points and act on them?
That would be great!
> It should be "fairly simple" to simplify the interface by hiding less
> often used apps.
This brings another question to mind. How much does it cost us to host
our own Phab instance and customize/maintain it? Would we save anything
by going to a third-party hosted solution like we did for the
repository? Note that I am not saying that third-party hosted review
tool must be GitHub. It could be something else, though GitHub is an
obvious choice since the repository already lives there.
> While writing documentation is not easy, it would also help I guess.
Documentation would help. In particular I would like some guidance on
when a patch series is appropriate vs. individual patches submitted for
review. I haven't been able to find any documentation on how that model
works in Phab, when you would want to use it, the features it offers,
etc. It may exist on phabricator.com but it's not obvious where it is.
It is clear to me that I am missing a lot of potential usefulness with
Phab and I would like to learn more!
One thing I *really* miss is a command-line tool to submit patches.
With GitHub you just use git (there are even extension commands for
opening GitHub pull requests). Easy-peasy.
Mozilla's Phab tools may fill that need. I am optimistic here. If they
work well the project should consider making them an officially
supported part of our tool belt with LLVM-specific documentation on how
to use them.
I am certainly open to sticking with Phab if we can address its issues.
It will be important to solicit input from those very new to the project
as many of us have figured out how to work around its quirks to the
point that we don't even recognize the issues anymore.
David Greene via llvm-dev
> The first was this one https://reviews.llvm.org/D69785, since then the
> series grew in all directions (see the stack).
How do I see the stack?
This is a great example of the (to me) unintuitive interface of Phab.
People talk about things like "the stack" as if it's obvious and it's
really not.
-David
David Greene via llvm-dev
>> I can see how having multiple patches up at once for review might speed
>> things up. But what if a very first patch in a series needs major
>> changes and that affects every single following patch? The series has
>> to be re-posted anyway, right?
>
> Even if it is being re-posted anyway, it does not mean the reviews for the
> latter patches "restart at zero". An "major" interface change flowing from
> the first patch might lead only to mechanical changes that would be
> considered NFC to "status quo" of the later patches in the series. A force
> push with GitHub would harm such a workflow, but Phabricator handles such
> use cases well.
I have never really played with a multiple-commits-per-PR model in
GitHub so I don't know what happens with a rebase. Do all the previous
review comments disappear or something else? I would like to understand
the differences between GitHub and Phab when a rebase happens.
David Greene via llvm-dev
> One typical case for a patch series is if you need infrastructure in a
> number of places in place first. Sending all changes at once allow
> others to see where you are going, independent of whether the individual
> pieces are acceptable immediately or not. Yes, this can mean later
> patches need to be reworked for larger structural changes, but that
> doesn't necessarily invalidate review remarks already made.
I can certainly see the utility of this. I've never tried it since
posting patches via the web interface is not at all convenient and as
I've mentioned before I can't get arcanist to work. Does arcanist take
care of organizing the series?
How do reviews work? Does each patch get a separate review page like
any other patch?
Let's say patch 3 in a series is approved and it could be committed
without patches 1 and 2 (like Renato I question why this is a patch
series if so, but let's assume it for argument's sake). That means I
need to rebase -i my branch in order to get patch 3 committed, right?
Like Renato, I actually find that more burdensome than just putting
patch 3 in its own branch in the first place and doing a separate review
of it. In other words, my definition of "patch series" is the same as
Renato's: a linear series of commits each of which is dependent on the
previous commits. With that definition, "early approval" of later
patches really doesn't help get things committed earlier.
I definitely see the advantage of not invalidating existing review
comments if the series is rebased/changed. I confess I don't know how
GitHub handles rebases (see my other post with questions about that).
> The other case is reworking a pass to handle a couple of similar, but not
> identical cases. It might not be possible to take out patch 2 in a
> series in this case, but most often the review changes here are smaller
> "stylistic" issues without huge impact on later steps.
I'm sorry I don't really follow this. How does it differ from the first
case? Are you saying you're grouping two similar but basically
independent changes together? I would think doing so violates the "one
topic per review" principle (that's maybe not an LLVM policy per se,
just the way I think to think about things).
-David
David Blaikie via llvm-dev
"Doerfert, Johannes via llvm-dev" <llvm...@lists.llvm.org> writes:
> The first was this one https://reviews.llvm.org/D69785, since then the
> series grew in all directions (see the stack).
How do I see the stack?
This is a great example of the (to me) unintuitive interface of Phab.
People talk about things like "the stack" as if it's obvious and it's
really not.
Yeah, once something's got a lot of commenting on it - but there's two instances of the word "stack" when I open and search that page. The first one is the one you're looking for I think - under "Revision Contents" there's "Files", "History", "Commits", "Stack", and "Similar".
If you open "Stack" it shows various related and not so related commits in the timeline - if you look at the line related to this review (highlighted in yellow) you can see that this patch is on the purple coloured stack. So you can see patches built up on top of it, such as D69922.
If you're reviewing this patch - in the timeline of comments you'd see a structured event description like "jdoerfert added a child revision: D69922: [OpenMP] Use the OpenMP-IR-Builder." telling you that there was a new patch added that is based on top of this one.
-David
_______________________________________________
cfe-dev mailing list
cfe...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
David Greene via llvm-dev
> Here's a somewhat more complex example of changes made by myself a
> year and a half ago, starting with https://reviews.llvm.org/D48013
Aha! I found it! The "Stack" tab under "Revision Contents" after all
of the review comments. That's *really* not obvious if there are lots
of review comments to scroll through.
So if I'm understanding this correctly, there are 17 commits in this
series, one of which is still open for review. Is that correct?
What is the graph on the left trying to show me? The commit graph?
This is a non-linear branch? Or is this a dependence graph between
patches in the series? If the latter, what generated those
dependencies?
> Reviewing all of this in a single amorphous diff is something that I
> wouldn't wish on anybody.
Certainly! I don't think anyone is advocating for that. I agree GitHub
by default presenting things as a giant patch is not helpful, but I'm
told that can be changed. I don't how GitHub reviews work for PRs with
multiple commits since I've never tried it. Hopefully someone with such
experience will chime in.
> Conversely, having the linkage between different commits provides
> context to reviewers.
I can see that too. For commits that are dependent on a long series of
other commits (D48017 for example), how hard is it to review without
reviewers also looking at the commits upon which it depends? Do people
really review such commits early in the process? Even without the early
review I agree the context can be useful.
> It is also helpful to me: I can keep track of reviews to the series
> while simultaneously working on other changes that happen to be
> unrelated, and having the commits in separate stacks helps by
> implicitly grouping them. Admittedly this advantage is comparatively
> minor because the UI isn't perfect.
I'm not sure how this differs from having multiple patches up for review
simultaneously. I do that all the time and is just as easy (for me)
with GitHub PRs. Are you simply saying that you can have multiple patch
series up for review at the same time? I feel like I am not grasping
some (to me) subtle point you're making.
-David
David Blaikie via llvm-dev
On Wed, 15 Jan 2020 at 23:19, David Blaikie <dbla...@gmail.com> wrote:
> I don't think creating a new thread (or series of N threads) because an early patch in a series necessitate some refactoring of later patches is ideal - while patch series can/should be avoided where reasonable (& if the series is small enough/the framing/design is obvious enough, you can hold on to the later patches rather than reviewing them all in parallel with the volatility that comes with that approach) - but there are certainly cases I've seen where it's helpful to send a dependent series together to help frame/justify the goals of the early refactoring patches. If the early refactoring requires changes - yeah, you get that done, signed off and committed (while possibly doing higher level design review of later patches that might be applicable regardless of the early refactoring - like if you decide to rename a newly created function in an early patch, that shouldn't hurt the review going on in later patches, for instance) & update later patches with the knock-on effects. No need to start new mail threads/lose review context/etc.
On Git, I normally would have done that on separate, but related,
branches and pull requests.
If the patches aren't dependent on each other - then that's not, to me, the situation we're discussing. That situation can/should always be handled as you say - separate branches/reviews/etc. Maybe related (link to the other reviews for context/design understanding/etc) but when talking about reviewing a patch series, at least I assume that means a set of /dependent/ patches, where later patches cannot be committed without earlier patches.
II find it much easier to work with
rebases and cherry-picks across branches than on the same branch. A
rebase -i can turn dreadful if the same part of the code is touched by
more than one patch, and it can be almost impossible if that pattern
happens more than once.
In my use of the term, a patch series is a mandated order of directly
related commits, like you would have in a branch, if the patches are
logically separated into clear individual steps, for the clarity of
review.
Perhaps the name clash has caused more trouble than the original
discussion. If that's not what most people would have thought,
apologies for the confusion.
I'm not sure where the idea that a patch series is anything other than that ^ came from. When I was talking about a patch series, it was/is with that definition in mind - ordered/dependent commits. I said "dependent series" to reinforce this idea that the kind of situation I was describing was one in which later patches in the series depend on the changes in earlier patches.
- Dave
--renato
David Blaikie via llvm-dev
Joerg Sonnenberger via cfe-dev <cfe...@lists.llvm.org> writes:
> One typical case for a patch series is if you need infrastructure in a
> number of places in place first. Sending all changes at once allow
> others to see where you are going, independent of whether the individual
> pieces are acceptable immediately or not. Yes, this can mean later
> patches need to be reworked for larger structural changes, but that
> doesn't necessarily invalidate review remarks already made.
I can certainly see the utility of this. I've never tried it since
posting patches via the web interface is not at all convenient
Perhaps this is covered elsewhere but I'm still not super clear on "not at all convenient" - there's a form you fill in and attach the patch file.
and as
I've mentioned before I can't get arcanist to work. Does arcanist take
care of organizing the series?
(I haven't made patch series myself)
How do reviews work? Does each patch get a separate review page like
any other patch?
Yes, see https://reviews.llvm.org/D69785 and https://reviews.llvm.org/D69922 as an example of two patches in a series.
Let's say patch 3 in a series is approved and it could be committed
without patches 1 and 2 (like Renato I question why this is a patch
series if so, but let's assume it for argument's sake). That means I
need to rebase -i my branch in order to get patch 3 committed, right?
If patch 3 can be committed without 1 and 2, it's not a series. It's a bunch of related reviews I'd do on separate branches - and I imght send them out at the same time to illustrate the broader design - but they're still effectively 3 separate reviews.
If they're a series they have ordering and having tools to help review them separately while understanding that one patch is built on top of the other is useful.
Like Renato, I actually find that more burdensome than just putting
patch 3 in its own branch in the first place and doing a separate review
of it. In other words, my definition of "patch series" is the same as
Renato's: a linear series of commits each of which is dependent on the
previous commits. With that definition, "early approval" of later
patches really doesn't help get things committed earlier.
Yeah, I don't /think/ anyone is using "series" To describe a set of independent but related patches. I think everyone would send those out as separate patches not in a single branch of stacked commits on git, or using phab's stack handling functionality.
I definitely see the advantage of not invalidating existing review
comments if the series is rebased/changed. I confess I don't know how
GitHub handles rebases (see my other post with questions about that).
> The other case is reworking a pass to handle a couple of similar, but not
> identical cases. It might not be possible to take out patch 2 in a
> series in this case, but most often the review changes here are smaller
> "stylistic" issues without huge impact on later steps.
I'm sorry I don't really follow this. How does it differ from the first
case? Are you saying you're grouping two similar but basically
independent changes together? I would think doing so violates the "one
topic per review" principle (that's maybe not an LLVM policy per se,
just the way I think to think about things).
So the first scenario Joerg was talking about was what I think almost everyone in this thread are talking about when they say "series" - dependent patches, built on top of each other, where later patches cannot be committed without earlier patches. Usually because they are all in service of some singular goal - but broken into incremental changes that we all know and love.
This second scenario is one in which the dependence is smaller - you want to make 3 changes to an optimization, each one doesn't require the others - but the code is close enough together that they touch the same bits of code. If the overlap is small enough - you could send these as 3 separate/unrelated reviews/patches - and whichever gets committed first you rebase your other two outstanding ones. But sometimes they might be a bit too connected ("might not be possible to take out patch 2" - that's what makes it a series, not 3 independent patches - because the API overlap is too close, so your 3 patches that are sort of independent, but overlap with each other such that they are ordered - you'd like to get them all reviewed, so you send them all out and use parent/child relationships so reviewers realize they are looking at increments that aren't from top of tree (because one patch is based on the incidental/nearby API changes caused by the earlier patch in the series))
It's still essentially a dependent patch series - just a more loosely connected one. 3 independent goals, but so close that they're dependent. Rather than 3 incremental steps towards 1 core goal.
Multiple cases where dependent patch series are useful/might be used.
- Dave
Renato Golin via llvm-dev
> I'm not sure where the idea that a patch series is anything other than that ^ came from. When I was talking about a patch series, it was/is with that definition in mind - ordered/dependent commits. I said "dependent series" to reinforce this idea that the kind of situation I was describing was one in which later patches in the series depend on the changes in earlier patches.
Perhaps it's my confusion in interpreting the answers.
Some comments mentioned "if a review spawns a related change", which
to me is a different review, and I've reviewed many of those cases.
Most people use the Phab link to express relationship, but that's not
a series.
Others said "some patches may be approved before others" which only
works in a series if they're from start to N, not N to M, nor M to the
end, nor randomly approved. In this case, the series is split in two,
with the latter having to be rebased on patches committed after the
first part is, so essentially, creating a new series.
Doing both these cases as a pull request is trivial.
Related changes become separate PRs with mention. GitHub creates the
links, like Phab if you tag on the commit message or comments.
Series split is harder, but still trivial. You create a new branch,
move up to the approved patch, push. Rebase your old branch and you
have a new series.
In Github you can choose to close the PR and open a new one, or just
push again and the UI should update the range. I prefer the former,
because you keep the comments history.
I would have done the same process in Phab, but with more clicks,
uploads, links, etc.
David Blaikie via llvm-dev
Nicolai Hähnle <nhae...@gmail.com> writes:
> Here's a somewhat more complex example of changes made by myself a
> year and a half ago, starting with https://reviews.llvm.org/D48013
Aha! I found it! The "Stack" tab under "Revision Contents" after all
of the review comments. That's *really* not obvious if there are lots
of review comments to scroll through.
There are notes within the comments about when child/parent revisions are added. So if you're actively reviewing, and open the review to see the latest comments - you'd probably see a comment like "Split this off into its own review" & an auto-comment/status update describing that new relationship. If you were opening a new patch series that had just been sent out - you'd see that relationship status update as the first comment sort of thing.
So if I'm understanding this correctly, there are 17 commits in this
series, one of which is still open for review. Is that correct?
Roughly speaking. At this point - that one remaining is just like a normal review, since it's based on committed code/not another outstanding review.
What is the graph on the left trying to show me? The commit graph?
This is a non-linear branch? Or is this a dependence graph between
patches in the series? If the latter, what generated those
dependencies?
Correct - it's the dependence graph. Humans generated the dependencies - there's an "Edit Related Revisions" button on the top right, where you can specify patches that are children or parents to this one.
> Reviewing all of this in a single amorphous diff is something that I
> wouldn't wish on anybody.
Certainly! I don't think anyone is advocating for that. I agree GitHub
by default presenting things as a giant patch is not helpful, but I'm
told that can be changed. I don't how GitHub reviews work for PRs with
multiple commits since I've never tried it. Hopefully someone with such
experience will chime in.
> Conversely, having the linkage between different commits provides
> context to reviewers.
I can see that too. For commits that are dependent on a long series of
other commits (D48017 for example), how hard is it to review without
reviewers also looking at the commits upon which it depends?
Sometimes easy, sometimes harder - and depends on the kind of review. for me this really helps keep the substantive parts of the review focussed - you can move depedent refactoring into separate reviews that are easy to eyeball knowing you aren't expecting to find anything "interesting". Then separately review the "interesting" stuff without all the noise of the refactoring. That's super important to me.
Do people
really review such commits early in the process? Even without the early
review I agree the context can be useful.
Yep - I might do superficial review early - basic stylistic stuff to get that out of the way even if the substance isn't ready to be reivewed yet (most often by suggesting ways to pull out refactorings to focus the main review on the substantive parts).
> It is also helpful to me: I can keep track of reviews to the series
> while simultaneously working on other changes that happen to be
> unrelated, and having the commits in separate stacks helps by
> implicitly grouping them. Admittedly this advantage is comparatively
> minor because the UI isn't perfect.
I'm not sure how this differs from having multiple patches up for review
simultaneously. I do that all the time and is just as easy (for me)
with GitHub PRs. Are you simply saying that you can have multiple patch
series up for review at the same time? I feel like I am not grasping
some (to me) subtle point you're making.
I believe he's saying that having patch series grouped together makes it easier to think of it as a logical unit. Rather than "I have 10 patches out for review" thinking of it as "I have 5 features out for review" -maybe one complicated on ein the form of a sries of 5 commits, and a small pair, and 4 singles, for instance.
Renato Golin via llvm-dev
<llvm...@lists.llvm.org> wrote:
> Yeah, I don't /think/ anyone is using "series" To describe a set of independent but related patches. I think everyone would send those out as separate patches not in a single branch of stacked commits on git, or using phab's stack handling functionality.
in Phab, related links and series links are the same thing, ie.
parent/child, no? Perhaps that's the source of confusion?
In GitHub, related is a mention "see #123" and series is a multi-commit PR.
--renato
PS: I haven't used links in Phab much, so I'm probably wrong...
David Blaikie via llvm-dev
On Thu, 16 Jan 2020 at 18:45, David Blaikie <dbla...@gmail.com> wrote:
> I'm not sure where the idea that a patch series is anything other than that ^ came from. When I was talking about a patch series, it was/is with that definition in mind - ordered/dependent commits. I said "dependent series" to reinforce this idea that the kind of situation I was describing was one in which later patches in the series depend on the changes in earlier patches.
Perhaps it's my confusion in interpreting the answers.
Some comments mentioned "if a review spawns a related change", which
to me is a different review, and I've reviewed many of those cases.
Most people use the Phab link to express relationship, but that's not
a series.
Can you point to examples of that - where Phab links have been used to express non-mechanically-dependent patches?
Others said "some patches may be approved before others"
which only
works in a series if they're from start to N, not N to M, nor M to the
end, nor randomly approved. In this case, the series is split in two,
with the latter having to be rebased on patches committed after the
first part is, so essentially, creating a new series.
Approval order isn't commit order - I'm more than happy to approve a later patch in a series, even if the review of an earlier patch necessitates some rework in a later patch - such as renaming a function. The later patch I already approved must be updated to use the new name of the function, but I'm fine with that, same as I would be if it'd been an independent change that did the rename in the time it took us to review that patch.
Doing both these cases as a pull request is trivial.
Related changes become separate PRs with mention.
What do you mean by "with mention" and what do you mean by "related" (I guess you man "not dependent, but interesting to consider together")
GitHub creates the
links, like Phab if you tag on the commit message or comments.
Series split is harder, but still trivial. You create a new branch,
move up to the approved patch, push. Rebase your old branch and you
have a new series.
My understanding is that this ^ is the case we're talking about with Phab patch series. Related but independent patches aren't "interesting", as you say - just mention them in passing in the review.
My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review feedback and updates?
In Github you can choose to close the PR and open a new one, or just
push again and the UI should update the range. I prefer the former,
because you keep the comments history.
Perhaps there's some misunderstanding/different experiences there (as there is with Phab). Maybe you could make a little example? Showing how a dependent patch series with ongoing review feedback works with github PRs?
David Blaikie via llvm-dev
On Thu, 16 Jan 2020 at 18:52, David Blaikie via llvm-dev
<llvm...@lists.llvm.org> wrote:
> Yeah, I don't /think/ anyone is using "series" To describe a set of independent but related patches. I think everyone would send those out as separate patches not in a single branch of stacked commits on git, or using phab's stack handling functionality.
in Phab, related links and series links are the same thing, ie.
parent/child, no?
Yes, in Phab the "related revisions" are specifically the parent/child relationships. (related, relationships, etc)
If you only want to refer to another review - there's no specific field for that, you'd just say (as you do in git) "see D4567 for this reason or that reason".
Renato Golin via llvm-dev
> Can you point to examples of that - where Phab links have been used to express non-mechanically-dependent patches?
Not at the top of my head, but since that's not what we're talking
about, I'll go to the next point.
> Approval order isn't commit order - I'm more than happy to approve a later patch in a series, even if the review of an earlier patch necessitates some rework in a later patch - such as renaming a function. The later patch I already approved must be updated to use the new name of the function, but I'm fine with that, same as I would be if it'd been an independent change that did the rename in the time it took us to review that patch.
I see what you mean. Once all are approved, the commit happens, but
they can be approved in any order.
I used to do that too, but I honestly don't like it. Earlier patches
can significantly change the following changes, then the approval has
to be reverted.
Nowadays, on both Phab and Git I just say "LGTM once the others are
approved as is" and once I approve the earlier ones, I scan the
remaining series and approve them all.
I think the process of uploading multiple commits in separate and then
creating a link from each one to the next is cumbersome and error
prone (to me particularly, I make many mistakes).
> What do you mean by "with mention" and what do you mean by "related" (I guess you man "not dependent, but interesting to consider together")
When you write a ticket number, either in phab (D12345) or GitHub
(#123), the UI creates a link to the mentioned issue/review. In
Github, if you say things like "fixes #123", it closes automatically
(which is not necessarily right, like "partially fixes #123" :).
> My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review feedback and updates?
Both Phab and GitHub are problematic in different ways. In Phab, an
earlier change that trickles to the rest of the series needs to be
updated on all patches. In GitHub, some information is lost,
especially if it's a force push, but it only sends one email for the
series.
> Perhaps there's some misunderstanding/different experiences there (as there is with Phab). Maybe you could make a little example? Showing how a dependent patch series with ongoing review feedback works with github PRs?
There's probably a better such example on the internet. I ended up
dragging myself into this corner, but I'm not a defender of GitHub
PRs. I just dislike Phab more than it. ;)
Hubert Tong via llvm-dev
Hubert Tong <hubert.rein...@gmail.com> writes:
>> I can see how having multiple patches up at once for review might speed
>> things up. But what if a very first patch in a series needs major
>> changes and that affects every single following patch? The series has
>> to be re-posted anyway, right?
>
> Even if it is being re-posted anyway, it does not mean the reviews for the
> latter patches "restart at zero". An "major" interface change flowing from
> the first patch might lead only to mechanical changes that would be
> considered NFC to "status quo" of the later patches in the series. A force
> push with GitHub would harm such a workflow, but Phabricator handles such
> use cases well.
I have never really played with a multiple-commits-per-PR model in
GitHub so I don't know what happens with a rebase. Do all the previous
review comments disappear or something else? I would like to understand
the differences between GitHub and Phab when a rebase happens.
Both Phab and GitHub are problematic in different ways. In Phab, an
earlier change that trickles to the rest of the series needs to be
updated on all patches. In GitHub, some information is lost,
especially if it's a force push, but it only sends one email for the
series.
-David
Hubert Tong via llvm-dev
Hubert Tong via llvm-dev
Nicolai Hähnle <nhae...@gmail.com> writes:
> Here's a somewhat more complex example of changes made by myself a
> year and a half ago, starting with https://reviews.llvm.org/D48013
Aha! I found it! The "Stack" tab under "Revision Contents" after all
of the review comments. That's *really* not obvious if there are lots
of review comments to scroll through.
So if I'm understanding this correctly, there are 17 commits in this
series, one of which is still open for review. Is that correct?
What is the graph on the left trying to show me? The commit graph?
This is a non-linear branch? Or is this a dependence graph between
patches in the series? If the latter, what generated those
dependencies?
> Reviewing all of this in a single amorphous diff is something that I
> wouldn't wish on anybody.
Certainly! I don't think anyone is advocating for that. I agree GitHub
by default presenting things as a giant patch is not helpful, but I'm
told that can be changed. I don't how GitHub reviews work for PRs with
multiple commits since I've never tried it. Hopefully someone with such
experience will chime in.
> Conversely, having the linkage between different commits provides
> context to reviewers.
I can see that too. For commits that are dependent on a long series of
other commits (D48017 for example), how hard is it to review without
reviewers also looking at the commits upon which it depends? Do people
really review such commits early in the process? Even without the early
review I agree the context can be useful.
> It is also helpful to me: I can keep track of reviews to the series
> while simultaneously working on other changes that happen to be
> unrelated, and having the commits in separate stacks helps by
> implicitly grouping them. Admittedly this advantage is comparatively
> minor because the UI isn't perfect.
I'm not sure how this differs from having multiple patches up for review
simultaneously. I do that all the time and is just as easy (for me)
with GitHub PRs. Are you simply saying that you can have multiple patch
series up for review at the same time? I feel like I am not grasping
some (to me) subtle point you're making.
-David
_______________________________________________
Joerg Sonnenberger via llvm-dev
Let's try to be a bit less theoretical. Consider that we want to
introduce a new optimisation. The patch set contains three parts:
(1) Refactoring of some existing code in preparation of reusing it.
(2) Adding test cases for existing cases that should be preserved as is.
(3) The new optimisation with matching tests.
Change (1) and (2) are independent of each other, but neither makes much
sense when (3) is rejected. As such, creating a patch series for review
is more sensible than submitting independent changes. Now when there is
agreement that (3) is desirable, it is not so unusual to see some back
and forth still going on for (1). In that case, a LGTM for (2) allows
taking that part of out of the patch series, allowing (new) reviewers to
focus on the parts still under active consideration.
> > The other case is reworking a pass to handle a couple of similar, but not
> > identical cases. It might not be possible to take out patch 2 in a
> > series in this case, but most often the review changes here are smaller
> > "stylistic" issues without huge impact on later steps.
>
> I'm sorry I don't really follow this. How does it differ from the first
> case? Are you saying you're grouping two similar but basically
> independent changes together? I would think doing so violates the "one
> topic per review" principle (that's maybe not an LLVM policy per se,
> just the way I think to think about things).
Again, let's try to pick a realistic example. Constant folding of fast
math for example. That includes a lot of very similar cases in multiple
places. There will be a lot of similarity in the code and there is a
common topic, but sub-topics like operator classes tend to be independet.
As such, it is helpful for the submitter and the reviewer to group them
together as patch series. Whenever something is done, it can drop out of
the review.
Joerg
Christian Kühnel via llvm-dev
David Blaikie via llvm-dev
Hi folks,Another thought on the topic is tooling support and tool integration:There is a hughe ecosystem around Github and very little around Phabricator.It took me 2 days to set up build jobs for the 10.x release branch [1]. There are nice build integrations for Github and it was smooth sailing. Setting up a build job for pull requests would just be a few clicks now.In contrast I've been spending weeks on setting up a proper integration of pre-merge tests [2] with Phabricator. And I'm far from having a proper solution and I know some corner cases we will never be able to solve properly with Phabricator. The root cause is: On Phabricator you review patches that might or might not be related to a git revision and the process for merging/landing might introduce even more changes. This is a fundamental problem with the data model and nothing that can be fixed easily.In Github pull requests there is always a git commit that you can just feed to the build server. And you can be sure of what really gets merged. You review, build and test exactly the change that gets merged afterwards.
So from the build server perspective, Github is clearly the better solution.Best,Christian
Christian Kühnel via llvm-dev
In Github pull requests there is always a git commit that you can just feed to the build server. And you can be sure of what really gets merged. You review, build and test exactly the change that gets merged afterwards.How would that be true? Given that upstream keep changing during the period of review? The commit is going to have to be rebased to head and that may involve making changes.
David Greene via llvm-dev
>> My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review feedback and updates?
>
> Both Phab and GitHub are problematic in different ways. In Phab, an
> earlier change that trickles to the rest of the series needs to be
> updated on all patches. In GitHub, some information is lost,
> especially if it's a force push, but it only sends one email for the
> series.
Isn't force push on a private branch (series) going to be rather common
as authors respond to reviews? Or is there a better model?
-David
David Greene via llvm-dev
> The update process in Phab in rather manual and it does lead to more noise.
> The GitHub force push risks loss of context for earlier comments (not just
> in terms of display, like viewing older comments on a later diff in Phab).
> Another issue with the GitHub workflow is that there is no metadata linking
> the corresponding commits between two versions of the same patch series.
> Reviewers looking at a patch-series-as-GitHub-PR that is a refresh of an
> older patch-series-as-GitHub-PR are less likely to notice the previous
> discussion than the corresponding Phab workflow.
I read this as the refresh being an entirely new GitHub PR. Is that
right? Normally I would expect the same PR to be used but the rebase
would cause a force-push of the branch which would update the PR with
the new commits but might lose comments. It's that later part I'm
unsure about. It would seem odd to me to open an entirely new PR due to
a rebase/update of commits to respond to review.
David Greene via llvm-dev
>> I can certainly see the utility of this. I've never tried it since
>> posting patches via the web interface is not at all convenient
>
> Perhaps this is covered elsewhere but I'm still not super clear on "not at
> all convenient" - there's a form you fill in and attach the patch file.
With GitHub PRs:
git push origin HEAD
load up GitHub in a browser
open a PR, select reviewers
click "Create"
With Phab:
load up Phab in a browser
for N = ${number of patches - 1} to 0; do
git diff -U9999999999 HEAD~${N} > patch.txt
click "Differential"
click "+ Create Diff", set reviewers, etc.
click "Next"
copy-and-paste text from patch.txt
click "Submit"
link to previous patch
done
Maybe there is a better way with the web interface but I don't know what
it is.
Even if there is only one patch, it's a significant amount of overhead
compared to GitHub. Generating patch.txt and doing the copy-paste is
the most expensive part in terms of losing context/focus and that
doesn't exist at all with GitHub. I know there are tools that work with
GitHub PRs via the command line. My guess is that using such tools for
GitHub and Phab would be roughly equivalent in effort but as I said I
can't use arcanist.
>> Let's say patch 3 in a series is approved and it could be committed
>> without patches 1 and 2 (like Renato I question why this is a patch
>> series if so, but let's assume it for argument's sake). That means I
>> need to rebase -i my branch in order to get patch 3 committed, right?
>>
>
> If patch 3 can be committed without 1 and 2, it's not a series. It's a
> bunch of related reviews I'd do on separate branches - and I imght send
> them out at the same time to illustrate the broader design - but they're
> still effectively 3 separate reviews.
Ok, that's how I would want it to work too.
> If they're a series they have ordering and having tools to help review them
> separately while understanding that one patch is built on top of the other
> is useful.
Yep. My understanding is that GitHub can present commits as a series
but the default PR model doesn't do that, it just presents on big ugly
diff. As I said I haven't played with it myself so it would be great to
hear from people who have first-hand experience.
>> > The other case is reworking a pass to handle a couple of similar, but not
>> > identical cases. It might not be possible to take out patch 2 in a
>> > series in this case, but most often the review changes here are smaller
>> > "stylistic" issues without huge impact on later steps.
>>
>> I'm sorry I don't really follow this. How does it differ from the first
>> case? Are you saying you're grouping two similar but basically
>> independent changes together? I would think doing so violates the "one
>> topic per review" principle (that's maybe not an LLVM policy per se,
>> just the way I think to think about things).
>>
>
> This second scenario is one in which the dependence is smaller - you want
> to make 3 changes to an optimization, each one doesn't require the others -
> but the code is close enough together that they touch the same bits of
> code. If the overlap is small enough - you could send these as 3
> separate/unrelated reviews/patches - and whichever gets committed first you
> rebase your other two outstanding ones. But sometimes they might be a bit
> too connected ("might not be possible to take out patch 2" - that's what
> makes it a series, not 3 independent patches - because the API overlap is
> too close, so your 3 patches that are sort of independent, but overlap with
> each other such that they are ordered - you'd like to get them all
> reviewed, so you send them all out and use parent/child relationships so
> reviewers realize they are looking at increments that aren't from top of
> tree (because one patch is based on the incidental/nearby API changes
> caused by the earlier patch in the series))
>
> It's still essentially a dependent patch series - just a more loosely
> connected one. 3 independent goals, but so close that they're dependent.
> Rather than 3 incremental steps towards 1 core goal.
Ok, got it. Thanks for explaining!
David Greene via llvm-dev
> Let's try to be a bit less theoretical. Consider that we want to
> introduce a new optimisation. The patch set contains three parts:
> (1) Refactoring of some existing code in preparation of reusing it.
> (2) Adding test cases for existing cases that should be preserved as is.
> (3) The new optimisation with matching tests.
>
> Change (1) and (2) are independent of each other, but neither makes much
> sense when (3) is rejected. As such, creating a patch series for review
> is more sensible than submitting independent changes. Now when there is
> agreement that (3) is desirable, it is not so unusual to see some back
> and forth still going on for (1). In that case, a LGTM for (2) allows
> taking that part of out of the patch series, allowing (new) reviewers to
> focus on the parts still under active consideration.
Thanks for these examples, they are helpful!
I'm not sure (1) and (2) are useless if (3) is rejected. Making code
more reusable is often desirable in its own right. Adding more tests is
often desirable in its own right. If nothing else, working on (3) has
brought to light some issues that hadn't been considered before (holes
in testing, suboptimal code design). It seems to be that committing
those changes independent of (3) is not necessarily wrong.
But your larger point stands, there may in general be cases where (1)
and (2) are not desired without (3).
> Again, let's try to pick a realistic example. Constant folding of fast
> math for example. That includes a lot of very similar cases in multiple
> places. There will be a lot of similarity in the code and there is a
> common topic, but sub-topics like operator classes tend to be independet.
> As such, it is helpful for the submitter and the reviewer to group them
> together as patch series. Whenever something is done, it can drop out of
> the review.
I can see the reason for linking them in a series, but is it really the
case that we would want some of these changes committed and not others?
That's essentially what we're saying by making them separate patches. I
would tend to put them all in one patch.
But again, in general I think your example certainly can apply to some
situations.
For both cases, I am not sure how often they apply or if there is a
different way of thinking about them that doesn't involve a patch series
(for example doing one patch for example 2).
-David
David Greene via llvm-dev
>> I can see that too. For commits that are dependent on a long series of
>> other commits (D48017 for example), how hard is it to review without
>> reviewers also looking at the commits upon which it depends?
>
> Sometimes easy, sometimes harder - and depends on the kind of review. for
> me this really helps keep the substantive parts of the review focussed -
> you can move depedent refactoring into separate reviews that are easy to
> eyeball knowing you aren't expecting to find anything "interesting". Then
> separately review the "interesting" stuff without all the noise of the
> refactoring. That's super important to me.
Yes, I can definitely appreciate that.
>> Do people really review such commits early in the process? Even
>> without the early review I agree the context can be useful.
>
> Yep - I might do superficial review early - basic stylistic stuff to get
> that out of the way even if the substance isn't ready to be reivewed yet
> (most often by suggesting ways to pull out refactorings to focus the main
> review on the substantive parts).
Ok. If it is true that GitHub can be configured to show a branch as a
series of diffs rather than a giant diff, hopefully we'd have similar
functionality. Of course someone would need to verify that. :)
From what I have read GitHub does *not* present comments attached to
individual commit diffs, which is certainly a downgrade from Phab.
>> I'm not sure how this differs from having multiple patches up for review
>> simultaneously. I do that all the time and is just as easy (for me)
>> with GitHub PRs. Are you simply saying that you can have multiple patch
>> series up for review at the same time? I feel like I am not grasping
>> some (to me) subtle point you're making.
>
> I believe he's saying that having patch series grouped together makes it
> easier to think of it as a logical unit. Rather than "I have 10 patches out
> for review" thinking of it as "I have 5 features out for review" -maybe
> one complicated on ein the form of a sries of 5 commits, and a small pair,
> and 4 singles, for instance.
Makes sense, that's what I assumed. I am not sure Phab provides any
benefit over GitHub PRs in this regard.
David Greene via llvm-dev
> It was already mentioned earlier in the thread that:
> - The individual commits cannot be approved in the sense of approving a PR.
Later conversation makes me wonder how often that really happens though.
In a true patch series I would think it would be very rare to be able to
land a later commit before an earlier commit.
> - Comments on individual commits are easily lost.
This is the piece that I feel needs more explanation. What does "lost"
mean, exactly? I will comment more on your explanation below.
> To elaborate on the latter:
> - GitHub would not show comments in-line if it unilaterally believes that
> the comment no longer applies to the version of the code you are looking at.
Ok, but doesn't Phab do the same? Or does Phab attach the comment to
some potentially completely inappropriate line? The latter seems worse
to me.
> - GitHub would proactively hide and collapse comments in the "conversation
> view", especially if it believes (for such bad reasons as line noise in
> later commits) that an earlier comment is not relevant.
I'm reading this as comments aren't "lost" in the sense that they are
completely deleted from the PR. They may be collapsed. What does
"hide" mean? Are they completely inaccessible?
This is an important point. If comments are simply collapsed and I can
easily get at them if I need to, that's not too much of a problem for
me. After all, Phab auto-hides comments all the time if the
conversation gets "too long." I am frequently annoyed by having to
un-collapse them. Sounds like I would be annoyed in the same way with
GitHub, so nothing really gained or lost to me.
David Greene via llvm-dev
It's not quite that simple though. In general most PRs will need to be
rebased just before merging to maintain linear history. What happens
then? Would things need to pass another build before the "final merge?"
If so, then something needs to keep a list of pending PRs waiting for
"final merge." This quickly gets very hairy as pending PRs have to
constantly be rebased and rebuilt as other PRs land.
Phab isn't any better in this regard. I'm just pointing out that
requiring things to build/test before "final merge" is a hard problem
regardless of the code review platform.
-David
Hubert Tong via llvm-dev
Hubert Tong <hubert.rein...@gmail.com> writes:
> The update process in Phab in rather manual and it does lead to more noise.
> The GitHub force push risks loss of context for earlier comments (not just
> in terms of display, like viewing older comments on a later diff in Phab).
> Another issue with the GitHub workflow is that there is no metadata linking
> the corresponding commits between two versions of the same patch series.
> Reviewers looking at a patch-series-as-GitHub-PR that is a refresh of an
> older patch-series-as-GitHub-PR are less likely to notice the previous
> discussion than the corresponding Phab workflow.
I read this as the refresh being an entirely new GitHub PR. Is that
right? Normally I would expect the same PR to be used but the rebase
would cause a force-push of the branch which would update the PR with
the new commits but might lose comments. It's that later part I'm
unsure about. It would seem odd to me to open an entirely new PR due to
a rebase/update of commits to respond to review.
-David
Hubert Tong via llvm-dev
Hubert Tong <hubert.rein...@gmail.com> writes:
> It was already mentioned earlier in the thread that:
> - The individual commits cannot be approved in the sense of approving a PR.
Later conversation makes me wonder how often that really happens though.
In a true patch series I would think it would be very rare to be able to
land a later commit before an earlier commit.
> - Comments on individual commits are easily lost.
This is the piece that I feel needs more explanation. What does "lost"
mean, exactly? I will comment more on your explanation below.
> To elaborate on the latter:
> - GitHub would not show comments in-line if it unilaterally believes that
> the comment no longer applies to the version of the code you are looking at.
Ok, but doesn't Phab do the same? Or does Phab attach the comment to
some potentially completely inappropriate line? The latter seems worse
to me.
> - GitHub would proactively hide and collapse comments in the "conversation
> view", especially if it believes (for such bad reasons as line noise in
> later commits) that an earlier comment is not relevant.
I'm reading this as comments aren't "lost" in the sense that they are
completely deleted from the PR.
They may be collapsed. What does
"hide" mean? Are they completely inaccessible?
This is an important point. If comments are simply collapsed and I can
easily get at them if I need to, that's not too much of a problem for
me.
After all, Phab auto-hides comments all the time if the
conversation gets "too long."
I am frequently annoyed by having to
un-collapse them. Sounds like I would be annoyed in the same way with
GitHub, so nothing really gained or lost to me.
-David
Christian Kühnel via llvm-dev
Christian Kühnel via llvm-dev <llvm...@lists.llvm.org> writes:
>>> In Github pull requests there is always a git commit that you can just
>>> feed to the build server. And you can be sure of what really gets merged.
>>> You review, build and test exactly the change that gets merged afterwards.
>>>
>>
>> How would that be true? Given that upstream keep changing during the
>> period of review? The commit is going to have to be rebased to head and
>> that may involve making changes.
>>
>
> Yes, github tells you, that your PR has a merge conflict, that you need to
> resolve manually. Once you've pushed the conflict resolution to the same
> PR, it triggers another round of builds and tests. And also potentially
> another review, depending on what permissions you have and how the project
> ist set up...
It's not quite that simple though. In general most PRs will need to be
rebased just before merging to maintain linear history.
What happens
then? Would things need to pass another build before the "final merge?"
If so, then something needs to keep a list of pending PRs waiting for
"final merge." This quickly gets very hairy as pending PRs have to
constantly be rebased and rebuilt as other PRs land.
David Greene via llvm-dev
>> I read this as the refresh being an entirely new GitHub PR. Is that
>> right? Normally I would expect the same PR to be used but the rebase
>> would cause a force-push of the branch which would update the PR with
>> the new commits but might lose comments. It's that later part I'm
>> unsure about. It would seem odd to me to open an entirely new PR due to
>> a rebase/update of commits to respond to review.
>>
> Use of force push damages the ability to retrieve context on older
> comments. I am not sure of the reason for the case I observed, but the
> context vanished within a week in one instance.
Does "vanish" mean it was completely gone, or just hidden in some way?
David Greene via llvm-dev
> Uncollapsing in Phab has been much easier for me than on GitHub. Also, for
> similarly sized reviews, I've had to uncollapse on GitHub more often than
> on Phab.
Thanks for relating your experience, it's very helpful! I will play
with GitHub a bit and see what it does. From your description it does
sound like Phab does a better job of keeping comments more accessible.
Is that worth the cost of maintaining a server/software changes and
using a very niche product with not as much general familiarity and
tooling around it? I don't know.
David Greene via llvm-dev
>> It's not quite that simple though. In general most PRs will need to be
>> rebased just before merging to maintain linear history.
>
>
> That depends on how frequent merge conflicts are. In my other projects that
> wasn't really an issue. I rarely had to rebase something manually.
It has little to do with merge conflicts. Because LLVM requires
fast-forward merges, everything has to be rebased after something else
gets merged.
>> What happens then? Would things need to pass another build before
>> the "final merge?"
>>
>> If so, then something needs to keep a list of pending PRs waiting for
>> "final merge." This quickly gets very hairy as pending PRs have to
>> constantly be rebased and rebuilt as other PRs land.
>
> Some projects have a flag they set and let some bot do the manual work:
> Rebase, re-test and then automatically merge.
> I do not expect this to be perfect, it should rather be much better than
> today in slipping new bugs into master.
I agree the process can be improved. But at some point builders are
going to be backed up as each PR gets rebased and then rebuilt just
before merging. Probably we will have to punt an assume that if the
rebase is clean, it can just be merged in without another build. We
will miss some small cases where non-conflicting code interacts in bad
ways but as you said, no system is perfect.
Hubert Tong via llvm-dev
Hubert Tong <hubert.rein...@gmail.com> writes:
>> I read this as the refresh being an entirely new GitHub PR. Is that
>> right? Normally I would expect the same PR to be used but the rebase
>> would cause a force-push of the branch which would update the PR with
>> the new commits but might lose comments. It's that later part I'm
>> unsure about. It would seem odd to me to open an entirely new PR due to
>> a rebase/update of commits to respond to review.
>>
> Use of force push damages the ability to retrieve context on older
> comments. I am not sure of the reason for the case I observed, but the
> context vanished within a week in one instance.
Does "vanish" mean it was completely gone, or just hidden in some way?
-David
Christian Kühnel via llvm-dev
Christian Kühnel <kuh...@google.com> writes:
>> It's not quite that simple though. In general most PRs will need to be
>> rebased just before merging to maintain linear history.
>
>
> That depends on how frequent merge conflicts are. In my other projects that
> wasn't really an issue. I rarely had to rebase something manually.
It has little to do with merge conflicts. Because LLVM requires
fast-forward merges, everything has to be rebased after something else
gets merged.
>> What happens then? Would things need to pass another build before
>> the "final merge?"
>>
>> If so, then something needs to keep a list of pending PRs waiting for
>> "final merge." This quickly gets very hairy as pending PRs have to
>> constantly be rebased and rebuilt as other PRs land.
>
> Some projects have a flag they set and let some bot do the manual work:
> Rebase, re-test and then automatically merge.
> I do not expect this to be perfect, it should rather be much better than
> today in slipping new bugs into master.
I agree the process can be improved. But at some point builders are
going to be backed up as each PR gets rebased and then rebuilt just
before merging. Probably we will have to punt an assume that if the
rebase is clean, it can just be merged in without another build. We
will miss some small cases where non-conflicting code interacts in bad
ways but as you said, no system is perfect.
Fedor Sergeev via llvm-dev
is the most
problematic GitHub PR =<= Phab difference in terms of review process!
Summarizing, I see two main pain points in GitHub PR experience as
explained here
that beg for improvement:
1. low information density of "conversation" view
2. inconvenient navigation/discovery of comments for multiple commits
in presence of force push
As I see it, these two points will constantly get in the way of review
process for nontrivial
patch series (and somehow I believe that quality of review for
nontrivial patches will have the
biggest impact on the quality of the project itself).
Do we have a good reason to believe that LLVM as a community has enough
weight
to nudge Github to really improve on these two points?
If yes, then can we start moving there? File bugs or whatnot...
It would be a very nontrivial effort for me to even describe the problem
in comprehensible
(and non-offensive ;) way, so I know I'm asking somebody else to do the
hard job.
best regards,
Fedor.
On 1/23/20 3:02 AM, Hubert Tong via llvm-dev wrote:
> On Wed, Jan 22, 2020 at 5:19 PM David Greene <gre...@obbligato.org
> <mailto:gre...@obbligato.org>> wrote:
>
> Hubert Tong <hubert.rein...@gmail.com
David Greene via llvm-dev
> Do we have a good reason to believe that LLVM as a community has
> enough weight to nudge Github to really improve on these two points?
I think we have reason to at least try. They've implemented other stuff
for us.
> If yes, then can we start moving there? File bugs or whatnot... It
> would be a very nontrivial effort for me to even describe the problem
> in comprehensible (and non-offensive ;) way, so I know I'm asking
> somebody else to do the hard job.
My guess is someone with greater public visibility/clout than myself
filing bugs and pushing this along would make better progress.
So yeah, I know I'm also asking someone else to do the hard job. But I
am happy to help in any way I can.
I did find these. This is an unofficial issue tracker for github.com.
The official way to communicate is apparently e-mailing
sup...@github.com, which is not very great. People who send the e-mail
are also posting to this issue tracker so others know what has been sent
and whether there have been any responses from GitHub.
Ability to approve each commit in a PR
https://github.com/isaacs/github/issues/1376
Do not auto dismiss PR comments on code change
https://github.com/isaacs/github/issues/1159
Partially addressed:
https://github.blog/changelog/2018-09-25-precision-review-comment-outdating/
Mark pull request as depending on another
https://github.com/isaacs/github/issues/959
Some workarounds:
https://github.com/ryanhiebert/probot-chain
https://github.com/z0al/dep
And maybe official support:
https://twitter.com/natfriedman/status/1170804894241972224
https://unhashable.com/stacked-pull-requests-keeping-github-diffs-small/
Daniel Sanders via llvm-dev
> On Jan 23, 2020, at 08:37, David Greene via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> Hubert Tong <hubert.rein...@gmail.com> writes:
>
>>> I read this as the refresh being an entirely new GitHub PR. Is that
>>> right? Normally I would expect the same PR to be used but the rebase
>>> would cause a force-push of the branch which would update the PR with
>>> the new commits but might lose comments. It's that later part I'm
>>> unsure about. It would seem odd to me to open an entirely new PR due to
>>> a rebase/update of commits to respond to review.
>>>
>> Use of force push damages the ability to retrieve context on older
>> comments. I am not sure of the reason for the case I observed, but the
>> context vanished within a week in one instance.
>
> Does "vanish" mean it was completely gone, or just hidden in some way?
I can provide a concrete example as I ran into this recently at https://github.com/pygments/pygments/pull/1361. If you scroll down that PR you'll see an entry 'pygments/lexers/asm.py Show resolved' (actually there's two, it doesn't matter which you pick). If you expand that by clicking 'Show Resolved' you'll see a small amount of context along with the reviewers comment. However, if you click the filename to look at the whole file you end up at https://github.com/pygments/pygments/pull/1361/files/626154e9498271420b5fddf54910f332d90626a6 which shows 'We went looking everywhere, but couldn’t find those commits.'. GitHub has already removed the previous version of the patch that the review comments referred to. The commit disappeared almost immediately after my force push.