[Development] Pushing rebases and unrelated changes together is a sin

[Samuel Rødal]

On 02/20/2013 01:01 PM, Shawn Rutledge wrote:
> On 20 February 2013 09:43, Samuel Rødal <samuel.rodal at digia.com> 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://<user>@codereview.qt-project.org:29418/qt/qtbase 
refs/changes/<some-change-identifier> && git cherry-pick FETCH_HEAD
# push the rebase
git push gerrit HEAD:refs/for/<branch>
# 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/<branch>

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