From be910538a575cb61e1e83b4c9fe7a196ac24ce0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Tue, 12 May 2015 12:47:17 -0400 Subject: [PATCH] Correct file list when comparing patchsets When comparing a patchset with another one, the added and deleted files were not displayed properly: -change 1,1 is adding file a, c and deleting d -change 1,2 is adding file a, b and deleting d When comparing 1,1 to 1,2, the file changed list should show that b was added and c was deleted but instead it was showing that only b was added. When comparing 1,2 to 1,1, the file changed list should show that b was deleted and c was added but instead it was showing that only c was added. I know that this behavior was intended[1], i.e. when comparing patchset with each other, the file list will not include files not in the base. The problem is that behavior was perceived as a bug by our users. When they compare patchset between each others, they expect to see the full list of files changed, even if the file is not in the base. [1] https://gerrit-review.googlesource.com/#/c/57086/4//COMMIT_MSG@10 Change-Id: I1bd7e6354617d921a77115c47543328a4a66999f (cherry picked from commit 314a096181e205faae5b0b66847b19d23db1d5d6) --- .../server/change/PatchListCacheIT.java | 18 ++++++++++++++++-- .../gerrit/server/patch/PatchListLoader.java | 14 +++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java index d598b06bc7..848eeb16af 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -142,11 +142,19 @@ public class PatchListCacheIT extends AbstractDaemonTest { pushHead(git, "refs/for/master", false); ObjectId b = getCurrentRevisionId(c.getChangeId()); - // Compare Change 1,1 with Change 1,2 (+FILE_B) + // Compare Change 1,1 with Change 1,2 (+FILE_B, -FILE_C) List entries = getPatches(a, b); - assertThat(entries).hasSize(2); + assertThat(entries).hasSize(3); assertModified(Patch.COMMIT_MSG, entries.get(0)); assertAdded(FILE_B, entries.get(1)); + assertDeleted(FILE_C, entries.get(2)); + + // Compare Change 1,2 with Change 1,1 (-FILE_B, +FILE_C) + List entriesReverse = getPatches(b, a); + assertThat(entriesReverse).hasSize(3); + assertModified(Patch.COMMIT_MSG, entriesReverse.get(0)); + assertDeleted(FILE_B, entriesReverse.get(1)); + assertAdded(FILE_C, entriesReverse.get(2)); } @Test @@ -180,6 +188,12 @@ public class PatchListCacheIT extends AbstractDaemonTest { assertThat(entries).hasSize(2); assertModified(Patch.COMMIT_MSG, entries.get(0)); assertAdded(FILE_C, entries.get(1)); + + // Compare Change 1,2 with Change 1,1 (-FILE_C) + List entriesReverse = getPatches(b, a); + assertThat(entriesReverse).hasSize(2); + assertModified(Patch.COMMIT_MSG, entriesReverse.get(0)); + assertDeleted(FILE_C, entriesReverse.get(1)); } private static void assertAdded(String expectedNewName, PatchListEntry e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 0b6688c737..878ff18923 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -19,6 +19,7 @@ import com.google.common.base.Function; import com.google.common.base.Throwables; import com.google.common.cache.CacheLoader; import com.google.common.collect.FluentIterable; +import com.google.common.collect.Iterables; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.RefNames; @@ -161,9 +162,16 @@ public class PatchListLoader extends CacheLoader { List diffEntries = df.scan(aTree, bTree); Set paths = key.getOldId() != null - ? FluentIterable.from(patchListCache.get( - new PatchListKey(key.projectKey, null, key.getNewId(), - key.getWhitespace())).getPatches()) + ? FluentIterable.from( + Iterables.concat( + patchListCache.get( + new PatchListKey(key.projectKey, null, + key.getNewId(), key.getWhitespace())) + .getPatches(), + patchListCache.get( + new PatchListKey(key.projectKey, null, + key.getOldId(), key.getWhitespace())) + .getPatches())) .transform(new Function() { @Override public String apply(PatchListEntry entry) {