New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 137 users

Issue metadata

Status: Released
Owner:
Closed: Jun 2017
Cc:



Sign in to add a comment

Patch history does not account for rebase

Reported by code-rev...@gtempaccount.com, Sep 24 2009

Issue description

Reported by Cary Clark <cary@android.com> on Tue Jun 09 07:23:38 PDT 2009
Source: JIRA GERRIT-217

The patch history diff options suggests that it is possible to see what
changes were made between patches by the patch author. In practice, if a
rebase was required between patches, it shows the differences including
changes unrelated to this patch. This makes it very difficult to know what
changes the author made, if any, between patches.

to reproduce, go to:
https://android-git.corp.google.com/g/Gerrit#patch,sidebyside,3449,2,core/java/android/webkit/WebView.java

choose in patch history:   old version: base, new version: patch set 1
choose in patch history:   old version: base, new version: patch set 2 (note
that it is the same as patch set 1)

choose in patch history:   patch set 1, patch set 2 --
  expected: no difference -- or, only rebase difference
  observed: rebase difference + patch difference
 

Comment 1 by s...@google.com, Sep 24 2009

Status: Accepted
Summary: Patch history does not account for rebase

Comment 2 by s...@google.com, Sep 24 2009

Comment 3 by s...@google.com, Sep 24 2009

Comment 4 by sop@google.com, Jun 10 2010

 Issue 551  has been merged into this issue.

Comment 5 by sop@google.com, Jun 8 2011

 Issue 990  has been merged into this issue.

Comment 6 by johnme@google.com, Jun 15 2011

An effective way of stripping out differences due to rebasing is to cherry-pick the older patch onto the parent of the newer patch, then diff them. Sometimes you'll get merge conflicts during the cherry-pick, but then you can just leave in diff3 conflict markers and the resulting diff will show how the conflicts were resolved (which is actually very useful).

For the example above, this method produces an empty diff (as expected):

$ git clone -l -s -n $ANDROID_BUILD_TOP/frameworks/base /tmp/interdiff > /dev/null
$ cd /tmp/interdiff
$ git fetch ssh://$USER@android-git.corp.google.com:29418/platform/frameworks/base refs/changes/49/3449/2:patchset2 &> /dev/null
$ git checkout patchset2^ > /dev/null
$ git fetch ssh://$USER@android-git.corp.google.com:29418/platform/frameworks/base refs/changes/49/3449/1 > &/dev/null
$ git config merge.conflictstyle diff3
$ git cherry-pick --no-commit FETCH_HEAD || git add .
$ git diff -R --cached patchset2
$ # Observe lack of output
$ cd -
$ rm -rf /tmp/interdiff

I've tested it on complicated changes where a large patch was simultaneously updated and rebased, and it nicely extracts only the relevant differences. It would be great to get this integrated into Gerrit.
Project Member

Comment 7 by nas...@codeaurora.org, Jun 15 2011

@johnme I tried that method just now and I'm pretty pleased with it. Not the cleanest looking thing, but does a much better job showing me what I want than anything Gerrit offers currently.
This feature would be a great value to do a thorough review.

I was thinking of the following approach to show proper diff between patch X and rebased patch Y:

1- diff patch X with Base which will give list of files X
2- diff rebased patch Y with Base which will give list of files Y
3- do the union of list of file X and list of files Y giving files XY.  This is the list of files that were modified by patch X and/or patch Y.  Those are the only files we care about.
4- diff patch Y with patch X.  Because of the rebase, we get many files that should not be there.  Take the intersection of this result with the list of files XY from step 3.  This should give you the part of the diff that we care about.


I normally use the following trick to compare a rebased branch against it's previous state while ignoring changes due to the branch it was rebased against. Obviously only works properly if the branches/commits you are comparing are both anchored off of the same branch, but that should hold true when comparing patchsets in gerrit.

git diff ^master my_branch@{1} my_branch

Order does seem to impact the output you get back, haven't worried about fully understanding why. But if someone could, and jgit supports the ^<branch> to ignore commits when generating the diff, it looks like it could be a solution.
Project Member

