fix(patching): commit now accepts relative path (#8405)

* fix(patching): commit now accepts relative path

* fix: correct the solution

* fix: correct again

* test: fix

* refactor: revert not needed changes

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/tidy-pots-trade.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/plugin-commands-patching": patch
+"pnpm": patch
+---
+
+Fix a bug in `patch-commit` in which relative path is rejected.

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

@@ -85,7 +85,9 @@ export async function handler (opts: PatchCommandOptions, params: string[]): Pro
   })
 
   const modulesDir = path.join(lockfileDir, opts.modulesDir ?? 'node_modules')
-  const editDir = opts.editDir ? opts.editDir : getEditDirPath(params[0], patchedDep, { modulesDir })
+  const editDir = opts.editDir
+    ? path.resolve(opts.dir, opts.editDir)
+    : getEditDirPath(params[0], patchedDep, { modulesDir })
 
   if (fs.existsSync(editDir) && fs.readdirSync(editDir).length !== 0) {
     throw new PnpmError('EDIT_DIR_NOT_EMPTY', `The directory ${editDir} is not empty`, {

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

@@ -55,8 +55,9 @@ export async function handler (opts: PatchCommitCommandOptions, params: string[]
   const patchesDirName = normalizePath(path.normalize(opts.patchesDir ?? 'patches'))
   const patchesDir = path.join(lockfileDir, patchesDirName)
   const patchedPkgManifest = await readPackageJsonFromDir(userDir)
+  const editDir = path.resolve(opts.dir, userDir)
   const stateValue = readEditDirState({
-    editDir: userDir,
+    editDir,
     modulesDir: opts.modulesDir ?? 'node_modules',
   })
   if (!stateValue) {
@@ -81,7 +82,7 @@ export async function handler (opts: PatchCommitCommandOptions, params: string[]
   const srcDir = tempy.directory()
   await writePackage(parseWantedDependency(gitTarballUrl ? `${patchedPkgManifest.name}@${gitTarballUrl}` : nameAndVersion), srcDir, opts)
   deleteEditDirState({
-    editDir: userDir,
+    editDir,
     modulesDir: opts.modulesDir ?? 'node_modules',
   })
   const patchedPkgDir = await preparePkgFilesForDiff(userDir)

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

@@ -142,6 +142,44 @@ describe('patch and commit', () => {
     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)
+
+    // 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)')
+
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+    fs.unlinkSync(path.join(patchDir, 'license'))
+
+    await patchCommit.handler({
+      ...DEFAULT_OPTS,
+      cacheDir,
+      dir: process.cwd(),
+      rootProjectManifestDir: process.cwd(),
+      frozenLockfile: false,
+      fixLockfile: true,
+      storeDir,
+    }, [path.relative(process.cwd(), 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('// test patching')
+    expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test patching')
+
+    expect(patchContent).not.toContain('The MIT License (MIT)')
+    expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
+  })
+
   test('patch and commit with filtered files', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
@@ -195,6 +233,31 @@ describe('patch and commit', () => {
     expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test patching')
   })
 
+  test('patch with relative path to custom edit dir and commit with absolute path', async () => {
+    const editDir = 'custom-edit-dir'
+
+    const output = await patch.handler({ ...defaultPatchOption, editDir }, ['is-positive@1.0.0'])
+    const patchDir = getPatchDirFromPatchOutput(output)
+
+    const editDirAbsolute = path.resolve(defaultPatchOption.dir, editDir)
+    expect(patchDir).toBe(editDirAbsolute)
+    expect(fs.existsSync(editDirAbsolute)).toBe(true)
+
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+
+    await patchCommit.handler({
+      ...DEFAULT_OPTS,
+      cacheDir,
+      dir: process.cwd(),
+      rootProjectManifestDir: process.cwd(),
+      frozenLockfile: false,
+      fixLockfile: true,
+      storeDir,
+    }, [path.resolve(editDir)])
+
+    expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test patching')
+  })
+
   test('patch and commit with custom patches dir', async () => {
     const patchesDir = 'ts/src/../custom-patches'
 
@@ -239,7 +302,6 @@ describe('patch and commit', () => {
     const output = await patch.handler({ ...defaultPatchOption, editDir }, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
 
-    expect(patchDir).toBe(editDir)
     expect(fs.existsSync(patchDir)).toBe(true)
 
     fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')