don't read files before they exist 🤦‍♂️

Author: ds300

property-based-tests/executeTestCase.ts

@@ -2,9 +2,9 @@ import * as tmp from "tmp"
 import * as path from "path"
 
 import { spawnSafeSync } from "../src/spawnSafe"
-import { patch } from "../src/patch"
 import { executeEffects } from "../src/patch/apply"
 import { parsePatch } from "../src/patch/parse"
+import { reversePatch } from "../src/patch/reverse"
 
 import { TestCase, Files } from "./testCases"
 import { appendFileSync, existsSync, writeFileSync } from "fs"
@@ -124,17 +124,17 @@ export function executeTestCase(testCase: TestCase) {
   it("works forwards", () => {
     fs.setWorkingFiles({ ...testCase.cleanFiles })
     reportingFailures(() => {
-      const effects = patch(patchFileContents)
-      executeEffects(effects)
+      const effects = parsePatch(patchFileContents)
+      executeEffects(effects, { dryRun: false })
       expect(fs.getWorkingFiles()).toEqual(testCase.modifiedFiles)
     })
   })
 
   it("works backwards", () => {
     fs.setWorkingFiles({ ...testCase.modifiedFiles })
     reportingFailures(() => {
-      const result = patch(patchFileContents, { reverse: true })
-      executeEffects(result)
+      const effects = reversePatch(parsePatch(patchFileContents))
+      executeEffects(effects, { dryRun: false })
       expect(fs.getWorkingFiles()).toEqual(testCase.cleanFiles)
     })
   })

property-based-tests/testCases.ts

@@ -63,11 +63,17 @@ function makeFiles(): Files {
   return fileSystem
 }
 
-type MutationKind = "deleteFile" | "createFile" | "deleteLine" | "insertLine"
+type MutationKind =
+  | "deleteFile"
+  | "createFile"
+  | "deleteLine"
+  | "insertLine"
+  | "renameFile"
 
 const mutationKindLikelihoods: Array<[MutationKind, number]> = [
   ["deleteFile", 1],
   ["createFile", 1],
+  ["renameFile", 1],
   ["deleteLine", 10],
   ["insertLine", 10],
 ]
@@ -105,6 +111,15 @@ function insertLinesIntoFile(file: File): File {
   return { ...file, contents: lines.join("\n") }
 }
 
+function getUniqueFilename(files: Files) {
+  let filename = makeFileName()
+  const ks = Object.keys(files)
+  while (ks.some(k => k.startsWith(filename))) {
+    filename = makeFileName()
+  }
+  return filename
+}
+
 function mutateFiles(files: Files): Files {
   const mutatedFiles = { ...files }
 
@@ -122,12 +137,7 @@ function mutateFiles(files: Files): Files {
         break
       }
       case "createFile": {
-        // TODO make sure there isn't a dir with that filename already
-        let filename = makeFileName()
-        while (Object.keys(mutatedFiles).some(k => k.startsWith(filename))) {
-          filename = makeFileName()
-        }
-        mutatedFiles[filename] = makeFileContents()
+        mutatedFiles[getUniqueFilename(mutatedFiles)] = makeFileContents()
         break
       }
       case "deleteLine": {
@@ -144,10 +154,16 @@ function mutateFiles(files: Files): Files {
         )
         // select a file at random and insert some text in there
         break
+      case "renameFile":
+        const pathToRename = selectRandomElement(Object.keys(mutatedFiles))
+        mutatedFiles[getUniqueFilename(mutatedFiles)] =
+          mutatedFiles[pathToRename]
+        delete mutatedFiles[pathToRename]
+        break
     }
   }
 
-  return mutatedFiles
+  return { ...mutatedFiles }
 }
 
 export function generateTestCase(): TestCase {

src/applyPatches.ts

@@ -1,11 +1,12 @@
 import { bold, cyan, green, red, yellow } from "chalk"
 import { getPatchFiles } from "./patchFs"
-import { patch } from "./patch"
 import { executeEffects } from "./patch/apply"
 import { existsSync, readFileSync } from "fs-extra"
 import { join, resolve } from "./path"
 import { posix } from "path"
 import { getPackageDetailsFromPatchFilename } from "./PackageDetails"
+import { parsePatch } from "./patch/parse"
+import { reversePatch } from "./patch/reverse"
 
 function findPatchFiles(patchesDirectory: string): string[] {
   if (!existsSync(patchesDirectory)) {
@@ -114,14 +115,12 @@ export const applyPatch = (
   reverse: boolean,
 ): boolean => {
   const patchFileContents = readFileSync(patchFilePath).toString()
+  const patch = parsePatch(patchFileContents)
   try {
-    const result = patch(patchFileContents, {
-      reverse,
-    })
-    executeEffects(result)
+    executeEffects(reverse ? reversePatch(patch) : patch, { dryRun: false })
   } catch (e) {
     try {
-      patch(patchFileContents, { reverse: !reverse })
+      executeEffects(reverse ? patch : reversePatch(patch), { dryRun: true })
     } catch (e) {
       return false
     }

src/patch/apply.ts

@@ -2,51 +2,53 @@ import fs from "fs-extra"
 import { dirname } from "path"
 import { ParsedPatchFile, FilePatch } from "./parse"
 
-export type Effect =
-  | {
-      type: "file deletion"
-      path: string
-      lines: string[]
-      mode: number
-      noNewlineAtEndOfFile: boolean
-    }
-  | {
-      type: "rename"
-      fromPath: string
-      toPath: string
-    }
-  | {
-      type: "file creation"
-      path: string
-      lines: string[]
-      mode: number
-      noNewlineAtEndOfFile: boolean
-    }
-  | {
-      type: "replace"
-      path: string
-      lines: string[]
-    }
-
-export const executeEffects = (effects: Effect[]) => {
+export const executeEffects = (
+  effects: ParsedPatchFile,
+  { dryRun }: { dryRun: boolean },
+) => {
   effects.forEach(eff => {
     switch (eff.type) {
       case "file deletion":
-        fs.unlinkSync(eff.path)
+        if (dryRun) {
+          if (!fs.existsSync(eff.path)) {
+            throw new Error(
+              "Trying to delete file that doesn't exist: " + eff.path,
+            )
+          }
+        } else {
+          fs.unlinkSync(eff.path)
+        }
         break
       case "rename":
-        fs.moveSync(eff.fromPath, eff.toPath)
+        if (dryRun) {
+          // TODO: see what patch files look like if moving to exising path
+          if (!fs.existsSync(eff.fromPath)) {
+            throw new Error(
+              "Trying to move file that doesn't exist: " + eff.fromPath,
+            )
+          }
+        } else {
+          fs.moveSync(eff.fromPath, eff.toPath)
+        }
         break
       case "file creation":
-        fs.ensureDirSync(dirname(eff.path))
-        fs.writeFileSync(
-          eff.path,
-          eff.lines.join("\n") + (eff.noNewlineAtEndOfFile ? "" : "\n"),
-          { mode: eff.mode },
-        )
+        if (dryRun) {
+          if (fs.existsSync(eff.path)) {
+            throw new Error(
+              "Trying to create file that already exists: " + eff.path,
+            )
+          }
+        } else {
+          fs.ensureDirSync(dirname(eff.path))
+          fs.writeFileSync(
+            eff.path,
+            eff.lines.join("\n") + (eff.noNewlineAtEndOfFile ? "" : "\n"),
+            { mode: eff.mode },
+          )
+        }
         break
-      case "replace":
-        fs.writeFileSync(eff.path, eff.lines.join("\n"))
+      case "patch":
+        applyPatch(eff, { dryRun })
         break
     }
   })
@@ -88,7 +90,10 @@ function assertLineEquality(onDisk: string, expected: string) {
  *
  */
 
-function applyPatch({ parts, path }: FilePatch): Effect {
+function applyPatch(
+  { parts, path }: FilePatch,
+  { dryRun }: { dryRun: boolean },
+): void {
   // modifying the file in place
   const fileContents = fs.readFileSync(path).toString()
 
@@ -149,27 +154,7 @@ function applyPatch({ parts, path }: FilePatch): Effect {
     }
   }
 
-  return {
-    type: "replace",
-    path,
-    lines: fileLines,
+  if (!dryRun) {
+    fs.writeFileSync(path, fileLines.join("\n"))
   }
 }
-
-export const applyPatchFile = (patch: ParsedPatchFile): Effect[] => {
-  const effects: Effect[] = []
-
-  for (const part of patch) {
-    switch (part.type) {
-      case "file creation":
-      case "file deletion":
-      case "rename":
-        effects.push(part)
-        break
-      case "patch":
-        effects.push(applyPatch(part))
-    }
-  }
-
-  return effects
-}