llvm dev - Jan 2020 - [cfe-dev] Phabricator -> GitHub PRs?

On 1/15/20 6:50 PM, David Greene wrote:
> "Doerfert, Johannes via llvm-dev" <llvm-dev at
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

Emilio Cobos Álvarez <emilio at 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?  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

On 1/15/20 8:30 PM, David Greene wrote:
> Emilio Cobos Álvarez <emilio at 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

On Wed, Jan 15, 2020 at 2:31 PM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Emilio Cobos Álvarez <emilio at 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?
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 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/86816441/attachment.html>

On Wed, Jan 15, 2020 at 01:30:34PM -0600, David Greene via cfe-dev
wrote:
> Emilio Cobos Álvarez <emilio at 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