opendev/gerrit
Code Issues Proposed changes

Correct file list when comparing patchsets

Browse Source 
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 314a096181)
This commit is contained in:
Hugo Arès 2015-05-12 12:47:17 -04:00
parent 1c4a7ffc35
commit be910538a5
2 changed files with 27 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java
View File

@ -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<PatchListEntry> 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<PatchListEntry> 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<PatchListEntry> 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) {

14
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@ -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<PatchListKey, PatchList> {
List<DiffEntry> diffEntries = df.scan(aTree, bTree);
Set<String> 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<PatchListEntry, String>() {
@Override
public String apply(PatchListEntry entry) {