Skip to content

Commit

Permalink
copy-tracking: Add copy tracking as a post iteration step
Browse files Browse the repository at this point in the history 
- force each diff command to explicitly enable copy tracking
- enable copy tracking in diff_summary
- post-process for diff iterator
- post-process for diff stream
- update changelog
  • Loading branch information
@fowles
fowles committed Aug 9, 2024
1 parent 72abc11 commit d988f3f
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 48 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### New features

* The following diff formats now include information about copies and moves if supported by the backend (the Git backend does): `--summary`

### Fixed bugs

## [0.20.0] - 2024-08-07
Expand Down
133 changes: 102 additions & 31 deletions cli/src/diff_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// 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};
Expand Down Expand Up @@ -284,29 +284,36 @@ impl<'a> DiffRenderer<'a> {
show_diff_summary(formatter, tree_diff, path_converter)?;
}
DiffFormat::Stat => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_diff_stat(formatter, store, tree_diff, path_converter, width)?;
}
DiffFormat::Types => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_types(formatter, tree_diff, path_converter)?;
}
DiffFormat::NameOnly => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_names(formatter, tree_diff, path_converter)?;
}
DiffFormat::Git { context } => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_git_diff(formatter, store, tree_diff, *context)?;
}
DiffFormat::ColorWords { context } => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_color_words_diff(formatter, store, tree_diff, path_converter, *context)?;
}
DiffFormat::Tool(tool) => {
match tool.diff_invocation_mode {
DiffToolMode::FileByFile => {
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
let no_copy_tracking = Default::default();
let tree_diff =
from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_file_by_file_diff(
ui,
formatter,
Expand Down Expand Up @@ -572,20 +579,28 @@ pub fn show_color_words_diff(
let mut diff_stream = materialized_diff_stream(store, tree_diff);
async {
while let Some(MaterializedTreeDiffEntry {
source: _,
target: path,
source: left_path,
target: right_path,
value: diff,
}) = diff_stream.next().await
{
let ui_path = path_converter.format_file_path(&path);
let left_ui_path = path_converter.format_file_path(&left_path);
let right_ui_path = path_converter.format_file_path(&right_path);
let (left_value, right_value) = diff?;

match (&left_value, &right_value) {
(_, MaterializedTreeValue::AccessDenied(source))
| (MaterializedTreeValue::AccessDenied(source), _) => {
(MaterializedTreeValue::AccessDenied(source), _) => {
write!(
formatter.labeled("access-denied"),
"Access denied to {ui_path}:"
"Access denied to {left_ui_path}:"
)?;
writeln!(formatter, " {source}")?;
continue;
}
(_, MaterializedTreeValue::AccessDenied(source)) => {
write!(
formatter.labeled("access-denied"),
"Access denied to {right_ui_path}:"
)?;
writeln!(formatter, " {source}")?;
continue;
Expand All @@ -596,9 +611,9 @@ pub fn show_color_words_diff(
let description = basic_diff_file_type(&right_value);
writeln!(
formatter.labeled("header"),
"Added {description} {ui_path}:"
"Added {description} {right_ui_path}:"
)?;
let right_content = diff_content(&path, right_value)?;
let right_content = diff_content(&right_path, right_value)?;
if right_content.is_empty() {
writeln!(formatter.labeled("empty"), " (empty)")?;
} else if right_content.is_binary {
Expand Down Expand Up @@ -659,9 +674,19 @@ pub fn show_color_words_diff(
)
}
};
let left_content = diff_content(&path, left_value)?;
let right_content = diff_content(&path, right_value)?;
writeln!(formatter.labeled("header"), "{description} {ui_path}:")?;
let left_content = diff_content(&left_path, left_value)?;
let right_content = diff_content(&right_path, right_value)?;
if left_path == right_path {
writeln!(
formatter.labeled("header"),
"{description} {right_ui_path}:"
)?;
} else {
writeln!(
formatter.labeled("header"),
"{description} {right_ui_path} ({left_ui_path} => {right_ui_path}):"
)?;
}
if left_content.is_binary || right_content.is_binary {
writeln!(formatter.labeled("binary"), " (binary)")?;
} else {
Expand All @@ -676,9 +701,9 @@ pub fn show_color_words_diff(
let description = basic_diff_file_type(&left_value);
writeln!(
formatter.labeled("header"),
"Removed {description} {ui_path}:"
"Removed {description} {right_ui_path}:"
)?;
let left_content = diff_content(&path, left_value)?;
let left_content = diff_content(&left_path, left_value)?;
if left_content.is_empty() {
writeln!(formatter.labeled("empty"), " (empty)")?;
} else if left_content.is_binary {
Expand Down Expand Up @@ -1128,27 +1153,73 @@ pub fn show_diff_summary(
mut tree_diff: TreeDiffStream,
path_converter: &RepoPathUiConverter,
) -> io::Result<()> {
#[derive(Debug)]
enum Type {
Copy(String, String),
Rename(String, String),
Modification(String),
Addition(String),
Deletion(String),
}

let mut file_modifications = Vec::<Type>::new();
let mut unresolved_renames = HashMap::<String, usize>::new();
async {
while let Some(TreeDiffEntry {
source: _, // TODO handle copy tracking
target: repo_path,
source: before_path,
target: after_path,
value: 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}")?;
let before_ui_path = path_converter.format_file_path(&before_path);
let after_ui_path = path_converter.format_file_path(&after_path);
if before_path != after_path {
if let Some(&idx) = unresolved_renames.get(&before_ui_path) {
if let Some(Type::Deletion(_)) = file_modifications.get(idx) {
file_modifications[idx] = Type::Rename(before_ui_path, after_ui_path);
}
} else {
unresolved_renames.insert(before_ui_path.clone(), file_modifications.len());
file_modifications.push(Type::Copy(before_ui_path, after_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) => file_modifications.push(Type::Modification(before_ui_path)),
(false, true) => file_modifications.push(Type::Addition(before_ui_path)),
(true, false) => {
if let Some(&idx) = unresolved_renames.get(&before_ui_path) {
if let Some(Type::Copy(before, after)) = file_modifications.get(idx) {
file_modifications[idx] =
Type::Rename(before.clone(), after.clone());
}
} else {
unresolved_renames
.insert(before_ui_path.clone(), file_modifications.len());
file_modifications.push(Type::Deletion(before_ui_path));
}
}
_ => {}
}
}
}
Ok(())
}
.block_on()
.block_on();

for modification in file_modifications {
match modification {
Type::Copy(before, after) => {
writeln!(formatter.labeled("copied"), "C {before} => {after}")?
}
Type::Rename(before, after) => {
writeln!(formatter.labeled("renamed"), "R {before} => {after}")?
}
Type::Modification(path) => writeln!(formatter.labeled("modified"), "M {path}")?,
Type::Addition(path) => writeln!(formatter.labeled("added"), "A {path}")?,
Type::Deletion(path) => writeln!(formatter.labeled("removed"), "D {path}")?,
}
}
Ok(())
}

struct DiffStat {
Expand Down
23 changes: 10 additions & 13 deletions cli/tests/test_diff_command.rs
View file
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
R file1 => file3
M file2
A 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
R file1 => file3
M file2
A file3
diff --git a/file1 b/file1
deleted file mode 100644
index 257cc5642c..0000000000
Expand Down Expand Up @@ -218,9 +216,8 @@ fn test_diff_basic() {
],
);
insta::assert_snapshot!(stdout.replace('\\', "/"), @r###"
D repo/file1
R repo/file1 => repo/file3
M repo/file2
A repo/file3
"###);
insta::assert_snapshot!(stderr.replace('\\', "/"), @r###"
Warning: No matching entries for paths: repo/x, repo/y/z
Expand Down Expand Up @@ -1253,12 +1250,12 @@ fn test_diff_external_file_by_file_tool() {
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
let repo_path = test_env.env_root().join("repo");

std::fs::write(repo_path.join("file1"), "foo\n").unwrap();
std::fs::write(repo_path.join("file2"), "foo\n").unwrap();
std::fs::write(repo_path.join("file1"), "file1\n").unwrap();
std::fs::write(repo_path.join("file2"), "file2\n").unwrap();
test_env.jj_cmd_ok(&repo_path, &["new"]);
std::fs::remove_file(repo_path.join("file1")).unwrap();
std::fs::write(repo_path.join("file2"), "foo\nbar\n").unwrap();
std::fs::write(repo_path.join("file3"), "foo\n").unwrap();
std::fs::write(repo_path.join("file2"), "file2\nfile2\n").unwrap();
std::fs::write(repo_path.join("file3"), "file3\n").unwrap();

let edit_script = test_env.set_up_fake_diff_editor();
std::fs::write(
Expand Down Expand Up @@ -1299,7 +1296,7 @@ fn test_diff_external_file_by_file_tool() {

insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["log", "-p", config]), @r###"
@ rlvkpnrz [email protected] 2001-02-03 08:05:09 39d9055d
@ rlvkpnrz [email protected] 2001-02-03 08:05:09 f12f62fb
│ (no description set)
│ ==
│ file1
Expand All @@ -1313,7 +1310,7 @@ fn test_diff_external_file_by_file_tool() {
│ file3
│ --
│ file3
○ qpvuntsm [email protected] 2001-02-03 08:05:08 0ad4ef22
○ qpvuntsm [email protected] 2001-02-03 08:05:08 6e485984
│ (no description set)
│ ==
│ file1
Expand All @@ -1328,7 +1325,7 @@ fn test_diff_external_file_by_file_tool() {

insta::assert_snapshot!(
test_env.jj_cmd_success(&repo_path, &["show", config]), @r###"
Commit ID: 39d9055d70873099fd924b9af218289d5663eac8
Commit ID: f12f62fb1d73629c1b44abd4d5bbb500d7f8b86c
Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp
Author: Test User <[email protected]> (2001-02-03 08:05:09)
Committer: Test User <[email protected]> (2001-02-03 08:05:09)
Expand Down
Loading

0 comments on commit d988f3f

Please sign in to comment.