Re: [Development] Pushing rebases and unrelated changes together is a sin
On 02/20/2013 01:01 PM, Shawn Rutledge wrote: > On 20 February 2013 09:43, Samuel Rødal wrote: >> On 02/20/2013 09:22 AM, Thiago Macieira wrote: >>> On quarta-feira, 20 de fevereiro de 2013 08.47.40, Samuel Rødal wrote: The correct process when doing rebases is: git pull --rebase # or your favorite git work-flow git push gerrit HEAD:refs/for/some_branch # add comment in change on gerrit about being a pure rebase # do some changes git commit -a --amend git push gerrit HEAD:refs/for/some_branch >>> >>> Actually, I'll go further: the above should be done ONLY if you are trying >>> to >>> solve a conflict. If you're not trying to solve a conflict, DON'T REBASE. >> >> I thought that went without saying :) > > When revisiting a patch from a few days or more ago, I guess I just > don't trust that the change will apply cleanly or is definitely still > applicable without further changes. So that would be the reason to > cherry-pick and ensure it, rather than assuming it will be OK and then > waiting for a CI failure, right? Fine, if the rebase has to be a > separate push, we can just clutter up gerrit with more pushes. I > typically do plenty of them anyway, due to various mistakes, to sync > up between machines, to store intermediate work and so on. (And then > people make snide comments like "geez, do they pay you by the > patch?!?" so somebody is always dissatisfied.) That doesn't cause a CI failure, it simply informs you that the change could not be applied. So you don't need to worry about the change not applying cleanly in the end, Gerrit will inform you when that happens without troubling anyone else. I still don't think there are good reasons for doing a lot of rebases unless you know that there are conflicts that need to be fixed. > Coming back and asking "would you please split the rebase from the > changes" is also too easy for the reviewer to ask for, and not so easy > to actually do, after having mixed them up. Indeed, so a reminder when pushing a rebase (or fixing the Gerrit interface) sounds useful. If you do accidentally rebase and commit some changes and you want to make them into separate changes, the best way is to follow this process: REBASE_AND_CHANGE=`git log -1 --pretty=format:%H HEAD` git checkout HEAD~ # replace the following with the cherry-pick option from gerrit git fetch ssh://@codereview.qt-project.org:29418/qt/qtbase refs/changes/ && git cherry-pick FETCH_HEAD # push the rebase git push gerrit HEAD:refs/for/ # cd to top-level of git repo cd `git rev-parse --show-toplevel` # checkout the contents of the REBASE_AND_CHANGE commit # which since we redid the rebase should result in only the changes # that were done on top of the rebase being staged for commit git checkout $REBASE_AND_CHANGE -- . # verify that these were the changes git diff --cached # amend before pushing git commit --amend # now push the changes git push gerrit HEAD:refs/for/ i.e. redo and push the rebase and checkout the original "rebase + changes" and then amend and push that. > At the very least, gerrit needs an undo function, or a way to make a > previous version of a patch become the latest for review again. > > And I agree it would be nice if gerrit could still show a clean diff > even when a patch contains both a rebase and some actual changes. True, as Giuseppe also noted that would be the best solution. -- Samuel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Pushing rebases and unrelated changes together is a sin
On 20 February 2013 09:43, Samuel Rødal wrote: > On 02/20/2013 09:22 AM, Thiago Macieira wrote: >> On quarta-feira, 20 de fevereiro de 2013 08.47.40, Samuel Rødal wrote: >>> The correct process when doing rebases is: >>> >>> git pull --rebase # or your favorite git work-flow >>> git push gerrit HEAD:refs/for/some_branch >>> # add comment in change on gerrit about being a pure rebase >>> # do some changes >>> git commit -a --amend >>> git push gerrit HEAD:refs/for/some_branch >> >> Actually, I'll go further: the above should be done ONLY if you are trying to >> solve a conflict. If you're not trying to solve a conflict, DON'T REBASE. > > I thought that went without saying :) When revisiting a patch from a few days or more ago, I guess I just don't trust that the change will apply cleanly or is definitely still applicable without further changes. So that would be the reason to cherry-pick and ensure it, rather than assuming it will be OK and then waiting for a CI failure, right? Fine, if the rebase has to be a separate push, we can just clutter up gerrit with more pushes. I typically do plenty of them anyway, due to various mistakes, to sync up between machines, to store intermediate work and so on. (And then people make snide comments like "geez, do they pay you by the patch?!?" so somebody is always dissatisfied.) Coming back and asking "would you please split the rebase from the changes" is also too easy for the reviewer to ask for, and not so easy to actually do, after having mixed them up. Then there's the scenario when my change depends on someone else's: then I don't have a choice - I have to base my work on top of the latest checkout (NOT latest cherry-pick) of his patch, in order to avoid doing a rebase of that change (plus its dependencies) when pushing. Then all of the patches get out-of-date together. And then some reviewer gives a -1 because I've been working on a checkout rather than a cherry-pick, so s/he is the one who finds out that it no longer applies cleanly. Now suppose the patch(es) that I depend on are rebased, and I didn't realize it: next time I push, I will push an outdated version of his patch(es) along with mine. Maybe that should be more interactive too, somehow, to be warned about such mistakes. Or maybe I should come up with a better way to push a single, isolated patch, without its dependencies. But then the trouble would be that you won't see the dependencies in gerrit, and have to remember the right order to merge them. At the very least, gerrit needs an undo function, or a way to make a previous version of a patch become the latest for review again. And I agree it would be nice if gerrit could still show a clean diff even when a patch contains both a rebase and some actual changes. So this bookkeeping stuff is still more complicated than it ought to be, IMO. I would very much like to be able to just concentrate on the code, but actually 90% of my job is overhead of various kinds. I'm not exaggerating. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Pushing rebases and unrelated changes together is a sin
On 02/20/2013 09:22 AM, Thiago Macieira wrote: > On quarta-feira, 20 de fevereiro de 2013 08.47.40, Samuel Rødal wrote: >> The correct process when doing rebases is: >> >> git pull --rebase # or your favorite git work-flow >> git push gerrit HEAD:refs/for/some_branch >> # add comment in change on gerrit about being a pure rebase >> # do some changes >> git commit -a --amend >> git push gerrit HEAD:refs/for/some_branch > > Actually, I'll go further: the above should be done ONLY if you are trying to > solve a conflict. If you're not trying to solve a conflict, DON'T REBASE. I thought that went without saying :) > If you rebase your branch often, like I do, then never push an update from the > actual branch. Instead, check out the old base for the commit, cherry-pick the > new commit, and push that. > > You can get the old base for the commit from the Gerrit interface. It lists > the SHA-1 of the previous commits, so it's just that plus ~. After I push my changes to Gerrit I don't keep them around locally, since Gerrit keeps track of them. My branchless work-flow for a new fix is: git fetch gerrit git checkout gerrit/stable # do necessary changes git commit -a git push gerrit HEAD:refs/for/stable If I need to push some modifications to any of my changes I simply use the "checkout" Download option in the Gerrit interface which gives you a command-line of the form: git fetch ssh://ro...@codereview.qt-project.org:29418/qt/qtbase refs/changes/37/48337/2 && git checkout FETCH_HEAD Gerrit has greatly cut down on the amount of local branches I need to have when working on multiple fixes intermittently. -- Samuel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Pushing rebases and unrelated changes together is a sin
On quarta-feira, 20 de fevereiro de 2013 08.47.40, Samuel Rødal wrote: > The correct process when doing rebases is: > > git pull --rebase # or your favorite git work-flow > git push gerrit HEAD:refs/for/some_branch > # add comment in change on gerrit about being a pure rebase > # do some changes > git commit -a --amend > git push gerrit HEAD:refs/for/some_branch Actually, I'll go further: the above should be done ONLY if you are trying to solve a conflict. If you're not trying to solve a conflict, DON'T REBASE. If you rebase your branch often, like I do, then never push an update from the actual branch. Instead, check out the old base for the commit, cherry-pick the new commit, and push that. You can get the old base for the commit from the Gerrit interface. It lists the SHA-1 of the previous commits, so it's just that plus ~. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center signature.asc Description: This is a digitally signed message part. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Pushing rebases and unrelated changes together is a sin
On 20 February 2013 08:47, Samuel Rødal wrote: > > > and it makes the diff between patch sets displayed by the Gerrit > interface (by choosing "Old Version History" to be other than Base) > totally unreadable. > > The correct process when doing rebases is: .. is it me or there's absolutely nothing wrong with the process itself, it's just Gerrit that it's unable to produce a the "right" diff? (So it's ok if we establish this convention, but as a workaround. Does anyone know if it has been fixed upstream?) Cheers, -- Giuseppe D'Angelo ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] Pushing rebases and unrelated changes together is a sin
Hello, it happens quite regularly that people do a rebase and in addition do changes in the same push. It looks like this: git pull --rebase # or cherry-pick or whatever you do # do some changes git commit -a --amend git push gerrit HEAD:refs/for/some_branch and it makes the diff between patch sets displayed by the Gerrit interface (by choosing "Old Version History" to be other than Base) totally unreadable. The correct process when doing rebases is: git pull --rebase # or your favorite git work-flow git push gerrit HEAD:refs/for/some_branch # add comment in change on gerrit about being a pure rebase # do some changes git commit -a --amend git push gerrit HEAD:refs/for/some_branch i.e. do rebases separately from changes (that are not essential for the rebase). Since it's a rather easy mistake to make (nothing in the interface prevents you from doing it and a lot of people probably aren't even aware of the correct process), how about we add some hook when pushing to gerrit that detects rebases and displays something like this? git push gerrit HEAD:refs/for/some_branch WARNING: You're pushing a rebase, please confirm that you have not made changes unrelated to the rebase (if you have, push the rebase first and then the changes) [y/n] At least that would give people a heads-up and the chance to correct their mistake. -- Samuel ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
