Open Stack

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.

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

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

> 
>  >> 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.
>>>>
>>>> I don't understand why this would even be in the domain of git-review. A
>>>> submitter can do the "puff magic" stuff himself using basic Git commands
>>>> before he submits the collapsed commit.
>>>>
>>>
>>> Isn't it the domain of git-review - "puff magic"? You can upload your
>>> changes with 'git push HEAD:refs/for/master', you can do all your
>> rebasing
>>> by yourself, but somehow we ended up with this tool that simplifies
>> common
>>> tasks related to uploading changes to Gerrit.
>>> And (at least for some) such change would simplify their day-to-day
>>> workflow with regards to uploading changes to Gerrit.
>>>
>>
>>
> 
>