Patch history does not account for rebase

[GoogleCodeExporter]

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/a
ndroid/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

Original issue reported on code.google.com by code-rev...@gtempaccount.com on 24 Sep 2009 at 7:42

[GoogleCodeExporter]

Original comment by sop+code@google.com on 24 Sep 2009 at 10:20

• Changed title: Patch history does not account for rebase
• Changed state: Accepted

[GoogleCodeExporter]

Original comment by sop+code@google.com on 24 Sep 2009 at 10:51

[GoogleCodeExporter]

Original comment by sop+code@google.com on 24 Sep 2009 at 10:58

[GoogleCodeExporter]

Issue 551 has been merged into this issue.

Original comment by sop@google.com on 10 Jun 2010 at 5:15

[GoogleCodeExporter]

Issue 990 has been merged into this issue.

Original comment by sop@google.com on 8 Jun 2011 at 2:14

[GoogleCodeExporter]

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.

Original comment by johnme@google.com on 15 Jun 2011 at 5:33

[GoogleCodeExporter]

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

Original comment by nas...@codeaurora.org on 15 Jun 2011 at 6:39

[GoogleCodeExporter]

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.

Original comment by marc.kho...@gmail.com on 25 Apr 2012 at 3:21

[GoogleCodeExporter]

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.

Original comment by dbailey%...@gtempaccount.com on 23 May 2012 at 4:42

[GoogleCodeExporter]

Issue 1098 has been merged into this issue.

Original comment by bklarson@gmail.com on 5 Oct 2012 at 10:05

[GoogleCodeExporter]

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.

Original comment by afzal...@gmail.com on 15 Nov 2012 at 4:11

[GoogleCodeExporter]

@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

Original comment by johnme@chromium.org on 15 Nov 2012 at 4:17

[GoogleCodeExporter]

Oh right, I missed that difference. Thanks!

Original comment by afzal...@gmail.com on 15 Nov 2012 at 4:20

[GoogleCodeExporter]

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.

Original comment by kamm.chr...@gmail.com on 3 Oct 2013 at 6:33

[GoogleCodeExporter]

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.

Original comment by mvtor...@gmail.com on 8 Oct 2013 at 4:21

[GoogleCodeExporter]

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

Original comment by mdemp...@google.com on 24 Oct 2013 at 9:02

[GoogleCodeExporter]

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

Original comment by gfide...@gmail.com on 30 Oct 2013 at 11:36

[GoogleCodeExporter]

refer the discuss in 
https://gerrit-review.googlesource.com/#/c/33091/5//COMMIT_MSG

Original comment by bruce.zu@sonymobile.com on 31 Oct 2013 at 5:31

[GoogleCodeExporter]

I made a reference to this issue in this discussion: 
https://groups.google.com/d/msg/repo-discuss/jCFPnrIIdLI/vczDFrdspdcJ

Original comment by gust...@cpqd.com.br on 5 Feb 2014 at 6:26

[GoogleCodeExporter]

Is this change fixing issue 217 [1]?
Can anyone please confirm?
[1] https://gerrit-review.googlesource.com/#/c/57086/

Original comment by mani.cha...@gmail.com on 18 Jun 2014 at 12:36

[GoogleCodeExporter]

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?

Original comment by piotr.fi...@gmail.com on 18 Jun 2014 at 7:34

[GoogleCodeExporter]

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

Original comment by k.arn...@sap.com on 6 Oct 2014 at 2:07