On Jul 28, 2011, at 2:16 PM, David A. Greene wrote:>>> The flow promoted by Git is precisely to make sure each and every commit >>> passes the tests. So, the granularity of "incremental development" is >>> really the commit, not how often you merge. >> This model is based on the idea of some trusted maintainer doing code >> review of the branch and then pushing it to mainline as a huge batch >> when everything is happy and working. This is fundamentally in >> tension with the LLVM model and I have no desire for us to switch. > > Ah. Here is where I think we have some miscommunication happening. > > Here's how I am thinking about things. Reviews should keep happening > the way they are.Agreed.> In fact, what we are doing now is exactly the same > work one would have to do to review a git branch, but that's immaterial.Disagreed. The point is that I should not see a stream of 20 decomposed patches from you. When I get to one that is "wrong" or needs changes (e.g. patch 6), then all the other patches after it get ignored. This is silly. Instead, what I should see are (for example) and be asked to review them. Here's a hypothetical example of what I want: Send in or directly commit 5 patches if they are "obvious" cleanups. These are patches that just move things around in obvious ways, fix formatting, whatever. Send in a patch 6 and wait for review of *just it* saying "this does something crazy, here is why, here is why it is the best thing to do". Someone will review it, and if there are any problems, it goes back to you for revision. It eventually goes in when it is good for the tree. After that patch is in, another stream of obvious patches are unblocked and go directly in. Patch 12 comes around, gets reviewed just like #6... etc. I *don't* want to see all 20 patches up front.> I think the disagreement (if we can call it that) is really over what > actually gets pushed to "mainline" which I assume would be defined as > "master" on llvm.org.No. The fundamental disagreement/misunderstanding here is that you are optimizing for an out-of-tree maintainer pushing "batches" of work upstream. This is not what I'm at all interesting in optimizing for, and if you're irritated about the turn-around time that it takes to get review, then you shouldn't be optimizing for this either. -Chris
Chris Lattner <clattner at apple.com> writes:> On Jul 28, 2011, at 2:16 PM, David A. Greene wrote:> Disagreed. The point is that I should not see a stream of 20 > decomposed patches from you. When I get to one that is "wrong" or > needs changes (e.g. patch 6), then all the other patches after it get > ignored. This is silly.It is silly. I see no reason to simply ignore the later patches unless patch 6 needs a substantial rework.> Instead, what I should see are (for example) and be asked to review > them.Oops, I think something got botched here. I can't parse the above. :)> Here's a hypothetical example of what I want: > > Send in or directly commit 5 patches if they are "obvious" cleanups. > These are patches that just move things around in obvious ways, fix > formatting, whatever.Ok.> Send in a patch 6 and wait for review of *just it* saying "this does > something crazy, here is why, here is why it is the best thing to do". > Someone will review it, and if there are any problems, it goes back to > you for revision. It eventually goes in when it is good for the tree.Ok.> After that patch is in, another stream of obvious patches are unblocked and go directly in.Yep.> Patch 12 comes around, gets reviewed just like #6... etc. > > I *don't* want to see all 20 patches up front.Ok, now I'm understanding you better.> No. The fundamental disagreement/misunderstanding here is that you > are optimizing for an out-of-tree maintainer pushing "batches" of work > upstream. This is not what I'm at all interesting in optimizing for, > and if you're irritated about the turn-around time that it takes to > get review, then you shouldn't be optimizing for this either.Thanks, that clarifies things. So you don't so much demand individual cherry-picks to master as you demand that merges to master be smallish and incremental. I think that is entirely reasonable. With my most recent patch, I submitted something I considered logically connected. I was asked to rework it and break it up. I did that and submitted the resulting patch stream for review. No one seemed to complain about that so I wasn't grokking your dislike of that apporoach. Now I am much more clear about how to use this git thing with LLVM. :) -Dave
Chris Lattner <clattner at apple.com> writes: [snip]> Disagreed. The point is that I should not see a stream of 20 > decomposed patches from you. When I get to one that is "wrong" or > needs changes (e.g. patch 6), then all the other patches after it get > ignored. This is silly. Instead, what I should see are (for example) > and be asked to review them. Here's a hypothetical example of what I > want: > > Send in or directly commit 5 patches if they are "obvious" cleanups. > These are patches that just move things around in obvious ways, fix > formatting, whatever. > > Send in a patch 6 and wait for review of *just it* saying "this does > something crazy, here is why, here is why it is the best thing to do". > Someone will review it, and if there are any problems, it goes back to > you for revision. It eventually goes in when it is good for the tree. > > After that patch is in, another stream of obvious patches are unblocked and go directly in. > > Patch 12 comes around, gets reviewed just like #6... etc. > > I *don't* want to see all 20 patches up front.This is not against git way of doing things. Actually, using git would easy the process for the submitter, as it provides convenient methods for decomposing and combining source code changes. And the reviewer would keep working as he does now.>> I think the disagreement (if we can call it that) is really over what >> actually gets pushed to "mainline" which I assume would be defined as >> "master" on llvm.org. > > No. The fundamental disagreement/misunderstanding here is that you > are optimizing for an out-of-tree maintainer pushing "batches" of work > upstream. This is not what I'm at all interesting in optimizing for, > and if you're irritated about the turn-around time that it takes to > get review, then you shouldn't be optimizing for this either.>From my understanding of David's position, there is no disagreement hereeither. As mentioned above, there is no problem with git for pushing a series of non-controversial changes and then submit for review some others. However, what David (and me) wish to point out is that sometimes presenting a series of patches for review may be also convenient for the reviewer, because he then can say "patch #1 looks fine but it is not really required by what you do in patch #2". Overall, the number of patches submitted for review would be the same as today (letting aside any effect git may induce in the productivity of some developers) but instead of seeing 2 patches from Alice and 2 from Bob today you would see 4 from Alice today and 4 from Bob tomorrow. There are other advantages, like keeping a more cohesive history of changes, and avoiding up to some point erratic development (people who commit preparatory changes too early, and then commit corrections for the previous changes...) but things like that you will observe with practice and always can adapt the micro-polices accordingly. Really, there are no disagreements on the review process, just misunderstandings.
On Jul 29, 2011, at 11:26 AM, David A. Greene wrote:>> Disagreed. The point is that I should not see a stream of 20 >> decomposed patches from you. When I get to one that is "wrong" or >> needs changes (e.g. patch 6), then all the other patches after it get >> ignored. This is silly. > > It is silly. I see no reason to simply ignore the later patches unless > patch 6 needs a substantial rework.Again, I value reviewer time (which is scarce if you haven't noticed) more than patch submitter time. If patch 6 needs rework, it can cause ripple effects through the rest of the patch series. I believe that your recent tblgen changes are a perfect example of this.>> Instead, what I should see are (for example) and be asked to review >> them. > > Oops, I think something got botched here. I can't parse the above. :)Yeah, parse error for me too. :)>> No. The fundamental disagreement/misunderstanding here is that you >> are optimizing for an out-of-tree maintainer pushing "batches" of work >> upstream. This is not what I'm at all interesting in optimizing for, >> and if you're irritated about the turn-around time that it takes to >> get review, then you shouldn't be optimizing for this either. > > Thanks, that clarifies things. So you don't so much demand individual > cherry-picks to master as you demand that merges to master be smallish > and incremental. I think that is entirely reasonable.Right.> With my most recent patch, I submitted something I considered logically > connected. I was asked to rework it and break it up. I did that and > submitted the resulting patch stream for review. No one seemed to > complain about that so I wasn't grokking your dislike of that apporoach. > Now I am much more clear about how to use this git thing with LLVM. :)Great, thanks. -Chris