Comment 10 by bklar...@gmail.com, Oct 5 2012

 Issue 1098  has been merged into this issue.

Comment 11 by afzal...@gmail.com, Nov 15 2012

Has this been fixed? I tried to see this in the link given in duplicate  issue 1098 

http://gerrit.chromium.org/gerrit/#change,6102

And I don't see this problem there anymore.
@afzal: don't think so. For example notice that the diff between patch sets 1 and 3 includes this irrelevant file which wasn't modified in either patch set:
https://gerrit.chromium.org/gerrit/#/c/6102/1..3/host/cros_workon

Comment 13 by afzal...@gmail.com, Nov 15 2012

Oh right, I missed that difference. Thanks!
I made a hack that works for me:

https://github.com/ckamm/gerrit/tree/interdiff

I don't think it's ready for upstream and I've no motivation left to polish it, add configuration switches, ..., and eventually champion it through code review. Maybe it can serve as inspiration for someone who wants to do it properly.
The following seem to work on my local, on a "simple" issue Im working on...

git difftool HEAD~1...<commit_id_of_other_patchset> -- ./

where HEAD points to the latest patchset.
As an interim solution, is it at least possible to have the patch set viewer include an option for showing a "base" link for all parent revision's, not just for the very first patch's parent revision?

E.g., if I post patch sets 1 and 2, rebase, post 3 and 4, rebase, and post 5, it would be nice if the Patch Sets list showed something like "[1^] 1 2 [3^] 3 4 [5^] 5" instead of "Base 1 2 3 4 5".

Comment 17 by gfide...@gmail.com, Oct 30 2013

I wonder if it wouldn't make sense to hack this in the view only by just hiding from the list of changed files the ones which do not appear in the patchset one is comparing to
I made a reference to this issue in this discussion: https://groups.google.com/d/msg/repo-discuss/jCFPnrIIdLI/vczDFrdspdcJ
Is this change fixing  issue 217  [1]?
Can anyone please confirm?
[1] https://gerrit-review.googlesource.com/#/c/57086/
First, "When comparing two patch sets only files that were touched in the new patch set should be listed" -- what about files that were touched in the old patch set but not in the new one? The listing will now include too little information (vs too much as it used to be).

Second, "this change fixing  issue 217 "? Does it hide or do something else smart with changes between patch sets that are result of some intermediate commits the new patch set includes because of rebase?

Comment 22 by k.arn...@sap.com, Oct 6 2014

I'm not sure if this is the correct place to post this, but:
The behavior discussed in [1] is very irritating, as diff is a noncommutative operation now. When diffing ps4..ps6 I'd expect the same results as when diffing ps6..ps4, up to a different sign.

In the current implementation, the noncommutativeness even propagates down to the side-by-side diff screen, which in certain cases tells me that that two files are the same, even if they aren't.

[1] https://gerrit-review.googlesource.com/#/c/57086/4//COMMIT_MSG@10

Comment 23 by new...@gmail.com, Sep 9 2015

The last two comments address the same bug as issue 2802 and are fixed by my patchset referenced there.

Between commit 67a2e7e (Limit file list to files that were touched in new patch set 2014-05-12) and my fix for issue 2802, this issue could be considered closed; however johnme@google.com's idea for a more robust way of filtering out "unrelated" changes seems interesting so it may be worth leaving open for that.
Another related issue is: review comments made on the "base" version magically disappear every time there's a rebase.

For instance imagine someone removes a block with ten lines of code (unrelated to each other). Then the reviewer asks something about one of the lines in the middle of that block. His question will disappear after the first rebase with zero clue of what happened.

> As an interim solution, is it at least possible to have the patch set viewer include an option for showing a "base" link for all parent revision's, not just for the very first patch's parent revision?
> E.g., if I post patch sets 1 and 2, rebase, post 3 and 4, rebase, and post 5, it would be nice if the Patch Sets list showed something like:
> - "[1^] 1 2 [3^] 3 4 [5^] 5" instead of:
> - "Base 1 2 3 4 5".

