Open Stack

On Tue, Dec 30, 2014 at 11:24 AM, Jeremy Stanley <fungi at yuggoth.org> wrote:
> 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.

Jeremy is correct here.  I've dreamed about how to enhance git to
support this sort of thing more formally but it isn't an easy problem
and wouldn't help us in the short term anyway.

To overcome this, I hacked out a script [1] which rebases older patch
sets to the same parent as the most current patch set to help me
compare across rebases.  I've found it very handy in certain
situations.  I can see how conflicts were handled as well as what
other changes were made outside the scope of merge conflict
resolution.  I use it by downloading the latest patch set with "git
review -d XXXXX" and then I compare to a previous patch set (NN) by
supplying that patch set number on the command line.

I once had dreams of adding this capability to gerrit but I found the
gerrit development learning curve to be a bit steep for the time I
had.

> 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?

I'm a strong proponent of splitting rebases (with merge conflict
resolution) from other manual changes.  This is a help to reviewers.
If someone tells me that a patch set is a pure rebase to resolve
conflicts then I can "review" it by repeating the rebase myself to see
if I get the same answer.

Both suggestions above are good ones.  Which one you use is a matter
of preference IMO.  I personally prefer the latter (push with -R and
then resolve conflicts) because it is easier on me.

>> 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.

I agree there is no danger.  As I've stated in my other post, I have
*always* used it for two years and have seen no danger.  I have come
to accept the failing jobs as a regular and welcome part of my work
flow.  If these failing jobs are taking a lot of resources then we
need some redesign in infrastructure to fail them more quickly and
cheaply so that resources can be spared from having to test patch sets
which are in conflict.

Carl

[1] http://paste.openstack.org/show/155614/