Re: [openstack-dev] [git-review] Supporting development in local branches
On Thu, Aug 7, 2014 at 10:28 AM, Chris Friesen chris.frie...@windriver.com wrote: On 08/06/2014 05:41 PM, Zane Bitter wrote: On 06/08/14 18:12, Yuriy Taraday wrote: Well, as per Git author, that's how you should do with not-CVS. You have cheap merges - use them instead of erasing parts of history. This is just not true. http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html Choice quotes from the author of Git: * 'People can (and probably should) rebase their _private_ trees' * 'you can go wild on the git rebase thing' * 'we use git rebase etc while we work on our problems.' * 'git rebase is not wrong.' Also relevant: ...you must never pull into a branch that isn't already in good shape. Don't merge upstream code at random points. keep your own history clean And in the very same thread he says I don't like how you always rebased patches and none of these rules should be absolutely black-and-white. But let's not get driven into discussion of what Linus said (or I'll have to rewatch his ages old talk in Google to get proper quotes). In no way I want to promote exposing private trees with all those intermediate changes. And my proposal is not against rebasing (although we could use -R option for git-review more often to publish what we've tested and to let reviewers see diffs between patchsets). It is for letting people keep history of their work towards giving you a crystal-clean change request series. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Thu, Aug 7, 2014 at 7:36 PM, Ben Nemec openst...@nemebean.com wrote:
On 08/06/2014 05:35 PM, Yuriy Taraday wrote:
On Wed, Aug 6, 2014 at 11:00 PM, Ben Nemec openst...@nemebean.com
wrote:
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.
It usually takes some time for me to build trust in utility that does a
lot
of different things at once while I need only one small piece of that.
So I
usually do smth like:
$ git checkout HEAD~2
$ vim
$ git commit
$ git checkout mybranch
$ git rebase --onto HEAD@{1} HEAD~2
instead of almost the same workflow with interactive rebase.
I'm sorry, but I don't trust the well-tested, widely used tool that Git
provides to make this easier so I'm going to reimplement essentially the
same thing in a messier way myself is a non-starter for me. I'm not
surprised you dislike rebases if you're doing this, but it's a solved
problem. Use git rebase -i.
I'm sorry, I must've mislead you by using word 'trust' in that sentence.
It's more like understanding. I like to understand how things work. I don't
like treating tools as black boxes. And I also don't like when tool does a
lot of things at once with no way back. So yes, I decompose 'rebase -i' a
bit and get slightly (1 command, really) longer workflow. But at least I
can stop at any point and think if I'm really finished at this step. And
sometimes interactive rebase works for me better than this, sometimes it
doesn't. It all depends on situation.
I don't dislike rebases because I sometimes use a bit longer version of it.
I would be glad to avoid them because they destroy history that can help me
later.
I think I've said all I'm going to say on this.
I hope you don't think that this thread was about rebases vs merges. It's
about keeping track of your changes without impact on review process.
--
Kind regards, Yuriy.
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/07/2014 04:52 PM, Yuriy Taraday wrote: I hope you don't think that this thread was about rebases vs merges. It's about keeping track of your changes without impact on review process. But if you rebase, what is stopping you from keeping whatever private history you want and then rebase the desired changes onto the version that the current review tools are using? Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Fri, Aug 8, 2014 at 3:03 AM, Chris Friesen chris.frie...@windriver.com wrote: On 08/07/2014 04:52 PM, Yuriy Taraday wrote: I hope you don't think that this thread was about rebases vs merges. It's about keeping track of your changes without impact on review process. But if you rebase, what is stopping you from keeping whatever private history you want and then rebase the desired changes onto the version that the current review tools are using? That's almost what my proposal is about - allowing developer to keep private history and store uploaded changes separately. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 8 August 2014 10:52, Yuriy Taraday yorik@gmail.com wrote: I don't dislike rebases because I sometimes use a bit longer version of it. I would be glad to avoid them because they destroy history that can help me later. rebase doesn't destroy any history. gc destroys history. See git reflog - you can recover all of your history in high fidelity (and there are options to let you control just how deep the rabbit hole goes). -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/06/2014 05:41 PM, Zane Bitter wrote: On 06/08/14 18:12, Yuriy Taraday wrote: 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); This is not how I'd use Git normally. Well, as per Git author, that's how you should do with not-CVS. You have cheap merges - use them instead of erasing parts of history. This is just not true. http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html Choice quotes from the author of Git: * 'People can (and probably should) rebase their _private_ trees' * 'you can go wild on the git rebase thing' * 'we use git rebase etc while we work on our problems.' * 'git rebase is not wrong.' Also relevant: ...you must never pull into a branch that isn't already in good shape. Don't merge upstream code at random points. keep your own history clean Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/06/2014 05:35 PM, Yuriy Taraday wrote: Oh, looks like we got a bit of a race condition in messages. I hope you don't mind. On Wed, Aug 6, 2014 at 11:00 PM, Ben Nemec openst...@nemebean.com wrote: 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. It won't be documented as such. It will include use with care and years of Git experience: 3+ stickers. Autosquashing will never be mentioned there. Only a detailed explanation of how to work with it and (probably) how it works. No rogue dev will get through it without getting the true understanding. By the way, currently git-review suggests to squash your outstanding commits but there is no overwhelming flow of overly huge change requests, is there? 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
Re: [openstack-dev] [git-review] Supporting development in local branches
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. 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 pgp2cgqzpZO4I.pgp Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
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 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. 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. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
Le 06/08/2014 10:35, Yuriy Taraday a écrit : 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. Well, I can understand that for ease, we could propose it as an option in git-review, but I'm just thinking that if you consider your local Git repo as your single source of truth (and not Gerrit), then you just have to make another branch and squash your intermediate commits for Gerrit upload only. If you need modifying (because of another iteration), you just need to amend the commit message on each top-squasher commit by adding the Change-Id on your local branch, and redo the process (make a branch, squash, upload) each time you need it. Gerrit is cool, it doesn't care about SHA-1s but only Change-Id, so cherry-picking and rebasing still works (hurrah) tl;dr: do as many as intermediate commits you want, but just generate a Change-ID on the commit you consider as patch, so you just squash the intermediate commits on a separate branch copy for Gerrit use only (one-way). Again, I can understand the above as hacky, so I'm not against your change, just emphasizing it as non-necessary (but anyway, everything can be done without git-review, even the magical -m option :-) ) -Sylvain ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Wed, Aug 6, 2014 at 12:55 PM, Sylvain Bauza sba...@redhat.com wrote: Le 06/08/2014 10:35, Yuriy Taraday a écrit : 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. Well, I can understand that for ease, we could propose it as an option in git-review, but I'm just thinking that if you consider your local Git repo as your single source of truth (and not Gerrit), then you just have to make another branch and squash your intermediate commits for Gerrit upload only. That's my proposal - generate such another branches automatically. And from this thread it looks like some people already do them by hand. If you need modifying (because of another iteration), you just need to amend the commit message on each top-squasher commit by adding the Change-Id on your local branch, and redo the process (make a branch, squash, upload) each time you need it. I don't quite understand the top-squasher commit part but what I'm suggesting is to automate this process to make users including myself happier. Gerrit is cool, it doesn't care about SHA-1s but only Change-Id, so cherry-picking and rebasing still works (hurrah) Yes, and that's the only stable part of those another branches. tl;dr: do as many as intermediate commits you want, but just generate a Change-ID on the commit you consider as patch, so you just squash the intermediate commits on a separate branch copy for Gerrit use only (one-way). Again, I can understand the above as hacky, so I'm not against your change, just emphasizing it as non-necessary (but anyway, everything can be done without git-review, even the magical -m option :-) ) I'd even prefer to leave it to git config file so that it won't get accidentally enabled unless user know what one's doing. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
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. 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. 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. 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
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/06/2014 12:41 AM, Yuriy Taraday wrote: On Wed, Aug 6, 2014 at 1:17 AM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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? 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. Ideally, no. In a perfect world every commit would work, so the version history would be a number of small changes that add up to this great application. In reality it's a combination of new features, oopses, and fixes for those oopses. I certainly wouldn't describe it as a history of failures though. I would hope the majority of commits to our projects are _not_ failures. :-) 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. 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?). You're not going to find the root of a bug in your code by looking at an old commit that was replaced by some other implementation. If anything, I see that as more confusing. And if you want to keep old versions of your code, either push it to Gerrit or create a new branch before changing it further. They're basically unnecessary conflicts waiting to happen. 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. Then don't rebase them. git checkout -b dead-end and move on. :-) Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are conflicts, you'd have to resolve them either way. 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. And with merge commits you end up with a tree that is meaningless except at the very tail end of the commit series, which I think is the root of your problems with rebasing. I imagine it would be very painful to work in a way where the only commit that you can test against is the last one. 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. I also reiterate my point about not keeping broken commits on your working branch. You know at some point they're going to get accidentally submitted. :-) Well... As long as you use 'git
Re: [openstack-dev] [git-review] Supporting development in local branches
On 04/08/14 19:18, Yuriy Taraday wrote: Hello, git-review users! I'd like to gather feedback on a feature I want to implement that might turn out useful for you. I like using Git for development. It allows me to keep track of current development process, it remembers everything I ever did with the code (and more). _CVS_ allowed you to remember everything you ever did; Git is _much_ more useful. I also really like using Gerrit for code review. It provides clean interfaces, forces clean histories (who needs to know that I changed one line of code in 3am on Monday?) and allows productive collaboration. +1 What I really hate is having to throw away my (local, precious for me) history for all change requests because I need to upload a change to Gerrit. IMO Ben is 100% correct and, to be blunt, the problem here is your workflow. Don't get me wrong, I sympathise - really, I do. Nobody likes to change their workflow. I *hate* it when I have to change mine. However what you're proposing is to modify the tools to make it easy for other people - perhaps new developers - to use a bad workflow instead of to learn a good one from the beginning, and that would be a colossal mistake. All of the things you want to be made easy are currently hard because doing them makes the world a worse place. The big advantage of Git is that you no longer have to choose between having no version control while you work or having a history full of broken commits, like you did back in the bad old days of CVS. The fact that local history is editable is incredibly powerful, and you should start making use of it. A history of small, incremental, *working* changes is the artifact we want to produce. For me, trying to reconstruct that from a set of broken changes, or changes that only worked with now-obsolete versions of master, is highly counterproductive. I work with patches in the form that I intend to submit them (which changes as I work) because that means I don't have to maintain that form in my head, instead Git can do it for me. Of course while I'm working I might need to retain a small amount of history - the most basic level of that just the working copy, but it often extends to some temporary patches that will later be squashed with others or dropped altogether. Don't forget about git add -p either - that makes it easy to split changes in a single file into separate commits, which is *much* more effective than using a purely time-based history. When master changes I rebase my work, because I need to test all of the patches that I will propose in a series against the current master into which they will be submitted. Retaining a change that did once work in a world that no longer exists is rarely interesting to me, but on those occasions where it is I would just create a local branch to hold on to it. You are struggling because you think of history as a linear set of temporal changes. If you accept that history can instead contain an arbitrarily-ordered set of logical changes then your life will be much easier, since that corresponds exactly to what you need to deliver for review. As soon as you stop fighting against Gerrit and learn how to work with it, all of your problems evaporate. Tools that make it easier to fight it will ultimately only add complexity without solving the underlying problems. (BTW a tool I use to help with this is Stacked Git. It makes things like editing an early patch in a series much easier than rebase -i... I just do `stg refresh -p patch_name` and the right patch gets the changes. For dead-end ideas I just do `stg pop` and the patch stays around for reference but isn't part of the history. I usually don't recommend this tool to the community, however, because StGit doesn't run the commit hook that is needed to insert the ChangeId for Gerrit. I'd be happy to send you my patches that fix it if you want to try it out though.) That's why I want to propose making git-review to support the workflow that will make me happy. Imagine you could do smth like this: 0. create new local branch; master: M-- \ feature: * 1. start hacking, doing small local meaningful (to you) commits; master: M-- \ feature: A-B-...-C 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); This is not how I'd use Git normally. master: M---N-O-... \\\ feature: A-B-...-C-D-... 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; You just said that this was a Cool Feature (tm) taking a tremendous amount of time. Yet your solution is to squash everything
Re: [openstack-dev] [git-review] Supporting development in local branches
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. 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. In fact, not having a
Re: [openstack-dev] [git-review] Supporting development in local branches
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
Re: [openstack-dev] [git-review] Supporting development in local branches
On Wed, Aug 6, 2014 at 7:23 PM, Ben Nemec openst...@nemebean.com wrote: On 08/06/2014 12:41 AM, Yuriy Taraday wrote: On Wed, Aug 6, 2014 at 1:17 AM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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? 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. Ideally, no. In a perfect world every commit would work, so the version history would be a number of small changes that add up to this great application. In reality it's a combination of new features, oopses, and fixes for those oopses. I certainly wouldn't describe it as a history of failures though. I would hope the majority of commits to our projects are _not_ failures. :-) Well, new features are merged just to be later fixed and refactored - how that's not a failure? And we basically do keep a record of how not to do it in our repositories. Why prevent developers do the same on the smaller scale? 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. 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?). You're not going to find the root of a bug in your code by looking at an old commit that was replaced by some other implementation. If anything, I see that as more confusing. And if you want to keep old versions of your code, either push it to Gerrit or create a new branch before changing it further. So you propose two options: - store history of your work within Gerrit's patchsets for each change request, which don't fit commit often approach (who'd want to see how I struggle with fixing some bug or write working test?); - store history of your work in new branches instead of commits in the same branch, which... is not how Git is supposed to be used. And both this options don't provide any proper way of searching through this history. Have you ever used bisect? Sometimes I find myself wanting to use it instead of manually digging through patchsets in Gerrit to find out which change I made broke some usecase I didn't put in unittests yet. They're basically unnecessary conflicts waiting to happen. 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. Then don't rebase them. git checkout -b dead-end and move on. :-) I never proposed to rebase anything. I want to use merge instead of rebase. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/06/2014 01:14 PM, Yuriy Taraday wrote: On Wed, Aug 6, 2014 at 7:23 PM, Ben Nemec openst...@nemebean.com mailto:openst...@nemebean.com wrote: Again, this is why the tests should pass against all of your commits. If that's the case, you can verify your changes as you rebase before you update the commit. Ok, one more time. You don't need to do rebase. You merge master with one local commit resolving dependencies in the process and then fix tests and everything with the second one. It's really simple. Personally I find rebasing my current changes onto the latest upstream to be an intuitive way to work. That way it's obvious what changes I'm making on top of the current upstream codebase. In my view merging the upstream onto my dev branch only makes sense if I've published my dev branch to someone and don't want to break their history the next time they try to pull. I would far rather rebase my current changes and explicitly resolve conflicts than merge upstream on top of my changes and then fix conflicts in merge commits. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
I'll start using pictures now, so let's assume M is the latest commit on the master. On Wed, Aug 6, 2014 at 9:31 PM, Zane Bitter zbit...@redhat.com wrote: On 04/08/14 19:18, Yuriy Taraday wrote: Hello, git-review users! I'd like to gather feedback on a feature I want to implement that might turn out useful for you. I like using Git for development. It allows me to keep track of current development process, it remembers everything I ever did with the code (and more). _CVS_ allowed you to remember everything you ever did; Git is _much_ more useful. I also really like using Gerrit for code review. It provides clean interfaces, forces clean histories (who needs to know that I changed one line of code in 3am on Monday?) and allows productive collaboration. +1 What I really hate is having to throw away my (local, precious for me) history for all change requests because I need to upload a change to Gerrit. IMO Ben is 100% correct and, to be blunt, the problem here is your workflow. Well... That's the workflow that was born with Git. Keeping track of all changes, do extremely cheap merges, and all that. Don't get me wrong, I sympathise - really, I do. Nobody likes to change their workflow. I *hate* it when I have to change mine. However what you're proposing is to modify the tools to make it easy for other people - perhaps new developers - to use a bad workflow instead of to learn a good one from the beginning, and that would be a colossal mistake. All of the things you want to be made easy are currently hard because doing them makes the world a worse place. And when OpenStack switched to Gerrit I was really glad. Instead of ugly master: ...-M-.-o-o-... \ / a1-b1-a2-a3-b2-c1-b3-c2 where a[1-3], b[1-3] and c[1-2] are iterations over the same piece of the feature, we can have pretty master: ...-M-.o-.-o-... \/ / A^-B^-C^ where A^, B^ and C^ are the perfect self-contained, independently reviewable and mergeable pieces of the feature. And this looked splendid and my workflow seemed clear. Suppose I have smth like: master: ...-M \ A3-B2-C1 and I need to update B to B3 and C to C2. So I go: $ git rebase -i M # and add edit action to B commit $ vim # do some changes, test them, etc $ git rebase --continue now I have master: ...-M \ A3-B2-C1 \ B3-C1' Then I fix C commit, amend it and get: master: ...-M \ A3-B2-C1 \ B3-C1' \ С2 Now everything's cool, isn't it? But world isn't fair. And C2 fails a test that I didn't expect to fail. Or the test suite failed to fail earlier. I'd like to see if I broke it just now or were it broken after rebase. How do I do it? With your workflow - I don't. I play smart and guess where the problem was or dig into reflog to find C1' (or C1), etc. Let's see what else I can't find. After full iteration over this feature (as in the first picture) I end up with total history like this: master: ...-M |\ | A1-B1 |\ | A2-B1' \ A3-B1'' |\ | B2-C1 \ B3-C1' \ С2 With only A3, B3 and C2 available, the rest are practically unreachable. Now you find out that something that you were sure was working in B1 is broken (you'll tell me hey, you're supposed to have tests with everything! - I'll answer: what if you've found a problem in the test suite that gave false success?). You can do absolutely nothing to localize the issue now. Just go and dig into your B code (which might've been written months ago). Or you slap your head understanding that the function you thought is not needed in B2 is actually needed. Well, you can hope you did upload B2 to Gerrit and you'll find it there. Or you didn't because you decided to make that change the minute after you committed C1, created B3 and B2 never existed now... Now imagine you could somehow link together all As, Bs and Cs. Let's draw vertical edges between them. And let's transpose the picture, shall we? master: ...-M \ A1-A2--A3 \ \ \\ \ B1-B1'-B1''-B2-B3 \ \ \ C1-C1'-C2 Note that all commits here are absolutely the same as in previous picture. They just have additional parents (and consequently differen IDs). No changes to any code in them happen. No harm done. So now it looks way better. I can just do: $ git checkout B3 $ git diff HEAD~ and find my lost function! Now let's be honest and admit that As, Bs and Cs are essentially branches - labels your commits have that shift with relevant
Re: [openstack-dev] [git-review] Supporting development in local branches
Oh, looks like we got a bit of a race condition in messages. I hope you don't mind. On Wed, Aug 6, 2014 at 11:00 PM, Ben Nemec openst...@nemebean.com wrote: 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. It won't be documented as such. It will include use with care and years of Git experience: 3+ stickers. Autosquashing will never be mentioned there. Only a detailed explanation of how to work with it and (probably) how it works. No rogue dev will get through it without getting the true understanding. By the way, currently git-review suggests to squash your outstanding commits but there is no overwhelming flow of overly huge change requests, is there? 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
Re: [openstack-dev] [git-review] Supporting development in local branches
On 06/08/14 18:12, Yuriy Taraday wrote: 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); This is not how I'd use Git normally. Well, as per Git author, that's how you should do with not-CVS. You have cheap merges - use them instead of erasing parts of history. This is just not true. http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html Choice quotes from the author of Git: * 'People can (and probably should) rebase their _private_ trees' * 'you can go wild on the git rebase thing' * 'we use git rebase etc while we work on our problems.' * 'git rebase is not wrong.' ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/04/2014 07:18 PM, Yuriy Taraday wrote: Hello, git-review users! snip 0. create new local branch; master: M-- \ feature: * 1. start hacking, doing small local meaningful (to you) commits; master: M-- \ feature: A-B-...-C 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); master: M---N-O-... \\\ feature: A-B-...-C-D-... 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; master: M---N-O-... \\\E* = uploaded feature: A-B-...-C-D-...-E snip +1, this is definitely a feature I'd want to see. Currently I run two branches bug/LPBUG#-local and bug/LPBUG# where the local is my full history of the change and the other branch is the squashed version I send out to Gerrit. Cheers, -- Ryan Brown / Software Engineer, Openstack / Red Hat, Inc. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
Le 05/08/2014 13:06, Ryan Brown a écrit : On 08/04/2014 07:18 PM, Yuriy Taraday wrote: Hello, git-review users! snip 0. create new local branch; master: M-- \ feature: * 1. start hacking, doing small local meaningful (to you) commits; master: M-- \ feature: A-B-...-C 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); master: M---N-O-... \\\ feature: A-B-...-C-D-... 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; master: M---N-O-... \\\E* = uploaded feature: A-B-...-C-D-...-E snip +1, this is definitely a feature I'd want to see. Currently I run two branches bug/LPBUG#-local and bug/LPBUG# where the local is my full history of the change and the other branch is the squashed version I send out to Gerrit. -1 to this as git-review default behaviour. Ideally, branches should be identical in between Gerrit and local Git. I can understand some exceptions where developers want to work on intermediate commits and squash them before updating Gerrit, but in that case, I can't see why it needs to be kept locally. If a new patchset has to be done on patch A, then the local branch can be rebased interactively on last master, edit patch A by doing an intermediate patch, then squash the change, and pick the later patches (B to E) That said, I can also understand that developers work their way, and so could dislike squashing commits, hence my proposal to have a --no-squash option when uploading, but use with caution (for a single branch, how many dependencies are outdated in Gerrit because developers work on separate branches for each single commit while they could work locally on a single branch ? I can't iimagine how often errors could happen if we don't force by default to squash commits before sending them to Gerrit) -Sylvain Cheers, ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/05/2014 09:27 AM, Sylvain Bauza wrote: Le 05/08/2014 13:06, Ryan Brown a écrit : -1 to this as git-review default behaviour. Ideally, branches should be identical in between Gerrit and local Git. Probably not as default behaviour (people who don't want that workflow would be driven mad!), but I think enough folks would want it that it should be available as an option. I can understand some exceptions where developers want to work on intermediate commits and squash them before updating Gerrit, but in that case, I can't see why it needs to be kept locally. If a new patchset has to be done on patch A, then the local branch can be rebased interactively on last master, edit patch A by doing an intermediate patch, then squash the change, and pick the later patches (B to E) That said, I can also understand that developers work their way, and so could dislike squashing commits, hence my proposal to have a --no-squash option when uploading, but use with caution (for a single branch, how many dependencies are outdated in Gerrit because developers work on separate branches for each single commit while they could work locally on a single branch ? I can't iimagine how often errors could happen if we don't force by default to squash commits before sending them to Gerrit) -Sylvain Cheers, ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I am well aware this may be straying into feature creep territory, and it wouldn't be terrible if this weren't implemented. -- Ryan Brown / Software Engineer, Openstack / Red Hat, Inc. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. Do we need to expose such feature under git review? we could define a new subcommand? git reviewflow? Cédric, ZZelle@IRC On Tue, Aug 5, 2014 at 4:49 PM, Ryan Brown rybr...@redhat.com wrote: On 08/05/2014 09:27 AM, Sylvain Bauza wrote: Le 05/08/2014 13:06, Ryan Brown a écrit : -1 to this as git-review default behaviour. Ideally, branches should be identical in between Gerrit and local Git. Probably not as default behaviour (people who don't want that workflow would be driven mad!), but I think enough folks would want it that it should be available as an option. I can understand some exceptions where developers want to work on intermediate commits and squash them before updating Gerrit, but in that case, I can't see why it needs to be kept locally. If a new patchset has to be done on patch A, then the local branch can be rebased interactively on last master, edit patch A by doing an intermediate patch, then squash the change, and pick the later patches (B to E) That said, I can also understand that developers work their way, and so could dislike squashing commits, hence my proposal to have a --no-squash option when uploading, but use with caution (for a single branch, how many dependencies are outdated in Gerrit because developers work on separate branches for each single commit while they could work locally on a single branch ? I can't iimagine how often errors could happen if we don't force by default to squash commits before sending them to Gerrit) -Sylvain Cheers, ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I am well aware this may be straying into feature creep territory, and it wouldn't be terrible if this weren't implemented. -- Ryan Brown / Software Engineer, Openstack / Red Hat, Inc. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? Do we need to expose such feature under git review? we could define a new subcommand? git reviewflow? Cédric, ZZelle@IRC On Tue, Aug 5, 2014 at 4:49 PM, Ryan Brown rybr...@redhat.com wrote: On 08/05/2014 09:27 AM, Sylvain Bauza wrote: Le 05/08/2014 13:06, Ryan Brown a écrit : -1 to this as git-review default behaviour. Ideally, branches should be identical in between Gerrit and local Git. Probably not as default behaviour (people who don't want that workflow would be driven mad!), but I think enough folks would want it that it should be available as an option. I can understand some exceptions where developers want to work on intermediate commits and squash them before updating Gerrit, but in that case, I can't see why it needs to be kept locally. If a new patchset has to be done on patch A, then the local branch can be rebased interactively on last master, edit patch A by doing an intermediate patch, then squash the change, and pick the later patches (B to E) That said, I can also understand that developers work their way, and so could dislike squashing commits, hence my proposal to have a --no-squash option when uploading, but use with caution (for a single branch, how many dependencies are outdated in Gerrit because developers work on separate branches for each single commit while they could work locally on a single branch ? I can't iimagine how often errors could happen if we don't force by default to squash commits before sending them to Gerrit) -Sylvain Cheers, ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I am well aware this may be straying into feature creep territory, and it wouldn't be terrible if this weren't implemented. -- Ryan Brown / Software Engineer, Openstack / Red Hat, Inc. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 5:15 AM, Angus Salkeld angus.salk...@rackspace.com wrote: On Tue, 2014-08-05 at 03:18 +0400, Yuriy Taraday wrote: Hello, git-review users! I'd like to gather feedback on a feature I want to implement that might turn out useful for you. I like using Git for development. It allows me to keep track of current development process, it remembers everything I ever did with the code (and more). I also really like using Gerrit for code review. It provides clean interfaces, forces clean histories (who needs to know that I changed one line of code in 3am on Monday?) and allows productive collaboration. What I really hate is having to throw away my (local, precious for me) history for all change requests because I need to upload a change to Gerrit. I just create a short-term branch to record this. I tend to use branches that are squashed down to one commit after the first upload and that's it. I'd love to keep all history during feature development, not just the tip of it. That's why I want to propose making git-review to support the workflow that will make me happy. Imagine you could do smth like this: 0. create new local branch; master: M-- \ feature: * 1. start hacking, doing small local meaningful (to you) commits; master: M-- \ feature: A-B-...-C 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); master: M---N-O-... \\\ feature: A-B-...-C-D-... 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; master: M---N-O-... \\\E* = uploaded feature: A-B-...-C-D-...-E 4. you repeat steps 1 and 2 as much as you like; 5. and all consecutive calls to 'git review' will show you last commit message you used for upload and use it to upload new state of your local branch to Gerrit, as one change request. Note that during this process git-review will never run rebase or merge operations. All such operations are done by user in local branch instead. Now, to the dirty implementations details. - Since suggested feature changes default behavior of git-review, it'll have to be explicitly turned on in config (review.shadow_branches? review.local_branches?). It should also be implicitly disabled on master branch (or whatever is in .gitreview config). - Last uploaded commit for branch branch-name will be kept in refs/review-branches/branch-name. - For every call of 'git review' it will find latest commit in gerrit/master (or remote and branch from .gitreview), create a new one that will have that commit as its parent and a tree of current commit from local branch as its tree. - While creating new commit, it'll open an editor to fix commit message for that new commit taking it's initial contents from refs/review-branches/branch-name if it exists. - Creating this new commit might involve generating a temporary bare repo (maybe even with shared objects dir) to prevent changes to current index and HEAD while using bare 'git commit' to do most of the work instead of loads of plumbing commands. Note that such approach won't work for uploading multiple change request without some complex tweaks, but I imagine later we can improve it and support uploading several interdependent change requests from several local branches. We can resolve dependencies between them by tracking latest merges (if branch myfeature-a has been merged to myfeature-b then change request from myfeature-b will depend on change request from myfeature-a): master:M---N-O-... \\\-E* myfeature-a: A-B-...-C-D-...-E \ \ \ J* = uploaded myfeature-b: F-...-G-I-J This improvement would be implemented later if needed. I hope such feature seams to be useful not just for me and I'm looking forward to some comments on it. Hi Yuriy, I like my local history matching what is up for review and don't value the interim messy commits (I make a short term branch to save the history so I can go back to it - if I mess up a merge). You'll still get this history in those special refs. But in your branch you'll have your own history. Tho' others might love this idea. -Angus -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 3:06 PM, Ryan Brown rybr...@redhat.com wrote: On 08/04/2014 07:18 PM, Yuriy Taraday wrote: snip +1, this is definitely a feature I'd want to see. Currently I run two branches bug/LPBUG#-local and bug/LPBUG# where the local is my full history of the change and the other branch is the squashed version I send out to Gerrit. And I'm too lazy to keep switching between these branches :) Great, you're first to support this feature! -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 5:27 PM, Sylvain Bauza sba...@redhat.com wrote: -1 to this as git-review default behaviour. I don't suggest to make it the default behavior. As I wrote there will definitely be a config option that would turn it on. Ideally, branches should be identical in between Gerrit and local Git. The thing is that there's no feature branches in Gerrit. Just some number of independent commits (patchsets). And you'll even get log of those locally in special refs! I can understand some exceptions where developers want to work on intermediate commits and squash them before updating Gerrit, but in that case, I can't see why it needs to be kept locally. If a new patchset has to be done on patch A, then the local branch can be rebased interactively on last master, edit patch A by doing an intermediate patch, then squash the change, and pick the later patches (B to E) And that works up to the point when your change requests evolves for several months and there's no easy way to dig up why did you change that default or how did this algorithm ended up in such shape. You can't simply run bisect to find what did you break since 10 patchsets ago. Git has been designed to be super easy to keep branches and most of them - locally. And we can't properly use them. That said, I can also understand that developers work their way, and so could dislike squashing commits, hence my proposal to have a --no-squash option when uploading, but use with caution (for a single branch, how many dependencies are outdated in Gerrit because developers work on separate branches for each single commit while they could work locally on a single branch ? I can't iimagine how often errors could happen if we don't force by default to squash commits before sending them to Gerrit) I don't quite get the reason for --no-squash option. With current git-review there's no squashing at all. You either upload all outstanding commits or you go a change smth by yourself. With my suggested approach you don't squash (in terms of rebasing) anything, you just create a new commit with the very same contents as in your branch. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 6:49 PM, Ryan Brown rybr...@redhat.com wrote: On 08/05/2014 09:27 AM, Sylvain Bauza wrote: Le 05/08/2014 13:06, Ryan Brown a écrit : -1 to this as git-review default behaviour. Ideally, branches should be identical in between Gerrit and local Git. Probably not as default behaviour (people who don't want that workflow would be driven mad!), but I think enough folks would want it that it should be available as an option. This would definitely be a feature that only some users would turn on in their config files. I am well aware this may be straying into feature creep territory, and it wouldn't be terrible if this weren't implemented. I'm not sure I understand what do you mean by this... -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 7:51 PM, ZZelle zze...@gmail.com wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. Do we need to expose such feature under git review? we could define a new subcommand? git reviewflow? Yes. I think we should definitely make it an enhancement for 'git review' command because it's essentially mostly the same 'git review' control flow with an extra preparation step and a bit shifted upload source. git-review is a magic command that does what you need finishing with change request upload. And this is exactly what I want here. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. And speaking about breaking down of change requests don't forget support for change requests chains that this feature would lead to. How to you deal with 5 consecutive change request that are up on review for half a year? The only way I could suggest to my colleague at a time was Erm... Learn Git and dance with rebases, detached heads and reflogs! My proposal might take care of that too. -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are conflicts, you'd have to resolve them either way. I also reiterate my point about not keeping broken commits on your working branch. You know at some point they're going to get accidentally submitted. :-) As far as letting git review do magic, how is that better than git rebase once and no magic required? You deal with the conflicts and you're good to go. And if someone asks you to split a commit, you can do it. With this proposal you can't, because anything but squashing into one commit is going to be a nightmare (which might be my biggest argument against this). And speaking about breaking down of change requests don't forget support for change requests chains that this feature would lead to. How to you deal with 5 consecutive change request that are up on review for half a year? The only way I could suggest to my colleague at a time was Erm... Learn Git and dance with rebases, detached heads and reflogs! My proposal might take care of that too. How does this relate to commit series? Squashing all the commits into one isn't a solution to any of the problems with those (if it were, we could do that today :-). 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. 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. /wall-o-text -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
On Aug 6, 2014 7:21 AM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are conflicts, you'd have to resolve them either way. I also reiterate my point about not keeping broken commits on your working branch. You know at some point they're going to get accidentally submitted. :-) As far as letting git review do magic, how is that better than git rebase once and no magic required? You deal with the conflicts and you're good to go. And if someone asks you to split a commit, you can do it. With this proposal you can't, because anything but squashing into one commit is going to be a nightmare (which might be my biggest argument against this). And speaking about breaking down of change requests don't forget support for change requests chains that this feature would lead to. How to you deal with 5 consecutive change request that are up on review for half a year? The only way I could suggest to my colleague at a time was Erm... Learn Git and dance with rebases, detached heads and reflogs! My proposal might take care of that too. How does this relate to commit series? Squashing all the commits into one isn't a solution to any of the problems with those (if it were, we could do that today :-). 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. 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. +1 /wall-o-text -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [git-review] Supporting development in local branches
What I really hate is having to throw away my (local, precious for me) history for all change requests because I need to upload a change to Gerrit. +1 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; +1 On 5 August 2014 22:31, Joe Gordon joe.gord...@gmail.com wrote: On Aug 6, 2014 7:21 AM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are conflicts, you'd have to resolve them either way. I also reiterate my point about not keeping broken commits on your working branch. You know at some point they're going to get accidentally submitted. :-) As far as letting git review do magic, how is that better than git rebase once and no magic required? You deal with the conflicts and you're good to go. And if someone asks you to split a commit, you can do it. With this proposal you can't, because anything but squashing into one commit is going to be a nightmare (which might be my biggest argument against this). And speaking about breaking down of change requests don't forget support for change requests chains that this feature would lead to. How to you deal with 5 consecutive change request that are up on review for half a year? The only way I could suggest to my colleague at a time was Erm... Learn Git and dance with rebases, detached heads and reflogs! My proposal might take care of that too. How does this relate to commit series? Squashing all the commits into one isn't a solution to any of the problems with those (if it were, we could do that today :-). 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. 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. +1 /wall-o-text -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Igor Duarte Cardoso. http://igordcard.com ___ OpenStack-dev mailing list
Re: [openstack-dev] [git-review] Supporting development in local branches
On Wed, Aug 6, 2014 at 1:17 AM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 03:14 PM, Yuriy Taraday wrote: On Tue, Aug 5, 2014 at 10:48 PM, Ben Nemec openst...@nemebean.com wrote: On 08/05/2014 10:51 AM, ZZelle wrote: Hi, I like the idea ... with complex change, it could useful for the understanding to split it into smaller changes during development. I don't understand this. If it's a complex change that you need multiple commits to keep track of locally, why wouldn't reviewers want the same thing? Squashing a bunch of commits together solely so you have one review for Gerrit isn't a good thing. Is it just the warning message that git-review prints when you try to push multiple commits that is the problem here? 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? 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. 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. 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?). They're basically unnecessary conflicts waiting to happen. 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. Merges are one of the strong sides of Git itself (and keeping them very easy is one of the founding principles behind it). With current workflow we don't use them at all. master went too far forward? You have to do rebase and screw all your local history and most likely squash everything anyway because you don't want to fix commits with known bugs in them. With proposed feature you can just do merge once and let 'git review' add some magic without ever hurting your code. How do rebases screw up your local history? All your commits are still there after a rebase, they just have a different parent. I also don't see how rebases are all that much worse than merges. If there are no conflicts, rebases are trivial. If there are conflicts, you'd have to resolve them either way. 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. 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. I also reiterate my point about not keeping broken commits on your working branch. You know at some point they're going to get accidentally submitted. :-) 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? As far as letting git review do magic, how is that better than git rebase once and no magic required? You deal with the conflicts and you're good to go. 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. 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. And if someone asks you to split a commit, you can do it. With this proposal you can't, because anything but squashing into one commit is going to be a nightmare (which might be my biggest argument against this). 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. And speaking about breaking down of change
Re: [openstack-dev] [git-review] Supporting development in local branches
On Tue, 2014-08-05 at 03:18 +0400, Yuriy Taraday wrote: Hello, git-review users! I'd like to gather feedback on a feature I want to implement that might turn out useful for you. I like using Git for development. It allows me to keep track of current development process, it remembers everything I ever did with the code (and more). I also really like using Gerrit for code review. It provides clean interfaces, forces clean histories (who needs to know that I changed one line of code in 3am on Monday?) and allows productive collaboration. What I really hate is having to throw away my (local, precious for me) history for all change requests because I need to upload a change to Gerrit. I just create a short-term branch to record this. That's why I want to propose making git-review to support the workflow that will make me happy. Imagine you could do smth like this: 0. create new local branch; master: M-- \ feature: * 1. start hacking, doing small local meaningful (to you) commits; master: M-- \ feature: A-B-...-C 2. since hacking takes tremendous amount of time (you're doing a Cool Feature (tm), nothing less) you need to update some code from master, so you're just merging master in to your branch (i.e. using Git as you'd use it normally); master: M---N-O-... \\\ feature: A-B-...-C-D-... 3. and now you get the first version that deserves to be seen by community, so you run 'git review', it asks you for desired commit message, and poof, magic-magic all changes from your branch is uploaded to Gerrit as _one_ change request; master: M---N-O-... \\\E* = uploaded feature: A-B-...-C-D-...-E 4. you repeat steps 1 and 2 as much as you like; 5. and all consecutive calls to 'git review' will show you last commit message you used for upload and use it to upload new state of your local branch to Gerrit, as one change request. Note that during this process git-review will never run rebase or merge operations. All such operations are done by user in local branch instead. Now, to the dirty implementations details. - Since suggested feature changes default behavior of git-review, it'll have to be explicitly turned on in config (review.shadow_branches? review.local_branches?). It should also be implicitly disabled on master branch (or whatever is in .gitreview config). - Last uploaded commit for branch branch-name will be kept in refs/review-branches/branch-name. - For every call of 'git review' it will find latest commit in gerrit/master (or remote and branch from .gitreview), create a new one that will have that commit as its parent and a tree of current commit from local branch as its tree. - While creating new commit, it'll open an editor to fix commit message for that new commit taking it's initial contents from refs/review-branches/branch-name if it exists. - Creating this new commit might involve generating a temporary bare repo (maybe even with shared objects dir) to prevent changes to current index and HEAD while using bare 'git commit' to do most of the work instead of loads of plumbing commands. Note that such approach won't work for uploading multiple change request without some complex tweaks, but I imagine later we can improve it and support uploading several interdependent change requests from several local branches. We can resolve dependencies between them by tracking latest merges (if branch myfeature-a has been merged to myfeature-b then change request from myfeature-b will depend on change request from myfeature-a): master:M---N-O-... \\\-E* myfeature-a: A-B-...-C-D-...-E \ \ \ J* = uploaded myfeature-b: F-...-G-I-J This improvement would be implemented later if needed. I hope such feature seams to be useful not just for me and I'm looking forward to some comments on it. Hi Yuriy, I like my local history matching what is up for review and don't value the interim messy commits (I make a short term branch to save the history so I can go back to it - if I mess up a merge). Tho' others might love this idea. -Angus -- Kind regards, Yuriy. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev signature.asc Description: This is a digitally signed message part ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
