Open Stack

On 08/06/2014 05:35 PM, Yuriy Taraday wrote:
> Oh, looks like we got a bit of a race condition in messages. I hope you
> don't mind.
> 
> 
> On Wed, Aug 6, 2014 at 11:00 PM, Ben Nemec <openstack at nemebean.com> wrote:
> 
>> On 08/06/2014 01:42 PM, Yuriy Taraday wrote:
>>> On Wed, Aug 6, 2014 at 6:20 PM, Ben Nemec <openstack at nemebean.com>
>> wrote:
>>>
>>>> On 08/06/2014 03:35 AM, Yuriy Taraday wrote:
>>>>> I'd like to stress this to everyone: I DO NOT propose squashing
>> together
>>>>> commits that should belong to separate change requests. I DO NOT
>> propose
>>>> to
>>>>> upload all your changes at once. I DO propose letting developers to
>> keep
>>>>> local history of all iterations they have with a change request. The
>>>>> history that absolutely doesn't matter to anyone but this developer.
>>>>
>>>> Right, I understand that may not be the intent, but it's almost
>>>> certainly going to be the end result.  You can't control how people are
>>>> going to use this feature, and history suggests if it can be abused, it
>>>> will be.
>>>>
>>>
>>> Can you please outline the abuse scenario that isn't present nowadays?
>>> People upload huge changes and are encouraged to split them during
>> review.
>>> The same will happen within proposed workflow. More experienced
>> developers
>>> split their change into a set of change requests. The very same will
>> happen
>>> within proposed workflow.
>>
>> There will be a documented option in git-review that automatically
>> squashes all commits.  People _will_ use that incorrectly because from a
>> submitter perspective it's easier to deal with one review than multiple,
>> but from a reviewer perspective it's exactly the opposite.
>>
> 
> It won't be documented as such. It will include "use with care" and "years
> of Git experience: 3+" stickers. Autosquashing will never be mentioned
> there. Only a detailed explanation of how to work with it and (probably)
> how it works. No rogue dev will get through it without getting the true
> understanding.
> 
> By the way, currently git-review suggests to squash your outstanding
> commits but there is no overwhelming flow of overly huge change requests,
> is there?
> 
>>>> On Wed, Aug 6, 2014 at 12:03 PM, Martin Geisler <martin at geisler.net>
>>>> wrote:
>>>>>
>>>>>> Ben Nemec <openstack at nemebean.com> writes:
>>>>>>
>>>>>>> On 08/05/2014 03:14 PM, Yuriy Taraday wrote:
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Yeah, I would never keep broken or unfinished commits around like
>> this.
>>>>>> In my opinion (as a core Mercurial developer), the best workflow is to
>>>>>> work on a feature and make small and large commits as you go along.
>> When
>>>>>> the feature works, you begin squashing/splitting the commits to make
>>>>>> them into logical pieces, if they aren't already in good shape. You
>> then
>>>>>> submit the branch for review and iterate on it until it is accepted.
>>>>>>
>>>>>
>>>>> Absolutely true. And it's mostly the same workflow that happens in
>>>>> OpenStack: you do your cool feature, you carve meaningful small
>>>>> self-contained pieces out of it, you submit series of change requests.
>>>>> And nothing in my proposal conflicts with it. It just provides a way to
>>>>> make developer's side of this simpler (which is the intent of
>> git-review,
>>>>> isn't it?) while not changing external artifacts of one's work: the
>> same
>>>>> change requests, with the same granularity.
>>>>>
>>>>>
>>>>>> As a reviewer, it cannot be stressed enough how much small, atomic,
>>>>>> commits help. Squashing things together into large commits make
>> reviews
>>>>>> very tricky and removes the possibility of me accepting a later commit
>>>>>> while still discussing or rejecting earlier commits (cherry-picking).
>>>>>>
>>>>>
>>>>> That's true, too. But please don't think I'm proposing to squash
>>>> everything
>>>>> together and push 10k-loc patches. I hate that, too. I'm proposing to
>> let
>>>>> developer use one's tools (Git) in a simpler way.
>>>>> And the simpler way (for some of us) would be to have one local branch
>>>> for
>>>>> every change request, not one branch for the whole series. Switching
>>>>> between branches is very well supported by Git and doesn't require
>> extra
>>>>> thinking. Jumping around in detached HEAD state and editing commits
>>>> during
>>>>> rebase requires remembering all those small details.
>>>>>
>>>>>> 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.
>>>>>>
>>>>>> I agree. The conflicts you talk about are intrinsic to the parallel
>>>>>> development. Doing a rebase is equivalent to doing a series of merges,
>>>>>> so if rebase gives you conflicts, you can be near certain that a plain
>>>>>> merge would give you conflicts too. The same applies other way around.
>>>>>>
>>>>>
>>>>> You disregard other issues that can happen with patch series. You might
>>>>> need something more that rebase. You might need to fix something. You
>>>> might
>>>>> need to focus on the one commit in the middle and do huge bunch of
>>>> changes
>>>>> in it alone. And I propose to just allow developer to keep track of
>>>> what's
>>>>> one been doing instead of forcing one to remember all of this.
>>>>
>>>> This is a separate issue though.  Editing a commit in the middle of a
>>>> series doesn't have to be done at the same time as a rebase to master.
>>>>
>>>
>>> No, this will be done with a separate interactive rebase or that detached
>>> HEAD and reflog dance. I don't see this as smth clearer than doing proper
>>> commits in a separate branches.
>>
>> You keep mentioning detached HEAD and reflog.  I have never had to deal
>> with either when doing a rebase, so I think there's a disconnect here.
>> The only time I see a detached HEAD is when I check out a change from
>> Gerrit (and I immediately stick it in a local branch, so it's a
>> transitive state), and the reflog is basically a safety net for when I
>> horribly botch something, not a standard tool that I use on a daily basis.
>>
> 
> It usually takes some time for me to build trust in utility that does a lot
> of different things at once while I need only one small piece of that. So I
> usually do smth like:
> $ git checkout HEAD~2
> $ vim
> $ git commit
> $ git checkout mybranch
> $ git rebase --onto HEAD@{1} HEAD~2
> instead of almost the same workflow with interactive rebase.

I'm sorry, but "I don't trust the well-tested, widely used tool that Git
provides to make this easier so I'm going to reimplement essentially the
same thing in a messier way myself" is a non-starter for me.  I'm not
surprised you dislike rebases if you're doing this, but it's a solved
problem.  Use git rebase -i.

I think I've said all I'm going to say on this.

> 
>> In fact, not having a bunch of small broken commits that can't be
>>>> submitted individually in your history makes it _easier_ to deal with
>>>> follow-up changes.  Then you know that the unit tests pass on every
>>>> commit, so you can work on it in isolation without constantly having to
>>>> rebase through your entire commit history.  This workflow seems to
>>>> encourage the painful rebases you're trying to avoid.
>>>>
>>>
>>> No, this workflow encourage using merges instead of rebases. You don't
>> need
>>> to rebase anything.
>>
>> /shrug
>>
>> The end result of both is the same (your change applied to master), but
>> the merge version leaves your local repo a mess with a merge commit that
>> might be overwriting things from your previous commits, but you won't
>> know just by looking at one in isolation.
>>
> 
> Just two commands:
> $ git diff HEAD~
> will show you what had changed in your branch with this merge;
> $ git diff HEAD^2
> will show your new diff against master.
> You won't be able to do both after rebase without scratching some commit ID
> in your notebook (or a temporary branch).
>