fix(patch): reuse the existing patch of a git-hosted dependency (#15666)
`pnpm patch-commit` records the patch of a git-hosted package under `name@version`, but `pnpm patch` looked the existing patch up under `name@<tarball URL>`, the bare specifier it picks for such a package. The lookup never matched, so re-running `pnpm patch` gave a clean copy without the committed changes. Fall back to `name@version` when the bare specifier finds no patch, in both the TypeScript and the Rust CLI. Closes https://github.com/pnpm/pnpm/issues/9699 Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
b161a970ce
commit
ffc5f29a3e
6 files changed
+78
-15
No files matched your search
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@pnpm/patching.commands": patch
|
||||
"pnpm": patch
|
||||
"pacquet": patch
|
||||
---
|
||||
|
||||
`pnpm patch` now applies the existing patch file to the edit directory of a git-hosted dependency, as it already does for packages from the registry [#9699](https://github.com/pnpm/pnpm/issues/9699).
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::{
|
||||
Component, PatchError, PatchTarget, Path, PathBuf, fs, io, is_subdir, lexical_normalize,
|
||||
};
|
||||
use indexmap::IndexMap;
|
||||
|
||||
pub(super) fn resolve_path(dir: &Path, path: &Path) -> PathBuf {
|
||||
if path.is_absolute() { path.to_path_buf() } else { dir.join(path) }
|
||||
@@ -163,15 +164,9 @@ pub(super) fn apply_existing_patch_file(
|
||||
edit_dir: &Path,
|
||||
) -> Result<(), PatchError> {
|
||||
let Some(patched_dependencies) = &config.patched_dependencies else { return Ok(()) };
|
||||
let exact_key = format!("{}@{}", target.alias, target.bare_specifier);
|
||||
let patch_file = patched_dependencies
|
||||
.get(&exact_key)
|
||||
.or_else(|| {
|
||||
target.apply_to_all
|
||||
.then(|| patched_dependencies.get(&target.alias))
|
||||
.flatten()
|
||||
});
|
||||
let Some(patch_file) = patch_file else { return Ok(()) };
|
||||
let Some(patch_file) = find_existing_patch_file(patched_dependencies, target) else {
|
||||
return Ok(());
|
||||
};
|
||||
let base_dir = config.workspace_dir
|
||||
.as_deref()
|
||||
.unwrap_or_else(|| config.modules_dir_anchor().unwrap_or_else(|| Path::new(".")));
|
||||
@@ -187,6 +182,23 @@ pub(super) fn apply_existing_patch_file(
|
||||
.map_err(PatchError::ApplyExistingPatch)
|
||||
}
|
||||
|
||||
/// The patch file configured for `target`, if any. `patch-commit` keys the
|
||||
/// patch of a git-hosted package by its version, while the target's bare
|
||||
/// specifier is the package's tarball URL, so both are tried.
|
||||
pub(super) fn find_existing_patch_file<'a>(
|
||||
patched_dependencies: &'a IndexMap<String, String>,
|
||||
target: &PatchTarget,
|
||||
) -> Option<&'a String> {
|
||||
[&target.bare_specifier, &target.version]
|
||||
.into_iter()
|
||||
.find_map(|specifier| patched_dependencies.get(&format!("{}@{specifier}", target.alias)))
|
||||
.or_else(|| {
|
||||
target.apply_to_all
|
||||
.then(|| patched_dependencies.get(&target.alias))
|
||||
.flatten()
|
||||
})
|
||||
}
|
||||
|
||||
struct ExistingPatchFileContext {
|
||||
project_root: PathBuf,
|
||||
patches_dir: PathBuf,
|
||||
|
||||
@@ -5,7 +5,8 @@ use super::{
|
||||
};
|
||||
#[cfg(unix)]
|
||||
use crate::cli_args::patch::paths::checked_existing_patch_file_path;
|
||||
use crate::cli_args::patch::paths::default_edit_dir_name;
|
||||
use crate::cli_args::patch::paths::{default_edit_dir_name, find_existing_patch_file};
|
||||
use indexmap::IndexMap;
|
||||
use std::{io::IsTerminal, path::Path};
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -223,6 +224,27 @@ fn default_edit_dir_name_falls_back_to_alias_then_requested_package() {
|
||||
assert_eq!(default_edit_dir_name("chalk@npm:chalk@5.3.0", &target), "chalk@npm:chalk@5.3.0");
|
||||
}
|
||||
|
||||
/// Regression test for <https://github.com/pnpm/pnpm/issues/9699>.
|
||||
#[test]
|
||||
fn existing_patch_file_of_a_git_hosted_package_is_found_by_version() {
|
||||
let tarball = "https://codeload.github.com/example/hi/tar.gz/deadbeef";
|
||||
let target = PatchTarget {
|
||||
alias: "hi".to_string(),
|
||||
version: "1.0.0".to_string(),
|
||||
bare_specifier: tarball.to_string(),
|
||||
apply_to_all: false,
|
||||
git_tarball_url: Some(tarball.to_string()),
|
||||
package_key: format!("hi@{tarball}").parse().expect("package key"),
|
||||
};
|
||||
let patched_dependencies =
|
||||
IndexMap::from([("hi@1.0.0".to_string(), "patches/hi@1.0.0.patch".to_string())]);
|
||||
|
||||
assert_eq!(
|
||||
find_existing_patch_file(&patched_dependencies, &target).map(String::as_str),
|
||||
Some("patches/hi@1.0.0.patch"),
|
||||
);
|
||||
}
|
||||
|
||||
struct FakePrompt {
|
||||
selected: usize,
|
||||
apply_to_all: bool,
|
||||
|
||||
@@ -13,7 +13,14 @@ export type GetPatchedDependencyOptions = {
|
||||
lockfileDir: string
|
||||
} & Pick<Config, 'virtualStoreDir' | 'modulesDir'>
|
||||
|
||||
export type GetPatchedDependencyResult = ParseWantedDependencyResult & { applyToAll: boolean }
|
||||
export type GetPatchedDependencyResult = ParseWantedDependencyResult & {
|
||||
applyToAll: boolean
|
||||
/**
|
||||
* The installed version of the chosen package. It differs from `bareSpecifier`
|
||||
* when the package is git-hosted, in which case `bareSpecifier` is its tarball URL.
|
||||
*/
|
||||
version?: string
|
||||
}
|
||||
|
||||
export async function getPatchedDependency (rawDependency: string, opts: GetPatchedDependencyOptions): Promise<GetPatchedDependencyResult> {
|
||||
const dep = parseWantedDependency(rawDependency)
|
||||
@@ -29,10 +36,10 @@ export async function getPatchedDependency (rawDependency: string, opts: GetPatc
|
||||
|
||||
dep.alias = dep.alias ?? rawDependency
|
||||
if (preferredVersions.length > 1) {
|
||||
let version: string
|
||||
let bareSpecifier: string
|
||||
let applyToAll: boolean
|
||||
try {
|
||||
version = await select({
|
||||
bareSpecifier = await select({
|
||||
message: 'Choose which version to patch',
|
||||
choices: preferredVersions.map(preferred => ({
|
||||
name: preferred.version,
|
||||
@@ -53,7 +60,8 @@ export async function getPatchedDependency (rawDependency: string, opts: GetPatc
|
||||
return {
|
||||
...dep,
|
||||
applyToAll,
|
||||
bareSpecifier: version,
|
||||
bareSpecifier,
|
||||
version: preferredVersions.find(preferred => (preferred.gitTarballUrl ?? preferred.version) === bareSpecifier)?.version,
|
||||
}
|
||||
} else {
|
||||
const preferred = preferredVersions[0]
|
||||
@@ -62,12 +70,14 @@ export async function getPatchedDependency (rawDependency: string, opts: GetPatc
|
||||
...dep,
|
||||
applyToAll: false,
|
||||
bareSpecifier: preferred.gitTarballUrl,
|
||||
version: preferred.version,
|
||||
}
|
||||
}
|
||||
return {
|
||||
...dep,
|
||||
applyToAll: !dep.bareSpecifier,
|
||||
bareSpecifier: preferred.version,
|
||||
version: preferred.version,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -133,7 +133,7 @@ To commit your changes, run:
|
||||
|
||||
function tryPatchWithExistingPatchFile (
|
||||
{
|
||||
patchedDep: { applyToAll, alias, bareSpecifier },
|
||||
patchedDep: { applyToAll, alias, bareSpecifier, version },
|
||||
patchedDir,
|
||||
patchedDependencies,
|
||||
lockfileDir,
|
||||
@@ -149,6 +149,9 @@ function tryPatchWithExistingPatchFile (
|
||||
if (bareSpecifier) {
|
||||
existingPatchFile = patchedDependencies[`${alias}@${bareSpecifier}`]
|
||||
}
|
||||
if (!existingPatchFile && version) {
|
||||
existingPatchFile = patchedDependencies[`${alias}@${version}`]
|
||||
}
|
||||
if (!existingPatchFile && applyToAll) {
|
||||
existingPatchFile = patchedDependencies[alias]
|
||||
}
|
||||
|
||||
@@ -1518,6 +1518,15 @@ describe('patch and commit in workspaces', () => {
|
||||
expect(patchContent).toContain('diff --git')
|
||||
expect(patchContent).toContain('// test patching')
|
||||
expect(fs.readFileSync('./project-2/node_modules/hi/index.js', 'utf8')).toContain('// test patching')
|
||||
|
||||
// re-patch
|
||||
fs.rmSync(patchDir, { recursive: true })
|
||||
const repatchOutput = await patch.handler({
|
||||
...defaultPatchOption,
|
||||
patchedDependencies: workspaceManifest!.patchedDependencies,
|
||||
}, ['hi'])
|
||||
const repatchDir = getPatchDirFromPatchOutput(repatchOutput)
|
||||
expect(fs.readFileSync(path.join(repatchDir, 'index.js'), 'utf8')).toContain('// test patching')
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Reference in new issue
Block a user