llvm dev - Jul 2011 - [LLVMdev] git

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