Fix loading branch=master patches in the v3 lock transition

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.
@@ -618,17 +683,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);
@@ -645,3 +701,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();
+}