From aa256ffddc8c777d26fbdf51573a7d343a1783ac Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 8 Dec 2014 10:46:07 -0800 Subject: [PATCH] Refactor git rev handling infrastructure This commit unifies the notion of a "git revision" between a SourceId and the GitSource. This pushes the request of a branch, tag, or revision all the way down into a GitSource so special care can be taken for each case. This primarily was discovered by #1069 where a git tag's id is different from the commit that it points at, and we need to push the knowledge of whether it's a tag or not all the way down to the point where we resolve what revision we want (and perform appropriate operations to find the commit we want). Closes #1069 --- src/bin/git_checkout.rs | 5 +- src/cargo/core/mod.rs | 2 +- src/cargo/core/source.rs | 77 ++++++++++++++++------- src/cargo/sources/git/source.rs | 28 +++++---- src/cargo/sources/git/utils.rs | 93 ++++++++++++---------------- src/cargo/util/toml.rs | 12 ++-- tests/resolve.rs | 8 ++- tests/test_cargo_compile_git_deps.rs | 15 ++--- 8 files changed, 133 insertions(+), 107 deletions(-) diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs index 184da359f5d..8e2712cdcf6 100644 --- a/src/bin/git_checkout.rs +++ b/src/bin/git_checkout.rs @@ -1,5 +1,5 @@ use cargo::core::MultiShell; -use cargo::core::source::{Source, SourceId}; +use cargo::core::source::{Source, SourceId, GitReference}; use cargo::sources::git::{GitSource}; use cargo::util::{Config, CliResult, CliError, human, ToUrl}; @@ -30,7 +30,8 @@ pub fn execute(options: Options, shell: &mut MultiShell) -> CliResult }) .map_err(|e| CliError::from_boxed(e, 1))); - let source_id = SourceId::for_git(&url, reference.as_slice()); + let reference = GitReference::Branch(reference.to_string()); + let source_id = SourceId::for_git(&url, reference); let mut config = try!(Config::new(shell, None, None).map_err(|e| { CliError::from_boxed(e, 1) diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index f7ffee33701..d345e3d67c5 100644 --- a/src/cargo/core/mod.rs +++ b/src/cargo/core/mod.rs @@ -6,7 +6,7 @@ pub use self::package_id_spec::PackageIdSpec; pub use self::registry::Registry; pub use self::resolver::Resolve; pub use self::shell::{Shell, MultiShell, ShellConfig}; -pub use self::source::{Source, SourceId, SourceMap, SourceSet}; +pub use self::source::{Source, SourceId, SourceMap, SourceSet, GitReference}; pub use self::summary::Summary; pub mod source; diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index fa0cdba0ee3..36b9ae2862d 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -42,16 +42,23 @@ pub trait Source: Registry { fn fingerprint(&self, pkg: &Package) -> CargoResult; } -#[deriving(RustcEncodable, RustcDecodable, Show, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[deriving(Show, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] enum Kind { /// Kind::Git() represents a git repository - Git(String), + Git(GitReference), /// represents a local path Path, /// represents the central registry Registry, } +#[deriving(Show, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum GitReference { + Tag(String), + Branch(String), + Rev(String), +} + type Error = Box; /// Unique identifier for a source of packages. @@ -97,16 +104,22 @@ impl SourceId { match kind { "git" => { let mut url = url.to_url().unwrap(); - let mut reference = "master".to_string(); + let mut reference = GitReference::Branch("master".to_string()); let pairs = url.query_pairs().unwrap_or(Vec::new()); for &(ref k, ref v) in pairs.iter() { - if k.as_slice() == "ref" { - reference = v.clone(); + match k.as_slice() { + // map older 'ref' to branch + "branch" | + "ref" => reference = GitReference::Branch(v.clone()), + + "rev" => reference = GitReference::Rev(v.clone()), + "tag" => reference = GitReference::Tag(v.clone()), + _ => {} } } url.query = None; let precise = mem::replace(&mut url.fragment, None); - SourceId::for_git(&url, reference.as_slice()) + SourceId::for_git(&url, reference) .with_precise(precise) }, "registry" => { @@ -128,11 +141,7 @@ impl SourceId { SourceIdInner { kind: Kind::Git(ref reference), ref url, ref precise, .. } => { - let ref_str = if reference.as_slice() != "master" { - format!("?ref={}", reference) - } else { - "".to_string() - }; + let ref_str = url_ref(reference); let precise_str = if precise.is_some() { format!("#{}", precise.as_ref().unwrap()) @@ -154,8 +163,8 @@ impl SourceId { Ok(SourceId::new(Kind::Path, url)) } - pub fn for_git(url: &Url, reference: &str) -> SourceId { - SourceId::new(Kind::Git(reference.to_string()), url.clone()) + pub fn for_git(url: &Url, reference: GitReference) -> SourceId { + SourceId::new(Kind::Git(reference), url.clone()) } pub fn for_registry(url: &Url) -> SourceId { @@ -203,9 +212,9 @@ impl SourceId { self.inner.precise.as_ref().map(|s| s.as_slice()) } - pub fn git_reference(&self) -> Option<&str> { + pub fn git_reference(&self) -> Option<&GitReference> { match self.inner.kind { - Kind::Git(ref s) => Some(s.as_slice()), + Kind::Git(ref s) => Some(s), _ => None, } } @@ -269,10 +278,7 @@ impl Show for SourceId { SourceIdInner { kind: Kind::Path, ref url, .. } => url.fmt(f), SourceIdInner { kind: Kind::Git(ref reference), ref url, ref precise, .. } => { - try!(write!(f, "{}", url)); - if reference.as_slice() != "master" { - try!(write!(f, "?ref={}", reference)); - } + try!(write!(f, "{}{}", url, url_ref(reference))); match *precise { Some(ref s) => { @@ -319,6 +325,29 @@ impl hash::Hash for SourceId { } } +fn url_ref(r: &GitReference) -> String { + match r.to_ref_string() { + None => "".to_string(), + Some(s) => format!("?{}", s), + } +} + +impl GitReference { + pub fn to_ref_string(&self) -> Option { + match *self { + GitReference::Branch(ref s) => { + if s.as_slice() == "master" { + None + } else { + Some(format!("branch={}", s)) + } + } + GitReference::Tag(ref s) => Some(format!("tag={}", s)), + GitReference::Rev(ref s) => Some(format!("rev={}", s)), + } + } +} + pub struct SourceMap<'src> { map: HashMap> } @@ -446,20 +475,22 @@ impl<'src> Source for SourceSet<'src> { #[cfg(test)] mod tests { - use super::{SourceId, Kind}; + use super::{SourceId, Kind, GitReference}; use util::ToUrl; #[test] fn github_sources_equal() { let loc = "https://github.com/foo/bar".to_url().unwrap(); - let s1 = SourceId::new(Kind::Git("master".to_string()), loc); + let master = Kind::Git(GitReference::Branch("master".to_string())); + let s1 = SourceId::new(master.clone(), loc); let loc = "git://github.com/foo/bar".to_url().unwrap(); - let s2 = SourceId::new(Kind::Git("master".to_string()), loc.clone()); + let s2 = SourceId::new(master, loc.clone()); assert_eq!(s1, s2); - let s3 = SourceId::new(Kind::Git("foo".to_string()), loc); + let foo = Kind::Git(GitReference::Branch("foo".to_string())); + let s3 = SourceId::new(foo, loc); assert!(s1 != s3); } } diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 2e7fb50b91e..605ff5a7143 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -5,10 +5,11 @@ use std::mem; use url::{mod, Url}; use core::source::{Source, SourceId}; +use core::GitReference; use core::{Package, PackageId, Summary, Registry, Dependency}; use util::{CargoResult, Config, to_hex}; use sources::PathSource; -use sources::git::utils::{GitReference, GitRemote, GitRevision}; +use sources::git::utils::{GitRemote, GitRevision}; /* TODO: Refactor GitSource to delegate to a PathSource */ @@ -39,17 +40,23 @@ impl<'a, 'b> GitSource<'a, 'b> { let db_path = config.git_db_path() .join(ident.as_slice()); + let reference_path = match *reference { + GitReference::Branch(ref s) | + GitReference::Tag(ref s) | + GitReference::Rev(ref s) => s.to_string(), + }; let checkout_path = config.git_checkout_path() - .join(ident.as_slice()).join(reference.as_slice()); + .join(ident) + .join(reference_path); let reference = match source_id.get_precise() { - Some(s) => s, - None => reference.as_slice(), + Some(s) => GitReference::Rev(s.to_string()), + None => source_id.git_reference().unwrap().clone(), }; GitSource { remote: remote, - reference: GitReference::for_str(reference.as_slice()), + reference: reference, db_path: db_path, checkout_path: checkout_path, source_id: source_id.clone(), @@ -140,9 +147,9 @@ impl<'a, 'b> Show for GitSource<'a, 'b> { fn fmt(&self, f: &mut Formatter) -> fmt::Result { try!(write!(f, "git repo at {}", self.remote.get_url())); - match self.reference { - GitReference::Master => Ok(()), - GitReference::Other(ref reference) => write!(f, " ({})", reference) + match self.reference.to_ref_string() { + Some(s) => write!(f, " ({})", s), + None => Ok(()) } } } @@ -157,8 +164,7 @@ impl<'a, 'b> Registry for GitSource<'a, 'b> { impl<'a, 'b> Source for GitSource<'a, 'b> { fn update(&mut self) -> CargoResult<()> { - let actual_rev = self.remote.rev_for(&self.db_path, - self.reference.as_slice()); + let actual_rev = self.remote.rev_for(&self.db_path, &self.reference); let should_update = actual_rev.is_err() || self.source_id.get_precise().is_none(); @@ -168,7 +174,7 @@ impl<'a, 'b> Source for GitSource<'a, 'b> { log!(5, "updating git source `{}`", self.remote); let repo = try!(self.remote.checkout(&self.db_path)); - let rev = try!(repo.rev_for(self.reference.as_slice())); + let rev = try!(repo.rev_for(&self.reference)); (repo, rev) } else { (try!(self.remote.db_at(&self.db_path)), actual_rev.unwrap()) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 0e7b3afeddb..8d030c7f511 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -5,52 +5,16 @@ use rustc_serialize::{Encodable, Encoder}; use url::Url; use git2; +use core::GitReference; use util::{CargoResult, ChainError, human, ToUrl, internal, Require}; -#[deriving(PartialEq,Clone,RustcEncodable)] -pub enum GitReference { - Master, - Other(String) -} - -#[deriving(PartialEq,Clone,RustcEncodable)] -pub struct GitRevision(String); - -impl GitReference { - pub fn for_str(string: S) -> GitReference { - if string.as_slice() == "master" { - GitReference::Master - } else { - GitReference::Other(string.as_slice().to_string()) - } - } -} - -impl Str for GitReference { - fn as_slice(&self) -> &str { - match *self { - GitReference::Master => "master", - GitReference::Other(ref string) => string.as_slice() - } - } -} - -impl Show for GitReference { - fn fmt(&self, f: &mut Formatter) -> fmt::Result { - self.as_slice().fmt(f) - } -} - -impl Str for GitRevision { - fn as_slice(&self) -> &str { - let GitRevision(ref me) = *self; - me.as_slice() - } -} +#[deriving(PartialEq, Clone)] +#[allow(missing_copy_implementations)] +pub struct GitRevision(git2::Oid); impl Show for GitRevision { fn fmt(&self, f: &mut Formatter) -> fmt::Result { - self.as_slice().fmt(f) + self.0.fmt(f) } } @@ -138,8 +102,8 @@ impl GitRemote { &self.url } - pub fn rev_for(&self, path: &Path, reference: S) - -> CargoResult { + pub fn rev_for(&self, path: &Path, reference: &GitReference) + -> CargoResult { let db = try!(self.db_at(path)); db.rev_for(reference) } @@ -215,9 +179,36 @@ impl GitDatabase { Ok(checkout) } - pub fn rev_for(&self, reference: S) -> CargoResult { - let rev = try!(self.repo.revparse_single(reference.as_slice())); - Ok(GitRevision(rev.id().to_string())) + pub fn rev_for(&self, reference: &GitReference) -> CargoResult { + let id = match *reference { + GitReference::Tag(ref s) => { + try!((|| { + let refname = format!("refs/tags/{}", s); + let id = try!(self.repo.refname_to_id(refname.as_slice())); + let tag = try!(self.repo.find_tag(id)); + let obj = try!(tag.peel()); + Ok(obj.id()) + }).chain_error(|| { + human(format!("failed to find tag `{}`", s)) + })) + } + GitReference::Branch(ref s) => { + try!((|| { + let b = try!(self.repo.find_branch(s.as_slice(), + git2::BranchType::Local)); + b.get().target().require(|| { + human(format!("branch `{}` did not have a target", s)) + }) + }).chain_error(|| { + human(format!("failed to find branch `{}`", s)) + })) + } + GitReference::Rev(ref s) => { + let obj = try!(self.repo.revparse_single(s.as_slice())); + obj.id() + } + }; + Ok(GitRevision(id)) } pub fn has_ref(&self, reference: S) -> CargoResult<()> { @@ -249,10 +240,6 @@ impl<'a> GitCheckout<'a> { Ok(checkout) } - pub fn get_rev(&self) -> &str { - self.revision.as_slice() - } - fn clone_repo(source: &Path, into: &Path) -> CargoResult { let dirname = into.dir_path(); @@ -293,10 +280,8 @@ impl<'a> GitCheckout<'a> { } fn reset(&self) -> CargoResult<()> { - info!("reset {} to {}", self.repo.path().display(), - self.revision.as_slice()); - let oid = try!(git2::Oid::from_str(self.revision.as_slice())); - let object = try!(self.repo.find_object(oid, None)); + info!("reset {} to {}", self.repo.path().display(), self.revision); + let object = try!(self.repo.find_object(self.revision.0, None)); try!(self.repo.reset(&object, git2::ResetType::Hard, None, None)); Ok(()) } diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 6320270b6e8..eea5edf6324 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -11,7 +11,7 @@ use semver; use rustc_serialize::{Decodable, Decoder}; use core::SourceId; -use core::{Summary, Manifest, Target, Dependency, PackageId}; +use core::{Summary, Manifest, Target, Dependency, PackageId, GitReference}; use core::dependency::Kind; use core::manifest::{LibKind, Profile, ManifestMetadata}; use core::package_id::Metadata; @@ -571,17 +571,17 @@ fn process_dependencies<'a>(cx: &mut Context<'a>, } TomlDependency::Detailed(ref details) => details.clone(), }; - let reference = details.branch.clone() - .or_else(|| details.tag.clone()) - .or_else(|| details.rev.clone()) - .unwrap_or_else(|| "master".to_string()); + let reference = details.branch.clone().map(GitReference::Branch) + .or_else(|| details.tag.clone().map(GitReference::Tag)) + .or_else(|| details.rev.clone().map(GitReference::Rev)) + .unwrap_or_else(|| GitReference::Branch("master".to_string())); let new_source_id = match details.git { Some(ref git) => { let loc = try!(git.as_slice().to_url().map_err(|e| { human(e) })); - Some(SourceId::for_git(&loc, reference.as_slice())) + Some(SourceId::for_git(&loc, reference)) } None => { details.path.as_ref().map(|path| { diff --git a/tests/resolve.rs b/tests/resolve.rs index 7fe4ffd53ca..d55b0a8604f 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -7,7 +7,7 @@ use std::collections::HashMap; use hamcrest::{assert_that, equal_to, contains}; -use cargo::core::source::SourceId; +use cargo::core::source::{SourceId, GitReference}; use cargo::core::dependency::Kind::Development; use cargo::core::{Dependency, PackageId, Summary, Registry}; use cargo::util::{CargoResult, ToUrl}; @@ -85,7 +85,8 @@ fn pkg_id(name: &str) -> PackageId { fn pkg_id_loc(name: &str, loc: &str) -> PackageId { let remote = loc.to_url(); - let source_id = SourceId::for_git(&remote.unwrap(), "master"); + let master = GitReference::Branch("master".to_string()); + let source_id = SourceId::for_git(&remote.unwrap(), master); PackageId::new(name, "1.0.0", &source_id).unwrap() } @@ -103,7 +104,8 @@ fn dep_req(name: &str, req: &str) -> Dependency { fn dep_loc(name: &str, location: &str) -> Dependency { let url = location.to_url().unwrap(); - let source_id = SourceId::for_git(&url, "master"); + let master = GitReference::Branch("master".to_string()); + let source_id = SourceId::for_git(&url, master); Dependency::parse(name, Some("1.0.0"), &source_id).unwrap() } diff --git a/tests/test_cargo_compile_git_deps.rs b/tests/test_cargo_compile_git_deps.rs index 51cd2bceacf..e0c5277da34 100644 --- a/tests/test_cargo_compile_git_deps.rs +++ b/tests/test_cargo_compile_git_deps.rs @@ -187,7 +187,7 @@ test!(cargo_compile_git_dep_branch { assert_that(project.cargo_process("build"), execs() .with_stdout(format!("{} git repository `{}`\n\ - {} dep1 v0.5.0 ({}?ref=branchy#[..])\n\ + {} dep1 v0.5.0 ({}?branch=branchy#[..])\n\ {} foo v0.5.0 ({})\n", UPDATING, path2url(git_root.clone()), COMPILING, path2url(git_root), @@ -257,18 +257,19 @@ test!(cargo_compile_git_dep_tag { assert_that(project.cargo_process("build"), execs() .with_stdout(format!("{} git repository `{}`\n\ - {} dep1 v0.5.0 ({}?ref=v0.1.0#[..])\n\ + {} dep1 v0.5.0 ({}?tag=v0.1.0#[..])\n\ {} foo v0.5.0 ({})\n", UPDATING, path2url(git_root.clone()), COMPILING, path2url(git_root), - COMPILING, path2url(root))) - .with_stderr("")); + COMPILING, path2url(root)))); assert_that(&project.bin("foo"), existing_file()); - assert_that( - cargo::util::process(project.bin("foo")).unwrap(), - execs().with_stdout("hello world\n")); + assert_that(cargo::util::process(project.bin("foo")).unwrap(), + execs().with_stdout("hello world\n")); + + assert_that(project.process(cargo_dir().join("cargo")).arg("build"), + execs().with_status(0)); }); test!(cargo_compile_with_nested_paths {