Skip to content

Commit e956cfe

Browse files
ds300ds300
committed
make patch sequence application idempotent
1 parent 1bb87b6 commit e956cfe

File tree

9 files changed

+276
-40
lines changed

9 files changed

+276
-40
lines changed

‎integration-tests/apply-multiple-patches/__snapshots__/apply-multiple-patches.test.ts.snap‎

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3-
exports[`Test apply-multiple-patches: 00: patch-package happily applies both good patches 1`] = `
4-
"SNAPSHOT: patch-package happily applies both good patches
3+
exports[`Test apply-multiple-patches: 00: patch-package happily applies all three good patches 1`] = `
4+
"SNAPSHOT: patch-package happily applies all three good patches
55
patch-package 0.0.0
66
Applying patches...
77
[email protected] (1 hello) ✔
@@ -10,15 +10,53 @@ [email protected] (4 goodbye) ✔
1010
END SNAPSHOT"
1111
`;
1212

13-
exports[`Test apply-multiple-patches: 01: patch-package only applies the first patch if the second of three is invalid 1`] = `
13+
exports[`Test apply-multiple-patches: 01: patch-package stores a state file to list the patches that have been applied 1`] = `
14+
"SNAPSHOT: patch-package stores a state file to list the patches that have been applied
15+
{
16+
\\"version\\": 0,
17+
\\"patches\\": [
18+
{
19+
\\"patchFilename\\": \\"left-pad+1.3.0+001+hello.patch\\",
20+
\\"didApply\\": true,
21+
\\"patchContentHash\\": \\"404c604ed830db6a0605f86cb9165ced136848f70986b23bf877bcf38968c1c9\\"
22+
},
23+
{
24+
\\"patchFilename\\": \\"left-pad+1.3.0+002+world.patch\\",
25+
\\"didApply\\": true,
26+
\\"patchContentHash\\": \\"f2859c7193de8d9578bdde7e226de516adc8d972d6e76997cbe1f41b1a535359\\"
27+
},
28+
{
29+
\\"patchFilename\\": \\"left-pad+1.3.0+003+goodbye.patch\\",
30+
\\"didApply\\": true,
31+
\\"patchContentHash\\": \\"946d4e578decc1e475ca9b0de07353791969312fd2bf5d9cfc5182b86d9804ad\\"
32+
}
33+
]
34+
}END SNAPSHOT"
35+
`;
36+
37+
exports[`Test apply-multiple-patches: 02: patch-package only applies the first patch if the second of three is invalid 1`] = `
1438
"SNAPSHOT: patch-package only applies the first patch if the second of three is invalid
1539
patch-package 0.0.0
1640
Applying patches...
1741
[email protected] (1 hello) ✔
1842
END SNAPSHOT"
1943
`;
2044

