<div dir="ltr">Oh, looks like we got a bit of a race condition in messages. I hope you don't mind.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 6, 2014 at 11:00 PM, Ben Nemec <span dir="ltr"><<a href="mailto:openstack@nemebean.com" target="_blank">openstack@nemebean.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 08/06/2014 01:42 PM, Yuriy Taraday wrote:<br>
> On Wed, Aug 6, 2014 at 6:20 PM, Ben Nemec <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>> wrote:<br>
><br>
>> On 08/06/2014 03:35 AM, Yuriy Taraday wrote:<br>
>>> I'd like to stress this to everyone: I DO NOT propose squashing together<br>
>>> commits that should belong to separate change requests. I DO NOT propose<br>
>> to<br>
>>> upload all your changes at once. I DO propose letting developers to keep<br>
>>> local history of all iterations they have with a change request. The<br>
>>> history that absolutely doesn't matter to anyone but this developer.<br>
>><br>
>> Right, I understand that may not be the intent, but it's almost<br>
>> certainly going to be the end result.  You can't control how people are<br>
>> going to use this feature, and history suggests if it can be abused, it<br>
>> will be.<br>
>><br>
><br>
> Can you please outline the abuse scenario that isn't present nowadays?<br>
> People upload huge changes and are encouraged to split them during review.<br>
> The same will happen within proposed workflow. More experienced developers<br>
> split their change into a set of change requests. The very same will happen<br>
> within proposed workflow.<br>
<br>
</div>There will be a documented option in git-review that automatically<br>
squashes all commits.  People _will_ use that incorrectly because from a<br>
submitter perspective it's easier to deal with one review than multiple,<br>
but from a reviewer perspective it's exactly the opposite.<br></blockquote><div><br></div><div style>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.</div>