+1

Or, probably more intuitive for new users not familiar with "git help revisions":
- "Base1 1 2 Base3 3 4 Base5 5" instead of "Base 1 2 3 4 5".

Or, if the above is too much work, then this should be cheap enough to implement:
- Base1 1 2 3 4 5 # When revision 1 is selected
- Base2 1 2 3 4 5 # When revision 2 is selected
- Base3 1 2 3 4 5 # When revision 3 is selected
- etc.

Project Member

Comment 25 by jrn@google.com, Aug 11 2016

 Issue 2375  has been merged into this issue.
Project Member

Comment 26 by logan@google.com, Aug 17 2016

Labels: Priority-2
Project Member

Comment 27 by alic...@google.com, May 16 2017

Owner: alic...@google.com
Status: ChangeUnderReview (was: Accepted)
https://gerrit-review.googlesource.com/105834
Wow, I can't wait for this, looks awesome. To give an example: this would (retroactively will?) reduce the diff between revision 25 and rebased 26 from 1200 lines to just ONE line!!

https://chromium-review.googlesource.com/c/346001/25..26/drivers/gpu/drm/i915/intel_display.c

Project Member

Comment 29 by alic...@google.com, Jun 8 2017

 Issue 5772  has been merged into this issue.
Project Member

Comment 30 by aga...@chromium.org, Jun 8 2017

 Issue chromium:730932  has been merged into this issue.
Project Member

Comment 31 by alic...@google.com, Jun 13 2017

Status: Submitted (was: ChangeUnderReview)
The relevant changes [1], [2], and [3] for this bug/feature were submitted yesterday and will hopefully be live soon. A few notes on the new behavior:
- Modifications which can clearly be attributed to other commits between two patch sets of a change are marked with a different color (orange/blue instead of red/green).
- If those modifications overlap or touch (with respect to lines) the modifications which exist between the two patch sets, they are marked as regular modifications between those patch sets. (If desired and reasonable, improvements could be made in the future.)
- The number of added/deleted lines on the change screen omits the lines of modifications which are attributed to other commits between the patch sets.
- Files which are touched by the patch sets but which contain only those other modifications are not listed among the modified files.
- The different coloring is only available on PolyGerrit (the new UI). The adjusted number of added/deleted lines is visible on both UIs.

FYI: To simplify finding the relevant modifications in a file, I intend to adjust the behavior of the "next/previous diff chunk" keyboard shortcuts in a future CL.

[1] https://gerrit-review.googlesource.com/105834
[2] https://gerrit-review.googlesource.com/106342
[3] https://gerrit-review.googlesource.com/105836
Are these live yet? I have a diff that combines both rebased and modified lines: https://chrome-internal-review.googlesource.com/c/391028/2..3 and would love to be able to see which ones are which.
Project Member

Comment 33 by ekempin@google.com, Jun 16 2017

No, this is not rolled out yet. Maybe next week.
Apparently this did not make it into a released version yet - any ETA?
Project Member

Comment 35 by alic...@google.com, Oct 10 2017

It's part of 2.15-rc1.
Can you tell whether this is will work retroactively on old reviews as long as the software is upgraded?
Project Member

Comment 37 by dborowitz@google.com, Oct 10 2017

re #34: It's in 2.15, which is currently at RC1.
re #36: Yes.
> this will retroactively reduce the diff between revision 25 and rebased 26 from 1200 lines to just ONE line!!
> https://chromium-review.googlesource.com/c/346001/25..26/drivers/gpu/drm/i915/intel_display.c

I think this is a very good test case, best I ever saw so far. Did you use it? The number of green and red lines there did go down a lot but not as much as I expected.

> If those modifications overlap or touch (with respect to lines) the modifications which exist between the two patch sets, they are marked as regular modifications between those patch sets.

Neither revision 25 or revision 26 touch any line of code in the 200-13500 range:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/346001/25/drivers/gpu/drm/i915/intel_display.c
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/346001/26/drivers/gpu/drm/i915/intel_display.c
(25 and 26 are nearly identical and quite small, about 130 lines changed)

