Showing review diffs from -current- parent commit

389 views
Skip to first unread message

Mark Derricutt

unread,
Jul 26, 2011, 5:56:50 PM7/26/11
to Repo and Gerrit Discussion
Hi all,

We're currently using Gerrit 2.1.6.1 and have started to notice a subtle behavior in the way gerrit shows its review diffs that 'allowed' some regressions to creep into our codebase.

The problem, as I see it is that the "base" revision of a review is the parent commit of the first patch set. Normally this isn't a problem and shows the diffs fine as you're committing against that patch, but in the case where someone has rebased a review onto a newer base - Gerrit now shows the changes from both the review AND the rebase - which gets mighty confusing, and in some cases actually hides changes that will be merged if the review is accepted.

The scenario I have is as follows ( assuming I can draw it properly )

- master
- develop - point-1
+- feature/review-a - review commit here
+- feature/review-b - review commit here

In the above we have two feature branches off the same parent commit on develop, if we review and accept accept review-a's commits and merge them into the develop branch, then rebase review-b ontop of develop:

- master
- develop - point-2 ( contains feature/review-a )
+- feature/review-b - review commit here

and push for review again, making new patch sets for our review, the diff's for the review will now include/show the diffs from review-a as being part of review-b, as the diff is made against the point-1 commit, rather than point-2.

I can see reasons for the current behavior, as you want to see -ALL- changes in this review, however after going back and forth on some (unfortunately) long standing reviews which after repeated rebases/push-for-reviews I ended up with something like 20+ patch sets, which grow exponentially due to pulling in changes from every other review that had been accepted.

Is there a way in Gerrit to make "base" point to the current reviews parent SHA? Or maybe to add a new entry in the patch history list for this new base?

If not, should I raise a bug ticket for this?

Mark

Martin Fick

unread,
Jul 26, 2011, 11:26:13 PM7/26/11
to Mark Derricutt, Repo and Gerrit Discussion
What you describe does not sound accurate. The base is always the parent of the patchset being diffed to. What you are describing sounds like the common complaint that when diffing between patchsets, the diff includes the diffs between the bases.

In other words, if ps1 is based on parent A, and ps2 is based on parent B, when diffing between ps1 and ps2, part of the diff will include the diff between A and B. However you should not see that if you diff between base and ps2. In that case, you should only see the diffs between B and ps2.

-Martin
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Mark Derricutt

unread,
Jul 28, 2011, 12:36:46 AM7/28/11
to Martin Fick, Repo and Gerrit Discussion
Now I read this it does make sense, we were doing a lot of ps1->ps2 style reviews, I guess it flew over the top of my head that its git doing the diff between the two separate parts of the tree.

Makes sense now ;-)  Cheers.

Mark

Edwin Kempin

unread,
Sep 27, 2012, 2:55:14 AM9/27/12
to JT Olds, repo-d...@googlegroups.com, Martin Fick


2012/9/27 JT Olds <he...@jtolds.com>
Does Gerrit have any ability to do a sort of higher level diff-of-diffs?

I (and many of my colleagues) commonly have this problem. We'll upload ps1, make some changes, rebase, upload ps2, and now a reviewer will want to see what changed between ps1 and ps2.

Gerrit gives you the ability to see the diff between ps1 and ps1^ easily enough, the diff between ps2 and ps2^, and the diff between ps1 and ps2 (including ancestors), but what this leaves out is the ability to see what the diff between the *diff of ps1 and ps1^* and the *diff of ps2 and ps2^* is. Which is the only thing actually semantically interesting to me as a reviewer.

What do other people do for this problem? If nothing right now, may I suggest a diff-of-diffs feature? 
Does this (open) issue [1] cover what you want?

[1] http://code.google.com/p/gerrit/issues/detail?id=217
 

-JT

JT Olds

unread,
Sep 27, 2012, 12:44:32 PM9/27/12
to Edwin Kempin, repo-d...@googlegroups.com, Martin Fick
Yep.

It's definitely possible to get the diff I want using Git. I'd love to
see something easier in Gerrit though.

-JT

Shawn Pearce

unread,
Sep 30, 2012, 8:37:00 PM9/30/12
to JT Olds, Edwin Kempin, repo-discuss, Martin Fick
On Thu, Sep 27, 2012 at 9:44 AM, JT Olds <he...@jtolds.com> wrote:
> It's definitely possible to get the diff I want using Git. I'd love to
> see something easier in Gerrit though.

How do you get the diff you want using git?

JT Olds

unread,
Oct 1, 2012, 2:45:49 PM10/1/12
to Shawn Pearce, Edwin Kempin, repo-discuss, Martin Fick
On Sun, Sep 30, 2012 at 6:37 PM, Shawn Pearce <s...@google.com> wrote:
> How do you get the diff you want using git?

The ticket Edwin linked[1] had one such suggestion. I guess I should
have said it's possible to get the diff I want manually, but it's a
pain.

[1] http://code.google.com/p/gerrit/issues/detail?id=217
Reply all
Reply to author
Forward
0 new messages