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

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

Joerg Sonnenberger via cfe-dev <cfe-dev at 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 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

On Thu, Jan 16, 2020 at 10:30 AM David Greene via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Joerg Sonnenberger via cfe-dev <cfe-dev at 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


>
>                      -David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/239a7ecd/attachment.html>

On Thu, Jan 16, 2020 at 1:30 PM David Greene via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Joerg Sonnenberger via cfe-dev <cfe-dev at 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 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).
>
I'm not sure what that the line on "basically independent" is
well-established, but sometimes syntactic merge conflicts on changes that
are related but not semantically ordered occur. For the purposes of
facilitating integration testing, allowing reviewers to reproduce a common
state of the code base, etc. it can be helpful to create a patch series in
Phabricator representing a local rebased linear history. Even if the
submitter has a good solution for merging the changes together whenever
they want to do integration testing, patches/PRs that are "pure" in
terms
of being based on some state of "master" is pushing work on reviewers
who
want to apply the patches to their own trees.

>
>                      -David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/49680d6d/attachment.html>

On Thu, Jan 16, 2020 at 12:29:56PM -0600, David Greene
wrote:
> Joerg Sonnenberger via cfe-dev <cfe-dev at 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 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).
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