Auto merge of #13250 - weihanglo:cargo-update, r=ehuss

bors · bors · commit 350098d5ef2a · 2024-01-15T17:57:47.000Z
fix(cargo-update): `--precise` accept arbitrary git revisions
diff --git a/src/cargo/core/source_id.rs b/src/cargo/core/source_id.rs
@@ -461,22 +461,11 @@ impl SourceId {
 
     pub fn precise_git_fragment(self) -> Option<&'static str> {
         match &self.inner.precise {
-            Some(Precise::GitUrlFragment(s)) => Some(&s[..8]),
+            Some(Precise::GitUrlFragment(s)) => Some(&s),
             _ => None,
         }
     }
 
-    pub fn precise_git_oid(self) -> CargoResult<Option<git2::Oid>> {
-        Ok(match self.inner.precise.as_ref() {
-            Some(Precise::GitUrlFragment(s)) => {
-                Some(git2::Oid::from_str(s).with_context(|| {
-                    format!("precise value for git is not a git revision: {}", s)
-                })?)
-            }
-            _ => None,
-        })
-    }
-
     /// Creates a new `SourceId` from this source with the given `precise`.
     pub fn with_git_precise(self, fragment: Option<String>) -> SourceId {
         SourceId::wrap(SourceIdInner {
diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs
@@ -167,7 +167,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
                 format!(
                     "{} -> #{}",
                     removed[0],
-                    &added[0].source_id().precise_git_fragment().unwrap()
+                    &added[0].source_id().precise_git_fragment().unwrap()[..8],
                 )
             } else {
                 format!("{} -> v{}", removed[0], added[0].version())
diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs
@@ -68,11 +68,10 @@ use url::Url;
 pub struct GitSource<'cfg> {
     /// The git remote which we're going to fetch from.
     remote: GitRemote,
-    /// The Git reference from the manifest file.
-    manifest_reference: GitReference,
     /// The revision which a git source is locked to.
-    /// This is expected to be set after the Git repository is fetched.
-    locked_rev: Option<git2::Oid>,
+    ///
+    /// Expected to always be [`Revision::Locked`] after the Git repository is fetched.
+    locked_rev: Revision,
     /// The unique identifier of this source.
     source_id: SourceId,
     /// The underlying path source to discover packages inside the Git repository.
@@ -102,8 +101,12 @@ impl<'cfg> GitSource<'cfg> {
         assert!(source_id.is_git(), "id is not git, id={}", source_id);
 
         let remote = GitRemote::new(source_id.url());
-        let manifest_reference = source_id.git_reference().unwrap().clone();
-        let locked_rev = source_id.precise_git_oid()?;
+        // Fallback to git ref from mainfest if there is no locked revision.
+        let locked_rev = source_id
+            .precise_git_fragment()
+            .map(|s| Revision::new(s.into()))
+            .unwrap_or_else(|| source_id.git_reference().unwrap().clone().into());
+
         let ident = ident_shallow(
             &source_id,
             config
@@ -114,7 +117,6 @@ impl<'cfg> GitSource<'cfg> {
 
         let source = GitSource {
             remote,
-            manifest_reference,
             locked_rev,
             source_id,
             path_source: None,
@@ -155,6 +157,48 @@ impl<'cfg> GitSource<'cfg> {
     }
 }
 
+/// Indicates a [Git revision] that might be locked or deferred to be resolved.
+///
+/// [Git revision]: https://git-scm.com/docs/revisions
+#[derive(Clone, Debug)]
+enum Revision {
+    /// A [Git reference] that would trigger extra fetches when being resolved.
+    ///
+    /// [Git reference]: https://git-scm.com/book/en/v2/Git-Internals-Git-References
+    Deferred(GitReference),
+    /// A locked revision of the actual Git commit object ID.
+    Locked(git2::Oid),
+}
+
+impl Revision {
+    fn new(rev: &str) -> Revision {
+        let oid = git2::Oid::from_str(rev).ok();
+        match oid {
+            // Git object ID is supposed to be a hex string of 20 (SHA1) or 32 (SHA256) bytes.
+            // Its length must be double to the underlying bytes (40 or 64),
+            // otherwise libgit2 would happily zero-pad the returned oid.
+            // See rust-lang/cargo#13188
+            Some(oid) if oid.as_bytes().len() * 2 == rev.len() => Revision::Locked(oid),
+            _ => Revision::Deferred(GitReference::Rev(rev.to_string())),
+        }
+    }
+}
+
+impl From<GitReference> for Revision {
+    fn from(value: GitReference) -> Self {
+        Revision::Deferred(value)
+    }
+}
+
+impl From<Revision> for GitReference {
+    fn from(value: Revision) -> Self {
+        match value {
+            Revision::Deferred(git_ref) => git_ref,
+            Revision::Locked(oid) => GitReference::Rev(oid.to_string()),
+        }
+    }
+}
+
 /// Create an identifier from a URL,
 /// essentially turning `proto://host/path/repo` into `repo-<hash-of-url>`.
 fn ident(id: &SourceId) -> String {
@@ -191,9 +235,12 @@ impl<'cfg> Debug for GitSource<'cfg> {
         // TODO(-Znext-lockfile-bump): set it to true when stabilizing
         // lockfile v4, because we want Source ID serialization to be
         // consistent with lockfile.
-        match self.manifest_reference.pretty_ref(false) {
-            Some(s) => write!(f, " ({})", s),
-            None => Ok(()),
+        match &self.locked_rev {
+            Revision::Deferred(git_ref) => match git_ref.pretty_ref(false) {
+                Some(s) => write!(f, " ({})", s),
+                None => Ok(()),
+            },
+            Revision::Locked(oid) => write!(f, " ({oid})"),
         }
     }
 }
@@ -252,16 +299,17 @@ impl<'cfg> Source for GitSource<'cfg> {
         let db_path = db_path.into_path_unlocked();
 
         let db = self.remote.db_at(&db_path).ok();
-        let (db, actual_rev) = match (self.locked_rev, db) {
+
+        let (db, actual_rev) = match (&self.locked_rev, db) {
             // If we have a locked revision, and we have a preexisting database
             // which has that revision, then no update needs to happen.
-            (Some(rev), Some(db)) if db.contains(rev) => (db, rev),
+            (Revision::Locked(oid), Some(db)) if db.contains(*oid) => (db, *oid),
 
             // If we're in offline mode, we're not locked, and we have a
             // 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(|| {
+            (Revision::Deferred(git_ref), Some(db)) if self.config.offline() => {
+                let rev = db.resolve(&git_ref).with_context(|| {
                     "failed to lookup reference in preexisting repository, and \
                          can't check for updates in offline mode (--offline)"
                 })?;
@@ -279,6 +327,7 @@ impl<'cfg> Source for GitSource<'cfg> {
                         self.remote.url()
                     );
                 }
+
                 if !self.quiet {
                     self.config.shell().status(
                         "Updating",
@@ -288,13 +337,9 @@ impl<'cfg> Source for GitSource<'cfg> {
 
                 trace!("updating git source `{:?}`", self.remote);
 
-                self.remote.checkout(
-                    &db_path,
-                    db,
-                    &self.manifest_reference,
-                    locked_rev,
-                    self.config,
-                )?
+                let locked_rev = locked_rev.clone().into();
+                self.remote
+                    .checkout(&db_path, db, &locked_rev, self.config)?
             }
         };
 
@@ -321,7 +366,7 @@ impl<'cfg> Source for GitSource<'cfg> {
 
         self.path_source = Some(path_source);
         self.short_id = Some(short_id.as_str().into());
-        self.locked_rev = Some(actual_rev);
+        self.locked_rev = Revision::Locked(actual_rev);
         self.path_source.as_mut().unwrap().update()?;
 
         // Hopefully this shouldn't incur too much of a performance hit since
@@ -350,7 +395,10 @@ impl<'cfg> Source for GitSource<'cfg> {
     }
 
     fn fingerprint(&self, _pkg: &Package) -> CargoResult<String> {
-        Ok(self.locked_rev.as_ref().unwrap().to_string())
+        match &self.locked_rev {
+            Revision::Locked(oid) => Ok(oid.to_string()),
+            _ => unreachable!("locked_rev must be resolved when computing fingerprint"),
+        }
     }
 
     fn describe(&self) -> String {
diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs
@@ -95,8 +95,6 @@ impl GitRemote {
     /// This ensures that it gets the up-to-date commit when a named reference
     /// is given (tag, branch, refs/*). Thus, network connection is involved.
     ///
-    /// When `locked_rev` is provided, it takes precedence over `reference`.
-    ///
     /// If we have a previous instance of [`GitDatabase`] then fetch into that
     /// if we can. If that can successfully load our revision then we've
     /// populated the database with the latest version of `reference`, so
@@ -106,11 +104,8 @@ impl GitRemote {
         into: &Path,
         db: Option<GitDatabase>,
         reference: &GitReference,
-        locked_rev: Option<git2::Oid>,
         cargo_config: &Config,
     ) -> CargoResult<(GitDatabase, git2::Oid)> {
-        let locked_ref = locked_rev.map(|oid| GitReference::Rev(oid.to_string()));
-        let reference = locked_ref.as_ref().unwrap_or(reference);
         if let Some(mut db) = db {
             fetch(
                 &mut db.repo,
@@ -121,11 +116,7 @@ impl GitRemote {
             )
             .with_context(|| format!("failed to fetch into: {}", into.display()))?;
 
-            let resolved_commit_hash = match locked_rev {
-                Some(rev) => db.contains(rev).then_some(rev),
-                None => resolve_ref(reference, &db.repo).ok(),
-            };
-            if let Some(rev) = resolved_commit_hash {
+            if let Some(rev) = resolve_ref(reference, &db.repo).ok() {
                 return Ok((db, rev));
             }
         }
@@ -146,10 +137,7 @@ impl GitRemote {
             RemoteKind::GitDependency,
         )
         .with_context(|| format!("failed to clone into: {}", into.display()))?;
-        let rev = match locked_rev {
-            Some(rev) => rev,
-            None => resolve_ref(reference, &repo)?,
-        };
+        let rev = resolve_ref(reference, &repo)?;
 
         Ok((
             GitDatabase {
diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs
@@ -757,13 +757,11 @@ fn update_with_shared_deps() {
         .with_status(101)
         .with_stderr(
             "\
+[UPDATING] git repository [..]
 [ERROR] Unable to update [..]
 
 Caused by:
-  precise value for git is not a git revision: 0.1.2
-
-Caused by:
-  unable to parse OID - contains invalid characters; class=Invalid (3)
+  revspec '0.1.2' not found; class=Reference (4); code=NotFound (-3)
 ",
         )
         .run();
diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs
@@ -1272,3 +1272,101 @@ rustdns.workspace = true
         )
         .run();
 }
+
+#[cargo_test]
+fn update_precise_git_revisions() {
+    let (git_project, git_repo) = git::new_repo("git", |p| {
+        p.file("Cargo.toml", &basic_lib_manifest("git"))
+            .file("src/lib.rs", "")
+    });
+    let tag_name = "Nazgûl";
+    git::tag(&git_repo, tag_name);
+    let tag_commit_id = git_repo.head().unwrap().target().unwrap().to_string();
+
+    git_project.change_file("src/lib.rs", "fn f() {}");
+    git::add(&git_repo);
+    let head_id = git::commit(&git_repo).to_string();
+    let short_id = &head_id[..8];
+    let url = git_project.url();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            &format!(
+                r#"
+                    [package]
+                    name = "foo"
+                    version = "0.1.0"
+
+                    [dependencies]
+                    git = {{ git = '{url}' }}
+                "#
+            ),
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("fetch")
+        .with_stderr(format!("[UPDATING] git repository `{url}`"))
+        .run();
+
+    assert!(p.read_lockfile().contains(&head_id));
+
+    p.cargo("update git --precise")
+        .arg(tag_name)
+        .with_stderr(format!(
+            "\
+[UPDATING] git repository `{url}`
+[UPDATING] git v0.5.0 ([..]) -> #{}",
+            &tag_commit_id[..8],
+        ))
+        .run();
+
+    assert!(p.read_lockfile().contains(&tag_commit_id));
+    assert!(!p.read_lockfile().contains(&head_id));
+
+    p.cargo("update git --precise")
+        .arg(short_id)
+        .with_stderr(format!(
+            "\
+[UPDATING] git repository `{url}`
+[UPDATING] git v0.5.0 ([..]) -> #{short_id}",
+        ))
+        .run();
+
+    assert!(p.read_lockfile().contains(&head_id));
+    assert!(!p.read_lockfile().contains(&tag_commit_id));
+
+    // updating back to tag still requires a git fetch,
+    // as the ref may change over time.
+    p.cargo("update git --precise")
+        .arg(tag_name)
+        .with_stderr(format!(
+            "\
+[UPDATING] git repository `{url}`
+[UPDATING] git v0.5.0 ([..]) -> #{}",
+            &tag_commit_id[..8],
+        ))
+        .run();
+
+    assert!(p.read_lockfile().contains(&tag_commit_id));
+    assert!(!p.read_lockfile().contains(&head_id));
+
+    // Now make a tag looks like an oid.
+    // It requires a git fetch, as the oid cannot be found in preexisting git db.
+    let arbitrary_tag: String = std::iter::repeat('a').take(head_id.len()).collect();
+    git::tag(&git_repo, &arbitrary_tag);
+
+    p.cargo("update git --precise")
+        .arg(&arbitrary_tag)
+        .with_stderr(format!(
+            "\
+[UPDATING] git repository `{url}`
+[UPDATING] git v0.5.0 ([..]) -> #{}",
+            &head_id[..8],
+        ))
+        .run();
+
+    assert!(p.read_lockfile().contains(&head_id));
+    assert!(!p.read_lockfile().contains(&tag_commit_id));
+}

Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes`
`167`	`167`	`format!(`
`168`	`168`	`"{} -> #{}",`
`169`	`169`	`removed[0],`
`170`		`- &added[0].source_id().precise_git_fragment().unwrap()`
	`170`	`+ &added[0].source_id().precise_git_fragment().unwrap()[..8],`
`171`	`171`	`)`
`172`	`172`	`} else {`
`173`	`173`	`format!("{} -> v{}", removed[0], added[0].version())`