diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs index 634cf2fb96..7ba09f50e8 100644 --- a/cli/src/commands/diff.rs +++ b/cli/src/commands/diff.rs @@ -125,11 +125,11 @@ pub(crate) fn cmd_diff( let matcher = fileset_expression.to_matcher(); let diff_renderer = workspace_command.diff_renderer_for(&args.format)?; ui.request_pager(); - diff_renderer.show_commit_diff( + diff_renderer.show_diff_commits( ui, ui.stdout_formatter().as_mut(), - from.id(), - to.id(), + &from, + &to, matcher.as_ref(), )?; print_unmatched_explicit_paths( diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index a00656261a..7f59805345 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1392,7 +1392,13 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T let template = self_property .map(move |diff| { diff.into_formatted(move |formatter, _store, tree_diff| { - diff_util::show_diff_summary(formatter, tree_diff, path_converter) + // TODO(kfm) + diff_util::show_diff_summary( + formatter, + tree_diff, + path_converter, + &Default::default(), + ) }) }) .into_template(); diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 644f38ef1b..3d21d49944 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -13,14 +13,16 @@ // limitations under the License. use std::cmp::max; -use std::collections::VecDeque; +use std::collections::{HashMap, VecDeque}; use std::ops::Range; use std::path::{Path, PathBuf}; use std::{io, mem}; +use futures::executor::block_on_stream; +use futures::stream::BoxStream; use futures::StreamExt; use itertools::Itertools; -use jj_lib::backend::{BackendError, CommitId, TreeValue}; +use jj_lib::backend::{BackendError, BackendResult, CopyRecord, TreeValue}; use jj_lib::commit::Commit; use jj_lib::conflicts::{materialized_diff_stream, MaterializedTreeValue}; use jj_lib::diff::{Diff, DiffHunk}; @@ -30,7 +32,7 @@ use jj_lib::merge::MergedTreeValue; use jj_lib::merged_tree::{MergedTree, TreeDiffStream}; use jj_lib::object_id::ObjectId; use jj_lib::repo::Repo; -use jj_lib::repo_path::{RepoPath, RepoPathUiConverter}; +use jj_lib::repo_path::{RepoPath, RepoPathBuf, RepoPathUiConverter}; use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::store::Store; use jj_lib::{diff, files}; @@ -217,6 +219,43 @@ pub enum DiffRenderError { Io(#[from] io::Error), } +#[derive(Default, Debug)] +pub struct CopyRecordMap { + records: Vec, + targets: HashMap, + sources: HashMap, +} + +impl CopyRecordMap { + pub fn new(stream: BoxStream>) -> CopyRecordMap { + let mut records = Vec::new(); + let mut targets = HashMap::new(); + let mut sources = HashMap::new(); + for record in block_on_stream(stream).filter_map(|r| r.ok()) { + targets.insert(record.target.clone(), records.len()); + sources.insert(record.source.clone(), records.len()); + records.push(record); + } + CopyRecordMap { + records, + targets, + sources, + } + } + + pub fn for_target(&self, target: &RepoPath) -> Option<&CopyRecord> { + self.targets + .get(target) + .and_then(|&i| self.records.get(i)) + } + + pub fn for_source(&self, source: &RepoPath) -> Option<&CopyRecord> { + self.sources + .get(source) + .and_then(|&i| self.records.get(i)) + } +} + /// Configuration and environment to render textual diff. pub struct DiffRenderer<'a> { repo: &'a dyn Repo, @@ -237,18 +276,44 @@ impl<'a> DiffRenderer<'a> { } } - /// Generates diff between `from_tree` and `to_tree`. - pub fn show_commit_diff( + /// Generates diff of the given `commit` compared to its parents. + pub fn show_patch( + &self, + ui: &Ui, + formatter: &mut dyn Formatter, + commit: &Commit, + matcher: &dyn Matcher, + ) -> Result<(), DiffRenderError> { + let from_tree = commit.parent_tree(self.repo)?; + let to_tree = commit.tree()?; + self.show_diff_impl( + ui, + formatter, + &from_tree, + &to_tree, + matcher, + &Default::default(), + ) + } + + /// Generates diff between two commits. + pub fn show_diff_commits( &self, ui: &Ui, // TODO: remove Ui dependency if possible formatter: &mut dyn Formatter, - from: &CommitId, - to: &CommitId, + from: &Commit, + to: &Commit, matcher: &dyn Matcher, ) -> Result<(), DiffRenderError> { - let from_tree = self.repo.store().get_commit(from)?.tree()?; - let to_tree = self.repo.store().get_commit(to)?.tree()?; - self.show_diff(ui, formatter, &from_tree, &to_tree, matcher) + let copy_records = CopyRecordMap::new(self.repo.store().get_copy_records( + matcher, + from.id(), + to.id(), + )?); + + let from_tree = from.tree()?; + let to_tree = to.tree()?; + self.show_diff_impl(ui, formatter, &from_tree, &to_tree, matcher, ©_records) } /// Generates diff between `from_tree` and `to_tree`. @@ -259,6 +324,25 @@ impl<'a> DiffRenderer<'a> { from_tree: &MergedTree, to_tree: &MergedTree, matcher: &dyn Matcher, + ) -> Result<(), DiffRenderError> { + self.show_diff_impl( + ui, + formatter, + from_tree, + to_tree, + matcher, + &Default::default(), + ) + } + + fn show_diff_impl( + &self, + ui: &Ui, // TODO: remove Ui dependency if possible + formatter: &mut dyn Formatter, + from_tree: &MergedTree, + to_tree: &MergedTree, + matcher: &dyn Matcher, + copy_records: &CopyRecordMap, ) -> Result<(), DiffRenderError> { let store = self.repo.store(); let path_converter = self.path_converter; @@ -266,7 +350,7 @@ impl<'a> DiffRenderer<'a> { match format { DiffFormat::Summary => { let tree_diff = from_tree.diff_stream(to_tree, matcher); - show_diff_summary(formatter, tree_diff, path_converter)?; + show_diff_summary(formatter, tree_diff, path_converter, copy_records)?; } DiffFormat::Stat => { let tree_diff = from_tree.diff_stream(to_tree, matcher); @@ -313,19 +397,6 @@ impl<'a> DiffRenderer<'a> { } Ok(()) } - - /// Generates diff of the given `commit` compared to its parents. - pub fn show_patch( - &self, - ui: &Ui, - formatter: &mut dyn Formatter, - commit: &Commit, - matcher: &dyn Matcher, - ) -> Result<(), DiffRenderError> { - let from_tree = commit.parent_tree(self.repo)?; - let to_tree = commit.tree()?; - self.show_diff(ui, formatter, &from_tree, &to_tree, matcher) - } } fn show_color_words_diff_hunks( @@ -1097,19 +1168,35 @@ pub fn show_diff_summary( formatter: &mut dyn Formatter, mut tree_diff: TreeDiffStream, path_converter: &RepoPathUiConverter, + copy_records: &CopyRecordMap, ) -> io::Result<()> { formatter.with_label("diff", |formatter| -> io::Result<()> { async { while let Some((repo_path, diff)) = tree_diff.next().await { let (before, after) = diff.unwrap(); let ui_path = path_converter.format_file_path(&repo_path); - if before.is_present() && after.is_present() { - writeln!(formatter.labeled("modified"), "M {ui_path}")?; - } else if before.is_absent() { - writeln!(formatter.labeled("added"), "A {ui_path}")?; + if let Some(record) = copy_records.for_target(&repo_path) { + let old_ui_path = path_converter.format_file_path(&record.source); + match (before.is_present(), after.is_present()) { + (true, true) => { + writeln!(formatter.labeled("copied"), "C {old_ui_path} => {ui_path}")? + } + (false, true) => { + writeln!(formatter.labeled("renamed"), "R {old_ui_path} => {ui_path}")? + } + _ => {} + } } else { - // `R` could be interpreted as "renamed" - writeln!(formatter.labeled("removed"), "D {ui_path}")?; + match (before.is_present(), after.is_present()) { + (true, true) => writeln!(formatter.labeled("modified"), "M {ui_path}")?, + (false, true) => writeln!(formatter.labeled("added"), "A {ui_path}")?, + (true, false) => { + if copy_records.for_source(&repo_path).is_none() { + writeln!(formatter.labeled("removed"), "D {ui_path}")?; + } + } + _ => {} + } } } Ok(()) diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 304374d216..17ca8e0590 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -67,9 +67,8 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###" - D file1 M file2 - A file3 + R file1 => file3 "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--types"]); @@ -161,9 +160,8 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "--git"]); insta::assert_snapshot!(stdout, @r###" - D file1 M file2 - A file3 + R file1 => file3 diff --git a/file1 b/file1 deleted file mode 100644 index 257cc5642c..0000000000 @@ -226,9 +224,8 @@ fn test_diff_basic() { ], ); insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" - D repo/file1 M repo/file2 - A repo/file3 + R repo/file1 => repo/file3 "###); insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" Warning: No matching entries for paths: repo/x, repo/y/z