feat: anchor a comment commit to the code it annotates
commit
80d23b9feat: anchor a comment commit to the code it annotates
git-store store/store_authored gain an extra-parents parameter so a document commit can carry other history it depends on, alongside the document own chain; a chain-parent header disambiguates the two only when a genesis commit needs one (the common case needs no header, so every other document format is untouched). git-comment now passes the anchor commit as an extra parent, closing the gap between abstractions.adoc’s stated reachability invariant and what git-store actually wrote.
feat: git-store store_with_parents/store_authored_with_parents/store_item_authored_with_parents feat: git-comment stores the anchored commit as a second parent docs: add anchor.reachability requirement to docs/spec/anchor.adoc Assisted-by: Claude:claude-sonnet-5
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
Cargo.lock
@@ -1455,6 +1455,7 @@
"facet",
"git-anchor",
"git-store",
+ "gix",
]
[[package]]
crates/git-comment/Cargo.toml
@@ -9,6 +9,7 @@
facet = { workspace = true }
git-anchor = { workspace = true }
git-store = { workspace = true }
+gix = { workspace = true }
[dev-dependencies]
git-store = { workspace = true, features = ["test-support"] }
docs/spec/anchor.adoc
@@ -32,3 +32,11 @@
An outdated or deleted projection MUST NOT lose the comment: the original
anchor remains displayable.
--
+
+[role="requirement", id="anchor.reachability"]
+.Anchor Reachability
+--
+The meta-ref commit storing an anchor MUST carry the anchored commit as an
+extra parent, so the anchored commit is reachable from `refs/meta/*` and
+survives force-push, branch deletion, and gc without any gc special-casing.
+--
crates/git-comment/src/lib.rs
@@ -27,6 +27,7 @@
use facet::Facet;
use git_anchor::{Anchor, Projection};
use git_store::Provenance;
+use gix::ObjectId;
// @relation(comments.ref)
/// The namespace under which comments are recorded: one ref,
@@ -66,23 +67,29 @@
/// Write `comment` to `refs/meta/comments/<id>` in `repo` as a new commit
/// authored by `author` (a `(name, email)` pair), so the ref's commit chain is
-/// the comment's edit history and carries its authorship.
+/// the comment's edit history and carries its authorship. The commit also
+/// carries the anchored commit as a second parent, so the commit the comment
+/// annotates stays reachable — and so cannot be garbage-collected — for as
+/// long as the comment's ref exists.
///
/// ## Requirements
///
-/// @relation(comments.ref, comments.authorship)
+/// @relation(comments.ref, comments.authorship, anchor.reachability)
pub fn store(
repo: &Path,
id: &str,
comment: &Comment,
author: (&str, &str),
) -> Result<(), git_store::Error> {
- git_store::Store::open(repo)?.store_item_authored(
+ let anchored = ObjectId::try_from(&comment.anchor.commit)
+ .map_err(|error| git_store::Error::Invalid(error.to_string()))?;
+ git_store::Store::open(repo)?.store_item_authored_with_parents(
COMMENTS_NS,
id,
comment,
"Update comment",
author,
+ &[anchored],
)
}
@@ -210,6 +217,42 @@
assert!(provenance.created.seconds > 0);
}
+ // @relation(anchor.reachability, role=Verifies)
+ #[test]
+ fn a_stored_comment_carries_its_anchored_commit_as_a_second_parent() {
+ let dir = repo();
+ std::fs::write(dir.path().join("file.txt"), "one\ntwo\nthree\n").unwrap();
+ commit_all(dir.path(), "one");
+
+ let anchor = git_anchor::capture(
+ dir.path(),
+ "HEAD",
+ "file.txt",
+ Some(LineRange { start: 2, end: 2 }),
+ )
+ .unwrap();
+ let anchored_commit = anchor.commit.to_string();
+ let written = Comment {
+ body: "Why two?".to_owned(),
+ anchor,
+ issue: None,
+ };
+ let id = new_id(None, &written).unwrap();
+ store(dir.path(), &id, &written, AUTHOR).unwrap();
+
+ let is_ancestor = Command::new("git")
+ .current_dir(dir.path())
+ .args([
+ "merge-base",
+ "--is-ancestor",
+ &anchored_commit,
+ &format!("{COMMENTS_NS}/{id}"),
+ ])
+ .status()
+ .unwrap();
+ assert!(is_ancestor.success());
+ }
+
// @relation(comments.anchor, comments.projection, role=Verifies)
#[test]
fn a_stored_comment_projects_onto_the_commit_it_was_written_against() {
crates/git-store/src/lib.rs
@@ -178,7 +178,23 @@
value: &T,
message: &str,
) -> Result<(), Error> {
- self.store_impl(refname, value, message, None)
+ self.store_impl(refname, value, message, None, &[])
+ }
+
+ /// Like [`store`](Self::store), but also parenting the written commit on
+ /// each of `extra_parents` (beyond the ref's own prior tip), so the
+ /// commit stays reachability-anchored to other history it depends on —
+ /// e.g. a comment's commit carries the commit it annotates as a second
+ /// parent, so the annotated commit can never be garbage-collected out
+ /// from under it.
+ pub fn store_with_parents<T: for<'a> Facet<'a>>(
+ &self,
+ refname: &str,
+ value: &T,
+ message: &str,
+ extra_parents: &[ObjectId],
+ ) -> Result<(), Error> {
+ self.store_impl(refname, value, message, None, extra_parents)
}
/// Like [`store`](Self::store), but attributing authorship to `author`
@@ -192,7 +208,21 @@
message: &str,
author: (&str, &str),
) -> Result<(), Error> {
- self.store_impl(refname, value, message, Some(author))
+ self.store_impl(refname, value, message, Some(author), &[])
+ }
+
+ /// Like [`store_authored`](Self::store_authored), but also parenting the
+ /// written commit on each of `extra_parents`, per
+ /// [`store_with_parents`](Self::store_with_parents).
+ pub fn store_authored_with_parents<T: for<'a> Facet<'a>>(
+ &self,
+ refname: &str,
+ value: &T,
+ message: &str,
+ author: (&str, &str),
+ extra_parents: &[ObjectId],
+ ) -> Result<(), Error> {
+ self.store_impl(refname, value, message, Some(author), extra_parents)
}
/// ## Requirements
@@ -204,12 +234,16 @@
value: &T,
message: &str,
author: Option<(&str, &str)>,
+ extra_parents: &[ObjectId],
) -> Result<(), Error> {
let mut expected = self.ref_commit(refname)?;
let mut tree = facet_git_tree::serialize_into(value, &self.odb)?;
for _ in 0..=MAX_MERGE_RETRIES {
- let parents = expected.into_iter().collect();
- let commit = self.write_commit(tree, parents, message, author)?;
+ let parents = expected
+ .into_iter()
+ .chain(extra_parents.iter().copied())
+ .collect();
+ let commit = self.write_commit(tree, parents, expected, message, author)?;
match self.try_set_ref(refname, expected, commit) {
Ok(()) => return Ok(()),
Err(Error::Conflict) => {
@@ -251,11 +285,14 @@
) -> Result<(), Error> {
let tree = facet_git_tree::serialize_into(value, &self.odb)?;
let expected = self.ref_commit(refname)?;
- let parents = match &expected {
- Some(tip) => self.read_commit(tip)?.parents,
- None => Vec::new(),
+ let (parents, chain_parent) = match &expected {
+ Some(tip) => {
+ let tip = self.read_commit(tip)?;
+ (tip.parents, tip.chain_parent)
+ }
+ None => (Vec::new(), None),
};
- let commit = self.write_commit(tree, parents, message, None)?;
+ let commit = self.write_commit(tree, parents, chain_parent, message, None)?;
self.try_set_ref(refname, expected, commit)
}
@@ -267,7 +304,7 @@
pub fn store_tree(&self, refname: &str, tree: ObjectId, message: &str) -> Result<(), Error> {
let expected = self.ref_commit(refname)?;
let parents = expected.into_iter().collect();
- let commit = self.write_commit(tree, parents, message, None)?;
+ let commit = self.write_commit(tree, parents, expected, message, None)?;
self.try_set_ref(refname, expected, commit)
}
@@ -333,6 +370,27 @@
self.store_authored(&item_ref(prefix, id)?, value, message, author)
}
+ /// Like [`store_item_authored`](Self::store_item_authored), but also
+ /// parenting the written commit on each of `extra_parents`, per
+ /// [`store_with_parents`](Self::store_with_parents).
+ pub fn store_item_authored_with_parents<T: for<'a> Facet<'a>>(
+ &self,
+ prefix: &str,
+ id: &str,
+ value: &T,
+ message: &str,
+ author: (&str, &str),
+ extra_parents: &[ObjectId],
+ ) -> Result<(), Error> {
+ self.store_authored_with_parents(
+ &item_ref(prefix, id)?,
+ value,
+ message,
+ author,
+ extra_parents,
+ )
+ }
+
/// Like [`store_item`](Self::store_item), but for a [`HasId`] value that
/// carries its own collection key, so the caller does not pass it twice.
pub fn store_keyed<T: for<'a> Facet<'a> + HasId>(
@@ -418,7 +476,7 @@
Err(facet_git_tree::Error::Message(_)) => break,
Err(error) => return Err(error.into()),
}
- cursor = commit.parents.into_iter().next();
+ cursor = commit.chain_parent;
}
Ok(out)
}
@@ -436,7 +494,7 @@
};
let mut commit = self.read_commit(&tip)?;
let updated = commit.author.clone();
- while let Some(parent) = commit.parents.first().copied() {
+ while let Some(parent) = commit.chain_parent {
commit = self.read_commit(&parent)?;
}
Ok(Some(Provenance {
@@ -491,8 +549,8 @@
}
}
- /// Read `oid`'s tree, parents, author, and committer date from the
- /// durable store.
+ /// Read `oid`'s tree, parents, chain parent, author, and committer date
+ /// from the durable store.
fn read_commit(&self, oid: &ObjectId) -> Result<CommitFacts, Error> {
let mut buffer = Vec::new();
let commit = self
@@ -506,9 +564,24 @@
let author = commit
.author()
.map_err(|error| Error::Object(error.to_string()))?;
+ let chain_parent = commit
+ .extra_headers()
+ .find(CHAIN_PARENT_HEADER)
+ .map(|value| {
+ if value.is_empty() {
+ Ok(None)
+ } else {
+ ObjectId::from_hex(value)
+ .map(Some)
+ .map_err(|error| Error::Object(error.to_string()))
+ }
+ })
+ .transpose()?
+ .unwrap_or_else(|| commit.parents().next());
Ok(CommitFacts {
tree: commit.tree(),
parents: commit.parents().collect(),
+ chain_parent,
seconds: u64::try_from(seconds).unwrap_or(0),
author: Authorship {
name: author.name.to_string(),
@@ -518,13 +591,19 @@
})
}
- /// Wrap `tree` in a commit over `parents` and write it to the durable store.
- /// The committer is always the git-ents system identity; `author` overrides
- /// the authorship when set, otherwise it too is the system identity.
+ /// Wrap `tree` in a commit over `parents` and write it to the durable
+ /// store, recording `chain_parent` (the document's own prior state, as
+ /// opposed to any other parent riding along for reachability, e.g. an
+ /// anchored commit) in a header when `parents` holds more than just it —
+ /// otherwise the chain and the parent list agree and no header is needed.
+ /// The committer is always the git-ents system identity; `author`
+ /// overrides the authorship when set, otherwise it too is the system
+ /// identity.
fn write_commit(
&self,
tree: ObjectId,
parents: Vec<ObjectId>,
+ chain_parent: Option<ObjectId>,
message: &str,
author: Option<(&str, &str)>,
) -> Result<ObjectId, Error> {
@@ -542,6 +621,16 @@
},
None => committer.clone(),
};
+ // The chain-parent header is only needed when the plain "first
+ // parent is the chain" convention would recover the wrong thing —
+ // i.e. a genesis commit (no chain parent) that still carries an
+ // extra parent, which would otherwise occupy the first slot.
+ let extra_headers = if parents.first().copied() == chain_parent {
+ Vec::new()
+ } else {
+ let value = chain_parent.map(|oid| oid.to_string()).unwrap_or_default();
+ vec![(CHAIN_PARENT_HEADER.into(), value.into())]
+ };
let commit = Commit {
tree,
parents: parents.into(),
@@ -549,7 +638,7 @@
committer,
encoding: None,
message: message.into(),
- extra_headers: Vec::new(),
+ extra_headers,
};
self.odb
.write(&commit)
@@ -590,6 +679,14 @@
/// contention; ordinary racing writers resolve within one or two rounds.
const MAX_MERGE_RETRIES: usize = 5;
+/// The commit header recording a document's chain parent explicitly, written
+/// only when the plain "first parent is the chain" convention would recover
+/// the wrong thing: a genesis commit (no prior document state) that still
+/// carries an extra parent for reachability (<<anchor.reachability>>), which
+/// would otherwise occupy the first — and only — parent slot. An empty value
+/// means the chain has no parent at all (this commit is the genesis).
+const CHAIN_PARENT_HEADER: &str = "chain-parent";
+
/// The ref name for item `id` under the collection namespace `prefix`
/// (`{prefix}/{id}`), rejecting an `id` that fails [`ref_segment_ok`] since it
/// becomes the ref's last path segment.
@@ -621,11 +718,13 @@
pub updated: Authorship,
}
-/// The facts read off a commit: its tree, its parents, its author, and its
-/// committer date.
+/// The facts read off a commit: its tree, its parents, its chain parent (the
+/// document's own prior state, distinct from any other parent riding along
+/// for reachability), its author, and its committer date.
struct CommitFacts {
tree: ObjectId,
parents: Vec<ObjectId>,
+ chain_parent: Option<ObjectId>,
seconds: u64,
author: Authorship,
}
@@ -1107,7 +1206,7 @@
// A write built from the now-stale snapshot loses the CAS race.
let tree = facet_git_tree::serialize_into(&"third".to_string(), &store.odb).unwrap();
let commit = store
- .write_commit(tree, stale.into_iter().collect(), "write", None)
+ .write_commit(tree, stale.into_iter().collect(), stale, "write", None)
.unwrap();
let result = store.try_set_ref(refname, stale, commit);
assert!(matches!(result, Err(Error::Conflict)));
@@ -1128,7 +1227,9 @@
// Our write, built assuming the ref was still absent, has no common
// ancestor with theirs and so cannot be merged.
let tree = facet_git_tree::serialize_into(&"ours".to_string(), &store.odb).unwrap();
- let commit = store.write_commit(tree, Vec::new(), "ours", None).unwrap();
+ let commit = store
+ .write_commit(tree, Vec::new(), None, "ours", None)
+ .unwrap();
let result = store.try_set_ref(refname, None, commit);
assert!(matches!(result, Err(Error::Conflict)));
}
@@ -1157,7 +1258,7 @@
};
let tree = facet_git_tree::serialize_into(&"pass".to_string(), &store.odb).unwrap();
let commit = store
- .write_commit(tree, parents, "advance to pass", None)
+ .write_commit(tree, parents, stale, "advance to pass", None)
.unwrap();
let result = store.try_set_ref(refname, stale, commit);
assert!(matches!(result, Err(Error::Conflict)));