From 0e2a524c9b4a9ae58495f1bde84220a62027ed77 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 15:31:12 -0500 Subject: [PATCH 01/12] refactor(update): Move the allocation out of PackageDiff --- src/cargo/ops/cargo_update.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 64990c2591f..6272e163c6b 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -491,7 +491,7 @@ fn print_lockfile_generation( resolve: &Resolve, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff = PackageDiff::new(&resolve); + let diff: Vec<_> = PackageDiff::new(&resolve).collect(); let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); if num_pkgs <= 1 { // just ourself, nothing worth reporting @@ -537,7 +537,7 @@ fn print_lockfile_sync( resolve: &Resolve, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff = PackageDiff::diff(&previous_resolve, &resolve); + let diff: Vec<_> = PackageDiff::diff(&previous_resolve, &resolve).collect(); let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); if num_pkgs == 0 { return Ok(()); @@ -619,7 +619,7 @@ fn print_lockfile_updates( precise: bool, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff = PackageDiff::diff(&previous_resolve, &resolve); + let diff: Vec<_> = PackageDiff::diff(&previous_resolve, &resolve).collect(); let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); if !precise { status_locking(ws, num_pkgs)?; @@ -836,7 +836,7 @@ pub struct PackageDiff { } impl PackageDiff { - pub fn new(resolve: &Resolve) -> Vec { + pub fn new(resolve: &Resolve) -> impl Iterator { let mut changes = BTreeMap::new(); let empty = Self::default(); for dep in resolve.iter() { @@ -847,10 +847,10 @@ impl PackageDiff { .push(dep); } - changes.into_iter().map(|(_, v)| v).collect() + changes.into_iter().map(|(_, v)| v) } - pub fn diff(previous_resolve: &Resolve, resolve: &Resolve) -> Vec { + pub fn diff(previous_resolve: &Resolve, resolve: &Resolve) -> impl Iterator { fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec { a.iter().filter(|a| !contains_id(b, a)).cloned().collect() } @@ -921,7 +921,7 @@ impl PackageDiff { } debug!("{:#?}", changes); - changes.into_iter().map(|(_, v)| v).collect() + changes.into_iter().map(|(_, v)| v) } fn key(dep: PackageId) -> (&'static str, SourceId) { From cae484d8c9f86cdb494e42a6b66d4dc152a48ee4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 15:34:27 -0500 Subject: [PATCH 02/12] refactor(update): Prefer Copy over references --- src/cargo/ops/cargo_update.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 6272e163c6b..9c4ed37f78d 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -641,8 +641,8 @@ fn print_lockfile_updates( }; if let Some((removed, added)) = diff.change() { - let required_rust_version = report_required_rust_version(ws, resolve, *added); - let latest = report_latest(&possibilities, *added); + let required_rust_version = report_required_rust_version(ws, resolve, added); + let latest = report_latest(&possibilities, added); let note = required_rust_version.or(latest).unwrap_or_default(); let msg = if removed.source_id().is_git() { @@ -933,9 +933,9 @@ impl PackageDiff { /// All `PackageDiff` knows is that entries were added/removed within [`Resolve`]. /// A package could be added or removed because of dependencies from other packages /// which makes it hard to definitively say "X was upgrade to N". - pub fn change(&self) -> Option<(&PackageId, &PackageId)> { + pub fn change(&self) -> Option<(PackageId, PackageId)> { if self.removed.len() == 1 && self.added.len() == 1 { - Some((&self.removed[0], &self.added[0])) + Some((self.removed[0], self.added[0])) } else { None } From 99ad632606c1cd225b2add8c5fec0b663b75db7f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:16:27 -0500 Subject: [PATCH 03/12] fix(update): Show required rust-version in one more case --- src/cargo/ops/cargo_update.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 9c4ed37f78d..9a6313a0d6e 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -559,18 +559,9 @@ fn print_lockfile_sync( }; if let Some((removed, added)) = diff.change() { - let latest = if !possibilities.is_empty() { - possibilities - .iter() - .map(|s| s.as_summary()) - .filter(|s| is_latest(s.version(), added.version())) - .map(|s| s.version().clone()) - .max() - .map(format_latest) - } else { - None - } - .unwrap_or_default(); + let required_rust_version = report_required_rust_version(ws, resolve, added); + let latest = report_latest(&possibilities, added); + let note = required_rust_version.or(latest).unwrap_or_default(); let msg = if removed.source_id().is_git() { format!( @@ -578,7 +569,7 @@ fn print_lockfile_sync( &added.source_id().precise_git_fragment().unwrap()[..8], ) } else { - format!("{removed} -> v{}{latest}", added.version()) + format!("{removed} -> v{}{note}", added.version()) }; // If versions differ only in build metadata, we call it an "update" From 2c9f9c29f7dbc566967f7c1670cb4edfdd9abc15 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:28:41 -0500 Subject: [PATCH 04/12] refactor(update): Be clearer, consistent in variable name --- src/cargo/ops/cargo_update.rs | 68 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 9a6313a0d6e..3d91b9c6371 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -513,15 +513,15 @@ fn print_lockfile_generation( vec![] }; - for package in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package); - let latest = report_latest(&possibilities, *package); + for package_id in diff.added.iter() { + let required_rust_version = report_required_rust_version(ws, resolve, *package_id); + let latest = report_latest(&possibilities, *package_id); let note = required_rust_version.or(latest); if let Some(note) = note { ws.gctx().shell().status_with_color( "Adding", - format!("{package}{note}"), + format!("{package_id}{note}"), &style::NOTE, )?; } @@ -558,25 +558,25 @@ fn print_lockfile_sync( vec![] }; - if let Some((removed, added)) = diff.change() { - let required_rust_version = report_required_rust_version(ws, resolve, added); - let latest = report_latest(&possibilities, added); + if let Some((previous_id, package_id)) = diff.change() { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if removed.source_id().is_git() { + let msg = if previous_id.source_id().is_git() { format!( - "{removed} -> #{}", - &added.source_id().precise_git_fragment().unwrap()[..8], + "{previous_id} -> #{}", + &package_id.source_id().precise_git_fragment().unwrap()[..8], ) } else { - format!("{removed} -> v{}{note}", added.version()) + format!("{previous_id} -> v{}{note}", package_id.version()) }; // If versions differ only in build metadata, we call it an "update" // regardless of whether the build metadata has gone up or down. // This metadata is often stuff like git commit hashes, which are // not meaningfully ordered. - if removed.version().cmp_precedence(added.version()) == Ordering::Greater { + if previous_id.version().cmp_precedence(package_id.version()) == Ordering::Greater { ws.gctx() .shell() .status_with_color("Downgrading", msg, &style::WARN)?; @@ -586,14 +586,14 @@ fn print_lockfile_sync( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package); - let latest = report_latest(&possibilities, *package); + for package_id in diff.added.iter() { + let required_rust_version = report_required_rust_version(ws, resolve, *package_id); + let latest = report_latest(&possibilities, *package_id); let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( "Adding", - format!("{package}{note}"), + format!("{package_id}{note}"), &style::NOTE, )?; } @@ -631,25 +631,25 @@ fn print_lockfile_updates( vec![] }; - if let Some((removed, added)) = diff.change() { - let required_rust_version = report_required_rust_version(ws, resolve, added); - let latest = report_latest(&possibilities, added); + if let Some((previous_id, package_id)) = diff.change() { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if removed.source_id().is_git() { + let msg = if previous_id.source_id().is_git() { format!( - "{removed} -> #{}{note}", - &added.source_id().precise_git_fragment().unwrap()[..8], + "{previous_id} -> #{}{note}", + &package_id.source_id().precise_git_fragment().unwrap()[..8], ) } else { - format!("{removed} -> v{}{note}", added.version()) + format!("{previous_id} -> v{}{note}", package_id.version()) }; // If versions differ only in build metadata, we call it an "update" // regardless of whether the build metadata has gone up or down. // This metadata is often stuff like git commit hashes, which are // not meaningfully ordered. - if removed.version().cmp_precedence(added.version()) == Ordering::Greater { + if previous_id.version().cmp_precedence(package_id.version()) == Ordering::Greater { ws.gctx() .shell() .status_with_color("Downgrading", msg, &style::WARN)?; @@ -659,28 +659,28 @@ fn print_lockfile_updates( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package in diff.removed.iter() { + for package_id in diff.removed.iter() { ws.gctx().shell().status_with_color( "Removing", - format!("{package}"), + format!("{package_id}"), &style::ERROR, )?; } - for package in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package); - let latest = report_latest(&possibilities, *package); + for package_id in diff.added.iter() { + let required_rust_version = report_required_rust_version(ws, resolve, *package_id); + let latest = report_latest(&possibilities, *package_id); let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( "Adding", - format!("{package}{note}"), + format!("{package_id}{note}"), &style::NOTE, )?; } } - for package in &diff.unchanged { - let required_rust_version = report_required_rust_version(ws, resolve, *package); - let latest = report_latest(&possibilities, *package); + for package_id in &diff.unchanged { + let required_rust_version = report_required_rust_version(ws, resolve, *package_id); + let latest = report_latest(&possibilities, *package_id); let note = required_rust_version.as_deref().or(latest.as_deref()); if let Some(note) = note { @@ -690,7 +690,7 @@ fn print_lockfile_updates( if ws.gctx().shell().verbosity() == Verbosity::Verbose { ws.gctx().shell().status_with_color( "Unchanged", - format!("{package}{note}"), + format!("{package_id}{note}"), &anstyle::Style::new().bold(), )?; } From 87850c2e76fc0024ba5e0e8d107eb3fa649832a1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:30:12 -0500 Subject: [PATCH 05/12] refactor(update): Remove unnecesary derefs --- src/cargo/ops/cargo_update.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 3d91b9c6371..d5a780490ec 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -513,9 +513,9 @@ fn print_lockfile_generation( vec![] }; - for package_id in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package_id); - let latest = report_latest(&possibilities, *package_id); + for package_id in diff.added { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest); if let Some(note) = note { @@ -586,9 +586,9 @@ fn print_lockfile_sync( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package_id in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package_id); - let latest = report_latest(&possibilities, *package_id); + for package_id in diff.added { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( @@ -659,16 +659,16 @@ fn print_lockfile_updates( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package_id in diff.removed.iter() { + for package_id in diff.removed { ws.gctx().shell().status_with_color( "Removing", format!("{package_id}"), &style::ERROR, )?; } - for package_id in diff.added.iter() { - let required_rust_version = report_required_rust_version(ws, resolve, *package_id); - let latest = report_latest(&possibilities, *package_id); + for package_id in diff.added { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( @@ -678,9 +678,9 @@ fn print_lockfile_updates( )?; } } - for package_id in &diff.unchanged { - let required_rust_version = report_required_rust_version(ws, resolve, *package_id); - let latest = report_latest(&possibilities, *package_id); + for package_id in diff.unchanged { + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); let note = required_rust_version.as_deref().or(latest.as_deref()); if let Some(note) = note { From e4a24b71e15198145e98ff2e16ac8a6c0bdf5cba Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:32:59 -0500 Subject: [PATCH 06/12] refactor(update): Be consistent in what source we interact with --- src/cargo/ops/cargo_update.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index d5a780490ec..75d0b4f8357 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -563,7 +563,7 @@ fn print_lockfile_sync( let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if previous_id.source_id().is_git() { + let msg = if package_id.source_id().is_git() { format!( "{previous_id} -> #{}", &package_id.source_id().precise_git_fragment().unwrap()[..8], @@ -636,7 +636,7 @@ fn print_lockfile_updates( let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if previous_id.source_id().is_git() { + let msg = if package_id.source_id().is_git() { format!( "{previous_id} -> #{}{note}", &package_id.source_id().precise_git_fragment().unwrap()[..8], From 88d8ace88f50291987ec3fdb532a0068f9368127 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:38:09 -0500 Subject: [PATCH 07/12] fix(update): Add missing required rust-version note for git --- src/cargo/ops/cargo_update.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 75d0b4f8357..41d7d0880fa 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -565,7 +565,7 @@ fn print_lockfile_sync( let msg = if package_id.source_id().is_git() { format!( - "{previous_id} -> #{}", + "{previous_id} -> #{}{note}", &package_id.source_id().precise_git_fragment().unwrap()[..8], ) } else { From d60fc64199ee567c0c1accf78af8e17d87b32e70 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:12:38 -0500 Subject: [PATCH 08/12] refactor(update): Centralize "what is a change" This is prep for consolidating a lot of code *and* tracking more information so we can have a richer output. --- src/cargo/ops/cargo_update.rs | 184 ++++++++++++++++++++++++---------- 1 file changed, 133 insertions(+), 51 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 41d7d0880fa..cffe9f308f8 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -17,6 +17,7 @@ use crate::util::{style, OptVersionReq}; use crate::util::{CargoResult, VersionExt}; use anyhow::Context as _; use cargo_util_schemas::core::PartialVersion; +use indexmap::IndexMap; use itertools::Itertools; use semver::{Op, Version, VersionReq}; use std::cmp::Ordering; @@ -491,16 +492,16 @@ fn print_lockfile_generation( resolve: &Resolve, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff: Vec<_> = PackageDiff::new(&resolve).collect(); - let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); + let changes = PackageChange::new(resolve); + let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count(); if num_pkgs <= 1 { // just ourself, nothing worth reporting return Ok(()); } status_locking(ws, num_pkgs)?; - for diff in diff { - let possibilities = if let Some(query) = diff.alternatives_query() { + for change in changes { + let possibilities = if let Some(query) = change.alternatives_query() { loop { match registry.query_vec(&query, QueryKind::Exact) { std::task::Poll::Ready(res) => { @@ -513,12 +514,14 @@ fn print_lockfile_generation( vec![] }; - for package_id in diff.added { + { + let package_id = change.package_id; let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest); if let Some(note) = note { + assert_eq!(change.kind, PackageChangeKind::Added); ws.gctx().shell().status_with_color( "Adding", format!("{package_id}{note}"), @@ -537,15 +540,15 @@ fn print_lockfile_sync( resolve: &Resolve, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff: Vec<_> = PackageDiff::diff(&previous_resolve, &resolve).collect(); - let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); + let changes = PackageChange::diff(previous_resolve, resolve); + let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count(); if num_pkgs == 0 { return Ok(()); } status_locking(ws, num_pkgs)?; - for diff in diff { - let possibilities = if let Some(query) = diff.alternatives_query() { + for change in changes { + let possibilities = if let Some(query) = change.alternatives_query() { loop { match registry.query_vec(&query, QueryKind::Exact) { std::task::Poll::Ready(res) => { @@ -558,7 +561,8 @@ fn print_lockfile_sync( vec![] }; - if let Some((previous_id, package_id)) = diff.change() { + let package_id = change.package_id; + if let Some(previous_id) = change.previous_id { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); @@ -572,11 +576,7 @@ fn print_lockfile_sync( format!("{previous_id} -> v{}{note}", package_id.version()) }; - // If versions differ only in build metadata, we call it an "update" - // regardless of whether the build metadata has gone up or down. - // This metadata is often stuff like git commit hashes, which are - // not meaningfully ordered. - if previous_id.version().cmp_precedence(package_id.version()) == Ordering::Greater { + if change.kind == PackageChangeKind::Downgraded { ws.gctx() .shell() .status_with_color("Downgrading", msg, &style::WARN)?; @@ -586,7 +586,7 @@ fn print_lockfile_sync( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package_id in diff.added { + if change.kind == PackageChangeKind::Added { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); @@ -610,15 +610,15 @@ fn print_lockfile_updates( precise: bool, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { - let diff: Vec<_> = PackageDiff::diff(&previous_resolve, &resolve).collect(); - let num_pkgs: usize = diff.iter().map(|d| d.added.len()).sum(); + let changes = PackageChange::diff(previous_resolve, resolve); + let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count(); if !precise { status_locking(ws, num_pkgs)?; } let mut unchanged_behind = 0; - for diff in diff { - let possibilities = if let Some(query) = diff.alternatives_query() { + for change in changes { + let possibilities = if let Some(query) = change.alternatives_query() { loop { match registry.query_vec(&query, QueryKind::Exact) { std::task::Poll::Ready(res) => { @@ -631,7 +631,8 @@ fn print_lockfile_updates( vec![] }; - if let Some((previous_id, package_id)) = diff.change() { + let package_id = change.package_id; + if let Some(previous_id) = change.previous_id { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); @@ -645,11 +646,7 @@ fn print_lockfile_updates( format!("{previous_id} -> v{}{note}", package_id.version()) }; - // If versions differ only in build metadata, we call it an "update" - // regardless of whether the build metadata has gone up or down. - // This metadata is often stuff like git commit hashes, which are - // not meaningfully ordered. - if previous_id.version().cmp_precedence(package_id.version()) == Ordering::Greater { + if change.kind == PackageChangeKind::Downgraded { ws.gctx() .shell() .status_with_color("Downgrading", msg, &style::WARN)?; @@ -659,14 +656,13 @@ fn print_lockfile_updates( .status_with_color("Updating", msg, &style::GOOD)?; } } else { - for package_id in diff.removed { + if change.kind == PackageChangeKind::Removed { ws.gctx().shell().status_with_color( "Removing", format!("{package_id}"), &style::ERROR, )?; - } - for package_id in diff.added { + } else if change.kind == PackageChangeKind::Added { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); @@ -678,7 +674,7 @@ fn print_lockfile_updates( )?; } } - for package_id in diff.unchanged { + if change.kind == PackageChangeKind::Unchanged { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.as_deref().or(latest.as_deref()); @@ -818,6 +814,113 @@ fn fill_with_deps<'a>( } } +#[derive(Clone, Debug)] +struct PackageChange { + package_id: PackageId, + previous_id: Option, + kind: PackageChangeKind, +} + +impl PackageChange { + pub fn new(resolve: &Resolve) -> Vec { + let diff = PackageDiff::new(resolve); + Self::with_diff(diff) + } + + pub fn diff(previous_resolve: &Resolve, resolve: &Resolve) -> Vec { + let diff = PackageDiff::diff(previous_resolve, resolve); + Self::with_diff(diff) + } + + fn with_diff(diff: impl Iterator) -> Vec { + let mut changes = IndexMap::new(); + for diff in diff { + if let Some((previous_id, package_id)) = diff.change() { + // If versions differ only in build metadata, we call it an "update" + // regardless of whether the build metadata has gone up or down. + // This metadata is often stuff like git commit hashes, which are + // not meaningfully ordered. + let kind = if previous_id.version().cmp_precedence(package_id.version()) + == Ordering::Greater + { + PackageChangeKind::Downgraded + } else { + PackageChangeKind::Upgraded + }; + let change = Self { + package_id, + previous_id: Some(previous_id), + kind, + }; + changes.insert(change.package_id, change); + } else { + for package_id in diff.removed { + let kind = PackageChangeKind::Removed; + let change = Self { + package_id, + previous_id: None, + kind, + }; + changes.insert(change.package_id, change); + } + for package_id in diff.added { + let kind = PackageChangeKind::Added; + let change = Self { + package_id, + previous_id: None, + kind, + }; + changes.insert(change.package_id, change); + } + } + for package_id in diff.unchanged { + let kind = PackageChangeKind::Unchanged; + let change = Self { + package_id, + previous_id: None, + kind, + }; + changes.insert(change.package_id, change); + } + } + + changes.into_values().collect() + } + + /// For querying [`PackageRegistry`] for alternative versions to report to the user + fn alternatives_query(&self) -> Option { + if !self.package_id.source_id().is_registry() { + return None; + } + + let query = crate::core::dependency::Dependency::parse( + self.package_id.name(), + None, + self.package_id.source_id(), + ) + .expect("already a valid dependency"); + Some(query) + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +enum PackageChangeKind { + Added, + Removed, + Upgraded, + Downgraded, + Unchanged, +} + +impl PackageChangeKind { + pub fn is_new(&self) -> bool { + match self { + Self::Added | Self::Upgraded | Self::Downgraded => true, + Self::Removed | Self::Unchanged => false, + } + } +} + /// All resolved versions of a package name within a [`SourceId`] #[derive(Default, Clone, Debug)] pub struct PackageDiff { @@ -931,25 +1034,4 @@ impl PackageDiff { None } } - - /// For querying [`PackageRegistry`] for alternative versions to report to the user - pub fn alternatives_query(&self) -> Option { - let package_id = [ - self.added.iter(), - self.unchanged.iter(), - self.removed.iter(), - ] - .into_iter() - .flatten() - .next() - // Limit to registry as that is the only source with meaningful alternative versions - .filter(|s| s.source_id().is_registry())?; - let query = crate::core::dependency::Dependency::parse( - package_id.name(), - None, - package_id.source_id(), - ) - .expect("already a valid dependency"); - Some(query) - } } From f31e37da28b3301e6ea138e83b77c6eb84b06e56 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:43:17 -0500 Subject: [PATCH 09/12] refactor(update): Centralize status key --- src/cargo/ops/cargo_update.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index cffe9f308f8..9d87d7930a1 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -523,7 +523,7 @@ fn print_lockfile_generation( if let Some(note) = note { assert_eq!(change.kind, PackageChangeKind::Added); ws.gctx().shell().status_with_color( - "Adding", + change.kind.status(), format!("{package_id}{note}"), &style::NOTE, )?; @@ -579,11 +579,11 @@ fn print_lockfile_sync( if change.kind == PackageChangeKind::Downgraded { ws.gctx() .shell() - .status_with_color("Downgrading", msg, &style::WARN)?; + .status_with_color(change.kind.status(), msg, &style::WARN)?; } else { ws.gctx() .shell() - .status_with_color("Updating", msg, &style::GOOD)?; + .status_with_color(change.kind.status(), msg, &style::GOOD)?; } } else { if change.kind == PackageChangeKind::Added { @@ -592,7 +592,7 @@ fn print_lockfile_sync( let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( - "Adding", + change.kind.status(), format!("{package_id}{note}"), &style::NOTE, )?; @@ -649,16 +649,16 @@ fn print_lockfile_updates( if change.kind == PackageChangeKind::Downgraded { ws.gctx() .shell() - .status_with_color("Downgrading", msg, &style::WARN)?; + .status_with_color(change.kind.status(), msg, &style::WARN)?; } else { ws.gctx() .shell() - .status_with_color("Updating", msg, &style::GOOD)?; + .status_with_color(change.kind.status(), msg, &style::GOOD)?; } } else { if change.kind == PackageChangeKind::Removed { ws.gctx().shell().status_with_color( - "Removing", + change.kind.status(), format!("{package_id}"), &style::ERROR, )?; @@ -668,7 +668,7 @@ fn print_lockfile_updates( let note = required_rust_version.or(latest).unwrap_or_default(); ws.gctx().shell().status_with_color( - "Adding", + change.kind.status(), format!("{package_id}{note}"), &style::NOTE, )?; @@ -685,7 +685,7 @@ fn print_lockfile_updates( } if ws.gctx().shell().verbosity() == Verbosity::Verbose { ws.gctx().shell().status_with_color( - "Unchanged", + change.kind.status(), format!("{package_id}{note}"), &anstyle::Style::new().bold(), )?; @@ -919,6 +919,16 @@ impl PackageChangeKind { Self::Removed | Self::Unchanged => false, } } + + pub fn status(&self) -> &'static str { + match self { + Self::Added => "Adding", + Self::Removed => "Removing", + Self::Upgraded => "Updating", + Self::Downgraded => "Downgrading", + Self::Unchanged => "Unchanged", + } + } } /// All resolved versions of a package name within a [`SourceId`] From 1a2844e7660c0f49fcae014a178dc70d9cadce54 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 16:46:09 -0500 Subject: [PATCH 10/12] refactor(update): Centralize status style --- src/cargo/ops/cargo_update.rs | 52 +++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 9d87d7930a1..570e3d13115 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -525,7 +525,7 @@ fn print_lockfile_generation( ws.gctx().shell().status_with_color( change.kind.status(), format!("{package_id}{note}"), - &style::NOTE, + &change.kind.style(), )?; } } @@ -577,13 +577,17 @@ fn print_lockfile_sync( }; if change.kind == PackageChangeKind::Downgraded { - ws.gctx() - .shell() - .status_with_color(change.kind.status(), msg, &style::WARN)?; + ws.gctx().shell().status_with_color( + change.kind.status(), + msg, + &change.kind.style(), + )?; } else { - ws.gctx() - .shell() - .status_with_color(change.kind.status(), msg, &style::GOOD)?; + ws.gctx().shell().status_with_color( + change.kind.status(), + msg, + &change.kind.style(), + )?; } } else { if change.kind == PackageChangeKind::Added { @@ -594,7 +598,7 @@ fn print_lockfile_sync( ws.gctx().shell().status_with_color( change.kind.status(), format!("{package_id}{note}"), - &style::NOTE, + &change.kind.style(), )?; } } @@ -647,20 +651,24 @@ fn print_lockfile_updates( }; if change.kind == PackageChangeKind::Downgraded { - ws.gctx() - .shell() - .status_with_color(change.kind.status(), msg, &style::WARN)?; + ws.gctx().shell().status_with_color( + change.kind.status(), + msg, + &change.kind.style(), + )?; } else { - ws.gctx() - .shell() - .status_with_color(change.kind.status(), msg, &style::GOOD)?; + ws.gctx().shell().status_with_color( + change.kind.status(), + msg, + &change.kind.style(), + )?; } } else { if change.kind == PackageChangeKind::Removed { ws.gctx().shell().status_with_color( change.kind.status(), format!("{package_id}"), - &style::ERROR, + &change.kind.style(), )?; } else if change.kind == PackageChangeKind::Added { let required_rust_version = report_required_rust_version(ws, resolve, package_id); @@ -670,7 +678,7 @@ fn print_lockfile_updates( ws.gctx().shell().status_with_color( change.kind.status(), format!("{package_id}{note}"), - &style::NOTE, + &change.kind.style(), )?; } } @@ -687,7 +695,7 @@ fn print_lockfile_updates( ws.gctx().shell().status_with_color( change.kind.status(), format!("{package_id}{note}"), - &anstyle::Style::new().bold(), + &change.kind.style(), )?; } } @@ -929,6 +937,16 @@ impl PackageChangeKind { Self::Unchanged => "Unchanged", } } + + pub fn style(&self) -> anstyle::Style { + match self { + Self::Added => style::NOTE, + Self::Removed => style::ERROR, + Self::Upgraded => style::GOOD, + Self::Downgraded => style::WARN, + Self::Unchanged => anstyle::Style::new().bold(), + } + } } /// All resolved versions of a package name within a [`SourceId`] From 68196584a7b2b72c32337868c590ade0e18106fe Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 20 Aug 2024 18:30:40 -0500 Subject: [PATCH 11/12] refactor(update): Consolidate change rendering --- src/cargo/ops/cargo_update.rs | 51 +++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 570e3d13115..6d2d2bd2eda 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -524,7 +524,7 @@ fn print_lockfile_generation( assert_eq!(change.kind, PackageChangeKind::Added); ws.gctx().shell().status_with_color( change.kind.status(), - format!("{package_id}{note}"), + format!("{change}{note}"), &change.kind.style(), )?; } @@ -562,19 +562,12 @@ fn print_lockfile_sync( }; let package_id = change.package_id; - if let Some(previous_id) = change.previous_id { + if change.previous_id.is_some() { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if package_id.source_id().is_git() { - format!( - "{previous_id} -> #{}{note}", - &package_id.source_id().precise_git_fragment().unwrap()[..8], - ) - } else { - format!("{previous_id} -> v{}{note}", package_id.version()) - }; + let msg = format!("{change}{note}"); if change.kind == PackageChangeKind::Downgraded { ws.gctx().shell().status_with_color( @@ -597,7 +590,7 @@ fn print_lockfile_sync( ws.gctx().shell().status_with_color( change.kind.status(), - format!("{package_id}{note}"), + format!("{change}{note}"), &change.kind.style(), )?; } @@ -636,19 +629,12 @@ fn print_lockfile_updates( }; let package_id = change.package_id; - if let Some(previous_id) = change.previous_id { + if change.previous_id.is_some() { let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); - let msg = if package_id.source_id().is_git() { - format!( - "{previous_id} -> #{}{note}", - &package_id.source_id().precise_git_fragment().unwrap()[..8], - ) - } else { - format!("{previous_id} -> v{}{note}", package_id.version()) - }; + let msg = format!("{change}{note}"); if change.kind == PackageChangeKind::Downgraded { ws.gctx().shell().status_with_color( @@ -667,7 +653,7 @@ fn print_lockfile_updates( if change.kind == PackageChangeKind::Removed { ws.gctx().shell().status_with_color( change.kind.status(), - format!("{package_id}"), + format!("{change}"), &change.kind.style(), )?; } else if change.kind == PackageChangeKind::Added { @@ -677,7 +663,7 @@ fn print_lockfile_updates( ws.gctx().shell().status_with_color( change.kind.status(), - format!("{package_id}{note}"), + format!("{change}{note}"), &change.kind.style(), )?; } @@ -694,7 +680,7 @@ fn print_lockfile_updates( if ws.gctx().shell().verbosity() == Verbosity::Verbose { ws.gctx().shell().status_with_color( change.kind.status(), - format!("{package_id}{note}"), + format!("{change}{note}"), &change.kind.style(), )?; } @@ -911,6 +897,25 @@ impl PackageChange { } } +impl std::fmt::Display for PackageChange { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let package_id = self.package_id; + if let Some(previous_id) = self.previous_id { + if package_id.source_id().is_git() { + write!( + f, + "{previous_id} -> #{}", + &package_id.source_id().precise_git_fragment().unwrap()[..8], + ) + } else { + write!(f, "{previous_id} -> v{}", package_id.version()) + } + } else { + write!(f, "{package_id}") + } + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] enum PackageChangeKind { Added, From f8a9cdaa7e0b3a06ffbc22cc77c95b2dded14827 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 21 Aug 2024 15:34:36 -0500 Subject: [PATCH 12/12] refactor(update): Consolidate status messages This builds on the prior work to consolidate everything, simplifying the code and making it clearer what behavior differences exist between change kinds. --- src/cargo/ops/cargo_update.rs | 170 ++++++++++-------------- tests/testsuite/global_cache_tracker.rs | 4 +- 2 files changed, 74 insertions(+), 100 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 6d2d2bd2eda..1725841274e 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -501,32 +501,39 @@ fn print_lockfile_generation( status_locking(ws, num_pkgs)?; for change in changes { - let possibilities = if let Some(query) = change.alternatives_query() { - loop { - match registry.query_vec(&query, QueryKind::Exact) { - std::task::Poll::Ready(res) => { - break res?; + match change.kind { + PackageChangeKind::Added => { + let possibilities = if let Some(query) = change.alternatives_query() { + loop { + match registry.query_vec(&query, QueryKind::Exact) { + std::task::Poll::Ready(res) => { + break res?; + } + std::task::Poll::Pending => registry.block_until_ready()?, + } } - std::task::Poll::Pending => registry.block_until_ready()?, - } - } - } else { - vec![] - }; + } else { + vec![] + }; - { - let package_id = change.package_id; - let required_rust_version = report_required_rust_version(ws, resolve, package_id); - let latest = report_latest(&possibilities, package_id); - let note = required_rust_version.or(latest); + let package_id = change.package_id; + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); + let note = required_rust_version.or(latest); - if let Some(note) = note { - assert_eq!(change.kind, PackageChangeKind::Added); - ws.gctx().shell().status_with_color( - change.kind.status(), - format!("{change}{note}"), - &change.kind.style(), - )?; + if let Some(note) = note { + ws.gctx().shell().status_with_color( + change.kind.status(), + format!("{change}{note}"), + &change.kind.style(), + )?; + } + } + PackageChangeKind::Upgraded + | PackageChangeKind::Downgraded + | PackageChangeKind::Removed + | PackageChangeKind::Unchanged => { + unreachable!("without a previous resolve, everything should be added") } } } @@ -548,42 +555,24 @@ fn print_lockfile_sync( status_locking(ws, num_pkgs)?; for change in changes { - let possibilities = if let Some(query) = change.alternatives_query() { - loop { - match registry.query_vec(&query, QueryKind::Exact) { - std::task::Poll::Ready(res) => { - break res?; + match change.kind { + PackageChangeKind::Added + | PackageChangeKind::Upgraded + | PackageChangeKind::Downgraded => { + let possibilities = if let Some(query) = change.alternatives_query() { + loop { + match registry.query_vec(&query, QueryKind::Exact) { + std::task::Poll::Ready(res) => { + break res?; + } + std::task::Poll::Pending => registry.block_until_ready()?, + } } - std::task::Poll::Pending => registry.block_until_ready()?, - } - } - } else { - vec![] - }; - - let package_id = change.package_id; - if change.previous_id.is_some() { - let required_rust_version = report_required_rust_version(ws, resolve, package_id); - let latest = report_latest(&possibilities, package_id); - let note = required_rust_version.or(latest).unwrap_or_default(); - - let msg = format!("{change}{note}"); + } else { + vec![] + }; - if change.kind == PackageChangeKind::Downgraded { - ws.gctx().shell().status_with_color( - change.kind.status(), - msg, - &change.kind.style(), - )?; - } else { - ws.gctx().shell().status_with_color( - change.kind.status(), - msg, - &change.kind.style(), - )?; - } - } else { - if change.kind == PackageChangeKind::Added { + let package_id = change.package_id; let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); let note = required_rust_version.or(latest).unwrap_or_default(); @@ -594,6 +583,7 @@ fn print_lockfile_sync( &change.kind.style(), )?; } + PackageChangeKind::Removed | PackageChangeKind::Unchanged => {} } } @@ -628,61 +618,45 @@ fn print_lockfile_updates( vec![] }; - let package_id = change.package_id; - if change.previous_id.is_some() { - let required_rust_version = report_required_rust_version(ws, resolve, package_id); - let latest = report_latest(&possibilities, package_id); - let note = required_rust_version.or(latest).unwrap_or_default(); - - let msg = format!("{change}{note}"); + match change.kind { + PackageChangeKind::Added + | PackageChangeKind::Upgraded + | PackageChangeKind::Downgraded => { + let package_id = change.package_id; + let required_rust_version = report_required_rust_version(ws, resolve, package_id); + let latest = report_latest(&possibilities, package_id); + let note = required_rust_version.or(latest).unwrap_or_default(); - if change.kind == PackageChangeKind::Downgraded { ws.gctx().shell().status_with_color( change.kind.status(), - msg, - &change.kind.style(), - )?; - } else { - ws.gctx().shell().status_with_color( - change.kind.status(), - msg, + format!("{change}{note}"), &change.kind.style(), )?; } - } else { - if change.kind == PackageChangeKind::Removed { + PackageChangeKind::Removed => { ws.gctx().shell().status_with_color( change.kind.status(), format!("{change}"), &change.kind.style(), )?; - } else if change.kind == PackageChangeKind::Added { + } + PackageChangeKind::Unchanged => { + let package_id = change.package_id; let required_rust_version = report_required_rust_version(ws, resolve, package_id); let latest = report_latest(&possibilities, package_id); - let note = required_rust_version.or(latest).unwrap_or_default(); + let note = required_rust_version.as_deref().or(latest.as_deref()); - ws.gctx().shell().status_with_color( - change.kind.status(), - format!("{change}{note}"), - &change.kind.style(), - )?; - } - } - if change.kind == PackageChangeKind::Unchanged { - let required_rust_version = report_required_rust_version(ws, resolve, package_id); - let latest = report_latest(&possibilities, package_id); - let note = required_rust_version.as_deref().or(latest.as_deref()); - - if let Some(note) = note { - if latest.is_some() { - unchanged_behind += 1; - } - if ws.gctx().shell().verbosity() == Verbosity::Verbose { - ws.gctx().shell().status_with_color( - change.kind.status(), - format!("{change}{note}"), - &change.kind.style(), - )?; + if let Some(note) = note { + if latest.is_some() { + unchanged_behind += 1; + } + if ws.gctx().shell().verbosity() == Verbosity::Verbose { + ws.gctx().shell().status_with_color( + change.kind.status(), + format!("{change}{note}"), + &change.kind.style(), + )?; + } } } } diff --git a/tests/testsuite/global_cache_tracker.rs b/tests/testsuite/global_cache_tracker.rs index 449cdc6e368..623a5bea0d0 100644 --- a/tests/testsuite/global_cache_tracker.rs +++ b/tests/testsuite/global_cache_tracker.rs @@ -439,7 +439,7 @@ fn frequency() { .env("CARGO_GC_AUTO_FREQUENCY", "1 day") .masquerade_as_nightly_cargo(&["gc"]) .run(); - assert_eq!(get_index_names().len(), 1); + assert_eq!(get_index_names().len(), 0); assert_eq!(get_registry_names("src").len(), 0); assert_eq!(get_registry_names("cache").len(), 0); } @@ -613,7 +613,7 @@ fn auto_gc_various_commands() { .acquire_package_cache_lock(CacheLockMode::MutateExclusive) .unwrap(); let indexes = tracker.registry_index_all().unwrap(); - assert_eq!(indexes.len(), 1); + assert_eq!(indexes.len(), 0); let crates = tracker.registry_crate_all().unwrap(); assert_eq!(crates.len(), 0); let srcs = tracker.registry_src_all().unwrap();