[openstack-dev] [all] Proper use of 'git review -R'

Jeremy Stanley fungi at yuggoth.org
Tue Dec 30 18:24:02 UTC 2014


On 2014-12-30 12:31:35 -0500 (-0500), David Kranz wrote:
[...]
> 1. This is really a UI issue, and one that is experienced by many.
> What is desired is an option to look at different revisions of the
> patch that show only what the author actually changed, unless
> there was a conflict.

I'm not sure it's entirely a UI issue. It runs deeper. There simply
isn't enough metadata in Git to separate intentional edits from
edits made to solve merge conflicts. Using merge commits instead of
rebases mostly solves this particular problem but at the expense of
introducing all sorts of new ones. A rebase-oriented workflow makes
it easier for merge conflicts to be resolved along the way, instead
of potentially nullifying valuable review effort at the very end
when it comes time to approve the change and it's no longer relevant
to the target branch.

There is a potential work-around, though it currently involves some
manual effort (not sure whether it would be sane to automate as a
feature of git-review). When you notice your change conflicts and
will need a rebase, first reset and stash your change, then reset
--hard to the previous patchset already in Gerrit, then rebase that
and push it (solving the merge conflicts if any), then pop your
stashed edits (solving any subsequent merge conflicts) and finally
push that as yet another patchset. This separates the rebase from
your intentional modifications though at the cost of rather a lot of
extra work.

Alternatively you could push your edits with git review -R and
_then_ follow up with another patchset rebasing on the target branch
and resolving the merge conflicts. Possibly slightly easier?

> 2. Using -R is dangerous unless you really know what you are
> doing. The doc string makes it sound like an innocuous way to help
> reviewers.

Not necessarily dangerous, but it does allow you to push changes
which are just going to flat fail all jobs because they can't be
merged to the target branch to get tested.
-- 
Jeremy Stanley



More information about the OpenStack-dev mailing list