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

[David Blaikie via cfe-dev]

On Wed, Jan 15, 2020 at 3:29 PM Renato Golin <rengolin at gmail.com> wrote:

> On Wed, 15 Jan 2020 at 23:19, David Blaikie <dblaikie at gmail.com> wrote:
> > I don't think creating a new thread (or series of N threads) because an
> early patch in a series necessitate some refactoring of later patches is
> ideal - while patch series can/should be avoided where reasonable (& if the
> series is small enough/the framing/design is obvious enough, you can hold
> on to the later patches rather than reviewing them all in parallel with the
> volatility that comes with that approach) - but there are certainly cases
> I've seen where it's helpful to send a dependent series together to help
> frame/justify the goals of the early refactoring patches. If the early
> refactoring requires changes - yeah, you get that done, signed off and
> committed (while possibly doing higher level design review of later patches
> that might be applicable regardless of the early refactoring - like if you
> decide to rename a newly created function in an early patch, that shouldn't
> hurt the review going on in later patches, for instance) & update later
> patches with the knock-on effects. No need to start new mail threads/lose
> review context/etc.
>
> On Git, I normally would have done that on separate, but related,
> branches and pull requests.


If the patches aren't dependent on each other - then that's not, to me, the
situation we're discussing. That situation can/should always be handled as
you say - separate branches/reviews/etc. Maybe related (link to the other
reviews for context/design understanding/etc) but when talking about
reviewing a patch series, at least I assume that means a set of /dependent/
patches, where later patches cannot be committed without earlier patches.


> II find it much easier to work with
> rebases and cherry-picks across branches than on the same branch. A
> rebase -i can turn dreadful if the same part of the code is touched by
> more than one patch, and it can be almost impossible if that pattern
> happens more than once.
>
> In my use of the term, a patch series is a mandated order of directly
> related commits, like you would have in a branch, if the patches are
> logically separated into clear individual steps, for the clarity of
> review.
>
> Perhaps the name clash has caused more trouble than the original
> discussion. If that's not what most people would have thought,
> apologies for the confusion.
>

I'm not sure where the idea that a patch series is anything other than that
^ came from. When I was talking about a patch series, it was/is with that
definition in mind - ordered/dependent commits. I said "dependent series"
to reinforce this idea that the kind of situation I was describing was one
in which later patches in the series depend on the changes in earlier
patches.

- Dave


>
> --renato
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200116/51155f2a/attachment.html>