Auto merge of #9133 - alexcrichton:git-default-branch, r=ehuss

Change git dependencies to use `HEAD` by default

This commit follows through with work started in #8522 to change the
default behavior of `git` dependencies where if not branch/tag/etc is
listed then `HEAD` is used instead of the `master` branch. This involves
also changing the default lock file format, now including a `version`
marker at the top of the file notably as well as changing the encoding
of `branch=master` directives in `Cargo.toml`.

If we did all our work correctly then this will be a seamless change.
First released on stable in 1.47.0 (2020-10-08) Cargo has been emitting
warnings about situations which may break in the future. This means that
if you don't specify `branch = 'master'` but your HEAD branch isn't
`master`, you've been getting a warning. Similarly if your dependency
graph used both `branch = 'master'` as well as specifying nothing, you
were receiving warnings as well. These two situations are broken by this
commit, but it's hoped that by giving enough times with warnings we
don't actually break anyone in practice.

Author: bors

src/cargo/core/resolver/dep_cache.rs

@@ -14,10 +14,8 @@ use crate::core::resolver::errors::describe_path;
 use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
 use crate::core::resolver::{ActivateError, 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::util::Config;
 use log::debug;
 use std::cmp::Ordering;
 use std::collections::{BTreeSet, HashMap, HashSet};
@@ -40,10 +38,6 @@ 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> {
@@ -52,7 +46,6 @@ impl<'a> RegistryQueryer<'a> {
         replacements: &'a [(PackageIdSpec, Dependency)],
         try_to_use: &'a HashSet<PackageId>,
         minimal_versions: bool,
-        config: Option<&'a Config>,
     ) -> Self {
         RegistryQueryer {
             registry,
@@ -62,8 +55,6 @@ impl<'a> RegistryQueryer<'a> {
             registry_cache: HashMap::new(),
             summary_cache: HashMap::new(),
             used_replacements: HashMap::new(),
-            config,
-            warned_git_collisions: HashSet::new(),
         }
     }
 
@@ -75,52 +66,13 @@ 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,8 +133,7 @@ pub fn resolve(
         Some(config) => config.cli_unstable().minimal_versions,
         None => false,
     };
-    let mut registry =
-        RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions, config);
+    let mut registry = RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions);
     let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
 
     let mut cksums = HashMap::new();

src/cargo/core/resolver/resolve.rs

@@ -409,6 +409,6 @@ impl Default for ResolveVersion {
     /// file anyway so it takes the opportunity to bump the lock file version
     /// forward.
     fn default() -> ResolveVersion {
-        ResolveVersion::V2
+        ResolveVersion::V3
     }
 }

src/cargo/core/source/source_id.rs

@@ -394,45 +394,9 @@ impl Ord for SourceId {
 
         // Sort first based on `kind`, deferring to the URL comparison below if
         // the kinds are equal.
-        match (&self.inner.kind, &other.inner.kind) {
-            (SourceKind::Path, SourceKind::Path) => {}
-            (SourceKind::Path, _) => return Ordering::Less,
-            (_, SourceKind::Path) => return Ordering::Greater,
-
-            (SourceKind::Registry, SourceKind::Registry) => {}
-            (SourceKind::Registry, _) => return Ordering::Less,
-            (_, SourceKind::Registry) => return Ordering::Greater,
-
-            (SourceKind::LocalRegistry, SourceKind::LocalRegistry) => {}
-            (SourceKind::LocalRegistry, _) => return Ordering::Less,
-            (_, SourceKind::LocalRegistry) => return Ordering::Greater,
-
-            (SourceKind::Directory, SourceKind::Directory) => {}
-            (SourceKind::Directory, _) => return Ordering::Less,
-            (_, SourceKind::Directory) => return Ordering::Greater,
-
-            (SourceKind::Git(a), SourceKind::Git(b)) => {
-                use GitReference::*;
-                let ord = match (a, b) {
-                    (Tag(a), Tag(b)) => a.cmp(b),
-                    (Tag(_), _) => Ordering::Less,
-                    (_, Tag(_)) => Ordering::Greater,
-
-                    (Rev(a), Rev(b)) => a.cmp(b),
-                    (Rev(_), _) => Ordering::Less,
-                    (_, Rev(_)) => Ordering::Greater,
-
-                    // See module comments in src/cargo/sources/git/utils.rs
-                    // for why `DefaultBranch` is treated specially here.
-                    (Branch(a), DefaultBranch) => a.as_str().cmp("master"),
-                    (DefaultBranch, Branch(b)) => "master".cmp(b),
-                    (Branch(a), Branch(b)) => a.cmp(b),
-                    (DefaultBranch, DefaultBranch) => Ordering::Equal,
-                };
-                if ord != Ordering::Equal {
-                    return ord;
-                }
-            }
+        match self.inner.kind.cmp(&other.inner.kind) {
+            Ordering::Equal => {}
+            other => return other,
         }
 
         // If the `kind` and the `url` are equal, then for git sources we also
@@ -509,43 +473,9 @@ impl fmt::Display for SourceId {
 // The hash of SourceId is used in the name of some Cargo folders, so shouldn't
 // vary. `as_str` gives the serialisation of a url (which has a spec) and so
 // insulates against possible changes in how the url crate does hashing.
-//
-// Note that the semi-funky hashing here is done to handle `DefaultBranch`
-// hashing the same as `"master"`, and also to hash the same as previous
-// versions of Cargo while it's somewhat convenient to do so (that way all
-// versions of Cargo use the same checkout).
 impl Hash for SourceId {
     fn hash<S: hash::Hasher>(&self, into: &mut S) {
-        match &self.inner.kind {
-            SourceKind::Git(GitReference::Tag(a)) => {
-                0usize.hash(into);
-                0usize.hash(into);
-                a.hash(into);
-            }
-            SourceKind::Git(GitReference::Branch(a)) => {
-                0usize.hash(into);
-                1usize.hash(into);
-                a.hash(into);
-            }
-            // For now hash `DefaultBranch` the same way as `Branch("master")`,
-            // and for more details see module comments in
-            // src/cargo/sources/git/utils.rs for why `DefaultBranch`
-            SourceKind::Git(GitReference::DefaultBranch) => {
-                0usize.hash(into);
-                1usize.hash(into);
-                "master".hash(into);
-            }
-            SourceKind::Git(GitReference::Rev(a)) => {
-                0usize.hash(into);
-                2usize.hash(into);
-                a.hash(into);
-            }
-
-            SourceKind::Path => 1usize.hash(into),
-            SourceKind::Registry => 2usize.hash(into),
-            SourceKind::LocalRegistry => 3usize.hash(into),
-            SourceKind::Directory => 4usize.hash(into),
-        }
+        self.inner.kind.hash(into);
         match self.inner.kind {
             SourceKind::Git(_) => self.inner.canonical_url.hash(into),
             _ => self.inner.url.as_str().hash(into),

src/cargo/ops/resolve.rs

@@ -13,10 +13,12 @@
 use crate::core::compiler::{CompileKind, RustcTargetData};
 use crate::core::registry::PackageRegistry;
 use crate::core::resolver::features::{FeatureResolver, ForceAllTargets, ResolvedFeatures};
-use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts};
+use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion};
 use crate::core::summary::Summary;
 use crate::core::Feature;
-use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
+use crate::core::{
+    GitReference, PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace,
+};
 use crate::ops;
 use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
@@ -597,7 +599,31 @@ fn register_previous_locks(
             .deps_not_replaced(node)
             .map(|p| p.0)
             .filter(keep)
-            .collect();
+            .collect::<Vec<_>>();
+
+        // In the v2 lockfile format and prior the `branch=master` dependency
+        // directive was serialized the same way as the no-branch-listed
+        // directive. Nowadays in Cargo, however, these two directives are
+        // considered distinct and are no longer represented the same way. To
+        // maintain compatibility with older lock files we register locked nodes
+        // for *both* the master branch and the default branch.
+        //
+        // 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());
+            }
+        }
+
         registry.register_lock(node, deps);
     }
 

src/cargo/sources/git/source.rs

@@ -126,12 +126,10 @@ impl<'cfg> Source for GitSource<'cfg> {
             // database, then try to resolve our reference with the preexisting
             // repository.
             (None, Some(db)) if self.config.offline() => {
-                let rev = db
-                    .resolve(&self.manifest_reference, None)
-                    .with_context(|| {
-                        "failed to lookup reference in preexisting repository, and \
+                let rev = db.resolve(&self.manifest_reference).with_context(|| {
+                    "failed to lookup reference in preexisting repository, and \
                          can't check for updates in offline mode (--offline)"
-                    })?;
+                })?;
                 (db, rev)
             }