From 85e0a8b0687e232a7187029ac4ab549c693b4733 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 29 Oct 2024 18:44:36 +0900 Subject: [PATCH] annotate: add option to not search all ancestors of starting commit The primary use case is to exclude immutable commits when calculating line ranges to absorb. For example, "jj absorb" will build annotation of @ revision with domain = mutable(). --- cli/src/commands/file/annotate.rs | 9 ++++- lib/src/annotate.rs | 38 +++++++++++++-------- lib/tests/test_annotate.rs | 57 +++++++++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 18 deletions(-) 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]