Warn when default branch is not master

This commit implements a warning in Cargo for when a dependency
directive is using `DefaultBranch` but the default branch of the remote
repository is not actually `master`. We will eventually break this
dependency directive and the warning indicates the fix, which is to
write down `branch = "master"`.

Author: alexcrichton

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 \
-                     can't check for updates in offline mode (--offline)"
-                })?;
+                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)
             }
 

src/cargo/sources/git/utils.rs

@@ -1,3 +1,111 @@
+//! Utilities for handling git repositories, mainly around
+//! authentication/cloning.
+//!
+//! # `DefaultBranch` vs `Branch("master")`
+//!
+//! Long ago in a repository not so far away, an author (*cough* me *cough*)
+//! didn't understand how branches work in Git. This led the author to
+//! interpret these two dependency declarations the exact same way with the
+//! former literally internally desugaring to the latter:
+//!
+//! ```toml
+//! [dependencies]
+//! foo = { git = "https://example.org/foo" }
+//! foo = { git = "https://example.org/foo", branch = "master" }
+//! ```
+//!
+//! It turns out there's this things called `HEAD` in git remotes which points
+//! to the "main branch" of a repository, and the main branch is not always
+//! literally called master. What Cargo would like to do is to differentiate
+//! these two dependency directives, with the first meaning "depend on `HEAD`".
+//!
+//! Unfortunately implementing this is a breaking change. This was first
+//! attempted in #8364 but resulted in #8468 which has two independent bugs
+//! listed on that issue. Despite this breakage we would still like to roll out
+//! this change in Cargo, but we're now going to take it very slow and try to
+//! break as few people as possible along the way. These comments are intended
+//! to log the current progress and what wonkiness you might see within Cargo
+//! when handling `DefaultBranch` vs `Branch("master")`
+//!
+//! ### Repositories with `master` and a default branch
+//!
+//! This is one of the most obvious sources of breakage. If our `foo` example
+//! in above had two branches, one called `master` and another which was
+//! actually the main branch, then Cargo's change will always be a breaking
+//! change. This is because what's downloaded is an entirely different branch
+//! if we change the meaning of the dependency directive.
+//!
+//! It's expected this is quite rare, but to handle this case nonetheless when
+//! Cargo fetches from a git remote and the dependency specification is
+//! `DefaultBranch` then it will issue a warning if the `HEAD` reference
+//! doesn't match `master`. It's expected in this situation that authors will
+//! fix builds locally by specifying `branch = 'master'`.
+//!
+//! ### Differences in `cargo vendor` configuration
+//!
+//! When executing `cargo vendor` it will print out configuration which can
+//! then be used to configure Cargo to use the `vendor` directory. Historically
+//! this configuration looked like:
+//!
+//! ```toml
+//! [source."https://example.org/foo"]
+//! git = "https://example.org/foo"
+//! branch = "master"
+//! replace-with = "vendored-sources"
+//! ```
+//!
+//! We would like to, however, transition this to not include the `branch =
+//! "master"` unless the dependency directive actually mentions a branch.
+//! Conveniently older Cargo implementations all interpret a missing `branch`
+//! as `branch = "master"` so it's a backwards-compatible change to remove the
+//! `branch = "master"` directive. As a result, `cargo vendor` will no longer
+//! emit a `branch` if the git reference is `DefaultBranch`
+//!
+//! ### Differences in lock file formats
+//!
+//! Another issue pointed out in #8364 was that `Cargo.lock` files were no
+//! longer compatible on stable and nightly with each other. The underlying
+//! issue is that Cargo was serializing `branch = "master"` *differently* on
+//! nightly than it was on stable. Historical implementations of Cargo
+//! desugared `branch = "master"` to having not dependency directives in
+//! `Cargo.lock`, which means when reading `Cargo.lock` we can't differentiate
+//! what's for the default branch and what's for the `master` branch.
+//!
+//! To handle this difference in encoding of `Cargo.lock` we'll be employing
+//! the standard scheme to change `Cargo.lock`:
+//!
+//! * Add support in Cargo for a future format, don't turn it on.
+//! * Wait a long time
+//! * Turn on the future format
+//!
+//! Here the "future format" is `branch=master` shows up if you have a `branch`
+//! in `Cargo.toml`, and otherwise nothing shows up in URLs. Due to the effect
+//! on crate graph resolution, however, this flows into the next point..
+//!
+//! ### Unification in the Cargo dependency graph
+//!
+//! Today dependencies with `branch = "master"` will unify with dependencies
+//! that say nothing. (that's because the latter simply desugars). This means
+//! the two `foo` directives above will resolve to the same dependency.
+//!
+//! The best idea I've got to fix this is to basically get everyone (if anyone)
+//! to stop doing this today. The crate graph resolver will start to warn if it
+//! detects that multiple `Cargo.toml` directives are detected and mixed.  The
+//! thinking is that when we turn on the new lock file format it'll also be
+//! hard breaking change for any project which still has dependencies to
+//! both the `master` branch and not.
+//!
+//! ### What we're doing today
+//!
+//! The general goal of Cargo today is to internally distinguish
+//! `DefaultBranch` and `Branch("master")`, but for the time being they should
+//! be functionally equivalent in terms of builds. The hope is that we'll let
+//! all these warnings and such bake for a good long time, and eventually we'll
+//! flip some switches if your build has no warnings it'll work before and
+//! after.
+//!
+//! That's the dream at least, we'll see how this plays out.
+
 use crate::core::GitReference;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::paths;
