Merge "PatchApplier: Check for existence of src/dest files before any operation"

4 files changed, 131 insertions, 23 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java
index 2a7ad2a815..eeec149871 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/patch/PatchApplierTest.java
@@ -93,12 +93,16 @@ public class PatchApplierTest {
 		}
 
 		protected void initPreImage(String aName) throws Exception {
-			File f = new File(db.getWorkTree(), aName);
 			preImage = IO
 					.readWholeStream(getTestResource(aName + "_PreImage"), 0)
 					.array();
+			addFile(aName, preImage);
+		}
+
+		protected void addFile(String aName, byte[] b) throws Exception {
+			File f = new File(db.getWorkTree(), aName);
+			Files.write(f.toPath(), b);
 			try (Git git = new Git(db)) {
-				Files.write(f.toPath(), preImage);
 				git.add().addFilepattern(aName).call();
 			}
 		}
@@ -373,6 +377,68 @@ public class PatchApplierTest {
 		}
 
 		@Test
+		public void testAddAlreadyExistingFile() throws Exception {
+			addFile("M1", "existing content".getBytes());
+			init("M1", false, false);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
+		public void testDeleteNonexistentFile() throws Exception {
+			init("NonASCIIDel", false, false);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
+		public void testModifyNonexistentFile() throws Exception {
+			init("ShiftDown", false, true);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
+		public void testRenameNonexistentFile() throws Exception {
+			init("RenameNoHunks", false, true);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
+		public void testCopyNonexistentFile() throws Exception {
+			init("CopyWithHunks", false, true);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
+		public void testCopyOnTopAlreadyExistingFile() throws Exception {
+			addFile("CopyResult", "existing content".getBytes());
+			init("CopyWithHunks", true, false);
+
+			Result result = applyPatch();
+
+			assertEquals(1, result.getErrors().size());
+			assertEquals(0, result.getPaths().size());
+		}
+
+		@Test
 		public void testDoesNotAffectUnrelatedFiles() throws Exception {
 			initPreImage("Unaffected");
 			String expectedUnaffectedText = initPostImage("Unaffected");
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 1655cce1e1..78dd9bd4c3 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -20,6 +20,9 @@ applyBinaryPatchTypeNotSupported=Couldn't apply binary patch of type {0}
 applyTextPatchCannotApplyHunk=Hunk cannot be applied
 applyTextPatchSingleClearingHunk=Expected a single hunk for clearing all content
 applyBinaryResultOidWrong=Result of binary patch for file {0} has wrong OID
+applyPatchWithoutSourceOnAlreadyExistingSource=Cannot perform {0} action on an existing file
+applyPatchWithCreationOverAlreadyExistingDestination=Cannot perform {0} action which overrides an existing file
+applyPatchWithSourceOnNonExistentSource=Cannot perform {0} action on a non-existent file
 applyTextPatchUnorderedHunkApplications=Current hunk must be applied after the last hunk
 applyTextPatchUnorderedHunks=Got unordered hunks
 applyingCommit=Applying {0}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 77377a3a4e..28634cbc4d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -46,6 +46,9 @@ public class JGitText extends TranslationBundle {
 	/***/ public String applyBinaryOidTooShort;
 	/***/ public String applyBinaryPatchTypeNotSupported;
 	/***/ public String applyBinaryResultOidWrong;
+	/***/ public String applyPatchWithoutSourceOnAlreadyExistingSource;
+	/***/ public String applyPatchWithCreationOverAlreadyExistingDestination;
+	/***/ public String applyPatchWithSourceOnNonExistentSource;
 	/***/ public String applyTextPatchCannotApplyHunk;
 	/***/ public String applyTextPatchSingleClearingHunk;
 	/***/ public String applyTextPatchUnorderedHunkApplications;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java b/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java
index 36420fcf9d..da698d6bf6 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/patch/PatchApplier.java
@@ -9,6 +9,11 @@
  */
 package org.eclipse.jgit.patch;
 
+import static org.eclipse.jgit.diff.DiffEntry.ChangeType.ADD;
+import static org.eclipse.jgit.diff.DiffEntry.ChangeType.COPY;
+import static org.eclipse.jgit.diff.DiffEntry.ChangeType.DELETE;
+import static org.eclipse.jgit.diff.DiffEntry.ChangeType.MODIFY;
+import static org.eclipse.jgit.diff.DiffEntry.ChangeType.RENAME;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import java.io.ByteArrayInputStream;
@@ -257,32 +262,33 @@ public class PatchApplier {
 		Set<String> modifiedPaths = new HashSet<>();
 		for (FileHeader fh : p.getFiles()) {
 			ChangeType type = fh.getChangeType();
+			File src = getFile(fh.getOldPath());
+			File dest = getFile(fh.getNewPath());
+			if (!verifyExistence(fh, src, dest, result)) {
+				continue;
+			}
 			switch (type) {
 			case ADD: {
-				File f = getFile(fh.getNewPath());
-				if (f != null) {
-					FileUtils.mkdirs(f.getParentFile(), true);
-					FileUtils.createNewFile(f);
+				if (dest != null) {
+					FileUtils.mkdirs(dest.getParentFile(), true);
+					FileUtils.createNewFile(dest);
 				}
-				apply(fh.getNewPath(), dirCache, dirCacheBuilder, f, fh, result);
+				apply(fh.getNewPath(), dirCache, dirCacheBuilder, dest, fh, result);
 			}
 				break;
-			case MODIFY:
-				apply(fh.getOldPath(), dirCache, dirCacheBuilder,
-						getFile(fh.getOldPath()), fh, result);
+			case MODIFY: {
+				apply(fh.getOldPath(), dirCache, dirCacheBuilder, src, fh, result);
 				break;
-			case DELETE:
+			}
+			case DELETE: {
 				if (!inCore()) {
-					File old = getFile(fh.getOldPath());
-					if (!old.delete())
+					if (!src.delete())
 						throw new IOException(MessageFormat.format(
-								JGitText.get().cannotDeleteFile, old));
+								JGitText.get().cannotDeleteFile, src));
 				}
 				break;
+			}
 			case RENAME: {
-				File src = getFile(fh.getOldPath());
-				File dest = getFile(fh.getNewPath());
-
 				if (!inCore()) {
 					/*
 					 * this is odd: we rename the file on the FS, but
@@ -299,9 +305,7 @@ public class PatchApplier {
 				break;
 			}
 			case COPY: {
-				File dest = getFile(fh.getNewPath());
 				if (!inCore()) {
-					File src = getFile(fh.getOldPath());
 					FileUtils.mkdirs(dest.getParentFile(), true);
 					Files.copy(src.toPath(), dest.toPath());
 				}
@@ -309,10 +313,10 @@ public class PatchApplier {
 				break;
 			}
 			}
-			if (fh.getChangeType() != ChangeType.DELETE)
+			if (fh.getChangeType() != DELETE)
 				modifiedPaths.add(fh.getNewPath());
-			if (fh.getChangeType() != ChangeType.COPY
-					&& fh.getChangeType() != ChangeType.ADD)
+			if (fh.getChangeType() != COPY
+					&& fh.getChangeType() != ADD)
 				modifiedPaths.add(fh.getOldPath());
 		}
 
@@ -368,6 +372,38 @@ public class PatchApplier {
 		return walk;
 	}
 
+	private boolean fileExists(String path, @Nullable File f)
+			throws IOException {
+		if (f != null) {
+			return f.exists();
+		}
+		return inCore() && TreeWalk.forPath(repo, path, beforeTree) != null;
+	}
+
+	private boolean verifyExistence(FileHeader fh, File src, File dest,
+			Result result) throws IOException {
+		boolean isValid = true;
+		boolean srcShouldExist = List.of(MODIFY, DELETE, RENAME, COPY)
+				.contains(fh.getChangeType());
+		boolean destShouldNotExist = List.of(ADD, RENAME, COPY)
+				.contains(fh.getChangeType());
+		if (srcShouldExist != fileExists(fh.getOldPath(), src)) {
+			result.addError(MessageFormat.format(srcShouldExist
+					? JGitText.get().applyPatchWithSourceOnNonExistentSource
+					: JGitText
+							.get().applyPatchWithoutSourceOnAlreadyExistingSource,
+					fh.getPatchType()), fh.getOldPath(), null);
+			isValid = false;
+		}
+		if (destShouldNotExist && fileExists(fh.getNewPath(), dest)) {
+			result.addError(MessageFormat.format(JGitText
+					.get().applyPatchWithCreationOverAlreadyExistingDestination,
+					fh.getPatchType()), fh.getNewPath(), null);
+			isValid = false;
+		}
+		return isValid;
+	}
+
 	private static final int FILE_TREE_INDEX = 1;
 
 	/**
@@ -681,7 +717,7 @@ public class PatchApplier {
 		boolean hashOk = false;
 		if (id != null) {
 			hashOk = baseId.equals(id);
-			if (!hashOk && ChangeType.ADD.equals(type)
+			if (!hashOk && ADD.equals(type)
 					&& ObjectId.zeroId().equals(baseId)) {
 				// We create a new file. The OID of an empty file is not the
 				// zero id!