Open Stack

On 08/05/2014 03:14 PM, Yuriy Taraday wrote:
> On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec <openstack at nemebean.com> wrote:
> 
>> On 08/05/2014 10:51 AM, ZZelle wrote:
>>> Hi,
>>>
>>>
>>> I like the idea  ... with complex change, it could useful for the
>>> understanding to split it into smaller changes during development.
>>
>> I don't understand this.  If it's a complex change that you need
>> multiple commits to keep track of locally, why wouldn't reviewers want
>> the same thing?  Squashing a bunch of commits together solely so you
>> have one review for Gerrit isn't a good thing.  Is it just the warning
>> message that git-review prints when you try to push multiple commits
>> that is the problem here?
> 
> 
> When you're developing some big change you'll end up with trying dozens of
> different approaches and make thousands of mistakes. For reviewers this is
> just unnecessary noise (commit title "Scratch my last CR, that was
> bullshit") while for you it's a precious history that can provide basis for
> future research or bug-hunting.

So basically keeping a record of how not to do it?  I get that, but I
think I'm more onboard with the suggestion of sticking those dead end
changes into a separate branch.  There's no particular reason to keep
them on your working branch anyway since they'll never merge to master.
 They're basically unnecessary conflicts waiting to happen.

> 
> Merges are one of the strong sides of Git itself (and keeping them very
> easy is one of the founding principles behind it). With current workflow we
> don't use them at all. master went too far forward? You have to do rebase
> and screw all your local history and most likely squash everything anyway
> because you don't want to fix commits with known bugs in them. With
> proposed feature you can just do merge once and let 'git review' add some
> magic without ever hurting your code.

How do rebases screw up your local history?  All your commits are still
there after a rebase, they just have a different parent.  I also don't
see how rebases are all that much worse than merges.  If there are no
conflicts, rebases are trivial.  If there are conflicts, you'd have to
resolve them either way.

I also reiterate my point about not keeping broken commits on your
working branch.  You know at some point they're going to get
accidentally submitted. :-)

As far as letting git review do magic, how is that better than "git
rebase once and no magic required"?  You deal with the conflicts and
you're good to go.  And if someone asks you to split a commit, you can
do it.  With this proposal you can't, because anything but squashing
into one commit is going to be a nightmare (which might be my biggest
argument against this).

> 
> And speaking about breaking down of change requests don't forget support
> for change requests chains that this feature would lead to. How to you deal
> with 5 consecutive change request that are up on review for half a year?
> The only way I could suggest to my colleague at a time was "Erm... Learn
> Git and dance with rebases, detached heads and reflogs!" My proposal might
> take care of that too.
> 

How does this relate to commit series?  Squashing all the commits into
one isn't a solution to any of the problems with those (if it were, we
could do that today :-).

FWIW, I have had long-lived patch series, and I don't really see what is
so difficult about running git rebase master.  Other than conflicts, of
course, which are going to be an issue with any long-running change no
matter how it's submitted.  There isn't a ton of git magic involved.

So as you may have guessed by now, I'm opposed to adding this to
git-review.  I think it's going to encourage bad committer behavior
(monolithic commits) and doesn't address a use case I find compelling
enough to offset that concern.

/wall-o-text

-Ben