Open Stack

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.

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.

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

-- 

Kind regards, Yuriy.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140806/753b9106/attachment.html>