refactor: replace hex-string object ids with typed Oid in Anchor
commit
ad82e48refactor: replace hex-string object ids with typed Oid in Anchor
Anchor.commit and Anchor.blob were bare Strings a caller could confuse with an arbitrary revision or pass around unvalidated. gix::ObjectId can’t implement Facet directly (orphan rule, foreign type), so git-anchor gains a local Oid newtype, transparent over the same hex String on disk (verified byte-for-byte by git-comment’s loads_the_on_disk_comment_format fixture test) but convertible to and from gix::ObjectId everywhere it is actually used.
capture/snippet/project now carry a real ObjectId internally instead of round-tripping through from_hex at every call site; anchor.commit resolution no longer goes through revision-string parsing since it already names a concrete object.
deps: patch facet-git-tree to a local checkout with transparent-newtype support, pending upstream release Assisted-by: Claude:claude-sonnet-5
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
Cargo.lock
@@ -1136,7 +1136,6 @@
[[package]]
name = "facet-git-tree"
version = "0.1.0"
-source = "git+https://github.com/git-ents/facet-git-tree#43f5298e489eefa3f6dd9238735e9975f533eb9c"
dependencies = [
"facet",
"gix-hash",
Cargo.toml
@@ -73,6 +73,13 @@
] }
uuid = { version = "1", features = ["v4"] }
+# Pending a push of the local facet-git-tree fix that adds transparent-newtype
+# support (`#[facet(transparent)]` wrappers now unwrap through
+# `Peek::innermost_peek`/`Partial::begin_inner` instead of encoding as a
+# one-field struct tree), needed to store `gix::ObjectId` newtypes directly.
+[patch."https://github.com/git-ents/facet-git-tree"]
+facet-git-tree = { path = "../facet-git-tree/crates/facet-git-tree" }
+
# These lint configurations were originally pulled from [Evan Schwartz][1].
# [1]: https://emschwartz.me/your-clippy-config-should-be-stricter/
[workspace.lints.clippy]
crates/git-anchor/src/lib.rs
@@ -25,6 +25,41 @@
use gix::diff::blob::{Algorithm, Diff, InternedInput};
use gix::diff::tree_with_rewrites::Change;
+/// A content-addressed object id, stored on disk as its 40-character hex text
+/// (identical to a bare `String` field, via `facet_git_tree`'s
+/// transparent-newtype support) and used everywhere else as gitoxide's own
+/// [`ObjectId`] — so an [`Anchor`] never carries a hex string a caller could
+/// mistake for an arbitrary revision.
+#[derive(Debug, Clone, PartialEq, Eq, Facet)]
+#[facet(transparent)]
+pub struct Oid(String);
+
+impl std::fmt::Display for Oid {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.write_str(&self.0)
+ }
+}
+
+impl From<ObjectId> for Oid {
+ fn from(id: ObjectId) -> Self {
+ Self(id.to_string())
+ }
+}
+
+impl From<&str> for Oid {
+ fn from(hex: &str) -> Self {
+ Self(hex.to_owned())
+ }
+}
+
+impl TryFrom<&Oid> for ObjectId {
+ type Error = gix::hash::decode::Error;
+
+ fn try_from(oid: &Oid) -> Result<Self, Self::Error> {
+ ObjectId::from_hex(oid.0.as_bytes())
+ }
+}
+
/// A failure opening the repository or resolving the objects an anchor names.
#[derive(Debug, thiserror::Error)]
pub enum Error {
@@ -44,7 +79,7 @@
#[error("no file at {path:?} in {commit}")]
MissingPath {
/// The commit the path was looked up in.
- commit: String,
+ commit: ObjectId,
/// The path that is not a file there.
path: String,
},
@@ -76,13 +111,13 @@
/// read-time view derived from this record.
#[derive(Debug, Clone, PartialEq, Eq, Facet)]
pub struct Anchor {
- /// The commit the anchor was created against, as a hex object id.
- pub commit: String,
+ /// The commit the anchor was created against.
+ pub commit: Oid,
/// The repository-relative path of the anchored file at that commit.
pub path: String,
- /// The object id of the anchored file's blob, as hex — an integrity check
- /// and the fast path for "has this file changed at all".
- pub blob: String,
+ /// The object id of the anchored file's blob — an integrity check and the
+ /// fast path for "has this file changed at all".
+ pub blob: Oid,
/// The anchored lines, or `None` for a whole-file anchor.
pub lines: Option<LineRange>,
}
@@ -124,7 +159,7 @@
) -> Result<Anchor, Error> {
let repo = gix::open(repo).map_err(|error| Error::Open(Box::new(error)))?;
let commit = resolve_commit(&repo, revision)?;
- let commit_id = commit.id().to_string();
+ let commit_id = commit.id().detach();
let tree = commit
.tree()
.map_err(|error| Error::Object(error.to_string()))?;
@@ -133,7 +168,7 @@
.map_err(|error| Error::Object(error.to_string()))?
.filter(|entry| entry.mode().is_blob())
.ok_or_else(|| Error::MissingPath {
- commit: commit_id.clone(),
+ commit: commit_id,
path: path.to_owned(),
})?;
let blob = entry.object_id();
@@ -142,9 +177,9 @@
lines_of(&data, path, range)?;
}
Ok(Anchor {
- commit: commit_id,
+ commit: commit_id.into(),
path: path.to_owned(),
- blob: blob.to_string(),
+ blob: blob.into(),
lines,
})
}
@@ -154,8 +189,8 @@
/// names, so it can never disagree with what was anchored.
pub fn snippet(repo: &Path, anchor: &Anchor) -> Result<String, Error> {
let repo = gix::open(repo).map_err(|error| Error::Open(Box::new(error)))?;
- let blob = ObjectId::from_hex(anchor.blob.as_bytes())
- .map_err(|_error| Error::Resolve(anchor.blob.clone()))?;
+ let blob = ObjectId::try_from(&anchor.blob)
+ .map_err(|_error| Error::Resolve(anchor.blob.to_string()))?;
let data = read_blob(&repo, blob)?;
match anchor.lines {
None => Ok(String::from_utf8_lossy(&data).into_owned()),
@@ -192,8 +227,10 @@
/// land entirely outside it, [`Projection::Outdated`] when an edit touches it.
pub fn project(repo: &Path, anchor: &Anchor, target: &str) -> Result<Projection, Error> {
let repo = gix::open(repo).map_err(|error| Error::Open(Box::new(error)))?;
- let anchor_blob = ObjectId::from_hex(anchor.blob.as_bytes())
- .map_err(|_error| Error::Resolve(anchor.blob.clone()))?;
+ let anchor_blob = ObjectId::try_from(&anchor.blob)
+ .map_err(|_error| Error::Resolve(anchor.blob.to_string()))?;
+ let anchor_commit_id = ObjectId::try_from(&anchor.commit)
+ .map_err(|_error| Error::Resolve(anchor.commit.to_string()))?;
let target_commit = resolve_commit(&repo, target)?;
let target_tree = target_commit
.tree()
@@ -208,7 +245,7 @@
return Ok(Projection::Current);
}
- let anchor_commit = resolve_commit(&repo, &anchor.commit)?;
+ let anchor_commit = commit_at(&repo, anchor_commit_id)?;
let anchor_tree = anchor_commit
.tree()
.map_err(|error| Error::Object(error.to_string()))?;
@@ -260,7 +297,7 @@
// the anchor's blob is not what its own commit holds there, so the
// anchor itself is broken.
return Err(Error::MissingPath {
- commit: anchor.commit.clone(),
+ commit: anchor_commit_id,
path: anchor.path.clone(),
});
};
@@ -305,6 +342,19 @@
.map_err(|_error| resolve())
}
+/// Look up the commit `id` names directly, with no revision parsing — for an
+/// [`Anchor`]'s own `commit`, which already names a concrete object rather
+/// than an arbitrary revision.
+fn commit_at(repo: &gix::Repository, id: ObjectId) -> Result<gix::Commit<'_>, Error> {
+ let resolve = || Error::Resolve(id.to_string());
+ repo.find_object(id)
+ .map_err(|_error| resolve())?
+ .peel_to_kind(gix::object::Kind::Commit)
+ .map_err(|_error| resolve())?
+ .try_into_commit()
+ .map_err(|_error| resolve())
+}
+
/// Read the full contents of the blob at `id`.
fn read_blob(repo: &gix::Repository, id: ObjectId) -> Result<Vec<u8>, Error> {
Ok(repo
@@ -383,10 +433,10 @@
commit_all(dir.path(), "one");
let anchor = capture(dir.path(), "HEAD", "file.txt", range(3, 4)).unwrap();
- assert_eq!(anchor.commit, head(dir.path()));
+ assert_eq!(anchor.commit.to_string(), head(dir.path()));
assert_eq!(anchor.path, "file.txt");
assert_eq!(anchor.lines, range(3, 4));
- assert!(!anchor.blob.is_empty());
+ assert!(!anchor.blob.to_string().is_empty());
assert_eq!(snippet(dir.path(), &anchor).unwrap(), "line 3\nline 4\n");
}
crates/git-comment/src/lib.rs
@@ -118,9 +118,9 @@
Comment {
body: body.to_owned(),
anchor: Anchor {
- commit: "0123456789abcdef0123456789abcdef01234567".to_owned(),
+ commit: "0123456789abcdef0123456789abcdef01234567".into(),
path: "src/lib.rs".to_owned(),
- blob: "89abcdef0123456789abcdef0123456789abcdef".to_owned(),
+ blob: "89abcdef0123456789abcdef0123456789abcdef".into(),
lines: Some(LineRange { start: 3, end: 4 }),
},
issue: issue.map(str::to_owned),
@@ -258,9 +258,9 @@
100644 blob {}\tpath\n\
100644 blob {}\tblob\n\
040000 tree {lines_tree}\tlines\n",
- blob(&expected.anchor.commit),
+ blob(&expected.anchor.commit.to_string()),
blob(&expected.anchor.path),
- blob(&expected.anchor.blob),
+ blob(&expected.anchor.blob.to_string()),
),
);
let issue_tree = git_with_stdin(
crates/git-ents/src/main.rs
@@ -474,7 +474,7 @@
println!(
"anchor {} @ {}",
location(&comment.anchor.path, comment.anchor.lines),
- short_id(&comment.anchor.commit)
+ short_id(&comment.anchor.commit.to_string())
);
println!("on {rev}: {}", describe_projection(&repo, &comment, rev));
if let Some(issue) = &comment.issue {