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
On Thu, Jan 16, 2020 at 11:00 AM Renato Golin <rengolin at gmail.com> wrote:> 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. >Can you point to examples of that - where Phab links have been used to express non-mechanically-dependent patches?> 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. >Approval order isn't commit order - I'm more than happy to approve a later patch in a series, even if the review of an earlier patch necessitates some rework in a later patch - such as renaming a function. The later patch I already approved must be updated to use the new name of the function, but I'm fine with that, same as I would be if it'd been an independent change that did the rename in the time it took us to review that patch.> Doing both these cases as a pull request is trivial. > > Related changes become separate PRs with mention.What do you mean by "with mention" and what do you mean by "related" (I guess you man "not dependent, but interesting to consider together")> 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. >My understanding is that this ^ is the case we're talking about with Phab patch series. Related but independent patches aren't "interesting", as you say - just mention them in passing in the review. My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review feedback and updates?> 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. >Perhaps there's some misunderstanding/different experiences there (as there is with Phab). Maybe you could make a little example? Showing how a dependent patch series with ongoing review feedback works with github PRs?> > I would have done the same process in Phab, but with more clicks, > uploads, links, etc. > > --renato >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/0fbc7eec/attachment.html>
On Thu, 16 Jan 2020 at 19:10, David Blaikie <dblaikie at gmail.com> wrote:> Can you point to examples of that - where Phab links have been used to express non-mechanically-dependent patches?Not at the top of my head, but since that's not what we're talking about, I'll go to the next point.> Approval order isn't commit order - I'm more than happy to approve a later patch in a series, even if the review of an earlier patch necessitates some rework in a later patch - such as renaming a function. The later patch I already approved must be updated to use the new name of the function, but I'm fine with that, same as I would be if it'd been an independent change that did the rename in the time it took us to review that patch.I see what you mean. Once all are approved, the commit happens, but they can be approved in any order. I used to do that too, but I honestly don't like it. Earlier patches can significantly change the following changes, then the approval has to be reverted. Nowadays, on both Phab and Git I just say "LGTM once the others are approved as is" and once I approve the earlier ones, I scan the remaining series and approve them all. I think the process of uploading multiple commits in separate and then creating a link from each one to the next is cumbersome and error prone (to me particularly, I make many mistakes).> What do you mean by "with mention" and what do you mean by "related" (I guess you man "not dependent, but interesting to consider together")When you write a ticket number, either in phab (D12345) or GitHub (#123), the UI creates a link to the mentioned issue/review. In Github, if you say things like "fixes #123", it closes automatically (which is not necessarily right, like "partially fixes #123" :).> My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review feedback and updates?Both Phab and GitHub are problematic in different ways. In Phab, an earlier change that trickles to the rest of the series needs to be updated on all patches. In GitHub, some information is lost, especially if it's a force push, but it only sends one email for the series.> Perhaps there's some misunderstanding/different experiences there (as there is with Phab). Maybe you could make a little example? Showing how a dependent patch series with ongoing review feedback works with github PRs?There's probably a better such example on the internet. I ended up dragging myself into this corner, but I'm not a defender of GitHub PRs. I just dislike Phab more than it. ;) --renato