Auto merge of #9392 - alexcrichton:fix-patch-git-head-issue, r=ehuss

Fix loading `branch=master` patches in the v3 lock transition

This commit fixes an issue pointed out during #9352 where in the v2->v3
lock file transition (currently happening on nightly) Cargo will not
correctly use the previous lock file entry for `[patch]` directives that
point to git dependencies using `branch = 'master'` explicitly. The
reason for this is that Cargo previously, with the v2 format, considered
`branch=master` and `DefaultBranch` to be equivalent dependencies. Now
that Cargo treats those as distinct resolve nodes we need to load lock
files that use `DefaultBranch` and transparently use those for
`branch=master` dependencies.

These lock file nodes do not naturally unify so we have to go out of our
way to get the two to line up in modern Cargo. This was previously done
for the lock file at large, but the previous logic didn't take `[patch]`
into account. Unfortunately almost everything to do with `[patch]` and
lock files is pretty complicated, and this is no exception. The fix here
is wordy, verbose, and quite subtle in how it works. I'm pretty sure it
does work though and I think that this should be good enough to at least
transition most users off the v2 lock file format. Once this has baked
in Cargo for some time (on the scale of a year) I would hope that we
could just remove this logic since it's only really here for a
transitionary period.

Closes #9352

Author: bors

src/cargo/core/registry.rs

@@ -107,6 +107,21 @@ enum Kind {
     Normal,
 }
 