... yet many areas in this untouched 200-13000 range are still shown in green/red, examples at lines 2360, 4556, 4662, 12834,... (line numbers for revision 26 on the right side)
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/346001/25..26/drivers/gpu/drm/i915/intel_display.c

Another relatively big and spurious chunk of red/green coloring caused by the rebase and not adjacent to any actual change on line 15682


> To simplify finding the relevant modifications in a file, I intend to adjust the behavior of the "next/previous diff chunk" keyboard shortcuts in a future CL.

A mode that just hides/folds the orange/blue sections would be really useful.

By the way I don't think it's a good idea to use two shades of the same color (blue-green) on the right side, too easily confused with each other (and I have no known color blindness problem :-) Use a darker blue?


Tested with Gerrit Code Review (2.15-rc1-652-g6ff69ab0a2)

Project Member

Comment 39 by alic...@google.com, Dec 7

Sorry for the late reply and thanks for the detailed analysis and feedback.

I agree that https://chromium-review.googlesource.com/c/346001/25..26/drivers/gpu/drm/i915/intel_display.c is a very nice test case. It's one of the examples I've been using as a benchmark to see how good the algorithm performs.

The algorithm actually is much better than the diff view shows. The diff area on the change screen claims that only 11 lines were added and 3 lines were removed. Those values reflect the actual output of the algorithm. Due to some 'optimizations' which we apply afterwards for the intraline diffs, a lot of the hunks which were identified as being introduced by a rebase aren't marked as such on the diff screen.

I've planned to fix this issue for quite some time but I had to postpone it due to more important matters. To fix it in a proper way without some kind of workaround/hack, we would need to rework the diff caches. That's something I had planned (and hoped) to do soon but it doesn't seem that I can work on this in the near future. If possible, I'll try to get in the workaround in the meantime but I can't promise anything.

By the way, I asked myself whether the algorithm can do better than those 11 added lines and the answer is: Yes, I think so. I've got some ideas on how to improve it but, again, I won't have time to work on them soon.

Regarding the colors: I'm totally aware that they are far from optimal and that we need a better representation. That's tracked by Issue 6926. Sorry for not implementing a better UI representation in the first place. I had to decide between not having the feature at all and a quickly implemented (but suboptimal) UI representation.
> The diff area on the change screen claims that only 11 lines were added and 3 lines were removed.

I missed that indeed, thanks!

> Due to some 'optimizations' which we apply afterwards for the intraline diffs, a lot of the hunks which were identified as being introduced by a rebase aren't marked as such on the diff screen.

Strange; any longer explanation somewhere?

> whether the algorithm can do better than those 11 added lines

Feels IMHO lower priority than presenting as rebase diff all the very many lines *already* identified as such.

> I had to decide between not having the feature at all and a quickly implemented (but suboptimal) UI representation.

As long as the expectations are clear(er) I think it's great to already have something up and running in production.

Project Member

Comment 41 by alic...@google.com, Dec 15

>> Due to some 'optimizations' which we apply afterwards for the intraline diffs, a lot of the hunks which were identified as being introduced by a rebase aren't marked as such on the diff screen.

> Strange; any longer explanation somewhere?

The individual edits are identified by their file path and position within the file. As we modify the position within the files due to https://gerrit-review.googlesource.com/c/gerrit/+/13844, we lose the knowledge whether those edits were introduced by a rebase.
Project Member

Comment 42 by alic...@google.com, Apr 6

Labels: FixedIn-2.15
Status: Released (was: Submitted)
I fixed the issues with the false negatives I mentioned in comment 39 (see [1] and [2]). The original feature is part of 2.15. The mentioned fixes will be part of 2.15.1.

[3] should show the improvements probably next week.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/161510
[2] https://gerrit-review.googlesource.com/c/gerrit/+/161570
[3] https://chromium-review.googlesource.com/c/346001/25..26/drivers/gpu/drm/i915/intel_display.c 

Sign in to add a comment