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 11, 2024
1 parent 0349d9e commit 5911e5c
Show file tree
Hide file tree
Showing 7 changed files with 317 additions and 72 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### New features

* The following diff formats now include information about copies and moves:
`--color-words`, `--summary`

### Fixed bugs

## [0.20.0] - 2024-08-07
Expand Down
9 changes: 8 additions & 1 deletion cli/src/commit_templater.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1394,8 +1394,15 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T
let path_converter = language.path_converter;
let template = self_property
.map(move |diff| {
let to_tree = diff.to_tree.clone();
diff.into_formatted(move |formatter, _store, tree_diff| {
diff_util::show_diff_summary(formatter, tree_diff, path_converter)
diff_util::show_diff_summary(
formatter,
tree_diff,
path_converter,
&Default::default(),
&to_tree,
)
})
})
.into_template();
Expand Down
105 changes: 74 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::{HashSet, VecDeque};
use std::ops::Range;
use std::path::{Path, PathBuf};
use std::{io, mem};
Expand Down Expand Up @@ -272,17 +272,16 @@ impl<'a> DiffRenderer<'a> {
from_tree: &MergedTree,
to_tree: &MergedTree,
matcher: &dyn Matcher,
_copy_records: &CopyRecords,
copy_records: &CopyRecords,
width: usize,
) -> Result<(), DiffRenderError> {
let store = self.repo.store();
let path_converter = self.path_converter;
for format in &self.formats {
match format {
DiffFormat::Summary => {
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_diff_summary(formatter, tree_diff, path_converter)?;
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
show_diff_summary(formatter, tree_diff, path_converter, copy_records, to_tree)?;
}
DiffFormat::Stat => {
let no_copy_tracking = Default::default();
Expand All @@ -305,8 +304,7 @@ impl<'a> DiffRenderer<'a> {
show_git_diff(formatter, store, tree_diff, *context)?;
}
DiffFormat::ColorWords { context } => {
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records);
show_color_words_diff(formatter, store, tree_diff, path_converter, *context)?;
}
DiffFormat::Tool(tool) => {
Expand Down Expand Up @@ -580,20 +578,28 @@ pub fn show_color_words_diff(
let mut diff_stream = materialized_diff_stream(store, tree_diff);
async {
while let Some(MaterializedTreeDiffEntry {
source: _, // TODO handle copy tracking
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 @@ -604,9 +610,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 @@ -667,9 +673,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 @@ -684,9 +700,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 @@ -1130,33 +1146,60 @@ pub fn show_git_diff(
.block_on()
}

// TODO rework this signature to pass both from_tree and to_tree explicitly
#[instrument(skip_all)]
pub fn show_diff_summary(
formatter: &mut dyn Formatter,
mut tree_diff: TreeDiffStream,
path_converter: &RepoPathUiConverter,
copy_records: &CopyRecords,
to_tree: &MergedTree,
) -> io::Result<()> {
let copied_sources: HashSet<&RepoPath> = copy_records
.iter()
.map(|record| record.source.as_ref())
.collect();

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 after_ui_path = path_converter.format_file_path(&after_path);
if before_path != after_path {
let before_ui_path = path_converter.format_file_path(&before_path);
if to_tree.path_value(&before_path).unwrap().is_absent() {
writeln!(
formatter.labeled("renamed"),
"R {before_ui_path} => {after_ui_path}"
)?
} else {
writeln!(
formatter.labeled("copied"),
"C {before_ui_path} => {after_ui_path}"
)?
}
} else {
// `R` could be interpreted as "renamed"
writeln!(formatter.labeled("removed"), "D {ui_path}")?;
let path = after_ui_path;
match (before.is_present(), after.is_present()) {
(true, true) => writeln!(formatter.labeled("modified"), "M {path}")?,
(false, true) => writeln!(formatter.labeled("added"), "A {path}")?,
(true, false) => {
if !copied_sources.contains(before_path.as_ref()) {
writeln!(formatter.labeled("removed"), "D {path}")?;
}
}
(false, false) => unreachable!(),
}
}
}
Ok(())
Ok::<(), io::Error>(())
}
.block_on()
.block_on()?;
Ok(())
}

struct DiffStat {
Expand Down
33 changes: 13 additions & 20 deletions cli/tests/test_diff_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ fn test_diff_basic() {
1 1: foo
2: bar
2 3: baz quxquux
Added regular file file3:
1: foo
Modified regular file file3 (file1 => file3):
"###);

let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--context=0"]);
Expand All @@ -49,8 +48,7 @@ fn test_diff_basic() {
1 1: foo
2: bar
2 3: baz quxquux
Added regular file file3:
1: foo
Modified regular file file3 (file1 => file3):
"###);

let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--color=debug"]);
Expand All @@ -61,15 +59,13 @@ fn test_diff_basic() {
<<diff removed line_number:: 1>><<diff:: >><<diff added line_number:: 1>><<diff::: foo>>
<<diff:: >><<diff added line_number:: 2>><<diff::: >><<diff added token::bar>>
<<diff removed line_number:: 2>><<diff:: >><<diff added line_number:: 3>><<diff::: baz >><<diff removed token::qux>><<diff added token::quux>><<diff::>>
<<diff header::Added regular file file3:>>
<<diff:: >><<diff added line_number:: 1>><<diff::: >><<diff added token::foo>>
<<diff header::Modified regular file file3 (file1 => file3):>>
"###);

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 +157,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 @@ -200,7 +195,6 @@ fn test_diff_basic() {
// Filter by glob pattern
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "glob:file[12]"]);
insta::assert_snapshot!(stdout, @r###"
D file1
M file2
"###);

Expand All @@ -218,9 +212,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 Expand Up @@ -1253,12 +1246,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 +1292,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 +1306,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 +1321,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
5 changes: 5 additions & 0 deletions lib/src/backend.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,11 @@ impl CopyRecords {
pub fn for_target(&self, target: &RepoPath) -> Option<&CopyRecord> {
self.targets.get(target).and_then(|&i| self.records.get(i))
}

/// Gets all copy records.
pub fn iter(&self) -> impl Iterator<Item = &CopyRecord> + '_ {
self.records.iter()
}
}

/// Error that may occur during backend initialization.
Expand Down
Loading

0 comments on commit 5911e5c

Please sign in to comment.