From 538fb1b4ed77c17c47b1af9cf932df787d6060f0 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 20 Jul 2020 08:47:57 -0700 Subject: [PATCH] Effectively revert #8364 This commit is intended to be an effective but not literal revert of #8364. Internally Cargo will still distinguish between `DefaultBranch` and `Branch("master")` when reading `Cargo.toml` files, but for almost all purposes the two are equivalent. This will namely fix the issue we have with lock file encodings where both are encoded with no `branch` (and without a branch it's parsed from a lock file as `DefaultBranch`). This will preserve the change that `cargo vendor` will not print out `branch = "master"` annotations but that desugars to match the lock file on the other end, so it should continue to work. Tests have been added in this commit for the regressions found on #8468. --- src/cargo/core/source/source_id.rs | 80 ++++++++++++++++- src/cargo/sources/git/utils.rs | 19 ++-- tests/testsuite/git.rs | 137 ++++++++++++++++++++++++++--- 3 files changed, 217 insertions(+), 19 deletions(-) diff --git a/src/cargo/core/source/source_id.rs b/src/cargo/core/source/source_id.rs index a493a1807..05815d1eb 100644 --- a/src/cargo/core/source/source_id.rs +++ b/src/cargo/core/source/source_id.rs @@ -1,7 +1,7 @@ use std::cmp::{self, Ordering}; use std::collections::HashSet; use std::fmt::{self, Formatter}; -use std::hash::{self, Hash}; +use std::hash::{self, Hash, Hasher}; use std::path::Path; use std::ptr; use std::sync::Mutex; @@ -59,7 +59,7 @@ enum SourceKind { } /// Information to find a specific commit in a Git repository. -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Debug, Clone)] pub enum GitReference { /// From a tag. Tag(String), @@ -553,8 +553,11 @@ impl GitReference { /// Returns a `Display`able view of this git reference, or None if using /// the head of the default branch pub fn pretty_ref(&self) -> Option> { - match *self { + match self { GitReference::DefaultBranch => None, + // See module comments in src/cargo/sources/git/utils.rs for why + // `DefaultBranch` is treated specially here. + GitReference::Branch(m) if m == "master" => None, _ => Some(PrettyRef { inner: self }), } } @@ -576,6 +579,77 @@ impl<'a> fmt::Display for PrettyRef<'a> { } } +// See module comments in src/cargo/sources/git/utils.rs for why `DefaultBranch` +// is treated specially here. +impl PartialEq for GitReference { + fn eq(&self, other: &GitReference) -> bool { + match (self, other) { + (GitReference::Tag(a), GitReference::Tag(b)) => a == b, + (GitReference::Rev(a), GitReference::Rev(b)) => a == b, + (GitReference::Branch(b), GitReference::DefaultBranch) + | (GitReference::DefaultBranch, GitReference::Branch(b)) => b == "master", + (GitReference::DefaultBranch, GitReference::DefaultBranch) => true, + (GitReference::Branch(a), GitReference::Branch(b)) => a == b, + _ => false, + } + } +} + +impl Eq for GitReference {} + +// See module comments in src/cargo/sources/git/utils.rs for why `DefaultBranch` +// is treated specially here. +impl Hash for GitReference { + fn hash(&self, hasher: &mut H) { + match self { + GitReference::Tag(a) => { + 0u8.hash(hasher); + a.hash(hasher); + } + GitReference::Rev(a) => { + 1u8.hash(hasher); + a.hash(hasher); + } + GitReference::Branch(a) => { + 2u8.hash(hasher); + a.hash(hasher); + } + GitReference::DefaultBranch => { + 2u8.hash(hasher); + "master".hash(hasher); + } + } + } +} + +impl PartialOrd for GitReference { + fn partial_cmp(&self, other: &GitReference) -> Option { + Some(self.cmp(other)) + } +} + +// See module comments in src/cargo/sources/git/utils.rs for why `DefaultBranch` +// is treated specially here. +impl Ord for GitReference { + fn cmp(&self, other: &GitReference) -> Ordering { + use GitReference::*; + match (self, other) { + (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, + + (Branch(b), DefaultBranch) => b.as_str().cmp("master"), + (DefaultBranch, Branch(b)) => "master".cmp(b), + (Branch(a), Branch(b)) => a.cmp(b), + (DefaultBranch, DefaultBranch) => Ordering::Equal, + } + } +} + #[cfg(test)] mod tests { use super::{GitReference, SourceId, SourceKind}; diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index fec8628b4..d2e7395fd 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -217,12 +217,17 @@ impl GitReference { .target() .ok_or_else(|| anyhow::format_err!("branch `{}` did not have a target", s))? } + + // See the module docs for why we're using `master` here. GitReference::DefaultBranch => { - let refname = "refs/remotes/origin/HEAD"; - let id = repo.refname_to_id(refname)?; - let obj = repo.find_object(id, None)?; - let obj = obj.peel(ObjectType::Commit)?; - obj.id() + let master = repo + .find_branch("origin/master", git2::BranchType::Remote) + .chain_err(|| "failed to find branch `master`")?; + let master = master + .get() + .target() + .ok_or_else(|| anyhow::format_err!("branch `master` did not have a target"))?; + master } GitReference::Rev(s) => { @@ -757,6 +762,7 @@ pub fn fetch( } GitReference::DefaultBranch => { + refspecs.push(format!("refs/heads/master:refs/remotes/origin/master")); refspecs.push(String::from("HEAD:refs/remotes/origin/HEAD")); } @@ -984,7 +990,8 @@ fn github_up_to_date( let github_branch_name = match reference { GitReference::Branch(branch) => branch, GitReference::Tag(tag) => tag, - GitReference::DefaultBranch => "HEAD", + // See the module docs for why we're using `master` here. + GitReference::DefaultBranch => "master", GitReference::Rev(_) => { debug!("can't use github fast path with `rev`"); return Ok(false); diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 212869af8..c4c535e3c 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -5,6 +5,7 @@ use std::fs; use std::io::prelude::*; use std::net::{TcpListener, TcpStream}; use std::path::Path; +use std::str; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use std::thread; @@ -2789,20 +2790,24 @@ to proceed despite [..] fn default_not_master() { let project = project(); - // Create a repository with a `master` branch ... + // Create a repository with a `master` branch, but switch the head to a + // branch called `main` at the same time. let (git_project, repo) = git::new_repo("dep1", |project| { - project.file("Cargo.toml", &basic_lib_manifest("dep1")) + project + .file("Cargo.toml", &basic_lib_manifest("dep1")) + .file("src/lib.rs", "pub fn foo() {}") }); - - // Then create a `main` branch with actual code, and set the head of the - // repository (default branch) to `main`. - git_project.change_file("src/lib.rs", "pub fn foo() {}"); - git::add(&repo); - let rev = git::commit(&repo); - let commit = repo.find_commit(rev).unwrap(); - repo.branch("main", &commit, false).unwrap(); + let head_id = repo.head().unwrap().target().unwrap(); + let head = repo.find_commit(head_id).unwrap(); + repo.branch("main", &head, false).unwrap(); repo.set_head("refs/heads/main").unwrap(); + // Then create a commit on the new `main` branch so `master` and `main` + // differ. + git_project.change_file("src/lib.rs", ""); + git::add(&repo); + git::commit(&repo); + let project = project .file( "Cargo.toml", @@ -2832,3 +2837,115 @@ fn default_not_master() { ) .run(); } + +#[cargo_test] +fn historical_lockfile_works() { + let project = project(); + + let (git_project, repo) = git::new_repo("dep1", |project| { + project + .file("Cargo.toml", &basic_lib_manifest("dep1")) + .file("src/lib.rs", "") + }); + let head_id = repo.head().unwrap().target().unwrap(); + + let project = project + .file( + "Cargo.toml", + &format!( + r#" + [project] + name = "foo" + version = "0.5.0" + + [dependencies] + dep1 = {{ git = '{}', branch = 'master' }} + "#, + git_project.url() + ), + ) + .file("src/lib.rs", "") + .build(); + + project.cargo("build").run(); + project.change_file( + "Cargo.lock", + &format!( + r#"# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "dep1" +version = "0.5.0" +source = "git+{}#{}" + +[[package]] +name = "foo" +version = "0.5.0" +dependencies = [ + "dep1", +] +"#, + git_project.url(), + head_id + ), + ); + project + .cargo("build") + .with_stderr("[FINISHED] [..]\n") + .run(); +} + +#[cargo_test] +fn historical_lockfile_works_with_vendor() { + let project = project(); + + let (git_project, repo) = git::new_repo("dep1", |project| { + project + .file("Cargo.toml", &basic_lib_manifest("dep1")) + .file("src/lib.rs", "") + }); + let head_id = repo.head().unwrap().target().unwrap(); + + let project = project + .file( + "Cargo.toml", + &format!( + r#" + [project] + name = "foo" + version = "0.5.0" + + [dependencies] + dep1 = {{ git = '{}', branch = 'master' }} + "#, + git_project.url() + ), + ) + .file("src/lib.rs", "") + .build(); + + let output = project.cargo("vendor").exec_with_output().unwrap(); + project.change_file(".cargo/config", str::from_utf8(&output.stdout).unwrap()); + project.change_file( + "Cargo.lock", + &format!( + r#"# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "dep1" +version = "0.5.0" +source = "git+{}#{}" + +[[package]] +name = "foo" +version = "0.5.0" +dependencies = [ + "dep1", +] +"#, + git_project.url(), + head_id + ), + ); + project.cargo("build").run(); +}