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

[Joerg Sonnenberger via llvm-dev]

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