auto merge of #1074 : alexcrichton/cargo/issue-1069, r=brson

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

Author: bors

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<Option<()>
                    })
                    .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)

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;

src/cargo/core/source.rs

@@ -42,16 +42,23 @@ pub trait Source: Registry {
     fn fingerprint(&self, pkg: &Package) -> CargoResult<String>;
 }
 
-#[deriving(RustcEncodable, RustcDecodable, Show, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[deriving(Show, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 enum Kind {
     /// Kind::Git(<git reference>) 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<CargoError + Send>;
 
 /// 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<S: hash::Writer> hash::Hash<S> 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<String> {
+        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<SourceId, Box<Source+'src>>
 }
@@ -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);
     }
 }

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())

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<S: 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<S: Str>(&self, path: &Path, reference: S)
-                           -> CargoResult<GitRevision> {
+    pub fn rev_for(&self, path: &Path, reference: &GitReference)
+                   -> CargoResult<GitRevision> {
         let db = try!(self.db_at(path));
         db.rev_for(reference)
     }
@@ -215,9 +179,36 @@ impl GitDatabase {
         Ok(checkout)
     }
 
-    pub fn rev_for<S: Str>(&self, reference: S) -> CargoResult<GitRevision> {
-        let rev = try!(self.repo.revparse_single(reference.as_slice()));
-        Ok(GitRevision(rev.id().to_string()))
+    pub fn rev_for(&self, reference: &GitReference) -> CargoResult<GitRevision> {
+        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<S: Str>(&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<git2::Repository> {
         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(())
     }