| Issue 217: | Patch history does not account for rebase |
1 of 58
Next ›
|
| 129 people starred this issue and may be notified of changes. | Back to list |
Sign in to add a comment
|
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
(No comment was entered for this change.)
Cc:
grzegorz.kossakowski
Sep 24, 2009
(No comment was entered for this change.)
Cc:
c...@android.com
Jun 10, 2010
Issue 551 has been merged into this issue.
Jun 7, 2011
Issue 990 has been merged into this issue.
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.
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.
Apr 25, 2012
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
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
Issue 1098 has been merged into this issue.
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.
Nov 15, 2012
@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
Oh right, I missed that difference. Thanks!
Oct 2, 2013
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
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
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
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
Oct 30, 2013
refer the discuss in https://gerrit-review.googlesource.com/#/c/33091/5//COMMIT_MSG
Feb 5, 2014
I made a reference to this issue in this discussion: https://groups.google.com/d/msg/repo-discuss/jCFPnrIIdLI/vczDFrdspdcJ
Jun 18, 2014
Is this change fixing issue 217 [1]? Can anyone please confirm? [1] https://gerrit-review.googlesource.com/#/c/57086/
Jun 18, 2014
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
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
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. |
||||||||
| ► Sign in to add a comment | |||||||||
Status: Accepted
Owner: ---