Skip to content

Commit

Permalink
annotate: add option to not search all ancestors of starting commit
Browse files Browse the repository at this point in the history
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().
  • Loading branch information
yuja committed Nov 11, 2024
1 parent 325402d commit 85e0a8b
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 18 deletions.
9 changes: 8 additions & 1 deletion cli/src/commands/file/annotate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(())
Expand All @@ -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)?;
Expand Down
38 changes: 24 additions & 14 deletions lib/src/annotate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use std::collections::hash_map;
use std::collections::HashMap;
use std::rc::Rc;

use bstr::BStr;
use bstr::BString;
Expand All @@ -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;
Expand All @@ -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<Item = (&CommitId, &BStr)> {
pub fn lines(&self) -> impl Iterator<Item = (Option<&CommitId>, &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.
Expand Down Expand Up @@ -102,16 +104,22 @@ impl Source {
type OriginalLineMap = Vec<Option<CommitId>>;

/// 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<ResolvedRevsetExpression>,
file_path: &RepoPath,
) -> Result<FileAnnotation, RevsetEvaluationError> {
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 })
}

Expand All @@ -122,17 +130,19 @@ fn process_commits(
repo: &dyn Repo,
starting_commit_id: &CommitId,
starting_source: Source,
domain: &Rc<ResolvedRevsetExpression>,
file_name: &RepoPath,
) -> Result<OriginalLineMap, RevsetEvaluationError> {
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)]);
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
}
}
Expand Down
57 changes: 54 additions & 3 deletions lib/tests/test_annotate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<ResolvedRevsetExpression>,
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
Expand Down Expand Up @@ -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]
Expand Down

0 comments on commit 85e0a8b

Please sign in to comment.