Open Stack

On Tue, Dec 30, 2014 at 9:37 AM, Jeremy Stanley <fungi at yuggoth.org> wrote:
> On 2014-12-30 09:46:35 -0500 (-0500), David Kranz wrote:
> [...]
>> Can some one explain when we should *not* use -R after doing 'git
>> commit --amend'?
> [...]
>
> In the standard workflow this should never be necessary. The default
> behavior in git-review is to attempt a rebase and then undo it
> before submitting. If the rebase shows merge conflicts, the push
> will be averted and the user instructed to deal with those
> conflicts. Using -R will skip this check and allow you to push
> changes which can't merge due to conflicts.

tl;dr:  I suggest an enhancement to git review which will help us
avoid unintentionally uploading new patch sets when a change depends
on another change.

I've been thinking about this a bit since I had a discussion in the
infra room last month.  I have been using --no-rebase every time I run
git review and I've been telling others to do the same.  I even
proposed setting defaultrebase to 0 for the neutron project [1].  At
that time, I learned that this is expected to be the default for
current versions of git review.

I had a few experiences during the development of the DVR feature this
past summer that leave me believing that there is still a problem.  I
saw a few cases where multiple authors were working on dependent
patches and one author's rebase of an older dependency clobbered newer
changes.  This required me to step in and manually find and restore
the clobbered changes.  Things got better when I asked all of the
authors to always use --no-rebase and we manually managed necessary
rebases due to merge conflicts independently of other changes to the
patch sets.

I haven't had time to dig up all of the details about what happened.
I will try to find some time to do that soon.  However, I have an idea
of where the problem is...

The problem happens when a chain of dependencies is rebased together
to master.  This creates new versions of dependencies as well as the
top patch.  The "new" version of the dependency might actually be a
rebased version of an older patch set.  When this "new" version is
uploaded, it clobbers changes to the dependency.  I think this is
generally the wrong thing to do; especially when a patch set chain has
multiple authors.

This is not the way gerrit rebases when you use the rebase button in
the UI.  Gerrit will rebase a patch set to the latest patch set of the
change on which it depends.  It there is no dependency, then it will
rebase to master.

I'm not sure if this is git review's fault or not.  I know in older
versions of git review it was at fault.  More recent incidents could
have been due to manually initiated rebases which were done
incorrectly.  However, I had the impression that git review would do
rebases in this way and our problems on DVR seemed to stop when I
trained the team to use --no-rebase.

*** I can suggest an enhancement to git review which will help out in
this situation.  The change is around how git review warns about
uploading multiple patch sets.  It doesn't seem to be smart enough to
tell when it will actually upload a new version of a dependency.  That
is, it warns even when the commit id of a dependency matches one that
is already in gerrit as if it were going to create a new patch set.
It is impossible to tell -- without manually checking out of band --
if it is *really* going to create a new patch set.  I doubt many
people (besides me) actually bother to go to gerrit to compare commit
ids to see what will really happen.

Git review should check the commit ids in gerrit.  It should not stop
to warn about commit ids that already exist in gerrit.  Then, it
should warn a bit *louder* about commit ids which are not in gerrit
because many people have become desensitized to the current warning.

Another habit that I have developed is to always download the latest
version of a patch set, work on it fairly quickly, and then upload it
again.  I don't keep a lot of WIP locally for extended periods of
time.  I never know when someone is going to depend on a patch of mine
and rebase it -- whether intentionally or not -- and upload the
rebased version.

I've dreamed about adding features to git/gerrit to manage patch set
iterations within a change, and dependent changes more formally so
that these problems can be more easily detected and handled by the
tools.  I think if git rebase could leave some sort of soft trail it
might help but I haven't thought through this completely.  I can see
problems with how to handle this in a distributed way.

Carl

[1] https://review.openstack.org/#/c/140863/