<div style><br></div><div style>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?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div><div class="h5">>>> On Wed, Aug 6, 2014 at 12:03 PM, Martin Geisler <<a href="mailto:martin@geisler.net">martin@geisler.net</a>><br>
>> wrote:<br>
>>><br>
>>>> Ben Nemec <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>> writes:<br>
>>>><br>
>>>>> On 08/05/2014 03:14 PM, Yuriy Taraday wrote:<br>
>>>>>><br>
>>>>>> When you're developing some big change you'll end up with trying<br>
>>>>>> dozens of different approaches and make thousands of mistakes. For<br>
>>>>>> reviewers this is just unnecessary noise (commit title "Scratch my<br>
>>>>>> last CR, that was bullshit") while for you it's a precious history<br>
>>>>>> that can provide basis for future research or bug-hunting.<br>
>>>>><br>
>>>>> So basically keeping a record of how not to do it?  I get that, but I<br>
>>>>> think I'm more onboard with the suggestion of sticking those dead end<br>
>>>>> changes into a separate branch.  There's no particular reason to keep<br>
>>>>> them on your working branch anyway since they'll never merge to master.<br>
>>>>>  They're basically unnecessary conflicts waiting to happen.<br>
>>>><br>
>>>> Yeah, I would never keep broken or unfinished commits around like this.<br>
>>>> In my opinion (as a core Mercurial developer), the best workflow is to<br>
>>>> work on a feature and make small and large commits as you go along. When<br>
>>>> the feature works, you begin squashing/splitting the commits to make<br>
>>>> them into logical pieces, if they aren't already in good shape. You then<br>
>>>> submit the branch for review and iterate on it until it is accepted.<br>
>>>><br>
>>><br>
>>> Absolutely true. And it's mostly the same workflow that happens in<br>
>>> OpenStack: you do your cool feature, you carve meaningful small<br>
>>> self-contained pieces out of it, you submit series of change requests.<br>
>>> And nothing in my proposal conflicts with it. It just provides a way to<br>
>>> make developer's side of this simpler (which is the intent of git-review,<br>
>>> isn't it?) while not changing external artifacts of one's work: the same<br>
>>> change requests, with the same granularity.<br>
>>><br>
>>><br>
>>>> As a reviewer, it cannot be stressed enough how much small, atomic,<br>
>>>> commits help. Squashing things together into large commits make reviews<br>
>>>> very tricky and removes the possibility of me accepting a later commit<br>
>>>> while still discussing or rejecting earlier commits (cherry-picking).<br>
>>>><br>
>>><br>
>>> That's true, too. But please don't think I'm proposing to squash<br>
>> everything<br>
>>> together and push 10k-loc patches. I hate that, too. I'm proposing to let<br>
>>> developer use one's tools (Git) in a simpler way.<br>
>>> And the simpler way (for some of us) would be to have one local branch<br>
>> for<br>
>>> every change request, not one branch for the whole series. Switching<br>
>>> between branches is very well supported by Git and doesn't require extra<br>
>>> thinking. Jumping around in detached HEAD state and editing commits<br>
>> during<br>
>>> rebase requires remembering all those small details.<br>
>>><br>
>>>> FWIW, I have had long-lived patch series, and I don't really see what<br>
>>>>> is so difficult about running git rebase master. Other than conflicts,<br>
>>>>> of course, which are going to be an issue with any long-running change<br>
>>>>> no matter how it's submitted. There isn't a ton of git magic involved.<br>
>>>><br>
>>>> I agree. The conflicts you talk about are intrinsic to the parallel<br>
>>>> development. Doing a rebase is equivalent to doing a series of merges,<br>
>>>> so if rebase gives you conflicts, you can be near certain that a plain<br>
>>>> merge would give you conflicts too. The same applies other way around.<br>
>>>><br>
>>><br>
>>> You disregard other issues that can happen with patch series. You might<br>
>>> need something more that rebase. You might need to fix something. You<br>
>> might<br>
>>> need to focus on the one commit in the middle and do huge bunch of<br>
>> changes<br>
>>> in it alone. And I propose to just allow developer to keep track of<br>
>> what's<br>
>>> one been doing instead of forcing one to remember all of this.<br>
>><br>
>> This is a separate issue though.  Editing a commit in the middle of a<br>
>> series doesn't have to be done at the same time as a rebase to master.<br>
>><br>
><br>
> No, this will be done with a separate interactive rebase or that detached<br>
> HEAD and reflog dance. I don't see this as smth clearer than doing proper<br>
> commits in a separate branches.<br>
<br>
</div></div>You keep mentioning detached HEAD and reflog.  I have never had to deal<br>
with either when doing a rebase, so I think there's a disconnect here.<br>
The only time I see a detached HEAD is when I check out a change from<br>
Gerrit (and I immediately stick it in a local branch, so it's a<br>
transitive state), and the reflog is basically a safety net for when I<br>
horribly botch something, not a standard tool that I use on a daily basis.<br></blockquote><div><br></div><div style>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:</div>

<div style>$ git checkout HEAD~2</div><div style>$ vim</div><div style>$ git commit</div><div style>$ git checkout mybranch</div><div style>$ git rebase --onto HEAD@{1} HEAD~2</div><div style>instead of almost the same workflow with interactive rebase.</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> In fact, not having a bunch of small broken commits that can't be<br>
>> submitted individually in your history makes it _easier_ to deal with<br>
>> follow-up changes.  Then you know that the unit tests pass on every<br>
>> commit, so you can work on it in isolation without constantly having to<br>
>> rebase through your entire commit history.  This workflow seems to<br>
>> encourage the painful rebases you're trying to avoid.<br>
>><br>
><br>
> No, this workflow encourage using merges instead of rebases. You don't need<br>
> to rebase anything.<br>
<br>
</div>/shrug<br>
<br>
The end result of both is the same (your change applied to master), but<br>
the merge version leaves your local repo a mess with a merge commit that<br>
might be overwriting things from your previous commits, but you won't<br>
know just by looking at one in isolation.<br></blockquote><div><br></div><div style>Just two commands:</div><div style>$ git diff HEAD~</div><div style>will show you what had changed in your branch with this merge;</div>

<div style>$ git diff HEAD^2</div><div style>will show your new diff against master.</div><div style>You won't be able to do both after rebase without scratching some commit ID in your notebook (or a temporary branch).</div>

</div><div><br></div>-- <br><br><div>Kind regards, Yuriy.</div>
</div></div>