[llvm-dev] Phabricator -> GitHub PRs?

Renato Golin via llvm-dev llvm-dev at lists.llvm.org
Wed Jan 15 15:28:59 PST 2020


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. 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.

--renato


More information about the llvm-dev mailing list