diff --git a/cli/src/commands/file/annotate.rs b/cli/src/commands/file/annotate.rs index 696ccc6b0d..be0b1c2a63 100644 --- a/cli/src/commands/file/annotate.rs +++ b/cli/src/commands/file/annotate.rs @@ -16,6 +16,7 @@ use jj_lib::annotate::get_annotation_for_file; use jj_lib::annotate::FileAnnotation; use jj_lib::commit::Commit; use jj_lib::repo::Repo; +use jj_lib::revset::RevsetExpression; use tracing::instrument; use crate::cli_util::CommandHelper; @@ -69,7 +70,12 @@ pub(crate) fn cmd_file_annotate( .get_string("templates.annotate_commit_summary")?; let template = workspace_command.parse_commit_template(ui, &annotate_commit_summary_text)?; - let annotation = get_annotation_for_file(repo.as_ref(), &starting_commit, &file_path)?; + // TODO: Should we add an option to limit the domain to e.g. recent commits? + // Note that this is probably different from "--skip REVS", which won't + // exclude the revisions, but will ignore diffs in those revisions as if + // ancestor revisions had new content. + let domain = RevsetExpression::all(); + let annotation = get_annotation_for_file(repo.as_ref(), &starting_commit, &domain, &file_path)?; render_file_annotation(repo.as_ref(), ui, &template, &annotation)?; Ok(()) @@ -84,6 +90,7 @@ fn render_file_annotation( ui.request_pager(); let mut formatter = ui.stdout_formatter(); for (line_no, (commit_id, line)) in annotation.lines().enumerate() { + let commit_id = commit_id.expect("should reached to the empty ancestor"); let commit = repo.store().get_commit(commit_id)?; template_render.format(&commit, formatter.as_mut())?; write!(formatter, " {:>4}: ", line_no + 1)?; diff --git a/lib/src/annotate.rs b/lib/src/annotate.rs index ab81b950db..4bc57c9e2a 100644 --- a/lib/src/annotate.rs +++ b/lib/src/annotate.rs @@ -20,6 +20,7 @@ use std::collections::hash_map; use std::collections::HashMap; +use std::rc::Rc; use bstr::BStr; use bstr::BString; @@ -40,6 +41,7 @@ use crate::graph::GraphEdgeType; use crate::merged_tree::MergedTree; use crate::repo::Repo; use crate::repo_path::RepoPath; +use crate::revset::ResolvedRevsetExpression; use crate::revset::RevsetEvaluationError; use crate::revset::RevsetExpression; use crate::revset::RevsetFilterPredicate; @@ -57,9 +59,9 @@ impl FileAnnotation { /// /// For each line, the `commit_id` points to the originator commit of the /// line. The `line` includes newline character. - pub fn lines(&self) -> impl Iterator { + pub fn lines(&self) -> impl Iterator, &BStr)> { itertools::zip_eq(&self.line_map, self.text.split_inclusive(|b| *b == b'\n')) - .map(|(commit_id, line)| (commit_id.as_ref().unwrap(), line.as_ref())) + .map(|(commit_id, line)| (commit_id.as_ref(), line.as_ref())) } /// File content at the starting commit. @@ -102,16 +104,22 @@ impl Source { type OriginalLineMap = Vec>; /// Get line by line annotations for a specific file path in the repo. +/// +/// The `domain` expression narrows the range of ancestors to search. It will be +/// intersected as `domain & ::starting_commit & files(file_path)`. The +/// `starting_commit` is assumed to be included in the `domain`. +/// /// If the file is not found, returns empty results. pub fn get_annotation_for_file( repo: &dyn Repo, starting_commit: &Commit, + domain: &Rc, file_path: &RepoPath, ) -> Result { let mut source = Source::load(starting_commit, file_path)?; source.fill_line_map(); let text = source.text.clone(); - let line_map = process_commits(repo, starting_commit.id(), source, file_path)?; + let line_map = process_commits(repo, starting_commit.id(), source, domain, file_path)?; Ok(FileAnnotation { line_map, text }) } @@ -122,17 +130,19 @@ fn process_commits( repo: &dyn Repo, starting_commit_id: &CommitId, starting_source: Source, + domain: &Rc, file_name: &RepoPath, ) -> Result { let predicate = RevsetFilterPredicate::File(FilesetExpression::file_path(file_name.to_owned())); + // TODO: If the domain isn't a contiguous range, changes masked out by it + // might not be caught by the closest ancestor revision. For example, + // domain=merges() would pick up almost nothing because merge revisions + // are usually empty. Perhaps, we want to query `files(file_path, + // within_sub_graph=domain)`, not `domain & files(file_path)`. + let ancestors = RevsetExpression::commit(starting_commit_id.clone()).ancestors(); let revset = RevsetExpression::commit(starting_commit_id.clone()) - .union( - &RevsetExpression::commit(starting_commit_id.clone()) - .ancestors() - .filtered(predicate), - ) - .evaluate(repo) - .map_err(|e| e.expect_backend_error())?; + .union(&domain.intersection(&ancestors).filtered(predicate)) + .evaluate(repo)?; let mut original_line_map = vec![None; starting_source.line_map.len()]; let mut commit_source_map = HashMap::from([(starting_commit_id.clone(), starting_source)]); @@ -171,9 +181,6 @@ fn process_commit( }; for parent_edge in edges { - if parent_edge.edge_type == GraphEdgeType::Missing { - continue; - } let parent_commit_id = &parent_edge.target; let parent_source = match commit_source_map.entry(parent_commit_id.clone()) { hash_map::Entry::Occupied(entry) => entry.into_mut(), @@ -215,7 +222,10 @@ fn process_commit( } else { itertools::merge(parent_source.line_map.iter().copied(), new_parent_line_map).collect() }; - if parent_source.line_map.is_empty() { + // If an omitted parent had the file, leave these lines unresolved. + // TODO: These unresolved lines could be copied to the original_line_map + // as Err(commit_id) or something instead of None. + if parent_source.line_map.is_empty() || parent_edge.edge_type == GraphEdgeType::Missing { commit_source_map.remove(parent_commit_id); } } diff --git a/lib/tests/test_annotate.rs b/lib/tests/test_annotate.rs index c2c1cb6efb..faef109b5a 100644 --- a/lib/tests/test_annotate.rs +++ b/lib/tests/test_annotate.rs @@ -13,6 +13,7 @@ // limitations under the License. use std::fmt::Write as _; +use std::rc::Rc; use jj_lib::annotate::get_annotation_for_file; use jj_lib::backend::CommitId; @@ -24,6 +25,8 @@ use jj_lib::commit::Commit; use jj_lib::repo::MutableRepo; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; +use jj_lib::revset::ResolvedRevsetExpression; +use jj_lib::revset::RevsetExpression; use jj_lib::settings::UserSettings; use testutils::create_tree; use testutils::TestRepo; @@ -54,11 +57,23 @@ fn create_commit_fn<'a>( } fn annotate(repo: &dyn Repo, commit: &Commit, file_path: &RepoPath) -> String { - let annotation = get_annotation_for_file(repo, commit, file_path).unwrap(); + let domain = RevsetExpression::all(); + annotate_within(repo, commit, &domain, file_path) +} + +fn annotate_within( + repo: &dyn Repo, + commit: &Commit, + domain: &Rc, + file_path: &RepoPath, +) -> String { + let annotation = get_annotation_for_file(repo, commit, domain, file_path).unwrap(); let mut output = String::new(); for (commit_id, line) in annotation.lines() { - let commit = repo.store().get_commit(commit_id).unwrap(); - let desc = commit.description().trim_end(); + let commit = commit_id.map(|id| repo.store().get_commit(id).unwrap()); + let desc = commit + .as_ref() + .map_or("*******", |commit| commit.description().trim_end()); write!(output, "{desc}: {line}").unwrap(); } output @@ -139,6 +154,42 @@ fn test_annotate_merge_simple() { commit1: 1 commit3: 3 "#); + + // Exclude the fork commit and its ancestors. + let domain = RevsetExpression::commit(commit1.id().clone()) + .ancestors() + .negated(); + insta::assert_snapshot!(annotate_within(tx.repo(), &commit4, &domain, file_path), @r" + commit2: 2 + *******: 1 + commit3: 3 + "); + + // Exclude one side of the merge and its ancestors. + let domain = RevsetExpression::commit(commit2.id().clone()) + .ancestors() + .negated(); + insta::assert_snapshot!(annotate_within(tx.repo(), &commit4, &domain, file_path), @r" + *******: 2 + *******: 1 + commit3: 3 + "); + + // Exclude both sides of the merge and their ancestors. + let domain = RevsetExpression::commit(commit4.id().clone()); + insta::assert_snapshot!(annotate_within(tx.repo(), &commit4, &domain, file_path), @r" + *******: 2 + *******: 1 + *******: 3 + "); + + // Exclude intermediate commit, which is useless but works. + let domain = RevsetExpression::commit(commit3.id().clone()).negated(); + insta::assert_snapshot!(annotate_within(tx.repo(), &commit4, &domain, file_path), @r" + commit2: 2 + commit1: 1 + commit4: 3 + "); } #[test]