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

Change-Id: Ia3ec0ce1af65114b48669157a934f70f1e22fd37
Bug: Google b/271474227

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 2a7ad2a8158a5f0c196f09f2670187044bf2b990..eeec1498717f16046422a1bcbf881d0613f16467 100644 (file)
--- 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();
                        }
                }
@@ -372,6 +376,68 @@ public class PatchApplierTest {
                        verifyChange(result, "ShiftDown2");
                }
 
+               @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");

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 1655cce1e16d15e7b1def28e8e6066e293ea028d..78dd9bd4c32614c9c324b93a56e0027b8dd6521c 100644 (file)
--- 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 77377a3a4e626364fea48e336f8c1c3a97843cf2..28634cbc4dfe3d0b249a0910552122adcaece983 100644 (file)
--- 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 36420fcf9de0abc2ddde9f45a5f669eee24465bd..da698d6bf675eb16e91f1f704cbd26aec7d37a48 100644 (file)
--- 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!