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

[David Greene via llvm-dev]

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