<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 6, 2014 at 1:17 AM, 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/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>> 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 of<br>
> different approaches and make thousands of mistakes. For reviewers this 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 for<br>
> future research or bug-hunting.<br>
<br>
</div>So basically keeping a record of how not to do it?</blockquote><div><br></div><div style>Well, yes, you can call version control system "a history of failures". Because if there were no failures there would've been one omnipotent commit that does everything you want it to.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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></blockquote><div><br></div><div style>The commits themselves are never going to merge to master but that's not the only meaning of their life. With current tooling "working branch" ends up a patch series that is constantly rewritten with no proper history of when did that happen and why. As I said, you can't find roots of bugs in your code, you can't dig into old versions of your code (what if you need a method that you've already created but removed because of some wrong suggestion?). </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
They're basically unnecessary conflicts waiting to happen.<br></blockquote><div><br></div><div style>No. They are your local history. They don't need to be rebased on top of master - you can just merge master into your branch and resolve conflicts once. After that your "autosquashed" commit will merge clearly back to master.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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 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>
</div>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></blockquote><div><br></div><div style>Merge is a new commit, new recorded point in history. Rebase is rewriting your commit, replacing it with a new one, without any record in history (of course there will be a record in reflog but there's not much tooling to work with it). Yes, you just apply your patch to a different version of master branch. And then fix some conflicts. And then fix some tests. And then you end up with totally different commit.</div>
<div style>I totally agree that life's very easy when there's no conflicts and you've written all your feature in one go. But that's almost never the true.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote><div><br></div><div style>Well... As long as you use 'git review' to upload CRs, you're safe. If you do 'git push gerrit HEAD:refs/for/master' you're screwed. But why would you do that?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div><div style>In a number of manual tasks it's the same. If your patch cannot be merged into master, you merge master to your local branch and you're good to go.</div>
<div style>But as I said, merge will be remembered, rebase won't. And after that rebase/merge you might end up having your tests failing, and you'll have to rewrite your commit again with --amend, with no record in history.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div style>You can do it with the new approach as well. See at the end of the proposal. You split your current branch into a number of branches and let git-review detect who depends on who between them.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> 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 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 might<br>
> take care of that too.<br>
><br>
<br>
</div>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></blockquote><div><br></div><div style>At the end of the proposal I mention how. You keep a separate branch for every commit in the series. You merge them into each other as needed, you can fix them separately, you can change the dependency order. And every single step will be recorded in your local repo.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote><div><br></div><div style>Yeah. As long as all you need is to keep them aligned with master, you can just rebase. But what if you have 5 commits in the series and you need to fix commit #2 that's heavily used in commit #4? You can either constantly do interactive rebase (for each small change, actually) or work on top of commit #4, then stash your changes and amend commit #2 with them. And you can't "commit early, commit often" during this process because a "commit" is a long task. You can only hope you won't lose one part of it while fixing the other one.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote></div><br>And I hope to show you that even if you wouldn't use this feature, others might find it extremely useful.</div><div class="gmail_extra">It won't encourage having single monolithic change request. It just allows you to keep track of its development locally. Nothing changes from reviewer's point of view.<br clear="all">
<div><br></div>-- <br><br><div>Kind regards, Yuriy.</div>
</div></div>