Skip to content

Commit

Permalink
copy-tracking: create CopyRecordMap and add it to diff summaries
Browse files Browse the repository at this point in the history
  • Loading branch information
fowles committed Aug 11, 2024
1 parent e667a2b commit ee6b922
Show file tree
Hide file tree
Showing 15 changed files with 129 additions and 31 deletions.
2 changes: 1 addition & 1 deletion cli/examples/custom-backend/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ impl Backend for JitBackend {

fn get_copy_records(
&self,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
root: &CommitId,
head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/debug/copy_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub fn cmd_debug_copy_detection(
let commit = ws.resolve_single_rev(&args.revision)?;
for parent_id in commit.parent_ids() {
for CopyRecord { target, source, .. } in
block_on_stream(git.get_copy_records(&[], parent_id, commit.id())?)
block_on_stream(git.get_copy_records(None, parent_id, commit.id())?)
.filter_map(|r| r.ok())
{
writeln!(
Expand Down
48 changes: 37 additions & 11 deletions cli/src/commands/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use itertools::Itertools;
use jj_lib::backend::CopyRecords;
use jj_lib::commit::Commit;
use jj_lib::repo::Repo;
use jj_lib::rewrite::merge_commit_trees;
use tracing::instrument;

use crate::cli_util::{print_unmatched_explicit_paths, CommandHelper, RevisionArg};
use crate::cli_util::{
print_unmatched_explicit_paths, CommandHelper, RevisionArg, WorkspaceCommandHelper,
};
use crate::command_error::CommandError;
use crate::diff_util::DiffFormatArgs;
use crate::ui::Ui;
Expand Down Expand Up @@ -59,31 +66,50 @@ pub(crate) fn cmd_diff(
args: &DiffArgs,
) -> Result<(), CommandError> {
let workspace_command = command.workspace_helper(ui)?;
let resolve_revision = |r: &Option<RevisionArg>| {
workspace_command.resolve_single_rev(r.as_ref().unwrap_or(&RevisionArg::AT))
};

let from_tree;
let to_tree;
let mut copy_records = CopyRecords::default();
if args.from.is_some() || args.to.is_some() {
let from =
workspace_command.resolve_single_rev(args.from.as_ref().unwrap_or(&RevisionArg::AT))?;
let from = resolve_revision(&args.from)?;
let to = resolve_revision(&args.to)?;
from_tree = from.tree()?;
let to =
workspace_command.resolve_single_rev(args.to.as_ref().unwrap_or(&RevisionArg::AT))?;
to_tree = to.tree()?;

copy_records.add_records(workspace_command.repo().store().get_copy_records(
None,
from.id(),
to.id(),
)?)?;
} else {
let commit = workspace_command
.resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
from_tree = commit.parent_tree(workspace_command.repo().as_ref())?;
to_tree = commit.tree()?
let to = resolve_revision(&args.revision)?;
let parents: Vec<_> = to.parents().try_collect()?;
from_tree = merge_commit_trees(workspace_command.repo().as_ref(), &parents)?;
to_tree = to.tree()?;

for p in &parents {
copy_records.add_records(workspace_command.repo().store().get_copy_records(
None,
p.id(),
to.id(),
)?)?;
}
}

let diff_renderer = workspace_command.diff_renderer_for(&args.format)?;
let fileset_expression = workspace_command.parse_file_patterns(&args.paths)?;
let matcher = fileset_expression.to_matcher();
let diff_renderer = workspace_command.diff_renderer_for(&args.format)?;
ui.request_pager();
diff_renderer.show_diff(
ui,
ui.stdout_formatter().as_mut(),
&from_tree,
&to_tree,
matcher.as_ref(),
&matcher,
&copy_records,
ui.term_width(),
)?;
print_unmatched_explicit_paths(
Expand Down
1 change: 1 addition & 0 deletions cli/src/commands/interdiff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub(crate) fn cmd_interdiff(
&from_tree,
&to_tree,
matcher.as_ref(),
&Default::default(),
ui.term_width(),
)?;
Ok(())
Expand Down
1 change: 1 addition & 0 deletions cli/src/commands/obslog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ fn show_predecessor_patch(
&predecessor_tree,
&tree,
&EverythingMatcher,
&Default::default(),
width,
)?;
Ok(())
Expand Down
1 change: 1 addition & 0 deletions cli/src/commands/operation/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ fn show_change_diff(
&predecessor_tree,
&tree,
&EverythingMatcher,
&Default::default(),
width,
)?;
}
Expand Down
10 changes: 9 additions & 1 deletion cli/src/commands/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,15 @@ pub(crate) fn cmd_status(
writeln!(formatter, "Working copy changes:")?;
let diff_renderer = workspace_command.diff_renderer(vec![DiffFormat::Summary]);
let width = ui.term_width();
diff_renderer.show_diff(ui, formatter, &parent_tree, &tree, &matcher, width)?;
diff_renderer.show_diff(
ui,
formatter,
&parent_tree,
&tree,
&matcher,
&Default::default(),
width,
)?;
}

// TODO: Conflicts should also be filtered by the `matcher`. See the related
Expand Down
24 changes: 21 additions & 3 deletions cli/src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::{io, mem};

use futures::StreamExt;
use itertools::Itertools;
use jj_lib::backend::{BackendError, TreeValue};
use jj_lib::backend::{BackendError, CopyRecords, TreeValue};
use jj_lib::commit::Commit;
use jj_lib::conflicts::{materialized_diff_stream, MaterializedTreeValue};
use jj_lib::diff::{Diff, DiffHunk};
Expand Down Expand Up @@ -245,10 +245,19 @@ impl<'a> DiffRenderer<'a> {
from_tree: &MergedTree,
to_tree: &MergedTree,
matcher: &dyn Matcher,
copy_records: &CopyRecords,
width: usize,
) -> Result<(), DiffRenderError> {
formatter.with_label("diff", |formatter| {
self.show_diff_inner(ui, formatter, from_tree, to_tree, matcher, width)
self.show_diff_inner(
ui,
formatter,
from_tree,
to_tree,
matcher,
copy_records,
width,
)
})
}

Expand All @@ -259,6 +268,7 @@ impl<'a> DiffRenderer<'a> {
from_tree: &MergedTree,
to_tree: &MergedTree,
matcher: &dyn Matcher,
_copy_records: &CopyRecords,
width: usize,
) -> Result<(), DiffRenderError> {
let store = self.repo.store();
Expand Down Expand Up @@ -324,7 +334,15 @@ impl<'a> DiffRenderer<'a> {
) -> 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, width)
self.show_diff(
ui,
formatter,
&from_tree,
&to_tree,
matcher,
&Default::default(),
width,
)
}
}

Expand Down
43 changes: 41 additions & 2 deletions lib/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@
#![allow(missing_docs)]

use std::any::Any;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};
use std::fmt::Debug;
use std::io::Read;
use std::time::SystemTime;

use async_trait::async_trait;
use futures::executor::block_on_stream;
use futures::stream::BoxStream;
use thiserror::Error;

Expand Down Expand Up @@ -180,6 +181,44 @@ pub struct CopyRecord {
pub source_commit: CommitId,
}

/// A collection of CopyRecords.
#[derive(Default, Debug)]
pub struct CopyRecords {
records: Vec<CopyRecord>,
// Maps from `target` to the index of the target in `records`. Conflicts
// are excluded by keeping an out of range value.
targets: HashMap<RepoPathBuf, usize>,
}

impl CopyRecords {
/// Adds information about a stream of CopyRecords to `self`. A target with
/// multiple conflicts is discarded and treated as not having an origin.
pub fn add_records(
&mut self,
stream: BoxStream<BackendResult<CopyRecord>>,
) -> BackendResult<()> {
for record in block_on_stream(stream) {
let r = record?;
let value = self
.targets
.entry(r.target.clone())
.or_insert(self.records.len());

if *value != self.records.len() {
// TODO: handle conflicts instead of ignoring both sides.
*value = usize::MAX;
}
self.records.push(r);
}
Ok(())
}

/// Gets any copy record associated with a target path.
pub fn for_target(&self, target: &RepoPath) -> Option<&CopyRecord> {
self.targets.get(target).and_then(|&i| self.records.get(i))
}
}

/// Error that may occur during backend initialization.
#[derive(Debug, Error)]
#[error(transparent)]
Expand Down Expand Up @@ -458,7 +497,7 @@ pub trait Backend: Send + Sync + Debug {
/// unnecessary resources.
fn get_copy_records(
&self,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
root: &CommitId,
head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>>;
Expand Down
4 changes: 2 additions & 2 deletions lib/src/git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1255,7 +1255,7 @@ impl Backend for GitBackend {

fn get_copy_records(
&self,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
root_id: &CommitId,
head_id: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down Expand Up @@ -1285,7 +1285,7 @@ impl Backend for GitBackend {
.map_err(|err| to_invalid_utf8_err(err, head_id))?;

let target = RepoPathBuf::from_internal_string(dest);
if !paths.is_empty() && !paths.contains(&target) {
if !paths.map_or(true, |paths| paths.contains(&target)) {
return Ok(None);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/src/local_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ impl Backend for LocalBackend {

fn get_copy_records(
&self,
_paths: &[RepoPathBuf],
_paths: Option<&[RepoPathBuf]>,
_root: &CommitId,
_head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down
2 changes: 1 addition & 1 deletion lib/src/secret_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ impl Backend for SecretBackend {

fn get_copy_records(
&self,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
root: &CommitId,
head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down
2 changes: 1 addition & 1 deletion lib/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl Store {

pub fn get_copy_records(
&self,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
root: &CommitId,
head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down
16 changes: 10 additions & 6 deletions lib/tests/test_git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ fn collect_no_gc_refs(git_repo_path: &Path) -> HashSet<CommitId> {

fn get_copy_records(
store: &Store,
paths: &[RepoPathBuf],
paths: Option<&[RepoPathBuf]>,
a: &Commit,
b: &Commit,
) -> HashMap<String, String> {
Expand Down Expand Up @@ -262,23 +262,27 @@ fn test_copy_detection() {

let store = repo.store();
assert_eq!(
get_copy_records(store, paths, &commit_a, &commit_b),
get_copy_records(store, Some(paths), &commit_a, &commit_b),
HashMap::from([("file1".to_string(), "file0".to_string())])
);
assert_eq!(
get_copy_records(store, paths, &commit_b, &commit_c),
get_copy_records(store, Some(paths), &commit_b, &commit_c),
HashMap::from([("file2".to_string(), "file1".to_string())])
);
assert_eq!(
get_copy_records(store, paths, &commit_a, &commit_c),
get_copy_records(store, Some(paths), &commit_a, &commit_c),
HashMap::from([("file2".to_string(), "file0".to_string())])
);
assert_eq!(
get_copy_records(store, &[paths[1].clone()], &commit_a, &commit_c),
get_copy_records(store, None, &commit_a, &commit_c),
HashMap::from([("file2".to_string(), "file0".to_string())])
);
assert_eq!(
get_copy_records(store, Some(&[paths[1].clone()]), &commit_a, &commit_c),
HashMap::default(),
);
assert_eq!(
get_copy_records(store, paths, &commit_c, &commit_c),
get_copy_records(store, Some(paths), &commit_c, &commit_c),
HashMap::default(),
);
}
2 changes: 1 addition & 1 deletion lib/testutils/src/test_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ impl Backend for TestBackend {

fn get_copy_records(
&self,
_paths: &[RepoPathBuf],
_paths: Option<&[RepoPathBuf]>,
_root: &CommitId,
_head: &CommitId,
) -> BackendResult<BoxStream<BackendResult<CopyRecord>>> {
Expand Down

0 comments on commit ee6b922

Please sign in to comment.