On 08/06/2014 01:42 PM, Yuriy Taraday wrote: > On Wed, Aug 6, 2014 at 6:20 PM, Ben Nemec <openst...@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.
There will be a documented option in git-review that automatically squashes all commits. People _will_ use that incorrectly because from a submitter perspective it's easier to deal with one review than multiple, but from a reviewer perspective it's exactly the opposite. > >> >>> On Wed, Aug 6, 2014 at 12:03 PM, Martin Geisler <mar...@geisler.net> >> wrote: >>> >>>> Ben Nemec <openst...@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. You keep mentioning detached HEAD and reflog. I have never had to deal with either when doing a rebase, so I think there's a disconnect here. The only time I see a detached HEAD is when I check out a change from Gerrit (and I immediately stick it in a local branch, so it's a transitive state), and the reflog is basically a safety net for when I horribly botch something, not a standard tool that I use on a daily basis. > > 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. /shrug The end result of both is the same (your change applied to master), but the merge version leaves your local repo a mess with a merge commit that might be overwriting things from your previous commits, but you won't know just by looking at one in isolation. > > >> 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. >>> >> >> > > _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
