diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index 9530ee4..4046dbe 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -21,6 +21,9 @@ jobs: - name: Test parser and schema run: cargo test -p alternator -p schema + - name: Test walker history + run: cargo test -p walker --test history + generator: name: DocGen diff --git a/chumbucket/src/accessors/git.rs b/chumbucket/src/accessors/git.rs index bf239de..083b893 100644 --- a/chumbucket/src/accessors/git.rs +++ b/chumbucket/src/accessors/git.rs @@ -11,8 +11,11 @@ use super::Chronicle; pub struct Git<'g>(DiffList<'g>); impl<'g> Git<'g> { - pub fn from_walker(from: Option, walker: &'g mut Walker) -> Result { - Ok(Self(walker.walk(from)?)) + pub fn from_walker(since: Option<&Versioning>, walker: &'g mut Walker) -> Result { + Ok(Self(walker.walk( + since.map(|v| v.hash.as_str()), + since.map(|v| v.time), + )?)) } } diff --git a/chumbucket/src/commands/generate.rs b/chumbucket/src/commands/generate.rs index 0f279dc..ec28b91 100644 --- a/chumbucket/src/commands/generate.rs +++ b/chumbucket/src/commands/generate.rs @@ -33,16 +33,14 @@ pub async fn generate_command(matches: &ArgMatches) -> Result<()> { let manifest: Manifest = toml::from_slice(&fs_content)?; let mut bundle: Option = None; - let mut from_time: Option = None; + let mut since: Option = None; if matches.is_present("bundle") { let bundle_str = std::fs::read(matches.value_of("bundle").unwrap())?; let parsed_bundle: Bundle = serde_json::from_slice(&bundle_str)?; - if let Some(version) = &parsed_bundle.version { - from_time = Some(version.time); - } + since = parsed_bundle.version.clone(); bundle = Some(parsed_bundle); } @@ -64,7 +62,7 @@ pub async fn generate_command(matches: &ArgMatches) -> Result<()> { )?; let latest_file_names = walker.latest_file_names()?; - let git = Git::from_walker(from_time, &mut walker)?; + let git = Git::from_walker(since.as_ref(), &mut walker)?; let it_ret = iterate_chronicles(git, manifest, bundle, latest_file_names).await?; @@ -313,52 +311,49 @@ impl<'b> ChronicleProcessor<'b> { new: &HashMap, version: Option, ) -> u64 { - // Handle any new entries - for (k, v) in new { - if let Entry::Vacant(entry) = existing.entry(k.clone()) { - entry.insert(v.clone()); - } - } - - let mut to_remove = Vec::new(); - - // Handle any deleted entries - for (k, _) in existing.iter() { - if !new.contains_key(k) { - to_remove.push(k.clone()); - } - } - - for k in to_remove { - existing.remove(&k); - } - let mut diff = 0; - // At this point, both list should have the same keys, so we can safely iterate over them - // Handle any updated entries - for (k, v) in existing.iter_mut() { - let dp_v = new.get(k).unwrap(); - - let is_diff = v != dp_v; - - if is_diff { - *v <<= dp_v.clone(); - diff += 1; - } - - // Update the metadata - match v.metadata() { - Some(m) if is_diff => { - m.last_updated = version.clone(); - } - None if !is_diff => { - *v.metadata() = Some(Metadata { + // Handle any deleted entries + let before = existing.len(); + existing.retain(|k, _| new.contains_key(k)); + diff += (before - existing.len()) as u64; + + for (k, dp_v) in new { + match existing.entry(k.clone()) { + // Newly added entry, created at this version + Entry::Vacant(entry) => { + *entry.insert(dp_v.clone()).metadata() = Some(Metadata { last_updated: version.clone(), created: version.clone(), }); + diff += 1; + } + // Existing entry, only its last updated version changes + Entry::Occupied(mut entry) => { + let v = entry.get_mut(); + + let is_diff = v != dp_v; + + if is_diff { + *v <<= dp_v.clone(); + diff += 1; + } + + match v.metadata() { + Some(m) => { + if is_diff { + m.last_updated = version.clone(); + } + } + // Entry without metadata (e.g. nested in a newly added parent) + m @ None => { + *m = Some(Metadata { + last_updated: version.clone(), + created: version.clone(), + }); + } + } } - _ => {} } } diff --git a/libschema/src/symbol/enum_struct.rs b/libschema/src/symbol/enum_struct.rs index 43272b3..a9252b4 100644 --- a/libschema/src/symbol/enum_struct.rs +++ b/libschema/src/symbol/enum_struct.rs @@ -51,8 +51,7 @@ impl Metable for EnumStruct { impl ShlAssign for EnumStruct { fn shl_assign(&mut self, rhs: Self) { self.declaration <<= rhs.declaration; - self.methods = rhs.methods; - self.fields = rhs.fields; + // Methods and fields are merged individually to preserve their metadata } } diff --git a/libschema/src/symbol/enumeration.rs b/libschema/src/symbol/enumeration.rs index 552e7b5..db3d1e2 100644 --- a/libschema/src/symbol/enumeration.rs +++ b/libschema/src/symbol/enumeration.rs @@ -48,7 +48,7 @@ impl Metable for Enumeration { impl ShlAssign for Enumeration { fn shl_assign(&mut self, rhs: Self) { self.declaration <<= rhs.declaration; - self.entries = rhs.entries; + // Entries are merged individually to preserve their metadata } } diff --git a/libschema/src/symbol/method_map.rs b/libschema/src/symbol/method_map.rs index 6046fdd..60933f9 100644 --- a/libschema/src/symbol/method_map.rs +++ b/libschema/src/symbol/method_map.rs @@ -32,8 +32,7 @@ impl ShlAssign for MethodMap { fn shl_assign(&mut self, rhs: Self) { self.declaration <<= rhs.declaration; self.parent = rhs.parent; - self.methods = rhs.methods; - self.properties = rhs.properties; + // Methods and properties are merged individually to preserve their metadata } } diff --git a/libschema/src/symbol/type_set.rs b/libschema/src/symbol/type_set.rs index de43a55..4f06695 100644 --- a/libschema/src/symbol/type_set.rs +++ b/libschema/src/symbol/type_set.rs @@ -52,7 +52,7 @@ impl Metable for TypeSet { impl ShlAssign for TypeSet { fn shl_assign(&mut self, rhs: Self) { self.declaration <<= rhs.declaration; - self.types = rhs.types; + // Types are merged individually to preserve their metadata } } diff --git a/libwalker/Cargo.toml b/libwalker/Cargo.toml index 6b0e0ea..b9c0415 100644 --- a/libwalker/Cargo.toml +++ b/libwalker/Cargo.toml @@ -9,3 +9,6 @@ edition = "2021" [dependencies] git2 = "0.13" thiserror = "1" + +[dev-dependencies] +git2 = "0.13" diff --git a/libwalker/src/lib.rs b/libwalker/src/lib.rs index 289bf9d..96686ee 100644 --- a/libwalker/src/lib.rs +++ b/libwalker/src/lib.rs @@ -1,7 +1,7 @@ use std::ops::Range; use std::path::{Path, PathBuf}; -use git2::{Delta, IntoCString, Oid, Pathspec, PathspecFlags, Repository, Sort}; +use git2::{Delta, IntoCString, Oid, Pathspec, PathspecFlags, Repository}; mod error; @@ -60,62 +60,96 @@ impl Walker { }) } - pub fn walk(&mut self, from: Option) -> Result { - let mut revwalk = self.repo.revwalk()?; - - revwalk.set_sorting(Sort::TIME | Sort::REVERSE)?; - - revwalk.push_head()?; - - let mut spec_diffs = Vec::new(); - - for (count, oid) in revwalk.enumerate() { - let oid = oid?; - - let commit = self.repo.find_commit(oid)?; - - // If a from time is specified - // Any commit that's older than this time is skipped - if let Some(from_time) = from { - if commit.time().seconds() < from_time { - continue; - } + /// Walks the first-parent history of HEAD, oldest first, collecting the commits that touched + /// any path matching the pathspec. + /// + /// `since_commit` is the last commit processed by a previous walk; only commits after it are + /// collected. If it isn't on the current history, commits older than `since_time` are skipped + /// instead. + pub fn walk( + &mut self, + since_commit: Option<&str>, + since_time: Option, + ) -> Result> { + let checkpoint = since_commit.and_then(|v| Oid::from_str(v).ok()); + + // Follow first parents only, so commits are visited in the order they landed on the branch. + // Commits from merged branches are observed through their merge commit, otherwise their + // contents would be interleaved with, and reverted by, the mainline commits around them + let mut chain = Vec::new(); + let mut reached_checkpoint = false; + let mut next = Some(self.repo.head()?.peel_to_commit()?); + + while let Some(commit) = next { + if Some(commit.id()) == checkpoint { + reached_checkpoint = true; + break; } - let c_tree = commit.tree()?; + next = match commit.parent_count() { + 0 => None, + _ => Some(commit.parent(0)?), + }; - let parent_count = commit.parent_count(); + chain.push(commit); + } - match parent_count { - c if c == 1 => { - let parent = commit.parent(0)?; + chain.reverse(); - let diff = - self.repo - .diff_tree_to_tree(Some(&parent.tree()?), Some(&c_tree), None)?; + // Rev-list count of the commit the chain starts from + let mut count = match chain.first().map(|v| v.parent_ids().next()) { + Some(Some(parent)) => self.rev_list_count(&[parent], None)?, + _ => 0, + }; - let ml = self.pathspec.match_diff(&diff, PathspecFlags::DEFAULT)?; + let mut spec_diffs = Vec::new(); - let diff_stems: Vec = ml - .diff_entries() - .filter(|v| v.status() != Delta::Deleted) - .map(|v| v.new_file().path()) - .filter(|v| v.is_some()) - .map(|v| v.unwrap().to_path_buf()) - .collect(); + for commit in chain { + let parents: Vec = commit.parent_ids().collect(); - if !diff_stems.is_empty() { - spec_diffs.push(CommitDiffs { - commit: commit.id(), - count: count as u64, - path_diffs: diff_stems, - }); - } + // A merge also brings in every commit of the merged branch not already in the first parent + count += 1 + match parents.split_first() { + Some((first, rest)) if !rest.is_empty() => { + self.rev_list_count(rest, Some(*first))? } - _ => { - continue; + _ => 0, + }; + + // If the checkpoint couldn't be found, fall back to skipping commits older than the time + if !reached_checkpoint { + if let Some(from_time) = since_time { + if commit.time().seconds() < from_time { + continue; + } } } + + // The root commit is compared against an empty tree, so files it adds are picked up + let parent_tree = match parents.first() { + Some(_) => Some(commit.parent(0)?.tree()?), + None => None, + }; + + let diff = + self.repo + .diff_tree_to_tree(parent_tree.as_ref(), Some(&commit.tree()?), None)?; + + let ml = self.pathspec.match_diff(&diff, PathspecFlags::DEFAULT)?; + + let diff_stems: Vec = ml + .diff_entries() + .filter(|v| v.status() != Delta::Deleted) + .filter_map(|v| v.new_file().path()) + .map(|v| v.to_path_buf()) + .collect(); + + if !diff_stems.is_empty() { + spec_diffs.push(CommitDiffs { + commit: commit.id(), + count, + path_diffs: diff_stems, + }); + } } Ok(DiffList { @@ -125,6 +159,28 @@ impl Walker { }) } + /// Number of commits reachable from `from`, excluding those reachable from `hide` + fn rev_list_count(&self, from: &[Oid], hide: Option) -> Result { + let mut revwalk = self.repo.revwalk()?; + + for oid in from { + revwalk.push(*oid)?; + } + + if let Some(oid) = hide { + revwalk.hide(oid)?; + } + + let mut count = 0; + + for oid in revwalk { + oid?; + count += 1; + } + + Ok(count) + } + pub fn latest_file_names(&mut self) -> Result> { let mut file_names = Vec::new(); @@ -163,6 +219,19 @@ impl<'w> Iterator for DiffList<'w> { type Item = Vec; fn next(&mut self) -> Option { + loop { + let bcs = self.next_contents()?; + + // Keep going if every entry of this commit was skipped + if !bcs.is_empty() { + return Some(bcs); + } + } + } +} + +impl<'w> DiffList<'w> { + fn next_contents(&mut self) -> Option> { let spec_diff = self.range.next().and_then(|i| self.spec_diffs.get(i))?; let commit = self.walker.repo.find_commit(spec_diff.commit).ok()?; @@ -172,11 +241,17 @@ impl<'w> Iterator for DiffList<'w> { let mut bcs = Vec::new(); for path in &spec_diff.path_diffs { - let te = tree.get_path(path).ok()?; - - let obj = te.to_object(&self.walker.repo).ok()?; - - let content = obj.as_blob()?.content().to_owned(); + // Skip entries that aren't readable blobs (submodules, etc.) rather than ending the walk + let content = match tree + .get_path(path) + .and_then(|v| v.to_object(&self.walker.repo)) + { + Ok(obj) => match obj.as_blob() { + Some(blob) => blob.content().to_owned(), + None => continue, + }, + Err(_) => continue, + }; bcs.push(BlobContent { commit: spec_diff.commit, diff --git a/libwalker/tests/history.rs b/libwalker/tests/history.rs new file mode 100644 index 0000000..0aadcc0 --- /dev/null +++ b/libwalker/tests/history.rs @@ -0,0 +1,180 @@ +extern crate walker; + +use std::path::{Path, PathBuf}; + +use git2::{Commit, Oid, Repository, Signature, Time}; + +use walker::Walker; + +struct Fixture { + path: PathBuf, + repo: Repository, +} + +impl Fixture { + fn new(name: &str) -> Self { + let path = std::env::temp_dir().join(format!("walker-{}-{}", name, std::process::id())); + let _ = std::fs::remove_dir_all(&path); + + Self { + repo: Repository::init(&path).unwrap(), + path, + } + } + + /// Commits `files` on top of `parents`' first tree, without moving any reference + fn commit(&self, time: i64, parents: &[Oid], files: &[(&str, &str)]) -> Oid { + let parents: Vec = parents + .iter() + .map(|v| self.repo.find_commit(*v).unwrap()) + .collect(); + + let mut index = git2::Index::new().unwrap(); + + if let Some(parent) = parents.first() { + index.read_tree(&parent.tree().unwrap()).unwrap(); + } + + for (path, content) in files { + let blob = self.repo.blob(content.as_bytes()).unwrap(); + let mut entry = git2::IndexEntry { + ctime: git2::IndexTime::new(0, 0), + mtime: git2::IndexTime::new(0, 0), + dev: 0, + ino: 0, + mode: 0o100644, + uid: 0, + gid: 0, + file_size: content.len() as u32, + id: blob, + flags: 0, + flags_extended: 0, + path: path.as_bytes().to_vec(), + }; + entry.flags = entry.path.len() as u16; + index.add(&entry).unwrap(); + } + + let tree = self + .repo + .find_tree(index.write_tree_to(&self.repo).unwrap()) + .unwrap(); + let sig = Signature::new("test", "test@example.com", &Time::new(time, 0)).unwrap(); + + self.repo + .commit( + None, + &sig, + &sig, + "commit", + &tree, + &parents.iter().collect::>(), + ) + .unwrap() + } + + fn set_head(&self, oid: Oid) { + self.repo + .reference("refs/heads/main", oid, true, "") + .unwrap(); + self.repo.set_head("refs/heads/main").unwrap(); + } + + fn walker(&self) -> Walker { + Walker::new(Path::new(&self.path), vec!["*.inc"]).unwrap() + } +} + +impl Drop for Fixture { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +/// (commit, rev-list count, paths) +fn collect( + walker: &mut Walker, + since: Option, + since_time: Option, +) -> Vec<(Oid, u64, Vec)> { + let since = since.map(|v| v.to_string()); + + walker + .walk(since.as_deref(), since_time) + .unwrap() + .map(|v| { + ( + v[0].commit, + v[0].count, + v.iter() + .map(|b| b.path.to_string_lossy().into_owned()) + .collect(), + ) + }) + .collect() +} + +#[test] +fn walks_first_parent_history_with_rev_list_counts() { + let f = Fixture::new("history"); + + let root = f.commit(100, &[], &[("a.inc", "a1")]); + let c2 = f.commit(200, &[root], &[("a.inc", "a2")]); + // Side branch commit is newer than the mainline commit it gets merged over + let side = f.commit(400, &[c2], &[("a.inc", "a2-side"), ("b.inc", "b1")]); + let c3 = f.commit(300, &[c2], &[("a.inc", "a3")]); + let merge = f.commit(500, &[c3, side], &[("a.inc", "a3-merged"), ("b.inc", "b1")]); + let readme = f.commit(600, &[merge], &[("README", "readme")]); + f.set_head(readme); + + let walked = collect(&mut f.walker(), None, None); + + assert_eq!( + walked, + vec![ + // Root commit is included + (root, 1, vec!["a.inc".to_string()]), + (c2, 2, vec!["a.inc".to_string()]), + (c3, 3, vec!["a.inc".to_string()]), + // Side branch content is only seen once merged, and counted in the merge + (merge, 5, vec!["a.inc".to_string(), "b.inc".to_string()]), + ] + ); + + let contents: Vec> = f + .walker() + .walk(None, None) + .unwrap() + .map(|v| v[0].content.clone()) + .collect(); + + assert_eq!(walked.len(), contents.len()); + assert_eq!(contents[0], b"a1"); + assert_eq!(contents[3], b"a3-merged"); +} + +#[test] +fn walks_from_checkpoint() { + let f = Fixture::new("checkpoint"); + + let root = f.commit(100, &[], &[("a.inc", "a1")]); + let c2 = f.commit(200, &[root], &[("a.inc", "a2")]); + // Committer time earlier than the checkpoint, but landed after it + let c3 = f.commit(150, &[c2], &[("a.inc", "a3")]); + f.set_head(c3); + + assert_eq!( + collect(&mut f.walker(), Some(c2), Some(200)), + vec![(c3, 3, vec!["a.inc".to_string()])] + ); + + // Nothing new since HEAD + assert!(collect(&mut f.walker(), Some(c3), Some(150)).is_empty()); + + // Unknown checkpoint falls back to time + let unknown = Oid::from_str("0123456789012345678901234567890123456789").unwrap(); + assert_eq!( + collect(&mut f.walker(), Some(unknown), Some(200)), + vec![(c2, 2, vec!["a.inc".to_string()])] + ); +} diff --git a/libwalker/tests/sm.rs b/libwalker/tests/sm.rs index b3f2823..00ec891 100644 --- a/libwalker/tests/sm.rs +++ b/libwalker/tests/sm.rs @@ -10,7 +10,7 @@ fn test_walk() -> Result<(), Box> { vec!["plugins/include/geoip.inc"], )?; - let spec_diffs = walker.walk(None)?; + let spec_diffs = walker.walk(None, None)?; for t in spec_diffs { for c in t {