Auto merge of #8522 - alexcrichton:revert-master, r=ehuss

Prepare for not defaulting to master branch for git deps

This PR is the "equivalent" of #8503 for the nightly channel of Rust (the master branch of Cargo). The purpose of this change is to fix the breakage from #8364 but to still pave a path forward to enabling the feature added in #8364, namely reinterpreting dependency directives without a `branch` directive as depending on `HEAD` insead of depending on `master`.

This is a series of commits which implements a few mitigation strategies, but they're all adding up into a larger plan to roll out this change with, ideally, no breaking changes for those "reasonably" keeping up with Rust. The changes here are:

* Cargo still internally differentiates between `DefaultBranch` and `Branch("master")`, so it knows what you wrote in the manifest. These two, however, hash and equate together so everything in Cargo considers them equivalent.
* Cargo will now issue a warning whenever it fetches a repository pinned to `DefaultBranch` and the branch's `master` branch doesn't actually match `HEAD`. This avenue of breakage didn't arise but I added it here for completionism about not having to deal with breakage from this change again.
* Cargo has now implemented a new future lockfile format. Internally this is dubbed "v3". The changes in this format is that it will have a `version` marker at the top of the file as well as encoding git dependencies different. Today `DefaultBranch` and `Branch("master")` encode the same way, but in the future they will encode differently.
* Cargo now has a warning where if you have a mixture of `DefaultBranch` and `Branch("master")` in your build. The intention here is to get everyone on one or the other so the transition to the new lock file format can go smoothly.

With all of these pieces in place the intention is that there is no breakage from #8364 as well. Additionally projects that *would* break will receive warnings. (note that projects "broken" today, those we've got issues for, will likely not get warnings, but that's because it was accidental we broke them). The long-term plan is to let this bake for quite some time, and then as part of the same change land an update to the new lock file format as well as "fixing" `SourceId` to consider these two directives different. When this change happens we should have guaranteed, for all currently active projects, they're either only using `branch = "master"` or no directive at all. We can therefore unambiguosly read the previous `Cargo.lock` to transition it into a new `Cargo.lock` which will either have `?branch=master` or nothing.

I'm not sure how long this will need to bake for because unfortunately version changes to `Cargo.lock` cannot be taken lightly. We haven't even bumped the default format for old lock files to `V2` yet, they're still on `V1`. Hopefully in getting this in early-ish, though, we can at least get the ball rolling.

Closes #8468

Author: bors

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);
         }