+/// Argument to `PackageRegistry::patch` which is information about a `[patch]`
+/// directive that we found in a lockfile, if present.
+pub struct LockedPatchDependency {
+    /// The original `Dependency` directive, except "locked" so it's version
+    /// requirement is `=foo` and its `SourceId` has a "precise" listed.
+    pub dependency: Dependency,
+    /// The `PackageId` that was previously found in a lock file which
+    /// `dependency` matches.
+    pub package_id: PackageId,
+    /// Something only used for backwards compatibility with the v2 lock file
+    /// format where `branch=master` is considered the same as `DefaultBranch`.
+    /// For more comments on this see the code in `ops/resolve.rs`.
+    pub alt_package_id: Option<PackageId>,
+}
+
 impl<'cfg> PackageRegistry<'cfg> {
     pub fn new(config: &'cfg Config) -> CargoResult<PackageRegistry<'cfg>> {
         let source_config = SourceConfigMap::new(config)?;
@@ -240,7 +255,7 @@ impl<'cfg> PackageRegistry<'cfg> {
     pub fn patch(
         &mut self,
         url: &Url,
-        deps: &[(&Dependency, Option<(Dependency, PackageId)>)],
+        deps: &[(&Dependency, Option<LockedPatchDependency>)],
     ) -> CargoResult<Vec<(Dependency, PackageId)>> {
         // NOTE: None of this code is aware of required features. If a patch
         // is missing a required feature, you end up with an "unused patch"
@@ -268,7 +283,7 @@ impl<'cfg> PackageRegistry<'cfg> {
                 let orig_patch = *orig_patch;
                 // Use the locked patch if it exists, otherwise use the original.
                 let dep = match locked {
-                    Some((locked_patch, _locked_id)) => locked_patch,
+                    Some(lock) => &lock.dependency,
                     None => orig_patch,
                 };
                 debug!(
@@ -338,13 +353,36 @@ impl<'cfg> PackageRegistry<'cfg> {
             }
         }
 
+        // Calculate a list of all patches available for this source which is
+        // then used later during calls to `lock` to rewrite summaries to point
+        // directly at these patched entries.
+        //
+        // Note that this is somewhat subtle where the list of `ids` for a
+        // canonical URL is extend with possibly two ids per summary. This is done
+        // to handle the transition from the v2->v3 lock file format where in
+        // v2 DefeaultBranch was either DefaultBranch or Branch("master") for
+        // git dependencies. In this case if `summary.package_id()` is
+        // Branch("master") then alt_package_id will be DefaultBranch. This
+        // signifies that there's a patch available for either of those
+        // dependency directives if we see them in the dependency graph.
+        //
+        // This is a bit complicated and hopefully an edge case we can remove
+        // in the future, but for now it hopefully doesn't cause too much
+        // harm...
+        let mut ids = Vec::new();
+        for (summary, (_, lock)) in unlocked_summaries.iter().zip(deps) {
+            ids.push(summary.package_id());
+            if let Some(lock) = lock {
+                ids.extend(lock.alt_package_id);
+            }
+        }
+        self.patches_available.insert(canonical.clone(), ids);
+
         // Note that we do not use `lock` here to lock summaries! That step
         // happens later once `lock_patches` is invoked. In the meantime though
         // we want to fill in the `patches_available` map (later used in the
         // `lock` method) and otherwise store the unlocked summaries in
         // `patches` to get locked in a future call to `lock_patches`.
-        let ids = unlocked_summaries.iter().map(|s| s.package_id()).collect();
-        self.patches_available.insert(canonical.clone(), ids);
         self.patches.insert(canonical, unlocked_summaries);
 
         Ok(unlock_patches)
@@ -747,7 +785,7 @@ fn lock(
 /// This is a helper for selecting the summary, or generating a helpful error message.
 fn summary_for_patch(
     orig_patch: &Dependency,
-    locked: &Option<(Dependency, PackageId)>,
+    locked: &Option<LockedPatchDependency>,
     mut summaries: Vec<Summary>,
     source: &mut dyn Source,
 ) -> CargoResult<(Summary, Option<PackageId>)> {
@@ -779,7 +817,7 @@ fn summary_for_patch(
     }
     assert!(summaries.is_empty());
     // No summaries found, try to help the user figure out what is wrong.
-    if let Some((_locked_patch, locked_id)) = locked {
+    if let Some(locked) = locked {
         // Since the locked patch did not match anything, try the unlocked one.
         let orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
             log::warn!(
@@ -792,7 +830,7 @@ fn summary_for_patch(
         let (summary, _) = summary_for_patch(orig_patch, &None, orig_matches, source)?;
         // The unlocked version found a match. This returns a value to
         // indicate that this entry should be unlocked.
-        return Ok((summary, Some(*locked_id)));
+        return Ok((summary, Some(locked.package_id)));
     }
     // Try checking if there are *any* packages that match this by name.
     let name_only_dep = Dependency::new_override(orig_patch.package_name(), orig_patch.source_id());

src/cargo/ops/resolve.rs

@@ -11,7 +11,7 @@
 //!   providing the most power and flexibility.
 
 use crate::core::compiler::{CompileKind, RustcTargetData};
-use crate::core::registry::PackageRegistry;
+use crate::core::registry::{LockedPatchDependency, PackageRegistry};
 use crate::core::resolver::features::{
     CliFeatures, FeatureOpts, FeatureResolver, ForceAllTargets, RequestedFeatures, ResolvedFeatures,
 };
@@ -257,26 +257,91 @@ pub fn resolve_with_previous<'cfg>(
                     continue;
                 }
             };
-            let patches = patches
-                .iter()
-                .map(|dep| {
-                    let unused = previous.unused_patches().iter().cloned();
-                    let candidates = previous.iter().chain(unused);
-                    match candidates
-                        .filter(pre_patch_keep)
-                        .find(|&id| dep.matches_id(id))
-                    {
-                        Some(id) => {
-                            let mut locked_dep = dep.clone();
-                            locked_dep.lock_to(id);
-                            (dep, Some((locked_dep, id)))
+
+            // This is a list of pairs where the first element of the pair is
+            // the raw `Dependency` which matches what's listed in `Cargo.toml`.
+            // The second element is, if present, the "locked" version of
+            // the `Dependency` as well as the `PackageId` that it previously
+            // resolved to. This second element is calculated by looking at the
+            // previous resolve graph, which is primarily what's done here to
+            // build the `registrations` list.
+            let mut registrations = Vec::new();
+            for dep in patches {
+                let candidates = || {
+                    previous
+                        .iter()
+                        .chain(previous.unused_patches().iter().cloned())
+                        .filter(&pre_patch_keep)
+                };
+
+                let lock = match candidates().find(|id| dep.matches_id(*id)) {
+                    // If we found an exactly matching candidate in our list of
+                    // candidates, then that's the one to use.
+                    Some(package_id) => {
+                        let mut locked_dep = dep.clone();
+                        locked_dep.lock_to(package_id);
+                        Some(LockedPatchDependency {
+                            dependency: locked_dep,
+                            package_id,
+                            alt_package_id: None,
+                        })
+                    }
+                    None => {
+                        // If the candidate does not have a matching source id
+                        // then we may still have a lock candidate. If we're
+                        // loading a v2-encoded resolve graph and `dep` is a
+                        // git dep with `branch = 'master'`, then this should
+                        // also match candidates without `branch = 'master'`
+                        // (which is now treated separately in Cargo).
+                        //
+                        // In this scenario we try to convert candidates located
+                        // in the resolve graph to explicitly having the
+                        // `master` branch (if they otherwise point to
+                        // `DefaultBranch`). If this works and our `dep`
+                        // matches that then this is something we'll lock to.
+                        match candidates().find(|&id| {
+                            match master_branch_git_source(id, previous) {
+                                Some(id) => dep.matches_id(id),
+                                None => false,
+                            }
+                        }) {
+                            Some(id_using_default) => {
+                                let id_using_master = id_using_default.with_source_id(
+                                    dep.source_id().with_precise(
+                                        id_using_default
+                                            .source_id()
+                                            .precise()
+                                            .map(|s| s.to_string()),
+                                    ),
+                                );
+
+                                let mut locked_dep = dep.clone();
+                                locked_dep.lock_to(id_using_master);
+                                Some(LockedPatchDependency {
+                                    dependency: locked_dep,
+                                    package_id: id_using_master,
+                                    // Note that this is where the magic
+                                    // happens, where the resolve graph
+                                    // probably has locks pointing to
+                                    // DefaultBranch sources, and by including
+                                    // this here those will get transparently
+                                    // rewritten to Branch("master") which we
+                                    // have a lock entry for.
+                                    alt_package_id: Some(id_using_default),
+                                })
+                            }
+
+                            // No locked candidate was found
+                            None => None,
                         }
-                        None => (dep, None),
                     }
-                })
-                .collect::<Vec<_>>();
+                };
+
+                registrations.push((dep, lock));
+            }
+
             let canonical = CanonicalUrl::new(url)?;
-            for (orig_patch, unlock_id) in registry.patch(url, &patches)? {
+            for (orig_patch, unlock_id) in registry.patch(url, &registrations)? {
                 // Avoid the locked patch ID.
                 avoid_patch_ids.insert(unlock_id);
                 // Also avoid the thing it is patching.
@@ -624,17 +689,8 @@ fn register_previous_locks(
         // Note that this is only applicable for loading older resolves now at
         // this point. All new lock files are encoded as v3-or-later, so this is
         // just compat for loading an old lock file successfully.
-        if resolve.version() <= ResolveVersion::V2 {
-            let source = node.source_id();
-            if let Some(GitReference::DefaultBranch) = source.git_reference() {
-                let new_source =
-                    SourceId::for_git(source.url(), GitReference::Branch("master".to_string()))
-                        .unwrap()
-                        .with_precise(source.precise().map(|s| s.to_string()));
-
-                let node = node.with_source_id(new_source);
-                registry.register_lock(node, deps.clone());
-            }
+        if let Some(node) = master_branch_git_source(node, resolve) {
+            registry.register_lock(node, deps.clone());
         }
 
         registry.register_lock(node, deps);
@@ -651,3 +707,17 @@ fn register_previous_locks(
         }
     }
 }
+
+fn master_branch_git_source(id: PackageId, resolve: &Resolve) -> Option<PackageId> {
+    if resolve.version() <= ResolveVersion::V2 {
+        let source = id.source_id();
+        if let Some(GitReference::DefaultBranch) = source.git_reference() {
+            let new_source =
+                SourceId::for_git(source.url(), GitReference::Branch("master".to_string()))
+                    .unwrap()
+                    .with_precise(source.precise().map(|s| s.to_string()));
+            return Some(id.with_source_id(new_source));
+        }
+    }
+    None
+}

tests/testsuite/patch.rs

@@ -2320,3 +2320,80 @@ fn can_update_with_alt_reg() {
         )
         .run();
 }
+
+#[cargo_test]
+fn old_git_patch() {
+    // Example where an old lockfile with an explicit branch="master" in Cargo.toml.
+    Package::new("bar", "1.0.0").publish();
+    let (bar, bar_repo) = git::new_repo("bar", |p| {
+        p.file("Cargo.toml", &basic_manifest("bar", "1.0.0"))
+            .file("src/lib.rs", "")
+    });
+
+    let bar_oid = bar_repo.head().unwrap().target().unwrap();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                    [package]
+                    name = "foo"
+                    version = "0.1.0"
+
+                    [dependencies]
+                    bar = "1.0"
+
+                    [patch.crates-io]
+                    bar = {{ git = "{}", branch = "master" }}
+                "#,
+                bar.url()
+            ),
+        )
+        .file(
+            "Cargo.lock",
+            &format!(
+                r#"
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+[[package]]
+name = "bar"
+version = "1.0.0"
+source = "git+{}#{}"
+
+[[package]]
+name = "foo"
+version = "0.1.0"
+dependencies = [
+ "bar",
+]
+            "#,
+                bar.url(),
+                bar_oid
+            ),
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    bar.change_file("Cargo.toml", &basic_manifest("bar", "2.0.0"));
+    git::add(&bar_repo);
+    git::commit(&bar_repo);
+
+    // This *should* keep the old lock.
+    p.cargo("tree")
+        // .env("CARGO_LOG", "trace")
+        .with_stderr(
+            "\
+[UPDATING] [..]
+",
+        )
+        // .with_status(1)
+        .with_stdout(format!(
+            "\
+foo v0.1.0 [..]
+└── bar v1.0.0 (file:///[..]branch=master#{})
+",
+            &bar_oid.to_string()[..8]
+        ))
+        .run();
+}