Warn if master unifies with DefaultBranch

This commit implements a simple warning to indicate when `DefaultBranch`
is unified with `Branch("master")`, meaning `Cargo.toml` inconsistently
lists `branch = "master"` and not. The intention here is to ensure that
all projects uniformly use one or the other to ensure we can smoothly
transition to the new lock file format.

Author: alexcrichton

src/cargo/core/dependency.rs

@@ -386,8 +386,16 @@ impl Dependency {
             self.source_id(),
             id
         );
-        self.set_version_req(VersionReq::exact(id.version()))
-            .set_source_id(id.source_id())
+        let me = Rc::make_mut(&mut self.inner);
+        me.req = VersionReq::exact(id.version());
+
+        // Only update the `precise` of this source to preserve other
+        // information about dependency's source which may not otherwise be
+        // tested during equality/hashing.
+        me.source_id = me
+            .source_id
+            .with_precise(id.source_id().precise().map(|s| s.to_string()));
+        self
     }
 
     /// Returns `true` if this is a "locked" dependency, basically whether it has

src/cargo/core/registry.rs

@@ -1,16 +1,15 @@
 use std::collections::{HashMap, HashSet};
 
-use anyhow::bail;
-use log::{debug, trace};
-use semver::VersionReq;
-use url::Url;
-
 use crate::core::PackageSet;
 use crate::core::{Dependency, PackageId, Source, SourceId, SourceMap, Summary};
 use crate::sources::config::SourceConfigMap;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::interning::InternedString;
 use crate::util::{profile, CanonicalUrl, Config};
+use anyhow::bail;
+use log::{debug, trace};
+use semver::VersionReq;
+use url::Url;
 
 /// Source of information about a group of packages.
 ///
