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

[David Blaikie via llvm-dev]

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>