Issue 217: Patch history does not account for rebase
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
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
Sep 24, 2009
#1 sop+code@google.com
(No comment was entered for this change.)
Summary: Patch history does not account for rebase
Status: Accepted
Owner: ---
Sep 24, 2009
#2 sop+code@google.com
(No comment was entered for this change.)
Cc: grzegorz.kossakowski
Sep 24, 2009
#3 sop+code@google.com
(No comment was entered for this change.)
Cc: c...@android.com
Jun 10, 2010
#4 sop@google.com
 Issue 551  has been merged into this issue.
Jun 7, 2011
#5 sop@google.com
 Issue 990  has been merged into this issue.
Jun 15, 2011
#6 johnme@google.com
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.
Jun 15, 2011
Project Member #7 nas...@codeaurora.org
@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.
Apr 25, 2012
#8 marc.kho...@gmail.com
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.


May 23, 2012
#9 dbailey%...@gtempaccount.com
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.
Oct 5, 2012
Project Member #10 bklarson@gmail.com
 Issue 1098  has been merged into this issue.
Nov 15, 2012
#11 afzal...@gmail.com
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.
Nov 15, 2012
#12 johnme@chromium.org
@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
Nov 15, 2012
#13 afzal...@gmail.com
Oh right, I missed that difference. Thanks!
Oct 2, 2013
#14 kamm.chr...@gmail.com
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.
Oct 8, 2013
#15 mvtor...@gmail.com
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.
Oct 24, 2013
#16 mdemp...@google.com
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".
Oct 30, 2013
#17 gfide...@gmail.com
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
Feb 5, 2014
#19 gust...@cpqd.com.br
I made a reference to this issue in this discussion: https://groups.google.com/d/msg/repo-discuss/jCFPnrIIdLI/vczDFrdspdcJ
Jun 18, 2014
#20 mani.cha...@gmail.com
Is this change fixing issue 217 [1]?
Can anyone please confirm?
[1] https://gerrit-review.googlesource.com/#/c/57086/
Jun 18, 2014
#21 piotr.fi...@gmail.com
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?
Oct 6, 2014
#22 k.arn...@sap.com
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
Sep 9, 2015
#23 new...@gmail.com
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.