fix: should not delete state.json info after apply patch (#8401)

* fix: should not delete state.json info after apply patch

* Update .changeset/yellow-dodos-press.md

Co-authored-by: Khải <hvksmr1996@gmail.com>

* Update .changeset/yellow-dodos-press.md

Co-authored-by: Khải <hvksmr1996@gmail.com>

* test: patch-commit multiple times

* refactor: remove unused deleteEditDirState

* fix: read patch state from correct directory

---------

Co-authored-by: Khải <hvksmr1996@gmail.com>
Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/yellow-dodos-press.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/plugin-commands-patching": patch
+"pnpm": patch
+---
+
+Remember the state of edit dir, allow running `pnpm patch-commit` the second time without having to re-run `pnpm patch`.

patching/plugin-commands-patching/src/patch.ts

@@ -99,7 +99,7 @@ export async function handler (opts: PatchCommandOptions, params: string[]): Pro
 
   writeEditDirState({
     editDir,
-    modulesDir: opts.modulesDir ?? 'node_modules',
+    modulesDir: path.join(opts.dir, opts.modulesDir ?? 'node_modules'),
     patchedPkg: params[0],
     applyToAll: patchedDep.applyToAll,
   })

patching/plugin-commands-patching/src/patchCommit.ts

@@ -20,7 +20,7 @@ import tempy from 'tempy'
 import { writePackage } from './writePackage'
 import { type ParseWantedDependencyResult, parseWantedDependency } from '@pnpm/parse-wanted-dependency'
 import { type GetPatchedDependencyOptions, getVersionsFromLockfile } from './getPatchedDependency'
-import { readEditDirState, deleteEditDirState } from './stateFile'
+import { readEditDirState } from './stateFile'
 
 export const rcOptionsTypes = cliOptionsTypes
 
@@ -58,7 +58,7 @@ export async function handler (opts: PatchCommitCommandOptions, params: string[]
   const editDir = path.resolve(opts.dir, userDir)
   const stateValue = readEditDirState({
     editDir,
-    modulesDir: opts.modulesDir ?? 'node_modules',
+    modulesDir: path.join(opts.dir, opts.modulesDir ?? 'node_modules'),
   })
   if (!stateValue) {
     throw new PnpmError('INVALID_PATCH_DIR', `${userDir} is not a valid patch directory`, {
@@ -81,10 +81,6 @@ export async function handler (opts: PatchCommitCommandOptions, params: string[]
   }
   const srcDir = tempy.directory()
   await writePackage(parseWantedDependency(gitTarballUrl ? `${patchedPkgManifest.name}@${gitTarballUrl}` : nameAndVersion), srcDir, opts)
-  deleteEditDirState({
-    editDir,
-    modulesDir: opts.modulesDir ?? 'node_modules',
-  })
   const patchedPkgDir = await preparePkgFilesForDiff(userDir)
   const patchContent = await diffFolders(srcDir, patchedPkgDir)
   if (patchedPkgDir !== userDir) {

patching/plugin-commands-patching/src/stateFile.ts

@@ -40,15 +40,6 @@ export function writeEditDirState (opts: WriteEditDirStateOptions): void {
   })
 }
 
-export interface DeleteEditDirStateOptions extends ReadEditDirStateOptions {}
-
-export function deleteEditDirState (opts: DeleteEditDirStateOptions): void {
-  modifyStateFile(opts.modulesDir, state => {
-    const key = createEditDirKey(opts)
-    delete state[key]
-  })
-}
-
 function modifyStateFile (modulesDir: string, modifyState: (state: State) => void): void {
   const filePath = getStateFilePath(modulesDir)
   let state = readStateFile(modulesDir)

patching/plugin-commands-patching/test/patch.test.ts

@@ -142,6 +142,116 @@ describe('patch and commit', () => {
     expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
   })
 
+  test('patch-commit multiple times', async () => {
+    const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
+    const patchDir = getPatchDirFromPatchOutput(output)
+
+    // store patch files in a directory inside modules dir when not given editDir option
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1.0.0'))
+    expect(path.basename(patchDir)).toBe('is-positive@1.0.0')
+    expect(fs.existsSync(patchDir)).toBe(true)
+
+    // sanity check to ensure that the license file contains the expected string
+    expect(fs.readFileSync(path.join(patchDir, 'license'), 'utf8')).toContain('The MIT License (MIT)')
+
+    {
+      // First edit
+      fs.appendFileSync(path.join(patchDir, 'index.js'), '// first edit\n', 'utf8')
+      fs.unlinkSync(path.join(patchDir, 'license'))
+
+      await patchCommit.handler({
+        ...DEFAULT_OPTS,
+        cacheDir,
+        dir: process.cwd(),
+        rootProjectManifestDir: process.cwd(),
+        frozenLockfile: false,
+        fixLockfile: true,
+        storeDir,
+      }, [patchDir])
+
+      const { manifest } = await readProjectManifest(process.cwd())
+      expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
+        'is-positive@1.0.0': 'patches/is-positive@1.0.0.patch',
+      })
+      const patchContent = fs.readFileSync('patches/is-positive@1.0.0.patch', 'utf8')
+      expect(patchContent).toContain('diff --git')
+      expect(patchContent).toContain('// first edit')
+      expect(patchContent).not.toContain('// second edit')
+      expect(patchContent).not.toContain('// third edit')
+      const indexFileContent = fs.readFileSync('node_modules/is-positive/index.js', 'utf8')
+      expect(indexFileContent).toContain('// first edit')
+      expect(indexFileContent).not.toContain('// second edit')
+      expect(indexFileContent).not.toContain('// third edit')
+
+      expect(patchContent).not.toContain('The MIT License (MIT)')
+      expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
+    }
+
+    {
+      // Second edit
+      fs.appendFileSync(path.join(patchDir, 'index.js'), '// second edit\n', 'utf8')
+
+      await patchCommit.handler({
+        ...DEFAULT_OPTS,
+        cacheDir,
+        dir: process.cwd(),
+        rootProjectManifestDir: process.cwd(),
+        frozenLockfile: false,
+        fixLockfile: true,
+        storeDir,
+      }, [patchDir])
+
+      const { manifest } = await readProjectManifest(process.cwd())
+      expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
+        'is-positive@1.0.0': 'patches/is-positive@1.0.0.patch',
+      })
+      const patchContent = fs.readFileSync('patches/is-positive@1.0.0.patch', 'utf8')
+      expect(patchContent).toContain('diff --git')
+      expect(patchContent).toContain('// first edit')
+      expect(patchContent).toContain('// second edit')
+      expect(patchContent).not.toContain('// third edit')
+      const indexFileContent = fs.readFileSync('node_modules/is-positive/index.js', 'utf8')
+      expect(indexFileContent).toContain('// first edit')
+      expect(indexFileContent).toContain('// second edit')
+      expect(indexFileContent).not.toContain('// third edit')
+
+      expect(patchContent).not.toContain('The MIT License (MIT)')
+      expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
+    }
+
+    {
+      // Third edit
+      fs.appendFileSync(path.join(patchDir, 'index.js'), '// third edit\n', 'utf8')
+
+      await patchCommit.handler({
+        ...DEFAULT_OPTS,
+        cacheDir,
+        dir: process.cwd(),
+        rootProjectManifestDir: process.cwd(),
+        frozenLockfile: false,
+        fixLockfile: true,
+        storeDir,
+      }, [patchDir])
+
+      const { manifest } = await readProjectManifest(process.cwd())
+      expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
+        'is-positive@1.0.0': 'patches/is-positive@1.0.0.patch',
+      })
+      const patchContent = fs.readFileSync('patches/is-positive@1.0.0.patch', 'utf8')
+      expect(patchContent).toContain('diff --git')
+      expect(patchContent).toContain('// first edit')
+      expect(patchContent).toContain('// second edit')
+      expect(patchContent).toContain('// third edit')
+      const indexFileContent = fs.readFileSync('node_modules/is-positive/index.js', 'utf8')
+      expect(indexFileContent).toContain('// first edit')
+      expect(indexFileContent).toContain('// second edit')
+      expect(indexFileContent).toContain('// third edit')
+
+      expect(patchContent).not.toContain('The MIT License (MIT)')
+      expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
+    }
+  })
+
   test('patch-commit with relative path', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)