Skip to content

Commit

Permalink
copy-tracking: add copy tracking to diff summaries
Browse files Browse the repository at this point in the history
  • Loading branch information
fowles committed Jul 18, 2024
1 parent 6d49831 commit 8efc14b
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 40 deletions.
6 changes: 3 additions & 3 deletions cli/src/commands/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 7 additions & 1 deletion cli/src/commit_templater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
143 changes: 113 additions & 30 deletions cli/src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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};
Expand Down Expand Up @@ -217,6 +219,39 @@ pub enum DiffRenderError {
Io(#[from] io::Error),
}

#[derive(Default, Debug)]
pub struct CopyRecordMap {
records: Vec<CopyRecord>,
targets: HashMap<RepoPathBuf, usize>,
sources: HashMap<RepoPathBuf, usize>,
}

impl CopyRecordMap {
pub fn new(stream: BoxStream<BackendResult<CopyRecord>>) -> 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).map(|&i| self.records.get(i)).flatten()
}

pub fn for_source(&self, source: &RepoPath) -> Option<&CopyRecord> {
self.sources.get(source).map(|&i| self.records.get(i)).flatten()
}
}

/// Configuration and environment to render textual diff.
pub struct DiffRenderer<'a> {
repo: &'a dyn Repo,
Expand All @@ -237,18 +272,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, &copy_records)
}

/// Generates diff between `from_tree` and `to_tree`.
Expand All @@ -259,14 +320,33 @@ 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;
for format in &self.formats {
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);
Expand Down Expand Up @@ -313,19 +393,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(
Expand Down Expand Up @@ -1097,19 +1164,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(())
Expand Down
9 changes: 3 additions & 6 deletions cli/tests/test_diff_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8efc14b

Please sign in to comment.