rebase continue behavior

Author: ds300

src/applyPatches.ts

@@ -3,6 +3,7 @@ import { existsSync } from "fs-extra"
 import { posix } from "path"
 import semver from "semver"
 import { hashFile } from "./hash"
+import { logPatchSequenceError } from "./makePatch"
 import { PackageDetails, PatchedPackageDetails } from "./PackageDetails"
 import { packageIsDevDependency } from "./packageIsDevDependency"
 import { executeEffects } from "./patch/apply"
@@ -13,6 +14,7 @@ import { join, relative } from "./path"
 import {
   clearPatchApplicationState,
   getPatchApplicationState,
+  PatchState,
   savePatchApplicationState,
 } from "./stateFile"
 
@@ -213,12 +215,13 @@ export function applyPatchesForPackage({
     appliedPatches.length = 0
   }
   if (appliedPatches.length) {
-    // all patches have already been applied
+    // some patches have already been applied
     appliedPatches.forEach(logPatchApplication)
   }
   if (!unappliedPatches.length) {
     return
   }
+  let failedPatch: PatchedPackageDetails | null = null
   packageLoop: for (const patchDetails of unappliedPatches) {
     try {
       const { name, version, path, isDevOnly, patchFilename } = patchDetails
@@ -271,6 +274,12 @@ export function applyPatchesForPackage({
           )
         }
         logPatchApplication(patchDetails)
+      } else if (patches.length > 1) {
+        logPatchSequenceError({ patchDetails })
+        // in case the package has multiple patches, we need to break out of this inner loop
+        // because we don't want to apply more patches on top of the broken state
+        failedPatch = patchDetails
+        break packageLoop
       } else if (installedPackageVersion === version) {
         // completely failed to apply patch
         // TODO: propagate useful error messages from patch application
@@ -282,8 +291,6 @@ export function applyPatchesForPackage({
             path,
           }),
         )
-        // in case the package has multiple patches, we need to break out of this inner loop
-        // because we don't want to apply more patches on top of the broken state
         break packageLoop
       } else {
         errors.push(
@@ -353,18 +360,29 @@ export function applyPatchesForPackage({
         })
       }
     } else {
-      const allPatchesSucceeded =
-        unappliedPatches.length === appliedPatches.length
-      savePatchApplicationState({
-        packageDetails: patches[0],
-        patches: appliedPatches.map((patch) => ({
+      const nextState = appliedPatches.map(
+        (patch): PatchState => ({
           didApply: true,
           patchContentHash: hashFile(
             join(appPath, patchDir, patch.patchFilename),
           ),
           patchFilename: patch.patchFilename,
-        })),
-        isRebasing: !allPatchesSucceeded,
+        }),
+      )
+
+      if (failedPatch) {
+        nextState.push({
+          didApply: false,
+          patchContentHash: hashFile(
+            join(appPath, patchDir, failedPatch.patchFilename),
+          ),
+          patchFilename: failedPatch.patchFilename,
+        })
+      }
+      savePatchApplicationState({
+        packageDetails: patches[0],
+        patches: nextState,
+        isRebasing: !!failedPatch,
       })
     }
   }
@@ -389,6 +407,10 @@ export function applyPatch({
     patchDir,
   })
   try {
+    executeEffects(reverse ? reversePatch(patch) : patch, {
+      dryRun: true,
+      cwd,
+    })
     executeEffects(reverse ? reversePatch(patch) : patch, {
       dryRun: false,
       cwd,

src/makePatch.ts

@@ -1,4 +1,5 @@
 import chalk from "chalk"
+import console from "console"
 import { renameSync } from "fs"
 import {
   copySync,
@@ -36,6 +37,7 @@ import { resolveRelativeFileDependencies } from "./resolveRelativeFileDependenci
 import { spawnSafeSync } from "./spawnSafe"
 import {
   clearPatchApplicationState,
+  getPatchApplicationState,
   PatchState,
   savePatchApplicationState,
   STATE_FILE_NAME,
@@ -78,22 +80,54 @@ export function makePatch({
     return
   }
 
+  const state = getPatchApplicationState(packageDetails)
+  const isRebasing = state?.isRebasing ?? false
+  // TODO: verify applied patch hashes
+  // TODO: handle case for --rebase 0
+  // TODO: handle empty diffs while rebasing
+  if (
+    mode.type === "overwrite_last" &&
+    isRebasing &&
+    state?.patches.length === 0
+  ) {
+    mode = { type: "append", name: "initial" }
+  }
+
   const existingPatches =
     getGroupedPatches(patchDir).pathSpecifierToPatchFiles[
       packageDetails.pathSpecifier
     ] || []
 
+  // apply all existing patches if appending
+  // otherwise apply all but the last
+  const previouslyAppliedPatches = state?.patches.filter((p) => p.didApply)
+  const patchesToApplyBeforeDiffing: PatchedPackageDetails[] = isRebasing
+    ? mode.type === "append"
+      ? existingPatches.slice(0, previouslyAppliedPatches!.length)
+      : state!.patches[state!.patches.length - 1].didApply
+      ? existingPatches.slice(0, previouslyAppliedPatches!.length - 1)
+      : existingPatches.slice(0, previouslyAppliedPatches!.length)
+    : mode.type === "append"
+    ? existingPatches
+    : existingPatches.slice(0, -1)
+
   if (createIssue && mode.type === "append") {
     console.error("--create-issue is not compatible with --append.")
     process.exit(1)
   }
 
+  if (createIssue && isRebasing) {
+    console.error("--create-issue is not compatible with rebasing.")
+    process.exit(1)
+  }
+
   const numPatchesAfterCreate =
     mode.type === "append" || existingPatches.length === 0
       ? existingPatches.length + 1
       : existingPatches.length
   const vcs = getPackageVCSDetails(packageDetails)
   const canCreateIssue =
+    !isRebasing &&
     shouldRecommendIssue(vcs) &&
     numPatchesAfterCreate === 1 &&
     mode.type !== "append"
@@ -224,11 +258,7 @@ export function makePatch({
     // remove ignored files first
     removeIgnoredFiles(tmpRepoPackagePath, includePaths, excludePaths)
 
-    // apply all existing patches if appending
-    // otherwise apply all but the last
-    const patchesToApplyBeforeCommit =
-      mode.type === "append" ? existingPatches : existingPatches.slice(0, -1)
-    for (const patchDetails of patchesToApplyBeforeCommit) {
+    for (const patchDetails of patchesToApplyBeforeDiffing) {
       if (
         !applyPatch({
           patchDetails,
@@ -339,10 +369,10 @@ export function makePatch({
     }
 
     // maybe delete existing
-    if (mode.type === "overwrite_last") {
-      const prevPatch = existingPatches[existingPatches.length - 1] as
-        | PatchedPackageDetails
-        | undefined
+    if (!isRebasing && mode.type === "overwrite_last") {
+      const prevPatch = patchesToApplyBeforeDiffing[
+        patchesToApplyBeforeDiffing.length - 1
+      ] as PatchedPackageDetails | undefined
       if (prevPatch) {
         const patchFilePath = join(appPath, patchDir, prevPatch.patchFilename)
         try {
@@ -351,7 +381,7 @@ export function makePatch({
           // noop
         }
       }
-    } else if (existingPatches.length === 1) {
+    } else if (!isRebasing && existingPatches.length === 1) {
       // if we are appending to an existing patch that doesn't have a sequence number let's rename it
       const prevPatch = existingPatches[0]
       if (prevPatch.sequenceNumber === undefined) {
@@ -370,9 +400,9 @@ export function makePatch({
       }
     }
 
-    const lastPatch = existingPatches[existingPatches.length - 1] as
-      | PatchedPackageDetails
-      | undefined
+    const lastPatch = existingPatches[
+      state ? state.patches.length - 1 : existingPatches.length - 1
+    ] as PatchedPackageDetails | undefined
     const sequenceName =
       mode.type === "append" ? mode.name : lastPatch?.sequenceName
     const sequenceNumber =
@@ -396,10 +426,33 @@ export function makePatch({
     console.log(
       `${chalk.green("✔")} Created file ${join(patchDir, patchFileName)}\n`,
     )
-    const prevState: PatchState[] = (mode.type === "append"
-      ? existingPatches
-      : existingPatches.slice(0, -1)
-    ).map(
+
+    // if we inserted a new patch into a sequence we may need to update the sequence numbers
+    if (isRebasing && mode.type === "append") {
+      const patchesToNudge = existingPatches.slice(state!.patches.length)
+      if (sequenceNumber === undefined) {
+        throw new Error("sequenceNumber is undefined while rebasing")
+      }
+      if (
+        patchesToNudge[0]?.sequenceNumber !== undefined &&
+        patchesToNudge[0].sequenceNumber <= sequenceNumber
+      ) {
+        let next = sequenceNumber + 1
+        for (const p of patchesToNudge) {
+          const newName = createPatchFileName({
+            packageDetails,
+            packageVersion,
+            sequenceName: p.sequenceName,
+            sequenceNumber: next++,
+          })
+          const oldPath = join(appPath, patchDir, p.patchFilename)
+          const newPath = join(appPath, patchDir, newName)
+          renameSync(oldPath, newPath)
+        }
+      }
+    }
+
+    const prevState: PatchState[] = patchesToApplyBeforeDiffing.map(
       (p): PatchState => ({
         patchFilename: p.patchFilename,
         didApply: true,
@@ -414,15 +467,61 @@ export function makePatch({
         patchContentHash: hashFile(patchPath),
       },
     ]
-    if (nextState.length > 1) {
+
+    // if any patches come after this one we just made, we should reapply them
+    let didFailWhileFinishingRebase = false
+    if (isRebasing) {
+      const previouslyUnappliedPatches = existingPatches.slice(
+        // if we overwrote a previously failing patch we should not include that in here
+        previouslyAppliedPatches!.length +
+          (mode.type === "overwrite_last" &&
+          !state?.patches[state.patches.length - 1].didApply
+            ? 1
+            : 0),
+      )
+      if (previouslyUnappliedPatches.length) {
+        console.log(`Fast forwarding...`)
+        for (const patch of previouslyUnappliedPatches) {
+          const patchFilePath = join(appPath, patchDir, patch.patchFilename)
+          if (
+            !applyPatch({
+              patchDetails: patch,
+              patchDir,
+              patchFilePath,
+              reverse: false,
+              cwd: tmpRepo.name,
+            })
+          ) {
+            didFailWhileFinishingRebase = true
+            logPatchSequenceError({ patchDetails: patch })
+            nextState.push({
+              patchFilename: patch.patchFilename,
+              didApply: false,
+              patchContentHash: hashFile(patchFilePath),
+            })
+            break
+          } else {
+            console.log(`  ${chalk.green("✔")} ${patch.patchFilename}`)
+            nextState.push({
+              patchFilename: patch.patchFilename,
+              didApply: true,
+              patchContentHash: hashFile(patchFilePath),
+            })
+          }
+        }
+      }
+    }
+
+    if (isRebasing || numPatchesAfterCreate > 1) {
       savePatchApplicationState({
         packageDetails,
         patches: nextState,
-        isRebasing: false,
+        isRebasing: didFailWhileFinishingRebase,
       })
     } else {
       clearPatchApplicationState(packageDetails)
     }
+
     if (canCreateIssue) {
       if (createIssue) {
         openIssueCreationLink({
@@ -466,3 +565,27 @@ function createPatchFileName({
 
   return `${nameAndVersion}${num}${name}.patch`
 }
+
+export function logPatchSequenceError({
+  patchDetails,
+}: {
+  patchDetails: PatchedPackageDetails
+}) {
+  console.log(`
+${chalk.red.bold("⛔ ERROR")}
+
+Failed to apply patch file ${chalk.bold(patchDetails.patchFilename)}.
+
+If this patch file is no longer useful, delete it and run
+
+  ${chalk.bold(`patch-package`)}
+  
+Otherwise you should open ${
+    patchDetails.path
+  }, manually apply the changes from the patch file, and run
+
+  ${chalk.bold(`patch-package ${patchDetails.pathSpecifier}`)}
+
+to update the patch file.
+`)
+}

src/stateFile.ts

@@ -5,7 +5,7 @@ import stringify from "json-stable-stringify"
 export interface PatchState {
   patchFilename: string
   patchContentHash: string
-  didApply: true
+  didApply: boolean
 }
 
 const version = 1