Skip to content

Commit 60fdebd

Browse files
Fix vendored NuGet revert on CRLF checkouts (#537) (#1342)
* Start refactor for #594 Assisted-by: Claude Code:claude-opus-5-5 * Wire vendored nuget.config via formats::nuget Vendored NuGet now reads the source keys and finds the <packageSources>, <packageSourceMapping> and <configuration> anchors through formats::nuget::parse_config, the reader that hosted, upstream restore and VEX already use. The private substring scanner (blank_comments, parse_config_source_keys, attr_value, self_closing_package_sources, insert_at_line) is deleted. User impact: - A close tag written with whitespace (</packageSources >) is now the section that gets extended; vendor used to append a second section NuGet ignores, so restore failed NU1100/NU1403 (#685). - An empty <packageSourceMapping /> is expanded in place instead of left beside a second mapping section. - A section opened and closed on one line receives the source inside it, not before its open tag. - Catch-all keys are written XML-encoded, so a key with & or a quote keeps its identity. - Malformed XML or a repeated section is refused with "malformed XML or a repeated section; not wired" instead of being spliced at the first substring match, as hosted already does. Output bytes for well-formed configs are unchanged. Fixes #685 Refs #594 Assisted-by: Claude Code:claude-opus-5-5 * Start NuGet fix: nuget-crlf-revert Draft placeholder while the fix is written. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Revert vendored NuGet on autocrlf checkouts After a core.autocrlf checkout (Git for Windows' default) nuget.config comes back CRLF. vendor --revert, remove and rollback compared it with the LF text vendor recorded, treated it as drift and left it wired, but had already put packages.lock.json back to the upstream contentHash, so every later restore failed NU1403 while --revert exited 0. The config restore now compares and excises line-ending-insensitively and writes the original back in the checkout's line endings. And the lock pin is only reverted when the config stops routing to the vendored feed: a drift-kept config keeps its lock pin too, so the project stays consistently vendored instead of half-reverted. Fixes #537. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Find LF NuGet fragments in a mixed CRLF config Vendor inserts LF lines even into a CRLF nuget.config, which stays mixed until git converts it. A sibling edit then sent the revert down the excision path, where the CRLF-majority spelling missed our LF fragments and drift-kept the package (review on #1342). The excision now tries the LF spelling first, then the file's own terminator. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent eca5d24 commit 60fdebd

2 files changed

Lines changed: 263 additions & 14 deletions

File tree

‎crates/socket-patch-cli/CLI_CONTRACT.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,11 @@ worse, lets a warm cache silently serve unpatched bytes):
878878
whole-file wiring cannot tell a converged fragment from a drifted one, keep the artifact exactly
879879
while the live `composer.lock` / `pom.xml` / `nuget.config` still names its
880880
`.socket/vendor/<eco>/<uuid>` dir — a file that no longer references it is warned about and the
881-
artifact removed; in the npm family (npm, yarn classic and berry, pnpm, bun) a recorded lock entry
881+
artifact removed; nuget (v5.0, #537) compares `nuget.config` line-ending-insensitively, so a
882+
`core.autocrlf` checkout of the file vendor wrote is not drift (the original is restored in the
883+
checkout's line endings), and while `nuget.config` is drift-kept the `packages.lock.json` pin is
884+
kept too (`vendor_lock_entry_drifted`), never reverted under a config that still routes the id to
885+
the vendored feed; in the npm family (npm, yarn classic and berry, pnpm, bun) a recorded lock entry
882886
that no longer exists at all — the user removed the dependency — is not drift: it warns
883887
`vendor_lock_entry_removed` and the artifact and entry are kept unless every wired file that exists
884888
was read and none mentions the uuid in any spelling (an unreadable lock keeps them), so `rollback` / `remove` / `scan --prune` clean up

‎crates/socket-patch-core/src/vendor/nuget_feed.rs‎

Lines changed: 258 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use crate::patch::path_safety::{is_safe_multi_segment, is_safe_single_segment};
1111
use crate::utils::fs::{
1212
atomic_write_artifact, atomic_write_bytes_preserving_mode, read_regular_to_string,
1313
};
14+
use crate::utils::line_endings::{eol_eq, respell, terminator};
1415
use crate::utils::purl::{build_nuget_purl, parse_nuget_purl};
1516

1617
use super::common::{
@@ -746,6 +747,27 @@ pub async fn revert_nuget_opts(
746747
};
747748
let mut warnings = Vec::new();
748749

750+
// The lock pin may only go back to the upstream contentHash when
751+
// nuget.config stops routing the id to the vendored feed. A config that
752+
// will be left wired (drift-kept: it still names the feed) with the lock
753+
// reverted under it fails every restore NU1403 (#537), so its lock pin
754+
// is kept and the package stays consistently vendored. Decided up front
755+
// (a read-only preview of the config restore), so the lock still goes
756+
// first and a lock failure leaves the config wired for the retry.
757+
let mut config_still_routes = false;
758+
for w in entry
759+
.wiring
760+
.iter()
761+
.filter(|w| w.kind == CONFIG_SOURCE_WIRING_KIND)
762+
{
763+
if matches!(
764+
revert_config_record(project_root, &uuid_dir_rel, w, true).await,
765+
Ok(false)
766+
) && config_references(project_root, &w.file, &uuid_dir_rel).await
767+
{
768+
config_still_routes = true;
769+
}
770+
}
749771
// Reverse application order: lock pin, then the (no-op) mapping audit
750772
// record, then the authoritative config restore.
751773
for w in entry.wiring.iter().rev() {
@@ -758,6 +780,22 @@ pub async fn revert_nuget_opts(
758780
"refusing revert: unsafe wiring file path {:?}",
759781
w.file
760782
)),
783+
LOCK_WIRING_KIND if config_still_routes => {
784+
warnings.push(VendorWarning::new(
785+
"vendor_lock_entry_drifted",
786+
format!(
787+
"{} still routes {} to the vendored feed, so its {} pin is kept",
788+
entry
789+
.wiring
790+
.iter()
791+
.find(|c| c.kind == CONFIG_SOURCE_WIRING_KIND)
792+
.map_or("nuget.config", |c| c.file.as_str()),
793+
w.key.as_deref().unwrap_or("<unknown>"),
794+
w.file
795+
),
796+
));
797+
continue;
798+
}
761799
LOCK_WIRING_KIND => revert_lock_record(&project_root.join(&w.file), w, dry_run).await,
762800
// Audit-only: the whole-file config restore lives on the source
763801
// record, so there is nothing to undo here.
@@ -1194,17 +1232,28 @@ async fn revert_config_record(
11941232
Err(e) => return Err(format!("unreadable {}: {e}", config_path.display())),
11951233
};
11961234

1197-
// (a) Byte-identical to what we wrote → the whole-file restore/delete is
1198-
// provably safe (nothing changed since vendoring).
1199-
let new_matches = matches!(&w.new, Some(Value::String(n)) if *n == live);
1235+
// (a) What we wrote, up to line endings → the whole-file restore/delete
1236+
// is provably safe (nothing changed since vendoring). A
1237+
// `core.autocrlf` checkout (Git for Windows' default) hands back
1238+
// our LF text as CRLF; that is git's encoding, not an edit (#537).
1239+
let new_matches =
1240+
matches!(&w.new, Some(Value::String(n)) if eol_eq(n.as_bytes(), live.as_bytes()));
12001241
if new_matches {
12011242
if dry_run {
12021243
return Ok(true);
12031244
}
12041245
match &w.original {
1205-
// Pre-existed → restore the verbatim original bytes.
1246+
// Pre-existed → restore the original, verbatim when the live
1247+
// file still has the line endings we wrote, else spelled in the
1248+
// live file's (the checkout converted it).
12061249
Some(Value::String(orig)) => {
1207-
atomic_write_bytes_preserving_mode(&config_path, orig.as_bytes())
1250+
let wrote_lf = matches!(&w.new, Some(Value::String(n)) if *n == live);
1251+
let restored = if wrote_lf {
1252+
orig.clone()
1253+
} else {
1254+
respell(orig, terminator(&live))
1255+
};
1256+
atomic_write_bytes_preserving_mode(&config_path, restored.as_bytes())
12081257
.await
12091258
.map_err(|e| format!("failed to restore {}: {e}", config_path.display()))?;
12101259
}
@@ -1222,8 +1271,22 @@ async fn revert_config_record(
12221271
// two authored elements. Both are reproduced verbatim from the source
12231272
// key + uuid dir (the source `<add>`) and matched structurally by our
12241273
// source key (the mapping `<packageSource>`).
1225-
let source_add = format!(" <add key=\"{source_key}\" value=\"{uuid_dir_rel}\" />\n");
1226-
let mapping_block = excise_source_mapping(&live, source_key);
1274+
// Vendor inserts LF lines, even into a CRLF file (which then has
1275+
// mixed endings until git converts it), so the LF spelling is tried
1276+
// first, then the file's own terminator (a `core.autocrlf` checkout).
1277+
let spelled = |nl: &str| {
1278+
(
1279+
format!(" <add key=\"{source_key}\" value=\"{uuid_dir_rel}\" />{nl}"),
1280+
excise_source_mapping(&live, source_key, nl),
1281+
)
1282+
};
1283+
let lf = spelled("\n");
1284+
let (source_add, mapping_block) =
1285+
if live.contains(&lf.0) || lf.1.is_some() || terminator(&live) == "\n" {
1286+
lf
1287+
} else {
1288+
spelled(terminator(&live))
1289+
};
12271290
if !live.contains(&source_add) && mapping_block.is_none() {
12281291
// (c) Neither authored element is present verbatim → drift, leave alone.
12291292
return Ok(false);
@@ -1246,16 +1309,32 @@ async fn revert_config_record(
12461309
Ok(true)
12471310
}
12481311

1312+
/// Whether the project-root config `file` (a recorded wiring basename)
1313+
/// still names the vendored feed dir `uuid_dir_rel`. An unsafe or
1314+
/// unreadable file answers yes: when in doubt the lock pin is kept with
1315+
/// the config that may route to it.
1316+
async fn config_references(project_root: &Path, file: &str, uuid_dir_rel: &str) -> bool {
1317+
if !is_safe_single_segment(file) {
1318+
return true;
1319+
}
1320+
match read_regular_to_string(&project_root.join(file)).await {
1321+
Ok(text) => text.contains(uuid_dir_rel),
1322+
Err(e) => e.kind() != std::io::ErrorKind::NotFound,
1323+
}
1324+
}
1325+
12491326
/// The exact `<packageSource key="{source_key}"> … </packageSource>\n` block we
12501327
/// authored in the mapping section, if present verbatim in `config`. Anchored on
12511328
/// our source key and closed at the first `</packageSource>` after it, then
12521329
/// extended through the trailing newline so the excision leaves no blank line.
12531330
/// `None` when our mapping block is absent (already reverted, or edited).
1254-
fn excise_source_mapping(config: &str, source_key: &str) -> Option<String> {
1255-
let open = format!(" <packageSource key=\"{source_key}\">\n");
1331+
/// `nl` is the config's line terminator ([`terminator`]): a `core.autocrlf`
1332+
/// checkout spells our LF block in CRLF.
1333+
fn excise_source_mapping(config: &str, source_key: &str, nl: &str) -> Option<String> {
1334+
let open = format!(" <packageSource key=\"{source_key}\">{nl}");
12561335
let open_at = config.find(&open)?;
1257-
let close = " </packageSource>\n";
1258-
let rel_close = config[open_at..].find(close)?;
1336+
let close = format!(" </packageSource>{nl}");
1337+
let rel_close = config[open_at..].find(&close)?;
12591338
let end = open_at + rel_close + close.len();
12601339
Some(config[open_at..end].to_string())
12611340
}
@@ -2584,6 +2663,172 @@ mod tests {
25842663
assert!(after.contains("key=\"nuget.org\""));
25852664
}
25862665

2666+
/// `path` rewritten with CRLF line endings, as a `core.autocrlf=true`
2667+
/// checkout (Git for Windows' default) hands it back.
2668+
async fn autocrlf(path: &Path) {
2669+
let text = tokio::fs::read_to_string(path).await.unwrap();
2670+
tokio::fs::write(path, text.replace("\r\n", "\n").replace('\n', "\r\n"))
2671+
.await
2672+
.unwrap();
2673+
}
2674+
2675+
/// #537: after an autocrlf checkout, revert restores BOTH the
2676+
/// pre-existing config (in the checkout's CRLF) and the lock, with no
2677+
/// drift warning and the feed removed.
2678+
#[tokio::test]
2679+
async fn revert_on_an_autocrlf_checkout_restores_config_and_lock() {
2680+
let orig_cfg = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n\
2681+
<configuration>\n\
2682+
\x20 <packageSources>\n\
2683+
\x20 <add key=\"nuget.org\" value=\"https://api.nuget.org/v3/index.json\" />\n\
2684+
\x20 </packageSources>\n\
2685+
</configuration>\n";
2686+
let (dir, blobs, installed, record) = fixture(true, Some(orig_cfg)).await;
2687+
let root = dir.path();
2688+
let lock_before = tokio::fs::read_to_string(root.join(PACKAGES_LOCK))
2689+
.await
2690+
.unwrap();
2691+
let (_r, entry, _w) =
2692+
unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await);
2693+
let entry = entry.unwrap();
2694+
autocrlf(&root.join("nuget.config")).await;
2695+
autocrlf(&root.join(PACKAGES_LOCK)).await;
2696+
2697+
let outcome = revert_nuget(&entry, root, false).await;
2698+
assert!(outcome.success, "{:?}", outcome.error);
2699+
assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings);
2700+
assert!(!outcome.kept_artifact);
2701+
assert_eq!(
2702+
tokio::fs::read_to_string(root.join("nuget.config"))
2703+
.await
2704+
.unwrap(),
2705+
orig_cfg.replace('\n', "\r\n"),
2706+
"the original config, in the checkout's line endings"
2707+
);
2708+
assert_eq!(
2709+
tokio::fs::read_to_string(root.join(PACKAGES_LOCK))
2710+
.await
2711+
.unwrap(),
2712+
lock_before.replace('\n', "\r\n")
2713+
);
2714+
assert!(!root.join(format!(".socket/vendor/nuget/{UUID}")).exists());
2715+
}
2716+
2717+
/// #537: a config we created is deleted on an autocrlf checkout too.
2718+
#[tokio::test]
2719+
async fn revert_on_an_autocrlf_checkout_deletes_a_created_config() {
2720+
let (dir, blobs, installed, record) = fixture(true, None).await;
2721+
let root = dir.path();
2722+
let (_r, entry, _w) =
2723+
unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await);
2724+
let entry = entry.unwrap();
2725+
autocrlf(&root.join("nuget.config")).await;
2726+
let outcome = revert_nuget(&entry, root, false).await;
2727+
assert!(outcome.success, "{:?}", outcome.error);
2728+
assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings);
2729+
assert!(!root.join("nuget.config").exists());
2730+
}
2731+
2732+
/// #537: the fragment excision (a sibling's wiring made the file differ
2733+
/// from what we wrote) finds our CRLF-spelled elements and keeps the
2734+
/// file's CRLF.
2735+
#[tokio::test]
2736+
async fn revert_excises_our_crlf_fragments_beside_a_sibling() {
2737+
let (dir, blobs, installed, record) = fixture(true, None).await;
2738+
let root = dir.path();
2739+
let (_r, entry, _w) =
2740+
unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await);
2741+
let entry = entry.unwrap();
2742+
let cfg = root.join("nuget.config");
2743+
let wired = tokio::fs::read_to_string(&cfg).await.unwrap();
2744+
let sibling = wired.replacen(
2745+
"</packageSources>",
2746+
" <add key=\"corp\" value=\"https://corp.example/v3/index.json\" />\n </packageSources>",
2747+
1,
2748+
);
2749+
tokio::fs::write(&cfg, &sibling).await.unwrap();
2750+
autocrlf(&cfg).await;
2751+
let outcome = revert_nuget(&entry, root, false).await;
2752+
assert!(outcome.success, "{:?}", outcome.error);
2753+
assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings);
2754+
let after = tokio::fs::read_to_string(&cfg).await.unwrap();
2755+
assert!(!after.contains(UUID), "{after}");
2756+
assert!(after.contains("key=\"corp\""), "{after}");
2757+
assert!(
2758+
!after.replace("\r\n", "").contains('\n'),
2759+
"CRLF kept: {after:?}"
2760+
);
2761+
}
2762+
2763+
/// #537 review: an existing CRLF config gets our LF lines (mixed until
2764+
/// git converts it); a CRLF sibling edit sends the revert down the
2765+
/// excision path, which must still find our LF fragments.
2766+
#[tokio::test]
2767+
async fn revert_excises_lf_fragments_from_a_mixed_crlf_config() {
2768+
let orig = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\r\n<configuration>\r\n <packageSources>\r\n <add key=\"nuget.org\" value=\"https://api.nuget.org/v3/index.json\" />\r\n </packageSources>\r\n <packageSourceMapping>\r\n <packageSource key=\"nuget.org\">\r\n <package pattern=\"*\" />\r\n </packageSource>\r\n </packageSourceMapping>\r\n</configuration>\r\n";
2769+
let (dir, blobs, installed, record) = fixture(true, Some(orig)).await;
2770+
let root = dir.path();
2771+
let (_r, entry, _w) =
2772+
unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await);
2773+
let entry = entry.unwrap();
2774+
let cfg = root.join("nuget.config");
2775+
let wired = tokio::fs::read_to_string(&cfg).await.unwrap();
2776+
let sibling = wired.replacen(
2777+
" </packageSources>",
2778+
" <add key=\"corp\" value=\"https://corp.example/v3/index.json\" />\r\n </packageSources>",
2779+
1,
2780+
);
2781+
tokio::fs::write(&cfg, &sibling).await.unwrap();
2782+
let outcome = revert_nuget(&entry, root, false).await;
2783+
assert!(outcome.success, "{:?}", outcome.error);
2784+
assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings);
2785+
assert!(!outcome.kept_artifact);
2786+
let after = tokio::fs::read_to_string(&cfg).await.unwrap();
2787+
assert!(!after.contains(UUID), "{after}");
2788+
assert!(after.contains("key=\"corp\""), "{after}");
2789+
}
2790+
2791+
/// #537: a config left wired (drift-kept, it still routes to the feed)
2792+
/// keeps its lock pin too, so restore never sees an upstream lock under
2793+
/// a vendored mapping.
2794+
#[tokio::test]
2795+
async fn drift_kept_config_keeps_the_lock_pin() {
2796+
let (dir, blobs, installed, record) = fixture(true, None).await;
2797+
let root = dir.path();
2798+
let (_r, entry, _w) =
2799+
unwrap_done(run_vendor(root, &blobs, &installed, &record, false).await);
2800+
let entry = entry.unwrap();
2801+
let pinned = tokio::fs::read_to_string(root.join(PACKAGES_LOCK))
2802+
.await
2803+
.unwrap();
2804+
// Tooling re-serialized the config: our elements no longer match
2805+
// verbatim, but the source still points at the feed.
2806+
let cfg = root.join("nuget.config");
2807+
let wired = tokio::fs::read_to_string(&cfg).await.unwrap();
2808+
tokio::fs::write(&cfg, wired.replace(" <", "\t\t<"))
2809+
.await
2810+
.unwrap();
2811+
let outcome = revert_nuget(&entry, root, false).await;
2812+
assert!(outcome.success, "{:?}", outcome.error);
2813+
assert!(outcome.kept_artifact);
2814+
assert_eq!(
2815+
tokio::fs::read_to_string(root.join(PACKAGES_LOCK))
2816+
.await
2817+
.unwrap(),
2818+
pinned,
2819+
"the lock keeps the vendored pin while the config routes to it"
2820+
);
2821+
assert!(
2822+
outcome
2823+
.warnings
2824+
.iter()
2825+
.any(|w| w.code == "vendor_lock_entry_drifted"
2826+
&& w.detail.contains("still routes")),
2827+
"{:?}",
2828+
outcome.warnings
2829+
);
2830+
}
2831+
25872832
#[tokio::test]
25882833
async fn revert_warns_when_our_source_key_already_gone() {
25892834
// The user regenerated nuget.config, dropping our source entirely.
@@ -2640,13 +2885,13 @@ mod tests {
26402885
\x20 <package pattern=\"Newtonsoft.Json\" />\n\
26412886
\x20 </packageSource>\n\
26422887
\x20 </packageSourceMapping>\n";
2643-
let block = excise_source_mapping(cfg, "socket-patch-abc").unwrap();
2888+
let block = excise_source_mapping(cfg, "socket-patch-abc", "\n").unwrap();
26442889
assert!(block.contains("key=\"socket-patch-abc\""));
26452890
assert!(block.contains("Newtonsoft.Json"));
26462891
// Does not swallow the sibling nuget.org block.
26472892
assert!(!block.contains("nuget.org"));
26482893
// Absent key → None.
2649-
assert!(excise_source_mapping(cfg, "socket-patch-missing").is_none());
2894+
assert!(excise_source_mapping(cfg, "socket-patch-missing", "\n").is_none());
26502895
}
26512896

26522897
#[tokio::test]

0 commit comments

Comments
 (0)