Skip to content

Commit 1e6967d

Browse files
committed
Keep drift when a renamed pin hides behind a twin
Review found a gap in the new array merge: if the user renamed the vendored element past recognition and also added a sibling equal to the original pin, the revert paired the sibling as "already restored", reported success, and deleted the vendored artifact the renamed line still pointed at. An "already restored" pairing now only counts when every other live element is accounted for. Otherwise the revert reports drift and keeps the artifact. Elements already back to their original text are also skipped regardless of their spacing. Assisted-by: Claude Code:claude-opus-5-5
1 parent bb4372e commit 1e6967d

1 file changed

Lines changed: 43 additions & 3 deletions

File tree

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

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,12 @@ fn bare_table(table: &Table) -> String {
450450
/// identity. Elements the vendoring left alone are not looked up at all, so
451451
/// the user may add, drop or reorder them freely. `None` (drift) when a
452452
/// changed element is gone, ambiguous, or two of them claim one live slot.
453+
///
454+
/// An identity match that already reads as the original proves nothing on
455+
/// its own: the vendored element may have been edited beyond recognition
456+
/// (renamed) while the user added a sibling equal to the original. So that
457+
/// "already restored" pairing only stands when no other live element is
458+
/// unaccounted for, i.e. neither paired nor one of the recorded elements.
453459
fn pair_changed(
454460
original: &[String],
455461
new: &[String],
@@ -459,18 +465,21 @@ fn pair_changed(
459465
) -> Option<Vec<(usize, usize)>> {
460466
let mut pairs = Vec::new();
461467
let mut claimed = BTreeSet::new();
468+
let mut already_original = false;
462469
for index in (0..new.len()).filter(|&index| original[index] != new[index]) {
463470
let unique = |found: Vec<usize>| (found.len() == 1).then(|| found[0]);
464471
let textual: Vec<usize> = (0..live.len())
465472
.filter(|&slot| live[slot] == new[index])
466473
.collect();
467474
let slot = if textual.is_empty() {
468475
let identity = new_identity[index].as_ref()?;
469-
unique(
476+
let slot = unique(
470477
(0..live.len())
471478
.filter(|&slot| live_identity[slot].as_ref() == Some(identity))
472479
.collect(),
473-
)?
480+
)?;
481+
already_original |= live[slot] == original[index];
482+
slot
474483
} else {
475484
unique(textual)?
476485
};
@@ -479,7 +488,10 @@ fn pair_changed(
479488
}
480489
pairs.push((slot, index));
481490
}
482-
Some(pairs)
491+
let unaccounted = (0..live.len()).any(|slot| {
492+
!claimed.contains(&slot) && !original.contains(&live[slot]) && !new.contains(&live[slot])
493+
});
494+
(!(already_original && unaccounted)).then_some(pairs)
483495
}
484496

485497
fn restore_table(live: &mut dyn TableLike, original: &dyn TableLike, new: &dyn TableLike) -> bool {
@@ -577,6 +589,9 @@ fn restore_value(live: &mut Value, original: &Value, new: &Value) -> bool {
577589
let mut drifted = false;
578590
for (slot, index) in pairs {
579591
let current = live.get_mut(slot).expect("paired slot is in range");
592+
if bare_value(current) == bare_value(original[index]) {
593+
continue;
594+
}
580595
if bare_value(current) == bare_value(new[index]) {
581596
replace_value(current, original[index], new[index]);
582597
} else {
@@ -635,6 +650,9 @@ fn restore_item(live: &mut Item, original: &Item, new: &Item) -> bool {
635650
let mut drifted = false;
636651
for (slot, index) in pairs {
637652
let current = live.get_mut(slot).expect("paired slot is in range");
653+
if bare_table(current) == bare_table(original[index]) {
654+
continue;
655+
}
638656
drifted |= restore_table(current, original[index], new[index]);
639657
}
640658
return drifted;
@@ -1051,13 +1069,35 @@ mod tests {
10511069
"dependencies = [\"one==1\", \"idna==3.7\"]\n",
10521070
"dependencies = [\"one==1\", \"six==1.17.0\", \"idna==3.7\"]\n",
10531071
"dependencies = [\"six @ file:///v/six.whl\", \"six @ file:///v/six.whl\"]\n",
1072+
// Renamed past recognition, with a sibling equal to the original.
1073+
"dependencies = [\"one==1\", \"other @ file:///v/six.whl\", \"six==1.16.0\"]\n",
10541074
] {
10551075
let (restored, drifted) = restore_document(live, original, new).unwrap();
10561076
assert!(drifted, "{live}");
10571077
assert_eq!(restored, live);
10581078
}
10591079
}
10601080

1081+
/// A pin the user already put back by hand still reverts cleanly (the
1082+
/// rest of the wiring is restored), but not when another, unknown
1083+
/// element could be the vendored one renamed (Bugbot on #481).
1084+
#[test]
1085+
fn already_restored_element_is_trusted_only_without_unknown_siblings() {
1086+
let original = "dependencies = [\"one==1\", \"six==1.16.0\"]\n";
1087+
let new = "dependencies = [\"one==1\", \"six @ file:///v/six.whl\"]\n";
1088+
let (restored, drifted) = restore_document(original, original, new).unwrap();
1089+
assert!(!drifted);
1090+
assert_eq!(restored, original);
1091+
let live = "dependencies = [\"six==1.16.0\", \"one==1\"]\n";
1092+
let (restored, drifted) = restore_document(live, original, new).unwrap();
1093+
assert!(!drifted);
1094+
assert_eq!(restored, live);
1095+
let live = "dependencies = [\"one==1\", \"six==1.16.0\", \"idna==3.7\"]\n";
1096+
let (restored, drifted) = restore_document(live, original, new).unwrap();
1097+
assert!(drifted);
1098+
assert_eq!(restored, live);
1099+
}
1100+
10611101
/// A re-resolved package (new version) is not the vendored entry, even
10621102
/// when siblings were added too.
10631103
#[test]

0 commit comments

Comments
 (0)