Auto merge of #9383 - alexcrichton:revert-git-head, r=ehuss

[beta] Revert #9133, moving to git HEAD dependencies by default

This PR reverts #9133 on the beta branch. The reason for this is that I would like to take some more time to investigate fixing #9352 and #9334. I think those should be relatively easy to fix but I would prefer to not be too rushed with the next release happening soon.

We realized today in the cargo meeting that this revert is likely to not have a ton of impact. For any projects using the v3 lock file format they'll continue to use it successfully. For projects on stable still using v2 they remain unaffected. This will ideally simply give some more time to fix these issues on nightly.

src/cargo/core/resolver/dep_cache.rs

@@ -14,8 +14,10 @@ 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};
@@ -38,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> {
@@ -46,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,
@@ -55,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(),
         }
     }
 
@@ -66,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();

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::V3
+        ResolveVersion::V2
     }
 }

src/cargo/core/source/source_id.rs

@@ -394,9 +394,45 @@ impl Ord for SourceId {
 
         // Sort first based on `kind`, deferring to the URL comparison below if
         // the kinds are equal.
-        match self.inner.kind.cmp(&other.inner.kind) {
-            Ordering::Equal => {}
-            other => return other,
+        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;
+                }
+            }
         }
 
         // If the `kind` and the `url` are equal, then for git sources we also
@@ -473,9 +509,43 @@ 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) {
-        self.inner.kind.hash(into);
+        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),
+        }
         match self.inner.kind {
             SourceKind::Git(_) => self.inner.canonical_url.hash(into),
             _ => self.inner.url.as_str().hash(into),

src/cargo/ops/resolve.rs

@@ -15,12 +15,10 @@ use crate::core::registry::PackageRegistry;
 use crate::core::resolver::features::{
     FeatureOpts, FeatureResolver, ForceAllTargets, ResolvedFeatures,
 };
-use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts, ResolveVersion};
+use crate::core::resolver::{self, HasDevUnits, Resolve, ResolveOpts};
 use crate::core::summary::Summary;
 use crate::core::Feature;
-use crate::core::{
-    GitReference, PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace,
-};
+use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
 use crate::ops;
 use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
@@ -601,31 +599,7 @@ fn register_previous_locks(
             .deps_not_replaced(node)
             .map(|p| p.0)
             .filter(keep)
-            .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());
-            }
-        }
-
+            .collect();
         registry.register_lock(node, deps);
     }
 

src/cargo/sources/git/source.rs

@@ -126,10 +126,12 @@ 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).with_context(|| {
-                    "failed to lookup reference in preexisting repository, and \
+                let rev = db
+                    .resolve(&self.manifest_reference, None)
+                    .with_context(|| {
+                        "failed to lookup reference in preexisting repository, and \
                          can't check for updates in offline mode (--offline)"
-                })?;
+                    })?;
                 (db, rev)
             }