On Wed, 15 Jan 2020 at 18:55, Nicolai Hähnle <nhaehnle at gmail.com> wrote:> This has simply not been true in my experience. Actually, not having > to re-send a new series is one of the main advantages that > Phabricator-based review has over the original review style for Git, > which is to send patch series via mailing lists.Interesting. If they can be committed separately, why were they a series in the first place? Or was that independent, but related, patches? In this case, they could have been different PRs with mention of each other, no?> It might be the case that you occasionally have series where major > redesigns are required, and then asking for a fresh start makes sense.What I mean by patch series is exactly what you would have in a sanitised git branch: a number of sequential patches that build on each other. You cannot commit patch 3 before 2 as Git wouldn't let you merge. In those cases, if we want to have patch 3 before the series, the author needs to rewrite history (rebase -i), push the patch up and pull into another branch, so that we can have the old tree with -1 patches as the derived series, and the cherry-picked patch merged sooner. But in those cases, the patch would most certainly have to be different, and that would also change the remaining series. So, new patch, new series. Makes sense? --renato
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. (sometimes review might indicate the whole patch series direction is not preferred - in which case, yeah, maybe throwing it out and starting a new thread/set of threads is appropriate - but not always) On Wed, Jan 15, 2020 at 2:16 PM Renato Golin via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Wed, 15 Jan 2020 at 18:55, Nicolai Hähnle <nhaehnle at gmail.com> wrote: > > This has simply not been true in my experience. Actually, not having > > to re-send a new series is one of the main advantages that > > Phabricator-based review has over the original review style for Git, > > which is to send patch series via mailing lists. > > Interesting. If they can be committed separately, why were they a > series in the first place? > > Or was that independent, but related, patches? In this case, they > could have been different PRs with mention of each other, no? > > > > It might be the case that you occasionally have series where major > > redesigns are required, and then asking for a fresh start makes sense. > > What I mean by patch series is exactly what you would have in a > sanitised git branch: a number of sequential patches that build on > each other. You cannot commit patch 3 before 2 as Git wouldn't let you > merge. > > In those cases, if we want to have patch 3 before the series, the > author needs to rewrite history (rebase -i), push the patch up and > pull into another branch, so that we can have the old tree with -1 > patches as the derived series, and the cherry-picked patch merged > sooner. > > But in those cases, the patch would most certainly have to be > different, and that would also change the remaining series. So, new > patch, new series. > > Makes sense? > > --renato > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/5cf9bfe0/attachment.html>
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