@@ -135,22 +134,22 @@ impl<'cfg> PackageRegistry<'cfg> {
             // We've previously loaded this source, and we've already locked it,
             // so we're not allowed to change it even if `namespace` has a
             // slightly different precise version listed.
-            Some(&(_, Kind::Locked)) => {
+            Some((_, Kind::Locked)) => {
                 debug!("load/locked   {}", namespace);
                 return Ok(());
             }
 
             // If the previous source was not a precise source, then we can be
             // sure that it's already been updated if we've already loaded it.
-            Some(&(ref previous, _)) if previous.precise().is_none() => {
+            Some((previous, _)) if previous.precise().is_none() => {
                 debug!("load/precise  {}", namespace);
                 return Ok(());
             }
 
             // If the previous source has the same precise version as we do,
             // then we're done, otherwise we need to need to move forward
             // updating this source.
-            Some(&(ref previous, _)) => {
+            Some((previous, _)) => {
                 if previous.precise() == namespace.precise() {
                     debug!("load/match    {}", namespace);
                     return Ok(());

src/cargo/core/resolver/context.rs

@@ -1,16 +1,13 @@
-use std::collections::HashMap;
-use std::num::NonZeroU64;
-
-use anyhow::format_err;
-use log::debug;
-
-use crate::core::{Dependency, PackageId, SourceId, Summary};
-use crate::util::interning::InternedString;
-use crate::util::Graph;
-
 use super::dep_cache::RegistryQueryer;
 use super::errors::ActivateResult;
 use super::types::{ConflictMap, ConflictReason, FeaturesSet, ResolveOpts};
+use crate::core::{Dependency, PackageId, SourceId, Summary};
+use crate::util::interning::InternedString;
+use crate::util::Graph;
+use anyhow::format_err;
+use log::debug;
+use std::collections::HashMap;
+use std::num::NonZeroU64;
 
 pub use super::encode::Metadata;
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};

src/cargo/core/resolver/dep_cache.rs

@@ -9,20 +9,19 @@
 //!
 //! This module impl that cache in all the gory details
 
-use std::cmp::Ordering;
-use std::collections::{BTreeSet, HashMap, HashSet};
-use std::rc::Rc;
-
-use log::debug;
-
 use crate::core::resolver::context::Context;
 use crate::core::resolver::errors::describe_path;
+use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
+use crate::core::resolver::{ActivateResult, ResolveOpts};
 use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
+use crate::core::{GitReference, SourceId};
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::interning::InternedString;
-
-use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
-use crate::core::resolver::{ActivateResult, ResolveOpts};
+use crate::util::Config;
+use log::debug;
+use std::cmp::Ordering;
+use std::collections::{BTreeSet, HashMap, HashSet};
+use std::rc::Rc;
 
 pub struct RegistryQueryer<'a> {
     pub registry: &'a mut (dyn Registry + 'a),
@@ -41,6 +40,10 @@ pub struct RegistryQueryer<'a> {
     >,
     /// all the cases we ended up using a supplied replacement
     used_replacements: HashMap<PackageId, Summary>,
+    /// Where to print warnings, if configured.
+    config: Option<&'a Config>,
+    /// Sources that we've already wared about possibly colliding in the future.
+    warned_git_collisions: HashSet<SourceId>,
 }
 
 impl<'a> RegistryQueryer<'a> {
@@ -49,6 +52,7 @@ impl<'a> RegistryQueryer<'a> {
         replacements: &'a [(PackageIdSpec, Dependency)],
         try_to_use: &'a HashSet<PackageId>,
         minimal_versions: bool,
+        config: Option<&'a Config>,
     ) -> Self {
         RegistryQueryer {
             registry,
@@ -58,6 +62,8 @@ impl<'a> RegistryQueryer<'a> {
             registry_cache: HashMap::new(),
             summary_cache: HashMap::new(),
             used_replacements: HashMap::new(),
+            config,
+            warned_git_collisions: HashSet::new(),
         }
     }
 
@@ -69,13 +75,52 @@ impl<'a> RegistryQueryer<'a> {
         self.used_replacements.get(&p)
     }
 
+    /// Issues a future-compatible warning targeted at removing reliance on
+    /// unifying behavior between these two dependency directives:
+    ///
+    /// ```toml
+    /// [dependencies]
+    /// a = { git = 'https://example.org/foo' }
+    /// a = { git = 'https://example.org/foo', branch = 'master }
+    /// ```
+    ///
+    /// Historical versions of Cargo considered these equivalent but going
+    /// forward we'd like to fix this. For more details see the comments in
+    /// src/cargo/sources/git/utils.rs
+    fn warn_colliding_git_sources(&mut self, id: SourceId) -> CargoResult<()> {
+        let config = match self.config {
+            Some(config) => config,
+            None => return Ok(()),
+        };
+        let prev = match self.warned_git_collisions.replace(id) {
+            Some(prev) => prev,
+            None => return Ok(()),
+        };
+        match (id.git_reference(), prev.git_reference()) {
+            (Some(GitReference::DefaultBranch), Some(GitReference::Branch(b)))
+            | (Some(GitReference::Branch(b)), Some(GitReference::DefaultBranch))
+                if b == "master" => {}
+            _ => return Ok(()),
+        }
+
+        config.shell().warn(&format!(
+            "two git dependencies found for `{}` \
+             where one uses `branch = \"master\"` and the other doesn't; \
+             this will break in a future version of Cargo, so please \
+             ensure the dependency forms are consistent",
+            id.url(),
+        ))?;
+        Ok(())
+    }
+
     /// Queries the `registry` to return a list of candidates for `dep`.
     ///
     /// This method is the location where overrides are taken into account. If
     /// any candidates are returned which match an override then the override is
     /// applied by performing a second query for what the override should
     /// return.
     pub fn query(&mut self, dep: &Dependency) -> CargoResult<Rc<Vec<Summary>>> {
+        self.warn_colliding_git_sources(dep.source_id())?;
         if let Some(out) = self.registry_cache.get(dep).cloned() {
             return Ok(out);
         }

src/cargo/core/resolver/mod.rs

@@ -133,7 +133,8 @@ pub fn resolve(
         Some(config) => config.cli_unstable().minimal_versions,
         None => false,
     };
-    let mut registry = RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions);
+    let mut registry =
+        RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions, config);
     let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
 
     let mut cksums = HashMap::new();