From b78cb373b8780fe1890cce0c7f0b84a9402757ad Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Sat, 16 May 2020 13:39:51 -0700
Subject: [PATCH 1/6] Provide better error messages for a bad `patch`.

---
 src/cargo/core/registry.rs | 153 ++++++++++++++++++++++++++++++-------
 src/cargo/ops/resolve.rs   |  11 +--
 tests/testsuite/patch.rs   | 136 ++++++++++++++++++++++++++++++++-
 3 files changed, 266 insertions(+), 34 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 8a05b4510d6..85317bf2082 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -2,7 +2,7 @@ use std::collections::{HashMap, HashSet};
 
 use anyhow::bail;
 use log::{debug, trace};
-use semver::VersionReq;
+use semver::{Version, VersionReq};
 use url::Url;
 
 use crate::core::PackageSet;
@@ -222,14 +222,22 @@ impl<'cfg> PackageRegistry<'cfg> {
     /// the manifest.
     ///
     /// Here the `deps` will be resolved to a precise version and stored
-    /// internally for future calls to `query` below. It's expected that `deps`
-    /// have had `lock_to` call already, if applicable. (e.g., if a lock file was
-    /// already present).
+    /// internally for future calls to `query` below. `deps` should be a tuple
+    /// where the first element is the patch definition straight from the
+    /// manifest, and the second element is an optional variant where the
+    /// patch has been locked. This locked patch is the patch locked to
+    /// a specific version found in Cargo.lock. This will be `None` if
+    /// `Cargo.lock` doesn't exist, or the patch did not match any existing
+    /// entries in `Cargo.lock`.
     ///
     /// Note that the patch list specified here *will not* be available to
     /// `query` until `lock_patches` is called below, which should be called
     /// once all patches have been added.
-    pub fn patch(&mut self, url: &Url, deps: &[Dependency]) -> CargoResult<()> {
+    pub fn patch(
+        &mut self,
+        url: &Url,
+        deps: &[(&Dependency, Option<Dependency>)],
+    ) -> CargoResult<()> {
         let canonical = CanonicalUrl::new(url)?;
 
         // First up we need to actually resolve each `deps` specification to
@@ -243,7 +251,8 @@ impl<'cfg> PackageRegistry<'cfg> {
         // of summaries which should be the same length as `deps` above.
         let unlocked_summaries = deps
             .iter()
-            .map(|dep| {
+            .map(|(orig_dep, locked_dep)| {
+                let dep = locked_dep.as_ref().unwrap_or(orig_dep);
                 debug!(
                     "registering a patch for `{}` with `{}`",
                     url,
@@ -261,30 +270,13 @@ impl<'cfg> PackageRegistry<'cfg> {
                         )
                     })?;
 
-                let mut summaries = self
+                let source = self
                     .sources
                     .get_mut(dep.source_id())
-                    .expect("loaded source not present")
-                    .query_vec(dep)?
-                    .into_iter();
-
-                let summary = match summaries.next() {
-                    Some(summary) => summary,
-                    None => anyhow::bail!(
-                        "patch for `{}` in `{}` did not resolve to any crates. If this is \
-                         unexpected, you may wish to consult: \
-                         https://github.com/rust-lang/cargo/issues/4678",
-                        dep.package_name(),
-                        url
-                    ),
-                };
-                if summaries.next().is_some() {
-                    anyhow::bail!(
-                        "patch for `{}` in `{}` resolved to more than one candidate",
-                        dep.package_name(),
-                        url
-                    )
-                }
+                    .expect("loaded source not present");
+                let summaries = source.query_vec(dep)?;
+                let summary = summary_for_patch(orig_dep, locked_dep, url, summaries, source)?;
+
                 if *summary.package_id().source_id().canonical_url() == canonical {
                     anyhow::bail!(
                         "patch for `{}` in `{}` points to the same source, but \
@@ -718,3 +710,108 @@ fn lock(
         dep
     })
 }
+
+/// This is a helper for generating a user-friendly error message for a bad patch.
+fn summary_for_patch(
+    orig_patch: &Dependency,
+    locked_patch: &Option<Dependency>,
+    url: &Url,
+    mut summaries: Vec<Summary>,
+    source: &mut dyn Source,
+) -> CargoResult<Summary> {
+    if summaries.len() == 1 {
+        return Ok(summaries.pop().unwrap());
+    }
+    // Helpers to create a comma-separated string of versions.
+    let versions = |versions: &mut [&Version]| -> String {
+        versions.sort();
+        let versions: Vec<_> = versions.into_iter().map(|v| v.to_string()).collect();
+        versions.join(", ")
+    };
+    let summary_versions = |summaries: &[Summary]| -> String {
+        let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
+        versions(&mut vers)
+    };
+    if summaries.len() > 1 {
+        anyhow::bail!(
+            "patch for `{}` in `{}` resolved to more than one candidate\n\
+            Found versions: {}\n\
+            Update the patch definition to select only one package, \
+            or remove the extras from the patch location.",
+            orig_patch.package_name(),
+            url,
+            summary_versions(&summaries)
+        );
+    }
+    // No summaries found, try to help the user figure out what is wrong.
+    let extra = if let Some(locked_patch) = locked_patch {
+        let found = match source.query_vec(orig_patch) {
+            Ok(unlocked_summaries) => format!(" (found {})", summary_versions(&unlocked_summaries)),
+            Err(e) => {
+                log::warn!(
+                    "could not determine unlocked summaries for dep {:?}: {:?}",
+                    orig_patch,
+                    e
+                );
+                "".to_string()
+            }
+        };
+        format!(
+            "The patch is locked to {} in Cargo.lock, \
+            but the version in the patch location does not match{}.\n\
+            Make sure the patch points to the correct version.\n\
+            If it does, run `cargo update -p {}` to update Cargo.lock.",
+            locked_patch.version_req(),
+            found,
+            locked_patch.package_name(),
+        )
+    } else {
+        // 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());
+        let found = match source.query_vec(&name_only_dep) {
+            Ok(name_summaries) => {
+                let mut vers = name_summaries
+                    .iter()
+                    .map(|summary| summary.version())
+                    .collect::<Vec<_>>();
+                match vers.len() {
+                    0 => format!(""),
+                    1 => format!("version `{}`", versions(&mut vers)),
+                    _ => format!("versions `{}`", versions(&mut vers)),
+                }
+            }
+            Err(e) => {
+                log::warn!(
+                    "failed to do name-only summary query for {:?}: {:?}",
+                    name_only_dep,
+                    e
+                );
+                "".to_string()
+            }
+        };
+        if found.is_empty() {
+            format!(
+                "The patch location does not appear to contain any packages \
+                matching the name `{}`.",
+                orig_patch.package_name()
+            )
+        } else {
+            format!(
+                "The patch location contains a `{}` package with {}, but the patch \
+                definition requires `{}`.\n\
+                Check that the version in the patch location is what you expect, \
+                and update the patch definition to match.",
+                orig_patch.package_name(),
+                found,
+                orig_patch.version_req()
+            )
+        }
+    };
+    anyhow::bail!(
+        "patch for `{}` in `{}` did not resolve to any crates.\n{}",
+        orig_patch.package_name(),
+        url,
+        extra
+    );
+}
diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index ec12f67862f..cf6e8053897 100644
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -253,7 +253,8 @@ pub fn resolve_with_previous<'cfg>(
             let previous = match previous {
                 Some(r) => r,
                 None => {
-                    registry.patch(url, patches)?;
+                    let patches: Vec<_> = patches.iter().map(|p| (p, None)).collect();
+                    registry.patch(url, &patches)?;
                     continue;
                 }
             };
@@ -264,11 +265,11 @@ pub fn resolve_with_previous<'cfg>(
                     let candidates = previous.iter().chain(unused);
                     match candidates.filter(keep).find(|&id| dep.matches_id(id)) {
                         Some(id) => {
-                            let mut dep = dep.clone();
-                            dep.lock_to(id);
-                            dep
+                            let mut locked_dep = dep.clone();
+                            locked_dep.lock_to(id);
+                            (dep, Some(locked_dep))
                         }
-                        None => dep.clone(),
+                        None => (dep, None),
                     }
                 })
                 .collect::<Vec<_>>();
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index 83b54dacd31..da833f938a4 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -1498,7 +1498,25 @@ fn update_unused_new_version() {
     // Create a backup so we can test it with different options.
     fs::copy(p.root().join("Cargo.lock"), p.root().join("Cargo.lock.bak")).unwrap();
 
-    // Try with `-p`.
+    // Try to build again.
+    p.cargo("build")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not \
+resolve to any crates.
+The patch is locked to = 0.1.4 in Cargo.lock, but the version in the patch \
+location does not match (found 0.1.6).
+Make sure the patch points to the correct version.
+If it does, run `cargo update -p bar` to update Cargo.lock.
+",
+        )
+        .run();
+
+    // Oh, OK, try `update -p`.
     p.cargo("update -p bar")
         .with_stderr(
             "\
@@ -1521,3 +1539,119 @@ fn update_unused_new_version() {
         )
         .run();
 }
+
+#[cargo_test]
+fn too_many_matches() {
+    // The patch locations has multiple versions that match.
+    Package::new("bar", "0.1.0").publish();
+    Package::new("bar", "0.1.0").alternative(true).publish();
+    Package::new("bar", "0.1.1").alternative(true).publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "0.1"
+
+                [patch.crates-io]
+                bar = { version = "0.1", registry = "alternative" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr("\
+[UPDATING] `[..]alternative-registry` index
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` resolved to more than one candidate
+Found versions: 0.1.0, 0.1.1
+Update the patch definition to select only one package, or remove the extras from the patch location.
+")
+        .run();
+}
+
+#[cargo_test]
+fn no_matches() {
+    // A patch to a location that does not contain the named package.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "0.1"
+
+                [patch.crates-io]
+                bar = { path = "bar" }
+           "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("abc", "0.1.0"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates.
+The patch location does not appear to contain any packages matching the name `bar`.
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn mismatched_version() {
+    // A patch to a location that has an old version.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "0.1.1"
+
+                [patch.crates-io]
+                bar = { path = "bar", version = "0.1.1" }
+           "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates.
+The patch location contains a `bar` package with version `0.1.0`, \
+but the patch definition requires `^0.1.1`.
+Check that the version in the patch location is what you expect, \
+and update the patch definition to match.
+",
+        )
+        .run();
+}

From afa3acedf01842cf305ecaa25de04203250bc8b9 Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Tue, 19 May 2020 18:01:23 -0700
Subject: [PATCH 2/6] Make patches automatically update if updated.

---
 src/cargo/core/registry.rs | 151 +++++++++------
 src/cargo/ops/resolve.rs   |  66 ++++---
 tests/testsuite/patch.rs   | 374 ++++++++++++++++++++++++++++++++++---
 3 files changed, 487 insertions(+), 104 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 85317bf2082..6e4207fb21d 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -2,7 +2,7 @@ use std::collections::{HashMap, HashSet};
 
 use anyhow::bail;
 use log::{debug, trace};
-use semver::{Version, VersionReq};
+use semver::VersionReq;
 use url::Url;
 
 use crate::core::PackageSet;
@@ -233,13 +233,24 @@ impl<'cfg> PackageRegistry<'cfg> {
     /// Note that the patch list specified here *will not* be available to
     /// `query` until `lock_patches` is called below, which should be called
     /// once all patches have been added.
+    ///
+    /// The return value is a `Vec` of patches that should *not* be locked.
+    /// This happens when the patch is locked, but the patch has been updated
+    /// so the locked value is no longer correct.
     pub fn patch(
         &mut self,
         url: &Url,
-        deps: &[(&Dependency, Option<Dependency>)],
-    ) -> CargoResult<()> {
+        deps: &[(&Dependency, Option<(Dependency, PackageId)>)],
+    ) -> 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"
+        // warning, which is very hard to understand. Ideally the warning
+        // would be tailored to indicate *why* it is unused.
         let canonical = CanonicalUrl::new(url)?;
 
+        // Return value of patches that shouldn't be locked.
+        let mut unlock_patches = Vec::new();
+
         // First up we need to actually resolve each `deps` specification to
         // precisely one summary. We're not using the `query` method below as it
         // internally uses maps we're building up as part of this method
@@ -251,8 +262,15 @@ impl<'cfg> PackageRegistry<'cfg> {
         // of summaries which should be the same length as `deps` above.
         let unlocked_summaries = deps
             .iter()
-            .map(|(orig_dep, locked_dep)| {
-                let dep = locked_dep.as_ref().unwrap_or(orig_dep);
+            .map(|(orig_patch, locked)| {
+                // Remove double reference in orig_patch. Is there maybe a
+                // magic pattern that could avoid this?
+                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,
+                    None => orig_patch,
+                };
                 debug!(
                     "registering a patch for `{}` with `{}`",
                     url,
@@ -275,7 +293,21 @@ impl<'cfg> PackageRegistry<'cfg> {
                     .get_mut(dep.source_id())
                     .expect("loaded source not present");
                 let summaries = source.query_vec(dep)?;
-                let summary = summary_for_patch(orig_dep, locked_dep, url, summaries, source)?;
+                let (summary, should_unlock) =
+                    summary_for_patch(orig_patch, &locked, summaries, source).chain_err(|| {
+                        format!(
+                            "patch for `{}` in `{}` did not resolve to any crates",
+                            orig_patch.package_name(),
+                            url,
+                        )
+                    })?;
+                debug!(
+                    "patch summary is {:?} should_unlock={:?}",
+                    summary, should_unlock
+                );
+                if let Some(unlock_id) = should_unlock {
+                    unlock_patches.push((orig_patch.clone(), unlock_id));
+                }
 
                 if *summary.package_id().source_id().canonical_url() == canonical {
                     anyhow::bail!(
@@ -313,7 +345,7 @@ impl<'cfg> PackageRegistry<'cfg> {
         self.patches_available.insert(canonical.clone(), ids);
         self.patches.insert(canonical, unlocked_summaries);
 
-        Ok(())
+        Ok(unlock_patches)
     }
 
     /// Lock all patch summaries added via `patch`, making them available to
@@ -327,6 +359,7 @@ impl<'cfg> PackageRegistry<'cfg> {
         assert!(!self.patches_locked);
         for summaries in self.patches.values_mut() {
             for summary in summaries {
+                debug!("locking patch {:?}", summary);
                 *summary = lock(&self.locked, &self.patches_available, summary.clone());
             }
         }
@@ -711,60 +744,68 @@ fn lock(
     })
 }
 
-/// This is a helper for generating a user-friendly error message for a bad patch.
+/// This is a helper for selecting the summary, or generating a helpful error message.
 fn summary_for_patch(
     orig_patch: &Dependency,
-    locked_patch: &Option<Dependency>,
-    url: &Url,
+    locked: &Option<(Dependency, PackageId)>,
     mut summaries: Vec<Summary>,
     source: &mut dyn Source,
-) -> CargoResult<Summary> {
+) -> CargoResult<(Summary, Option<PackageId>)> {
     if summaries.len() == 1 {
-        return Ok(summaries.pop().unwrap());
+        return Ok((summaries.pop().unwrap(), None));
     }
-    // Helpers to create a comma-separated string of versions.
-    let versions = |versions: &mut [&Version]| -> String {
-        versions.sort();
-        let versions: Vec<_> = versions.into_iter().map(|v| v.to_string()).collect();
-        versions.join(", ")
-    };
-    let summary_versions = |summaries: &[Summary]| -> String {
-        let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
-        versions(&mut vers)
+    let best_summary = |summaries: &mut Vec<Summary>| -> Summary {
+        // TODO: This could maybe honor -Zminimal-versions?
+        summaries.sort_by(|a, b| a.version().cmp(b.version()));
+        summaries.pop().unwrap()
     };
     if summaries.len() > 1 {
-        anyhow::bail!(
-            "patch for `{}` in `{}` resolved to more than one candidate\n\
-            Found versions: {}\n\
-            Update the patch definition to select only one package, \
-            or remove the extras from the patch location.",
-            orig_patch.package_name(),
-            url,
-            summary_versions(&summaries)
-        );
+        let summary = best_summary(&mut summaries);
+        if let Some((_dep, lock_id)) = locked {
+            // I can't think of a scenario where this might happen (locked by
+            // definition should only match at most one summary). Maybe if the
+            // source is broken?
+            return Ok((summary, Some(*lock_id)));
+        } else {
+            return Ok((summary, None));
+        }
     }
+    assert!(summaries.is_empty());
     // No summaries found, try to help the user figure out what is wrong.
-    let extra = if let Some(locked_patch) = locked_patch {
-        let found = match source.query_vec(orig_patch) {
-            Ok(unlocked_summaries) => format!(" (found {})", summary_versions(&unlocked_summaries)),
+    if let Some((locked_patch, locked_id)) = locked {
+        // Since the locked patch did not match anything, try the unlocked one.
+        let mut orig_matches = match source.query_vec(orig_patch) {
+            Ok(summaries) => summaries,
             Err(e) => {
                 log::warn!(
                     "could not determine unlocked summaries for dep {:?}: {:?}",
                     orig_patch,
                     e
                 );
-                "".to_string()
+                Vec::new()
             }
         };
-        format!(
-            "The patch is locked to {} in Cargo.lock, \
-            but the version in the patch location does not match{}.\n\
-            Make sure the patch points to the correct version.\n\
-            If it does, run `cargo update -p {}` to update Cargo.lock.",
-            locked_patch.version_req(),
-            found,
-            locked_patch.package_name(),
-        )
+        if orig_matches.is_empty() {
+            // This should be relatively unusual. For example, a patch of
+            // {version="0.1.2", ...} and the patch location no longer contains a
+            // version that matches "0.1.2". It is unusual to explicitly write a
+            // version in the patch.
+            anyhow::bail!(
+                "The patch is locked to {} in Cargo.lock, but the version in the \
+                patch location does not match any packages in the patch location.\n\
+                Make sure the patch points to the correct version.",
+                locked_patch.version_req(),
+            );
+        }
+        let summary = best_summary(&mut orig_matches);
+        debug!(
+            "locked patch no longer matches, but unlocked version should work. \
+            locked={:?} unlocked={:?} summary={:?}",
+            locked, orig_patch, summary
+        );
+        // The unlocked version found a match. This returns a value to
+        // indicate that this entry should be unlocked.
+        return Ok((summary, Some(*locked_id)));
     } else {
         // Try checking if there are *any* packages that match this by name.
         let name_only_dep =
@@ -777,8 +818,12 @@ fn summary_for_patch(
                     .collect::<Vec<_>>();
                 match vers.len() {
                     0 => format!(""),
-                    1 => format!("version `{}`", versions(&mut vers)),
-                    _ => format!("versions `{}`", versions(&mut vers)),
+                    1 => format!("version `{}`", vers[0]),
+                    _ => {
+                        vers.sort();
+                        let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+                        format!("versions `{}`", strs.join(", "))
+                    }
                 }
             }
             Err(e) => {
@@ -791,13 +836,13 @@ fn summary_for_patch(
             }
         };
         if found.is_empty() {
-            format!(
+            anyhow::bail!(
                 "The patch location does not appear to contain any packages \
                 matching the name `{}`.",
                 orig_patch.package_name()
-            )
+            );
         } else {
-            format!(
+            anyhow::bail!(
                 "The patch location contains a `{}` package with {}, but the patch \
                 definition requires `{}`.\n\
                 Check that the version in the patch location is what you expect, \
@@ -805,13 +850,7 @@ fn summary_for_patch(
                 orig_patch.package_name(),
                 found,
                 orig_patch.version_req()
-            )
+            );
         }
-    };
-    anyhow::bail!(
-        "patch for `{}` in `{}` did not resolve to any crates.\n{}",
-        orig_patch.package_name(),
-        url,
-        extra
-    );
+    }
 }
diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index cf6e8053897..221edcd54c4 100644
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -20,7 +20,7 @@ use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Worksp
 use crate::ops;
 use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
-use crate::util::profile;
+use crate::util::{profile, CanonicalUrl};
 use log::{debug, trace};
 use std::collections::HashSet;
 
@@ -224,7 +224,7 @@ pub fn resolve_with_previous<'cfg>(
         );
     }
 
-    let keep = |p: &PackageId| {
+    let pre_patch_keep = |p: &PackageId| {
         !to_avoid_sources.contains(&p.source_id())
             && match to_avoid {
                 Some(set) => !set.contains(p),
@@ -232,29 +232,18 @@ pub fn resolve_with_previous<'cfg>(
             }
     };
 
-    // In the case where a previous instance of resolve is available, we
-    // want to lock as many packages as possible to the previous version
-    // without disturbing the graph structure.
-    let mut try_to_use = HashSet::new();
-    if let Some(r) = previous {
-        trace!("previous: {:?}", r);
-        register_previous_locks(ws, registry, r, &keep);
-
-        // Everything in the previous lock file we want to keep is prioritized
-        // in dependency selection if it comes up, aka we want to have
-        // conservative updates.
-        try_to_use.extend(r.iter().filter(keep).inspect(|id| {
-            debug!("attempting to prefer {}", id);
-        }));
-    }
-
+    // This is a set of PackageIds of `[patch]` entries that should not be
+    // locked.
+    let mut avoid_patch_ids = HashSet::new();
     if register_patches {
         for (url, patches) in ws.root_patch() {
             let previous = match previous {
                 Some(r) => r,
                 None => {
                     let patches: Vec<_> = patches.iter().map(|p| (p, None)).collect();
-                    registry.patch(url, &patches)?;
+                    let unlock_ids = registry.patch(url, &patches)?;
+                    // Since nothing is locked, this shouldn't possibly return anything.
+                    assert!(unlock_ids.is_empty());
                     continue;
                 }
             };
@@ -263,19 +252,52 @@ pub fn resolve_with_previous<'cfg>(
                 .map(|dep| {
                     let unused = previous.unused_patches().iter().cloned();
                     let candidates = previous.iter().chain(unused);
-                    match candidates.filter(keep).find(|&id| dep.matches_id(id)) {
+                    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))
+                            (dep, Some((locked_dep, id)))
                         }
                         None => (dep, None),
                     }
                 })
                 .collect::<Vec<_>>();
-            registry.patch(url, &patches)?;
+            let canonical = CanonicalUrl::new(url)?;
+            for (orig_patch, unlock_id) in registry.patch(url, &patches)? {
+                // Avoid the locked patch ID.
+                avoid_patch_ids.insert(unlock_id);
+                // Also avoid the thing it is patching.
+                avoid_patch_ids.extend(previous.iter().filter(|id| {
+                    orig_patch.matches_ignoring_source(*id)
+                        && *id.source_id().canonical_url() == canonical
+                }));
+            }
         }
+    }
+    debug!("avoid_patch_ids={:?}", avoid_patch_ids);
 
+    let keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
+
+    // In the case where a previous instance of resolve is available, we
+    // want to lock as many packages as possible to the previous version
+    // without disturbing the graph structure.
+    let mut try_to_use = HashSet::new();
+    if let Some(r) = previous {
+        trace!("previous: {:?}", r);
+        register_previous_locks(ws, registry, r, &keep);
+
+        // Everything in the previous lock file we want to keep is prioritized
+        // in dependency selection if it comes up, aka we want to have
+        // conservative updates.
+        try_to_use.extend(r.iter().filter(keep).inspect(|id| {
+            debug!("attempting to prefer {}", id);
+        }));
+    }
+
+    if register_patches {
         registry.lock_patches();
     }
 
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index da833f938a4..ee83a8ccb6c 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -1498,25 +1498,25 @@ fn update_unused_new_version() {
     // Create a backup so we can test it with different options.
     fs::copy(p.root().join("Cargo.lock"), p.root().join("Cargo.lock.bak")).unwrap();
 
-    // Try to build again.
+    // Try to build again, this should automatically update Cargo.lock.
     p.cargo("build")
-        .with_status(101)
         .with_stderr(
             "\
-[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
-
-Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not \
-resolve to any crates.
-The patch is locked to = 0.1.4 in Cargo.lock, but the version in the patch \
-location does not match (found 0.1.6).
-Make sure the patch points to the correct version.
-If it does, run `cargo update -p bar` to update Cargo.lock.
+[UPDATING] `[..]/registry` index
+[COMPILING] bar v0.1.6 ([..]/bar)
+[COMPILING] foo v0.0.1 ([..]/foo)
+[FINISHED] [..]
 ",
         )
         .run();
+    // This should not update any registry.
+    p.cargo("build").with_stderr("[FINISHED] [..]").run();
+    assert!(!p.read_lockfile().contains("unused"));
 
-    // Oh, OK, try `update -p`.
+    // Restore the lock file, and see if `update` will work, too.
+    fs::copy(p.root().join("Cargo.lock.bak"), p.root().join("Cargo.lock")).unwrap();
+
+    // Try `update -p`.
     p.cargo("update -p bar")
         .with_stderr(
             "\
@@ -1565,17 +1565,19 @@ fn too_many_matches() {
         .file("src/lib.rs", "")
         .build();
 
+    // Picks 0.1.1, the most recent version.
     p.cargo("check")
-        .with_status(101)
-        .with_stderr("\
-[UPDATING] `[..]alternative-registry` index
-[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
-
-Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` resolved to more than one candidate
-Found versions: 0.1.0, 0.1.1
-Update the patch definition to select only one package, or remove the extras from the patch location.
-")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/alternative-registry` index
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.1.1 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.1 (registry `[..]/alternative-registry`)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
         .run();
 }
 
@@ -1609,8 +1611,10 @@ fn no_matches() {
 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates.
-The patch location does not appear to contain any packages matching the name `bar`.
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+
+Caused by:
+  The patch location does not appear to contain any packages matching the name `bar`.
 ",
         )
         .run();
@@ -1646,8 +1650,10 @@ fn mismatched_version() {
 [ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates.
-The patch location contains a `bar` package with version `0.1.0`, \
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+
+Caused by:
+  The patch location contains a `bar` package with version `0.1.0`, \
 but the patch definition requires `^0.1.1`.
 Check that the version in the patch location is what you expect, \
 and update the patch definition to match.
@@ -1655,3 +1661,319 @@ and update the patch definition to match.
         )
         .run();
 }
+
+#[cargo_test]
+fn patch_walks_backwards() {
+    // Starting with a locked patch, change the patch so it points to an older version.
+    Package::new("bar", "0.1.0").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+
+            [patch.crates-io]
+            bar = {path="bar"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[CHECKING] bar v0.1.1 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    // Somehow the user changes the version backwards.
+    p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"));
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[CHECKING] bar v0.1.0 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn patch_walks_backwards_restricted() {
+    // This is the same as `patch_walks_backwards`, but the patch contains a
+    // `version` qualifier. This is unusual, just checking a strange edge case.
+    Package::new("bar", "0.1.0").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+
+            [patch.crates-io]
+            bar = {path="bar", version="0.1.1"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[CHECKING] bar v0.1.1 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    // Somehow the user changes the version backwards.
+    p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"));
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+
+Caused by:
+  The patch is locked to = 0.1.1 in Cargo.lock, but the version in the patch location does not match any packages in the patch location.
+Make sure the patch points to the correct version.
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn patched_dep_new_version() {
+    // What happens when a patch is locked, and then one of the patched
+    // dependencies needs to be updated. In this case, the baz requirement
+    // gets updated from 0.1.0 to 0.1.1.
+    Package::new("bar", "0.1.0").dep("baz", "0.1.0").publish();
+    Package::new("baz", "0.1.0").publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+
+            [patch.crates-io]
+            bar = {path="bar"}
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+            [package]
+            name = "bar"
+            version = "0.1.0"
+
+            [dependencies]
+            baz = "0.1"
+            "#,
+        )
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    // Lock everything.
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] baz v0.1.0 [..]
+[CHECKING] baz v0.1.0
+[CHECKING] bar v0.1.0 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    Package::new("baz", "0.1.1").publish();
+
+    // Just the presence of the new version should not have changed anything.
+    p.cargo("check").with_stderr("[FINISHED] [..]").run();
+
+    // Modify the patch so it requires the new version.
+    p.change_file(
+        "bar/Cargo.toml",
+        r#"
+            [package]
+            name = "bar"
+            version = "0.1.0"
+
+            [dependencies]
+            baz = "0.1.1"
+        "#,
+    );
+
+    // Should unlock and update cleanly.
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] baz v0.1.1 (registry `[..]/registry`)
+[CHECKING] baz v0.1.1
+[CHECKING] bar v0.1.0 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn patch_update_doesnt_update_other_sources() {
+    // Very extreme edge case, make sure a patch update doesn't update other
+    // sources.
+    Package::new("bar", "0.1.0").publish();
+    Package::new("bar", "0.1.0").alternative(true).publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+            bar_alt = { version = "0.1", registry = "alternative", package = "bar"  }
+
+            [patch.crates-io]
+            bar = { path = "bar" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_stderr_unordered(
+            "\
+[UPDATING] `[..]/registry` index
+[UPDATING] `[..]/alternative-registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.1.0 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.0 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.0 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    // Publish new versions in both sources.
+    Package::new("bar", "0.1.1").publish();
+    Package::new("bar", "0.1.1").alternative(true).publish();
+
+    // Since it is locked, nothing should change.
+    p.cargo("check").with_stderr("[FINISHED] [..]").run();
+
+    // Require new version on crates.io.
+    p.change_file("bar/Cargo.toml", &basic_manifest("bar", "0.1.1"));
+
+    // This should not update bar_alt.
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/registry` index
+[CHECKING] bar v0.1.1 ([..]/foo/bar)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn can_update_with_alt_reg() {
+    // A patch to an alt reg can update.
+    Package::new("bar", "0.1.0").publish();
+    Package::new("bar", "0.1.0").alternative(true).publish();
+    Package::new("bar", "0.1.1").alternative(true).publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar = "0.1"
+
+                [patch.crates-io]
+                bar = { version = "0.1.1", registry = "alternative" }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/alternative-registry` index
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.1.1 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.1 (registry `[..]/alternative-registry`)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    Package::new("bar", "0.1.2").alternative(true).publish();
+
+    // Should remain locked.
+    p.cargo("check").with_stderr("[FINISHED] [..]").run();
+
+    p.cargo("update -p bar")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/alternative-registry` index
+[UPDATING] `[..]/registry` index
+[UPDATING] bar v0.1.1 (registry `[..]/alternative-registry`) -> v0.1.2
+",
+        )
+        .run();
+}

From 2c0cd9741d8f4e9363ebb63164700865bccc95f2 Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Wed, 20 May 2020 08:33:52 -0700
Subject: [PATCH 3/6] Simplify error handling.

---
 src/cargo/core/registry.rs | 61 +++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 6e4207fb21d..fbb8525113e 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -774,17 +774,14 @@ fn summary_for_patch(
     // No summaries found, try to help the user figure out what is wrong.
     if let Some((locked_patch, locked_id)) = locked {
         // Since the locked patch did not match anything, try the unlocked one.
-        let mut orig_matches = match source.query_vec(orig_patch) {
-            Ok(summaries) => summaries,
-            Err(e) => {
-                log::warn!(
-                    "could not determine unlocked summaries for dep {:?}: {:?}",
-                    orig_patch,
-                    e
-                );
-                Vec::new()
-            }
-        };
+        let mut orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
+            log::warn!(
+                "could not determine unlocked summaries for dep {:?}: {:?}",
+                orig_patch,
+                e
+            );
+            Vec::new()
+        });
         if orig_matches.is_empty() {
             // This should be relatively unusual. For example, a patch of
             // {version="0.1.2", ...} and the patch location no longer contains a
@@ -810,29 +807,25 @@ fn summary_for_patch(
         // 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());
-        let found = match source.query_vec(&name_only_dep) {
-            Ok(name_summaries) => {
-                let mut vers = name_summaries
-                    .iter()
-                    .map(|summary| summary.version())
-                    .collect::<Vec<_>>();
-                match vers.len() {
-                    0 => format!(""),
-                    1 => format!("version `{}`", vers[0]),
-                    _ => {
-                        vers.sort();
-                        let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
-                        format!("versions `{}`", strs.join(", "))
-                    }
-                }
-            }
-            Err(e) => {
-                log::warn!(
-                    "failed to do name-only summary query for {:?}: {:?}",
-                    name_only_dep,
-                    e
-                );
-                "".to_string()
+        let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {
+            log::warn!(
+                "failed to do name-only summary query for {:?}: {:?}",
+                name_only_dep,
+                e
+            );
+            Vec::new()
+        });
+        let mut vers = name_summaries
+            .iter()
+            .map(|summary| summary.version())
+            .collect::<Vec<_>>();
+        let found = match vers.len() {
+            0 => format!(""),
+            1 => format!("version `{}`", vers[0]),
+            _ => {
+                vers.sort();
+                let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+                format!("versions `{}`", strs.join(", "))
             }
         };
         if found.is_empty() {

From 6fd11a77682f056110c7a2b49a4d115d4ca4bf7e Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Wed, 20 May 2020 08:44:26 -0700
Subject: [PATCH 4/6] Don't include a special-case error for a locked patch
 matching 0 entries.

---
 src/cargo/core/registry.rs | 103 +++++++++++++++----------------------
 tests/testsuite/patch.rs   |   4 +-
 2 files changed, 44 insertions(+), 63 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index fbb8525113e..19c97d18551 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -772,9 +772,9 @@ 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_patch, locked_id)) = locked {
         // Since the locked patch did not match anything, try the unlocked one.
-        let mut orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
+        let orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
             log::warn!(
                 "could not determine unlocked summaries for dep {:?}: {:?}",
                 orig_patch,
@@ -782,68 +782,49 @@ fn summary_for_patch(
             );
             Vec::new()
         });
-        if orig_matches.is_empty() {
-            // This should be relatively unusual. For example, a patch of
-            // {version="0.1.2", ...} and the patch location no longer contains a
-            // version that matches "0.1.2". It is unusual to explicitly write a
-            // version in the patch.
-            anyhow::bail!(
-                "The patch is locked to {} in Cargo.lock, but the version in the \
-                patch location does not match any packages in the patch location.\n\
-                Make sure the patch points to the correct version.",
-                locked_patch.version_req(),
-            );
-        }
-        let summary = best_summary(&mut orig_matches);
-        debug!(
-            "locked patch no longer matches, but unlocked version should work. \
-            locked={:?} unlocked={:?} summary={:?}",
-            locked, orig_patch, summary
-        );
+        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)));
-    } else {
-        // 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());
-        let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {
-            log::warn!(
-                "failed to do name-only summary query for {:?}: {:?}",
-                name_only_dep,
-                e
-            );
-            Vec::new()
-        });
-        let mut vers = name_summaries
-            .iter()
-            .map(|summary| summary.version())
-            .collect::<Vec<_>>();
-        let found = match vers.len() {
-            0 => format!(""),
-            1 => format!("version `{}`", vers[0]),
-            _ => {
-                vers.sort();
-                let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
-                format!("versions `{}`", strs.join(", "))
-            }
-        };
-        if found.is_empty() {
-            anyhow::bail!(
-                "The patch location does not appear to contain any packages \
-                matching the name `{}`.",
-                orig_patch.package_name()
-            );
-        } else {
-            anyhow::bail!(
-                "The patch location contains a `{}` package with {}, but the patch \
-                definition requires `{}`.\n\
-                Check that the version in the patch location is what you expect, \
-                and update the patch definition to match.",
-                orig_patch.package_name(),
-                found,
-                orig_patch.version_req()
-            );
+    }
+    // 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());
+    let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {
+        log::warn!(
+            "failed to do name-only summary query for {:?}: {:?}",
+            name_only_dep,
+            e
+        );
+        Vec::new()
+    });
+    let mut vers = name_summaries
+        .iter()
+        .map(|summary| summary.version())
+        .collect::<Vec<_>>();
+    let found = match vers.len() {
+        0 => format!(""),
+        1 => format!("version `{}`", vers[0]),
+        _ => {
+            vers.sort();
+            let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+            format!("versions `{}`", strs.join(", "))
         }
+    };
+    if found.is_empty() {
+        anyhow::bail!(
+            "The patch location does not appear to contain any packages \
+            matching the name `{}`.",
+            orig_patch.package_name()
+        );
+    } else {
+        anyhow::bail!(
+            "The patch location contains a `{}` package with {}, but the patch \
+            definition requires `{}`.\n\
+            Check that the version in the patch location is what you expect, \
+            and update the patch definition to match.",
+            orig_patch.package_name(),
+            found,
+            orig_patch.version_req()
+        );
     }
 }
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index ee83a8ccb6c..45011f2295f 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -1763,8 +1763,8 @@ Caused by:
   patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
 
 Caused by:
-  The patch is locked to = 0.1.1 in Cargo.lock, but the version in the patch location does not match any packages in the patch location.
-Make sure the patch points to the correct version.
+  The patch location contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
+Check that the version in the patch location is what you expect, and update the patch definition to match.
 ",
         )
         .run();

From 5dde9cc911bd00e3e6390a2cd23945465f383acd Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Wed, 20 May 2020 09:25:53 -0700
Subject: [PATCH 5/6] Show patch location in error message.

---
 src/cargo/core/registry.rs | 6 ++++--
 tests/testsuite/patch.rs   | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index 19c97d18551..b1259a56806 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -812,16 +812,18 @@ fn summary_for_patch(
     };
     if found.is_empty() {
         anyhow::bail!(
-            "The patch location does not appear to contain any packages \
+            "The patch location `{}` does not appear to contain any packages \
             matching the name `{}`.",
+            orig_patch.source_id(),
             orig_patch.package_name()
         );
     } else {
         anyhow::bail!(
-            "The patch location contains a `{}` package with {}, but the patch \
+            "The patch location `{}` contains a `{}` package with {}, but the patch \
             definition requires `{}`.\n\
             Check that the version in the patch location is what you expect, \
             and update the patch definition to match.",
+            orig_patch.source_id(),
             orig_patch.package_name(),
             found,
             orig_patch.version_req()
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index 45011f2295f..2ed61b3d40e 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -1614,7 +1614,7 @@ Caused by:
   patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
 
 Caused by:
-  The patch location does not appear to contain any packages matching the name `bar`.
+  The patch location `[..]/foo/bar` does not appear to contain any packages matching the name `bar`.
 ",
         )
         .run();
@@ -1653,7 +1653,7 @@ Caused by:
   patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
 
 Caused by:
-  The patch location contains a `bar` package with version `0.1.0`, \
+  The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, \
 but the patch definition requires `^0.1.1`.
 Check that the version in the patch location is what you expect, \
 and update the patch definition to match.
@@ -1763,7 +1763,7 @@ Caused by:
   patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
 
 Caused by:
-  The patch location contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
+  The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
 Check that the version in the patch location is what you expect, and update the patch definition to match.
 ",
         )

From 4f2bae93770048c3310a41021dc159f40f2bc80f Mon Sep 17 00:00:00 2001
From: Eric Huss <eric@huss.org>
Date: Wed, 20 May 2020 10:13:17 -0700
Subject: [PATCH 6/6] Revert change to automatically select the greatest patch
 match.

---
 src/cargo/core/registry.rs | 37 +++++++++++++++----------
 tests/testsuite/patch.rs   | 57 ++++++++++++++++++++++++++++++--------
 2 files changed, 68 insertions(+), 26 deletions(-)

diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
index b1259a56806..2dccec54c4d 100644
--- a/src/cargo/core/registry.rs
+++ b/src/cargo/core/registry.rs
@@ -296,7 +296,7 @@ impl<'cfg> PackageRegistry<'cfg> {
                 let (summary, should_unlock) =
                     summary_for_patch(orig_patch, &locked, summaries, source).chain_err(|| {
                         format!(
-                            "patch for `{}` in `{}` did not resolve to any crates",
+                            "patch for `{}` in `{}` failed to resolve",
                             orig_patch.package_name(),
                             url,
                         )
@@ -754,21 +754,28 @@ fn summary_for_patch(
     if summaries.len() == 1 {
         return Ok((summaries.pop().unwrap(), None));
     }
-    let best_summary = |summaries: &mut Vec<Summary>| -> Summary {
-        // TODO: This could maybe honor -Zminimal-versions?
-        summaries.sort_by(|a, b| a.version().cmp(b.version()));
-        summaries.pop().unwrap()
-    };
     if summaries.len() > 1 {
-        let summary = best_summary(&mut summaries);
-        if let Some((_dep, lock_id)) = locked {
-            // I can't think of a scenario where this might happen (locked by
-            // definition should only match at most one summary). Maybe if the
-            // source is broken?
-            return Ok((summary, Some(*lock_id)));
-        } else {
-            return Ok((summary, None));
-        }
+        // TODO: In the future, it might be nice to add all of these
+        // candidates so that version selection would just pick the
+        // appropriate one. However, as this is currently structured, if we
+        // added these all as patches, the unselected versions would end up in
+        // the "unused patch" listing, and trigger a warning. It might take a
+        // fair bit of restructuring to make that work cleanly, and there
+        // isn't any demand at this time to support that.
+        let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
+        vers.sort();
+        let versions: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+        anyhow::bail!(
+            "patch for `{}` in `{}` resolved to more than one candidate\n\
+            Found versions: {}\n\
+            Update the patch definition to select only one package.\n\
+            For example, add an `=` version requirement to the patch definition, \
+            such as `version = \"={}\"`.",
+            orig_patch.package_name(),
+            orig_patch.source_id(),
+            versions.join(", "),
+            versions.last().unwrap()
+        );
     }
     assert!(summaries.is_empty());
     // No summaries found, try to help the user figure out what is wrong.
diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
index 2ed61b3d40e..d80bd9b6da5 100644
--- a/tests/testsuite/patch.rs
+++ b/tests/testsuite/patch.rs
@@ -1567,15 +1567,20 @@ fn too_many_matches() {
 
     // Picks 0.1.1, the most recent version.
     p.cargo("check")
+        .with_status(101)
         .with_stderr(
             "\
 [UPDATING] `[..]/alternative-registry` index
-[UPDATING] `[..]/registry` index
-[DOWNLOADING] crates ...
-[DOWNLOADED] bar v0.1.1 (registry `[..]/alternative-registry`)
-[CHECKING] bar v0.1.1 (registry `[..]/alternative-registry`)
-[CHECKING] foo v0.1.0 ([..]/foo)
-[FINISHED] [..]
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
+
+Caused by:
+  patch for `bar` in `registry `[..]/alternative-registry`` resolved to more than one candidate
+Found versions: 0.1.0, 0.1.1
+Update the patch definition to select only one package.
+For example, add an `=` version requirement to the patch definition, such as `version = \"=0.1.1\"`.
 ",
         )
         .run();
@@ -1611,7 +1616,7 @@ fn no_matches() {
 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` does not appear to contain any packages matching the name `bar`.
@@ -1650,7 +1655,7 @@ fn mismatched_version() {
 [ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, \
@@ -1760,7 +1765,7 @@ fn patch_walks_backwards_restricted() {
 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
@@ -1942,7 +1947,7 @@ fn can_update_with_alt_reg() {
                 bar = "0.1"
 
                 [patch.crates-io]
-                bar = { version = "0.1.1", registry = "alternative" }
+                bar = { version = "=0.1.1", registry = "alternative" }
             "#,
         )
         .file("src/lib.rs", "")
@@ -1967,12 +1972,42 @@ fn can_update_with_alt_reg() {
     // Should remain locked.
     p.cargo("check").with_stderr("[FINISHED] [..]").run();
 
+    // This does nothing, due to `=` requirement.
     p.cargo("update -p bar")
         .with_stderr(
             "\
 [UPDATING] `[..]/alternative-registry` index
 [UPDATING] `[..]/registry` index
-[UPDATING] bar v0.1.1 (registry `[..]/alternative-registry`) -> v0.1.2
+",
+        )
+        .run();
+
+    // Bump to 0.1.2.
+    p.change_file(
+        "Cargo.toml",
+        r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+
+            [patch.crates-io]
+            bar = { version = "=0.1.2", registry = "alternative" }
+        "#,
+    );
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/alternative-registry` index
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.1.2 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.2 (registry `[..]/alternative-registry`)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
 ",
         )
         .run();