[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Hubert Tong via llvm-dev
llvm-dev at lists.llvm.org
Thu Jan 16 15:24:05 PST 2020
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.
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-dev