<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 6, 2014 at 7:23 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On 08/06/2014 12:41 AM, Yuriy Taraday wrote:<br>
> On Wed, Aug 6, 2014 at 1:17 AM, Ben Nemec <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>> wrote:<br>
><br>
>> On 08/05/2014 03:14 PM, Yuriy Taraday wrote:<br>
>>> On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>><br>
>> wrote:<br>
>>><br>
>>>> On 08/05/2014 10:51 AM, ZZelle wrote:<br>
>>>>> Hi,<br>
>>>>><br>
>>>>><br>
>>>>> I like the idea ... with complex change, it could useful for the<br>
>>>>> understanding to split it into smaller changes during development.<br>
>>>><br>
>>>> I don't understand this. If it's a complex change that you need<br>
>>>> multiple commits to keep track of locally, why wouldn't reviewers want<br>
>>>> the same thing? Squashing a bunch of commits together solely so you<br>
>>>> have one review for Gerrit isn't a good thing. Is it just the warning<br>
>>>> message that git-review prints when you try to push multiple commits<br>
>>>> that is the problem here?<br>
>>><br>
>>><br>
>>> When you're developing some big change you'll end up with trying dozens<br>
>> of<br>
>>> different approaches and make thousands of mistakes. For reviewers this<br>
>> is<br>
>>> just unnecessary noise (commit title "Scratch my last CR, that was<br>
>>> bullshit") while for you it's a precious history that can provide basis<br>
>> for<br>
>>> future research or bug-hunting.<br>
>><br>
>> So basically keeping a record of how not to do it?<br>
><br>
><br>
> Well, yes, you can call version control system "a history of failures".<br>
> Because if there were no failures there would've been one omnipotent commit<br>
> that does everything you want it to.<br>
<br>
</div></div>Ideally, no. In a perfect world every commit would work, so the version<br>
history would be a number of small changes that add up to this great<br>
application. In reality it's a combination of new features, oopses, and<br>
fixes for those oopses. I certainly wouldn't describe it as a history<br>
of failures though. I would hope the majority of commits to our<br>
projects are _not_ failures. :-)<br></blockquote><div><br></div><div style>Well, new features are merged just to be later fixed and refactored - how that's not a failure? And we basically do "keep a record of how not to do it" in our repositories. Why prevent developers do the same on the smaller scale?</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">>> 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>
>><br>
><br>
> The commits themselves are never going to merge to master but that's not<br>
> the only meaning of their life. With current tooling "working branch" ends<br>
> up a patch series that is constantly rewritten with no proper history of<br>
> when did that happen and why. As I said, you can't find roots of bugs in<br>
> your code, you can't dig into old versions of your code (what if you need a<br>
> method that you've already created but removed because of some wrong<br>
> suggestion?).<br>
<br>
</div>You're not going to find the root of a bug in your code by looking at an<br>
old commit that was replaced by some other implementation. If anything,<br>
I see that as more confusing. And if you want to keep old versions of<br>
your code, either push it to Gerrit or create a new branch before<br>
changing it further.<br></blockquote><div><br></div><div style>So you propose two options:</div><div style>- store history of your work within Gerrit's patchsets for each change request, which don't fit "commit often" approach (who'd want to see how I struggle with fixing some bug or write working test?);</div>
<div style>- store history of your work in new branches instead of commits in the same branch, which... is not how Git is supposed to be used.</div><div style>And both this options don't provide any proper way of searching through this history.</div>
<div style><br></div><div style>Have you ever used bisect? Sometimes I find myself wanting to use it instead of manually digging through patchsets in Gerrit to find out which change I made broke some usecase I didn't put in unittests yet.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">> They're basically unnecessary conflicts waiting to happen.<br>
>><br>
><br>
> No. They are your local history. They don't need to be rebased on top of<br>
> master - you can just merge master into your branch and resolve conflicts<br>
> once. After that your "autosquashed" commit will merge clearly back to<br>
> master.<br>
<br>
</div>Then don't rebase them. git checkout -b dead-end and move on. :-)<br></blockquote><div><br></div><div style>I never proposed to rebase anything. I want to use merge instead of rebase.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="">
>>> Merges are one of the strong sides of Git itself (and keeping them very<br>
>>> easy is one of the founding principles behind it). With current workflow<br>
>> we<br>
>>> don't use them at all. master went too far forward? You have to do rebase<br>
>>> and screw all your local history and most likely squash everything anyway<br>
>>> because you don't want to fix commits with known bugs in them. With<br>
>>> proposed feature you can just do merge once and let 'git review' add some<br>
>>> magic without ever hurting your code.<br>
>><br>
>> How do rebases screw up your local history? All your commits are still<br>
>> there after a rebase, they just have a different parent. I also don't<br>
>> see how rebases are all that much worse than merges. If there are no<br>
>> conflicts, rebases are trivial. If there are conflicts, you'd have to<br>
>> resolve them either way.<br>
>><br>
><br>
> Merge is a new commit, new recorded point in history. Rebase is rewriting<br>
> your commit, replacing it with a new one, without any record in history (of<br>
> course there will be a record in reflog but there's not much tooling to<br>
> work with it). Yes, you just apply your patch to a different version of<br>
> master branch. And then fix some conflicts. And then fix some tests. And<br>
> then you end up with totally different commit.<br>
<br>
</div>And with merge commits you end up with a tree that is meaningless except<br>
at the very tail end of the commit series, which I think is the root of<br>
your problems with rebasing.</blockquote><div><br></div><div style>With proposed workflow you don't need to rebase. That's the whole point - let developer keep track of one's own progress, not rewrite history every 5 minutes.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I imagine it would be very painful to work<br>
in a way where the only commit that you can test against is the last one.<br></blockquote><div><br></div><div style>I don't understand how that's painful. You always test only against the last commit in your branch. Or you checkout older commit and test that one.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="">> I totally agree that life's very easy when there's no conflicts and you've<br>
> written all your feature in one go. But that's almost never the true.<br>
><br>
><br>
>> I also reiterate my point about not keeping broken commits on your<br>
>> working branch. You know at some point they're going to get<br>
>> accidentally submitted. :-)<br>
>><br>
><br>
> Well... As long as you use 'git review' to upload CRs, you're safe. If you<br>
> do 'git push gerrit HEAD:refs/for/master' you're screwed. But why would you<br>
> do that?<br>
<br>
</div>Or if you forget this new proposed option to git-review. :-)<br></blockquote><div><br></div><div style>I don't propose it as an option. A concrete config parameter will turn on new behavior for all your repos or for some of them.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
>> As far as letting git review do magic, how is that better than "git<br>
>> rebase once and no magic required"? You deal with the conflicts and<br>
>> you're good to go.<br>
><br>
><br>
> In a number of manual tasks it's the same. If your patch cannot be merged<br>
> into master, you merge master to your local branch and you're good to go.<br>
> But as I said, merge will be remembered, rebase won't. And after that<br>
> rebase/merge you might end up having your tests failing, and you'll have to<br>
> rewrite your commit again with --amend, with no record in history.<br>
<br>
</div>Again, this is why the tests should pass against all of your commits.<br>
If that's the case, you can verify your changes as you rebase before you<br>
update the commit.<br></blockquote><div><br></div><div style>Ok, one more time. You don't need to do rebase. You merge master with one local commit resolving dependencies in the process and then fix tests and everything with the second one. It's really simple.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
>> And if someone asks you to split a commit, you can<br>
>> do it. With this proposal you can't, because anything but squashing<br>
>> into one commit is going to be a nightmare (which might be my biggest<br>
>> argument against this).<br>
>><br>
><br>
> You can do it with the new approach as well. See at the end of the<br>
> proposal. You split your current branch into a number of branches and let<br>
> git-review detect who depends on who between them.<br>
<br>
</div>!<br>
<br>
So instead of a chain of commits, you want to have a separate branch for<br>
every commit?</blockquote><div><br></div><div style>For every change request, yes. That's what we basically have in Gerrit (although those commits are not structured as well).</div><div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
How would you test that locally?</blockquote><div><br></div><div style>Easy - you merge locally changes from branch 'cr-1' to branch 'cr-2' and voila - 'cr-2' contains all changes you've applied to 'cr-1' and want to test.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Even if git-review had<br>
the smarts to resolve dependencies that doesn't help me while I'm<br>
actively working on a change.<br></blockquote><div><br></div><div style>It does help because you can independently work on each change request until they are ready to be combined once again. And you don't need to manually keep track of which CR's require the one you're working on.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
This sounds very much like a reimplementation of git rebase -i. Instead<br>
of picking the commits you want to edit with that, you'd switch branches<br>
to edit them. Dependency issues aside, how is that an improvement?<br></blockquote><div><br></div><div style>You keep track of all changes that happen to every change request locally. That's the point of this proposal.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
><br>
>> And speaking about breaking down of change requests don't forget support<br>
>>> for change requests chains that this feature would lead to. How to you<br>
>> deal<br>
>>> with 5 consecutive change request that are up on review for half a year?<br>
>>> The only way I could suggest to my colleague at a time was "Erm... Learn<br>
>>> Git and dance with rebases, detached heads and reflogs!" My proposal<br>
>> might<br>
>>> take care of that too.<br>
>>><br>
>><br>
>> How does this relate to commit series? Squashing all the commits into<br>
>> one isn't a solution to any of the problems with those (if it were, we<br>
>> could do that today :-).<br>
>><br>
><br>
> At the end of the proposal I mention how. You keep a separate branch for<br>
> every commit in the series. You merge them into each other as needed, you<br>
> can fix them separately, you can change the dependency order. And every<br>
> single step will be recorded in your local repo.<br>
><br>
><br>
>> FWIW, I have had long-lived patch series, and I don't really see what is<br>
>> so difficult about running git rebase master. Other than conflicts, of<br>
>> course, which are going to be an issue with any long-running change no<br>
>> matter how it's submitted. There isn't a ton of git magic involved.<br>
>><br>
><br>
> Yeah. As long as all you need is to keep them aligned with master, you can<br>
> just rebase. But what if you have 5 commits in the series and you need to<br>
> fix commit #2 that's heavily used in commit #4? You can either constantly<br>
> do interactive rebase (for each small change, actually) or work on top of<br>
> commit #4, then stash your changes and amend commit #2 with them. And you<br>
> can't "commit early, commit often" during this process because a "commit"<br>
> is a long task. You can only hope you won't lose one part of it while<br>
> fixing the other one.<br>
<br>
</div></div>This doesn't fix that though. If commit 4 depends on commit 2 and you<br>
have to make changes to commit 2, you still have to switch back to<br>
commit 4 to verify that your changes in 2 didn't break anything. This<br>
is why having commits be self-contained is important - if commit 2 has<br>
good unit tests to verify the changes, you can hopefully be confident<br>
that your updates to it won't break commit 4.<br></blockquote><div><br></div><div style>But when unittests are not good (enough) you'll break your commit 4 and you'll have to go back and fix that again. And some time later you'll find out that some other issue appeared during this process and you'll want to find out which change to commit #2 did that, and you run bisect... Only if you didn't use rebase.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">All this would change is the command you run to switch between commits.<br>
Since git already provides this functionality, I don't think we should<br>
reinvent it.<br>
<div class=""><br>
><br>
><br>
>> So as you may have guessed by now, I'm opposed to adding this to<br>
>> git-review. I think it's going to encourage bad committer behavior<br>
>> (monolithic commits) and doesn't address a use case I find compelling<br>
>> enough to offset that concern.<br>
>><br>
><br>
> And I hope to show you that even if you wouldn't use this feature, others<br>
> might find it extremely useful.<br>
> It won't encourage having single monolithic change request. It just allows<br>
> you to keep track of its development locally. Nothing changes from<br>
> reviewer's point of view.<br>
><br>
<br>
</div>As I noted in the other e-mail, my concern isn't even so much whether<br>
this feature makes sense (although I think it's a sign of a flawed git<br>
workflow if you feel the need to use it, but I have enough flawed<br>
workflows that I can't really throw stones on that ;-), it's whether<br>
this feature is going to be abused by people who don't understand the<br>
intent. I think it will, and because of that I think the fix here is to<br>
better explain the intended workflow for Git/git-review/Gerrit.<br>
Reimplementing stuff that Git already does in a slightly different way<br>
is only going to cause more confusion.<br></blockquote></div><div><br></div><div class="gmail_extra" style>I'm not suggesting to reinvent Git features. I suggest to use them. I suggest to embrace ability to keep track of everything, not only lousy history of your uploads.</div>
<div class="gmail_extra"><br></div>-- <br><br><div>Kind regards, Yuriy.</div>
</div></div>