21-
exports[`Test apply-multiple-patches: 02: patch-package fails when a patch in the sequence is invalid 1`] = `
45+
exports[`Test apply-multiple-patches: 03: patch-package stores a state file of only the first patch if there was an error 1`] = `
46+
"SNAPSHOT: patch-package stores a state file of only the first patch if there was an error
47+
{
48+
\\"version\\": 0,
49+
\\"patches\\": [
50+
{
51+
\\"patchFilename\\": \\"left-pad+1.3.0+001+hello.patch\\",
52+
\\"patchContentHash\\": \\"404c604ed830db6a0605f86cb9165ced136848f70986b23bf877bcf38968c1c9\\",
53+
\\"didApply\\": true
54+
}
55+
]
56+
}END SNAPSHOT"
57+
`;
58+
59+
exports[`Test apply-multiple-patches: 04: patch-package fails when a patch in the sequence is invalid 1`] = `
2260
"SNAPSHOT: patch-package fails when a patch in the sequence is invalid
2361
2462
**ERROR** Failed to apply patch for package left-pad at path
@@ -45,3 +83,9 @@ exports[`Test apply-multiple-patches: 02: patch-package fails when a patch in th
4583
patch-package finished with 1 error(s).
4684
END SNAPSHOT"
4785
`;
86+
87+
exports[`Test apply-multiple-patches: 05: patch-package fails when a patch file is removed 1`] = `
88+
"SNAPSHOT: patch-package fails when a patch file is removed
89+
Error: The patches for left-pad have changed. You should reinstall your node_modules folder to make sure the package is up to date
90+
END SNAPSHOT"
91+
`;

‎integration-tests/apply-multiple-patches/apply-multiple-patches.sh‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,14 @@ npm add $1
88
alias patch-package=./node_modules/.bin/patch-package
99

1010

11-
echo "SNAPSHOT: patch-package happily applies both good patches"
11+
echo "SNAPSHOT: patch-package happily applies all three good patches"
1212
patch-package
1313
echo "END SNAPSHOT"
1414

15+
echo "SNAPSHOT: patch-package stores a state file to list the patches that have been applied"
16+
cat node_modules/left-pad/.patch-package.json
17+
echo "END SNAPSHOT"
18+
1519
echo "it should work if we apply them again even though they touch the same parts of the code"
1620
if ! patch-package
1721
then
@@ -20,6 +24,8 @@ fi
2024

2125
cp *broken.patch patches/
2226

27+
rm -rf node_modules
28+
npm install
2329
(>&2 echo "SNAPSHOT: patch-package fails when a patch in the sequence is invalid")
2430
if patch-package
2531
then
@@ -32,4 +38,17 @@ if patch-package
3238
then
3339
exit 1
3440
fi
35-
echo "END SNAPSHOT"
41+
echo "END SNAPSHOT"
42+
43+
echo "SNAPSHOT: patch-package stores a state file of only the first patch if there was an error"
44+
cat node_modules/left-pad/.patch-package.json
45+
echo "END SNAPSHOT"
46+
47+
48+
rm patches/*hello.patch
49+
(>&2 echo "SNAPSHOT: patch-package fails when a patch file is removed")
50+
if patch-package
51+
then
52+
exit 1
53+
fi
54+
(>&2 echo "END SNAPSHOT")

‎integration-tests/apply-multiple-patches/patches/left-pad+1.3.0+001+hello.patch‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
diff --git a/node_modules/left-pad/index.js b/node_modules/left-pad/index.js
2-
index e90aec3..fd5a1d0 100644
2+
index e90aec3..1a2ec5f 100644
33
--- a/node_modules/left-pad/index.js
44
+++ b/node_modules/left-pad/index.js
5-
@@ -1,3 +1,4 @@
6-
+// hello
7-
/* This program is free software. It comes without any warranty, to
8-
* the extent permitted by applicable law. You can redistribute it
5+
@@ -3,7 +3,7 @@
96
* and/or modify it under the terms of the Do What The Fuck You Want
7+
* To Public License, Version 2, as published by Sam Hocevar. See
8+
* http://www.wtfpl.net/ for more details. */
9+
-'use strict';
10+
+'use hello';
11+
module.exports = leftPad;
12+
13+
var cache = [

‎integration-tests/apply-multiple-patches/patches/left-pad+1.3.0+003+world.patch‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
diff --git a/node_modules/left-pad/index.js b/node_modules/left-pad/index.js
2-
index fd5a1d0..65f8689 100644
2+
index 1a2ec5f..5aa41be 100644
33
--- a/node_modules/left-pad/index.js
44
+++ b/node_modules/left-pad/index.js
5-
@@ -1,3 +1,4 @@
6-
+// world
7-
// hello
8-
/* This program is free software. It comes without any warranty, to
9-
* the extent permitted by applicable law. You can redistribute it
5+
@@ -3,7 +3,7 @@
6+
* and/or modify it under the terms of the Do What The Fuck You Want
7+
* To Public License, Version 2, as published by Sam Hocevar. See
8+
* http://www.wtfpl.net/ for more details. */
9+
-'use hello';
10+
+'use world';
11+
module.exports = leftPad;
12+
13+
var cache = [

‎integration-tests/apply-multiple-patches/patches/left-pad+1.3.0+004+goodbye.patch‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
diff --git a/node_modules/left-pad/index.js b/node_modules/left-pad/index.js
2-
index 65f8689..2f2abc3 100644
2+
index 5aa41be..2beca7e 100644
33
--- a/node_modules/left-pad/index.js
44
+++ b/node_modules/left-pad/index.js
5-
@@ -1,5 +1,5 @@
6-
// world
7-
-// hello
8-
+// goodbye
9-
/* This program is free software. It comes without any warranty, to
10-
* the extent permitted by applicable law. You can redistribute it
5+
@@ -3,7 +3,7 @@
116
* and/or modify it under the terms of the Do What The Fuck You Want
7+
* To Public License, Version 2, as published by Sam Hocevar. See
8+
* http://www.wtfpl.net/ for more details. */
9+
-'use world';
10+
+'goodbye world';
11+
module.exports = leftPad;
12+
13+
var cache = [

‎src/applyPatches.ts‎

Lines changed: 65 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,19 @@ import chalk from "chalk"
22
import { existsSync } from "fs-extra"
33
import { posix } from "path"
44
import semver from "semver"
5-
import { PackageDetails } from "./PackageDetails"
5+
import { hashFile } from "./hash"
6+
import { PackageDetails, PatchedPackageDetails } from "./PackageDetails"
67
import { packageIsDevDependency } from "./packageIsDevDependency"
78
import { executeEffects } from "./patch/apply"
89
import { readPatch } from "./patch/read"
910
import { reversePatch } from "./patch/reverse"
1011
import { getGroupedPatches } from "./patchFs"
1112
import { join, relative, resolve } from "./path"
13+
import {
14+
getPatchApplicationState,
15+
PatchState,
16+
savePatchApplicationState,
17+
} from "./stateFile"
1218

1319
class PatchApplicationError extends Error {
1420
constructor(msg: string) {
@@ -68,6 +74,20 @@ function getInstalledPackageVersion({
6874
return result as string
6975
}
7076

77+
function logPatchApplication(patchDetails: PatchedPackageDetails) {
78+
const sequenceString =
79+
patchDetails.sequenceNumber != null
80+
? ` (${patchDetails.sequenceNumber}${
81+
patchDetails.sequenceName ? " " + patchDetails.sequenceName : ""
82+
})`
83+
: ""
84+
console.log(
85+
`${chalk.bold(patchDetails.pathSpecifier)}@${
86+
patchDetails.version
87+
}${sequenceString} ${chalk.green("✔")}`,
88+
)
89+
}
90+
7191
export function applyPatchesForApp({
7292
appPath,
7393
reverse,
@@ -92,10 +112,42 @@ export function applyPatchesForApp({
92112
const errors: string[] = []
93113
const warnings: string[] = [...groupedPatches.warnings]
94114

95-
for (const [pathSpecifier, details] of Object.entries(
115+
for (const [pathSpecifier, patches] of Object.entries(
96116
groupedPatches.pathSpecifierToPatchFiles,
97117
)) {
98-
packageLoop: for (const patchDetails of details) {
118+
const state =
119+
patches.length > 1 ? getPatchApplicationState(patches[0]) : null
120+
const unappliedPatches = patches.slice(0)
121+
const newState: PatchState[] | null = patches.length > 1 ? [] : null
122+
// if there are multiple patches to apply, we can't rely on the reverse-patch-dry-run behavior to make this operation
123+
// idempotent, so instead we need to check the state file to see whether we have already applied any of the patches
124+
// todo: once this is battle tested we might want to use the same approach for single patches as well, but it's not biggie since the dry run thing is fast
125+
if (unappliedPatches && state) {
126+
for (let i = 0; i < state.patches.length; i++) {
127+
const patchThatWasApplied = state.patches[i]
128+
const patchToApply = unappliedPatches[0]
129+
const currentPatchHash = hashFile(
130+
join(appPath, patchDir, patchToApply.patchFilename),
131+
)
132+
if (patchThatWasApplied.patchContentHash === currentPatchHash) {
133+
// this patch was applied we can skip it
134+
unappliedPatches.shift()
135+
} else {
136+
console.error(
137+
chalk.red("Error:"),
138+
`The patches for ${chalk.bold(pathSpecifier)} have changed.`,
139+
`You should reinstall your node_modules folder to make sure the package is up to date`,
140+
)
141+
process.exit(1)
142+
}
143+
}
144+
}
145+
if (unappliedPatches.length === 0) {
146+
// all patches have already been applied
147+
patches.forEach(logPatchApplication)
148+
continue
149+
}
150+
packageLoop: for (const patchDetails of patches) {
99151
try {
100152
const { name, version, path, isDevOnly, patchFilename } = patchDetails
101153

@@ -132,6 +184,11 @@ export function applyPatchesForApp({
132184
cwd: process.cwd(),
133185
})
134186
) {
187+
newState?.push({
188+
patchFilename,
189+
patchContentHash: hashFile(join(appPath, patchDir, patchFilename)),
190+
didApply: true,
191+
})
135192
// yay patch was applied successfully
136193
// print warning if version mismatch
137194
if (installedPackageVersion !== version) {
@@ -145,19 +202,7 @@ export function applyPatchesForApp({
145202
}),
146203
)
147204
}
148-
const sequenceString =
149-
patchDetails.sequenceNumber != null
150-
? ` (${patchDetails.sequenceNumber}${
151-
patchDetails.sequenceName
152-
? " " + patchDetails.sequenceName
153-
: ""
154-
})`
155-
: ""
156-
console.log(
157-
`${chalk.bold(
158-
pathSpecifier,
159-
)}@${version}${sequenceString} ${chalk.green("✔")}`,
160-
)
205+
logPatchApplication(patchDetails)
161206
} else if (installedPackageVersion === version) {
162207
// completely failed to apply patch
163208
// TODO: propagate useful error messages from patch application
@@ -203,6 +248,10 @@ export function applyPatchesForApp({
203248
break packageLoop
204249
}
205250
}
251+
252+
if (newState) {
253+
savePatchApplicationState(patches[0], newState)
254+
}
206255
}
207256

208257
for (const warning of warnings) {

‎src/hash.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { createHash } from "crypto"
2+
import { openSync, readSync, closeSync, statSync } from "fs"
3+
4+
const bufferSize = 1024
5+
6+
const buffer = Buffer.alloc(bufferSize)
7+
8+
export function hashFile(filePath: string) {
9+
const sha = createHash("sha256")
10+
const fileDescriptor = openSync(filePath, "r")
11+
const size = statSync(filePath).size
12+
let totalBytesRead = 0
13+
while (totalBytesRead < size) {
14+
const bytesRead = readSync(
15+
fileDescriptor,
16+
buffer,
17+
0,
18+
Math.min(size - totalBytesRead, bufferSize),
19+
totalBytesRead,
20+
)
21+
if (bytesRead < bufferSize) {
22+
sha.update(buffer.slice(0, bytesRead))
23+
} else {
24+
sha.update(buffer)
25+
}
26+
totalBytesRead += bytesRead
27+
}
28+
closeSync(fileDescriptor)
29+
return sha.digest("hex")
30+
}

0 commit comments

Comments
 (0)