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
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/llvm-dev/attachments/20200116/51155f2a/attachment-0001.html>
On Thu, 16 Jan 2020 at 18:45, David Blaikie <dblaikie at gmail.com> wrote:> 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.Perhaps it's my confusion in interpreting the answers. Some comments mentioned "if a review spawns a related change", which to me is a different review, and I've reviewed many of those cases. Most people use the Phab link to express relationship, but that's not a series. Others said "some patches may be approved before others" which only works in a series if they're from start to N, not N to M, nor M to the end, nor randomly approved. In this case, the series is split in two, with the latter having to be rebased on patches committed after the first part is, so essentially, creating a new series. Doing both these cases as a pull request is trivial. Related changes become separate PRs with mention. GitHub creates the links, like Phab if you tag on the commit message or comments. Series split is harder, but still trivial. You create a new branch, move up to the approved patch, push. Rebase your old branch and you have a new series. In Github you can choose to close the PR and open a new one, or just push again and the UI should update the range. I prefer the former, because you keep the comments history. I would have done the same process in Phab, but with more clicks, uploads, links, etc. --renato