@@ -74,7 +182,7 @@ impl GitRemote {
     }
 
     pub fn rev_for(&self, path: &Path, reference: &GitReference) -> CargoResult<git2::Oid> {
-        reference.resolve(&self.db_at(path)?.repo)
+        reference.resolve(&self.db_at(path)?.repo, None)
     }
 
     pub fn checkout(
@@ -99,7 +207,7 @@ impl GitRemote {
                     }
                 }
                 None => {
-                    if let Ok(rev) = reference.resolve(&db.repo) {
+                    if let Ok(rev) = reference.resolve(&db.repo, Some((&self.url, cargo_config))) {
                         return Ok((db, rev));
                     }
                 }
@@ -118,7 +226,7 @@ impl GitRemote {
             .context(format!("failed to clone into: {}", into.display()))?;
         let rev = match locked_rev {
             Some(rev) => rev,
-            None => reference.resolve(&repo)?,
+            None => reference.resolve(&repo, Some((&self.url, cargo_config)))?,
         };
 
         Ok((
@@ -187,13 +295,21 @@ impl GitDatabase {
         self.repo.revparse_single(&oid.to_string()).is_ok()
     }
 
-    pub fn resolve(&self, r: &GitReference) -> CargoResult<git2::Oid> {
-        r.resolve(&self.repo)
+    pub fn resolve(
+        &self,
+        r: &GitReference,
+        remote_and_config: Option<(&Url, &Config)>,
+    ) -> CargoResult<git2::Oid> {
+        r.resolve(&self.repo, remote_and_config)
     }
 }
 
 impl GitReference {
-    pub fn resolve(&self, repo: &git2::Repository) -> CargoResult<git2::Oid> {
+    pub fn resolve(
+        &self,
+        repo: &git2::Repository,
+        remote_and_config: Option<(&Url, &Config)>,
+    ) -> CargoResult<git2::Oid> {
         let id = match self {
             // Note that we resolve the named tag here in sync with where it's
             // fetched into via `fetch` below.
@@ -227,6 +343,28 @@ impl GitReference {
                     .get()
                     .target()
                     .ok_or_else(|| anyhow::format_err!("branch `master` did not have a target"))?;
+
+                if let Some((remote, config)) = remote_and_config {
+                    let head_id = repo.refname_to_id("refs/remotes/origin/HEAD")?;
+                    let head = repo.find_object(head_id, None)?;
+                    let head = head.peel(ObjectType::Commit)?.id();
+
+                    if head != master {
+                        config.shell().warn(&format!(
+                            "\
+                                fetching `master` branch from `{}` but the `HEAD` \
+                                reference for this repository is not the \
+                                `master` branch. This behavior will change \
+                                in Cargo in the future and your build may \
+                                break, so it's recommended to place \
+                                `branch = \"master\"` in Cargo.toml when \
+                                depending on this git repository to ensure \
+                                that your build will continue to work.\
+                            ",
+                            remote,
+                        ))?;
+                    }
+                }
                 master
             }
 
@@ -762,6 +900,7 @@ pub fn fetch(
         }
 
         GitReference::DefaultBranch => {
+            // See the module docs for why we're fetching `master` here.
             refspecs.push(format!("refs/heads/master:refs/remotes/origin/master"));
             refspecs.push(String::from("HEAD:refs/remotes/origin/HEAD"));
         }
@@ -1032,7 +1171,10 @@ fn github_up_to_date(
     handle.useragent("cargo")?;
     let mut headers = List::new();
     headers.append("Accept: application/vnd.github.3.sha")?;
-    headers.append(&format!("If-None-Match: \"{}\"", reference.resolve(repo)?))?;
+    headers.append(&format!(
+        "If-None-Match: \"{}\"",
+        reference.resolve(repo, None)?
+    ))?;
     handle.http_headers(headers)?;
     handle.perform()?;
     Ok(handle.response_code()? == 304)

src/cargo/sources/registry/remote.rs

@@ -102,7 +102,7 @@ impl<'cfg> RemoteRegistry<'cfg> {
     fn head(&self) -> CargoResult<git2::Oid> {
         if self.head.get().is_none() {
             let repo = self.repo()?;
-            let oid = self.index_git_ref.resolve(repo)?;
+            let oid = self.index_git_ref.resolve(repo, None)?;
             self.head.set(Some(oid));
         }
         Ok(self.head.get().unwrap())

tests/testsuite/git.rs

@@ -2831,6 +2831,14 @@ fn default_not_master() {
         .with_stderr(
             "\
 [UPDATING] git repository `[..]`
+warning: fetching `master` branch from `[..]` but the `HEAD` \
+    reference for this repository is not the \
+    `master` branch. This behavior will change \
+    in Cargo in the future and your build may \
+    break, so it's recommended to place \
+    `branch = \"master\"` in Cargo.toml when \
+    depending on this git repository to ensure \
+    that your build will continue to work.
 [COMPILING] dep1 v0.5.0 ([..])
 [COMPILING] foo v0.5.0 ([..])
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",