Open Stack

On Wed, Aug 6, 2014 at 6:20 PM, Ben Nemec <openstack at nemebean.com> wrote:

> On 08/06/2014 03:35 AM, Yuriy Taraday wrote:
> > 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.
>
> Right, I understand that may not be the intent, but it's almost
> certainly going to be the end result.  You can't control how people are
> going to use this feature, and history suggests if it can be abused, it
> will be.
>

Can you please outline the abuse scenario that isn't present nowadays?
People upload huge changes and are encouraged to split them during review.
The same will happen within proposed workflow. More experienced developers
split their change into a set of change requests. The very same will happen
within proposed workflow.

>
> > 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.
>
> This is a separate issue though.  Editing a commit in the middle of a
> series doesn't have to be done at the same time as a rebase to master.
>

No, this will be done with a separate interactive rebase or that detached
HEAD and reflog dance. I don't see this as smth clearer than doing proper
commits in a separate branches.

In fact, not having a bunch of small broken commits that can't be
> submitted individually in your history makes it _easier_ to deal with
> follow-up changes.  Then you know that the unit tests pass on every
> commit, so you can work on it in isolation without constantly having to
> rebase through your entire commit history.  This workflow seems to
> encourage the painful rebases you're trying to avoid.
>

No, this workflow encourage using merges instead of rebases. You don't need
to rebase anything.

 >> 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/82b7e001/attachment.html>