Open Stack

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.

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

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

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

-- 
Martin Geisler

http://google.com/+MartinGeisler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140806/2c24a9c8/attachment.pgp>