474262: cannot retrieve patch set containing renamed binary file

Change-Id: I5bf2a6c71ce04580fca3ea74bf8855f36927d0bf
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=474262

3 files changed, 30 insertions, 16 deletions

diff --git a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
index 94808d325..6af841ab5 100644
--- a/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
+++ b/org.eclipse.mylyn.gerrit.core/src/org/eclipse/mylyn/internal/gerrit/core/client/GerritClient.java
@@ -29,6 +29,7 @@ import java.net.UnknownHostException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -455,7 +456,7 @@ public abstract class GerritClient extends ReviewsClient {
 
 	protected void fetchLeftBinaryContent(final PatchScriptX patchScript, final Patch.Key key,
 			final PatchSet.Id leftId, final IProgressMonitor monitor) throws GerritException {
-		if (patchScript.getChangeType() != ChangeType.ADDED) {
+		if (EnumSet.of(ChangeType.DELETED, ChangeType.MODIFIED).contains(patchScript.getChangeType())) {
 			byte[] binaryContent = fetchBinaryContent(getUrlForPatchSetOrBase(key, leftId), monitor);
 			patchScript.setBinaryA(binaryContent);
 		}
diff --git a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/gerrit/tests/core/client/GerritClientTest.java b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/gerrit/tests/core/client/GerritClientTest.java
index 7783a2b24..ab9af976e 100644
--- a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/gerrit/tests/core/client/GerritClientTest.java
+++ b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/gerrit/tests/core/client/GerritClientTest.java
@@ -27,6 +27,7 @@ import static org.mockito.Mockito.when;
 
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
+import java.util.EnumSet;
 import java.util.List;
 
 import junit.framework.TestCase;
@@ -350,19 +351,6 @@ public class GerritClientTest extends TestCase {
 
 	@Test
 	public void testFetchLeftBinaryContent() throws Exception {
-		Id ps2 = Id.fromRef("refs/changes/34/1234/2");
-		Id ps4 = Id.fromRef("refs/changes/34/1234/4");
-		Key key = new Key(ps4, "/mylyn/gerrit/File.jpg");
-		PatchScriptX script = mock(PatchScriptX.class);
-
-		for (ChangeType type : ChangeType.values()) {
-			TestGerritClient client = createSpy();
-			doReturn(null).when(client).fetchBinaryContent(any(String.class), any(IProgressMonitor.class));
-			when(script.getChangeType()).thenReturn(type);
-			client.fetchLeftBinaryContent(script, key, ps2, new NullProgressMonitor());
-			verify(client, type == ChangeType.ADDED ? never() : times(1)).fetchBinaryContent(any(String.class),
-					any(IProgressMonitor.class));
-		}
 		fetchBinaryContentForSide(false);
 	}
 
@@ -378,9 +366,9 @@ public class GerritClientTest extends TestCase {
 		PatchScriptX script = mock(PatchScriptX.class);
 
 		for (ChangeType type : ChangeType.values()) {
-			when(script.getChangeType()).thenReturn(type);
 			TestGerritClient client = createSpy();
 			doReturn(null).when(client).fetchBinaryContent(any(String.class), any(IProgressMonitor.class));
+			when(script.getChangeType()).thenReturn(type);
 			VerificationMode fetchBinaryContentExpected = times(1);
 			if (rightSide) {
 				client.fetchRightBinaryContent(script, key, ps2, new NullProgressMonitor());
@@ -389,7 +377,7 @@ public class GerritClientTest extends TestCase {
 				}
 			} else {
 				client.fetchLeftBinaryContent(script, key, ps2, new NullProgressMonitor());
-				if (type == ChangeType.ADDED) {
+				if (!EnumSet.of(ChangeType.DELETED, ChangeType.MODIFIED).contains(type)) {
 					fetchBinaryContentExpected = never();
 				}
 			}
diff --git a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/PatchSetRemoteFactoryTest.java b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/PatchSetRemoteFactoryTest.java
index f9825ee02..470f2cbfb 100644
--- a/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/PatchSetRemoteFactoryTest.java
+++ b/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/PatchSetRemoteFactoryTest.java
@@ -219,6 +219,20 @@ public class PatchSetRemoteFactoryTest extends GerritRemoteTest {
 		assertPatchContent(patchScript, empty(), equalTo(fileContent3));
 	}
 
+	@Test
+	public void testCompareRenamedImage() throws Exception {
+		String fileName = "test.png";
+		String newFileName = "renamed-" + fileName;
+		String path = "testdata/binary/gerrit.png";
+		byte[] fileContent = commitAndRenameFile(fileName, newFileName, path);
+
+		PatchSetDetail detail2 = retrievePatchSetDetail("2");
+		PatchSetDetail detail3 = retrievePatchSetDetail("3");
+
+		PatchScriptX patchScript = loadPatchSetContent(newFileName, detail2, detail3);
+		assertPatchContent(patchScript, empty(), equalTo(fileContent));
+	}
+
 	private PatchScriptX loadPatchSetContent(String fileName, PatchSetDetail base, PatchSetDetail target)
 			throws GerritException {
 		PatchSetContent content = new PatchSetContent(base.getPatchSet(), target.getPatchSet());
@@ -257,6 +271,17 @@ public class PatchSetRemoteFactoryTest extends GerritRemoteTest {
 		reviewHarness.retrieve();
 	}
 
+	private byte[] commitAndRenameFile(String fileName, String newFileName, String path) throws Exception, IOException {
+		byte[] fileContent = commitFile(fileName, path);
+		File file = CommonTestUtil.getFile(this, path);
+		CommitCommand command = reviewHarness.createCommitCommand();
+		reviewHarness.removeFile(fileName);
+		reviewHarness.addFile(newFileName, file);
+		reviewHarness.commitAndPush(command);
+		reviewHarness.retrieve();
+		return fileContent;
+	}
+
 	@Test
 	public void testPatchSetComments() throws Exception {
 		TestRemoteObserverConsumer<IReviewItemSet, List<IFileItem>, String, PatchSetContent, String, Long> patchSetObserver //