Skip to content

Commit bfdd29c

Browse files
ByronByron
committed
refactor!: add tree() function to diff a tree.
Remove `tree::Changes()` and the ceremony required to diff a tree.
1 parent 2bbfe6d commit bfdd29c

File tree

11 files changed

+135
-152
lines changed

11 files changed

+135
-152
lines changed

‎gix-diff/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ pub mod rewrites;
4545

4646
///
4747
pub mod tree;
48+
pub use tree::function::diff as tree;
4849

4950
///
5051
#[cfg(feature = "blob")]

‎gix-diff/src/tree/changes.rs renamed to ‎gix-diff/src/tree/function.rs

Lines changed: 114 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -1,136 +1,117 @@
11
use std::{borrow::BorrowMut, collections::VecDeque};
22

3-
use gix_object::{tree::EntryRef, FindExt};
3+
use gix_object::{tree::EntryRef, FindExt, TreeRefIter};
44

55
use crate::tree::visit::{ChangeId, Relation};
6-
use crate::{
7-
tree,
8-
tree::{visit::Change, TreeInfoTuple},
9-
};
6+
use crate::tree::{visit::Change, Error, State, TreeInfoTuple, Visit};
107

11-
/// The error returned by [`tree::Changes::needed_to_obtain()`].
12-
#[derive(Debug, thiserror::Error)]
13-
#[allow(missing_docs)]
14-
pub enum Error {
15-
#[error(transparent)]
16-
Find(#[from] gix_object::find::existing_iter::Error),
17-
#[error("The delegate cancelled the operation")]
18-
Cancelled,
19-
#[error(transparent)]
20-
EntriesDecode(#[from] gix_object::decode::Error),
21-
}
22-
23-
impl tree::Changes<'_> {
24-
/// Calculate the changes that would need to be applied to `self` to get `other` using `objects` to obtain objects as needed for traversal.
25-
///
26-
/// * The `state` maybe owned or mutably borrowed to allow reuses allocated data structures through multiple runs.
27-
/// * `locate` is a function `f(object_id, &mut buffer) -> Option<TreeIter>` to return a `TreeIter` for the given object id backing
28-
/// its data in the given buffer. Returning `None` is unexpected as these trees are obtained during iteration, and in a typical
29-
/// database errors are not expected either which is why the error case is omitted. To allow proper error reporting, [`Error::Find`]
30-
/// should be converted into a more telling error.
31-
/// * `delegate` will receive the computed changes, see the [`Visit`][`tree::Visit`] trait for more information on what to expect.
32-
///
33-
/// # Notes
34-
///
35-
/// * To obtain progress, implement it within the `delegate`.
36-
/// * Tree entries are expected to be ordered using [`tree-entry-comparison`][git_cmp_c] (the same [in Rust][git_cmp_rs])
37-
/// * it does a breadth first iteration as buffer space only fits two trees, the current one on the one we compare with.
38-
/// * does not do rename tracking but attempts to reduce allocations to zero (so performance is mostly determined
39-
/// by the delegate implementation which should be as specific as possible. Rename tracking can be computed on top of the changes
40-
/// received by the `delegate`.
41-
/// * cycle checking is not performed, but can be performed in the delegate which can return [`tree::visit::Action::Cancel`] to stop the traversal.
42-
/// * [`std::mem::ManuallyDrop`] is used because `Peekable` is needed. When using it as wrapper around our no-drop iterators, all of the sudden
43-
/// borrowcheck complains as Drop is present (even though it's not)
44-
///
45-
/// [git_cmp_c]: https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/tree-diff.c#L49:L65
46-
/// [git_cmp_rs]: https://github.com/Byron/gitoxide/blob/a4d5f99c8dc99bf814790928a3bf9649cd99486b/gix-object/src/mutable/tree.rs#L52-L55
47-
pub fn needed_to_obtain<StateMut>(
48-
mut self,
49-
other: gix_object::TreeRefIter<'_>,
50-
mut state: StateMut,
51-
objects: impl gix_object::Find,
52-
delegate: &mut impl tree::Visit,
53-
) -> Result<(), Error>
54-
where
55-
StateMut: BorrowMut<tree::State>,
56-
{
57-
let state = state.borrow_mut();
58-
state.clear();
59-
let mut lhs_entries = peekable(self.0.take().unwrap_or_default());
60-
let mut rhs_entries = peekable(other);
61-
let mut relation = None;
62-
let mut pop_path = false;
8+
/// Calculate the changes that would need to be applied to `lhs` to get `rhs` using `objects` to obtain objects as needed for traversal.
9+
/// `state` can be used between multiple calls to re-use memory.
10+
///
11+
/// * The `state` maybe owned or mutably borrowed to allow reuses allocated data structures through multiple runs.
12+
/// * `delegate` will receive the computed changes, see the [`Visit`] trait for more information on what to expect.
13+
///
14+
/// # Notes
15+
///
16+
/// * `lhs` can be an empty tree to simulate what would happen if the left-hand side didn't exist.
17+
/// * To obtain progress, implement it within the `delegate`.
18+
/// * Tree entries are expected to be ordered using [`tree-entry-comparison`][git_cmp_c] (the same [in Rust][git_cmp_rs])
19+
/// * it does a breadth first iteration as buffer space only fits two trees, the current one on the one we compare with.
20+
/// * does not do rename tracking but attempts to reduce allocations to zero (so performance is mostly determined
21+
/// by the delegate implementation which should be as specific as possible. Rename tracking can be computed on top of the changes
22+
/// received by the `delegate`.
23+
/// * cycle checking is not performed, but can be performed in the delegate which can return
24+
/// [`tree::visit::Action::Cancel`](crate::tree::visit::Action::Cancel) to stop the traversal.
25+
///
26+
/// [git_cmp_c]: https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/tree-diff.c#L49:L65
27+
/// [git_cmp_rs]: https://github.com/Byron/gitoxide/blob/a4d5f99c8dc99bf814790928a3bf9649cd99486b/gix-object/src/mutable/tree.rs#L52-L55
28+
#[doc(alias = "diff_tree_to_tree", alias = "git2")]
29+
pub fn diff<StateMut>(
30+
lhs: TreeRefIter<'_>,
31+
rhs: TreeRefIter<'_>,
32+
mut state: StateMut,
33+
objects: impl gix_object::Find,
34+
delegate: &mut impl Visit,
35+
) -> Result<(), Error>
36+
where
37+
StateMut: BorrowMut<State>,
38+
{
39+
let state = state.borrow_mut();
40+
state.clear();
41+
let mut lhs_entries = peekable(lhs);
42+
let mut rhs_entries = peekable(rhs);
43+
let mut relation = None;
44+
let mut pop_path = false;
6345

64-
loop {
65-
if pop_path {
66-
delegate.pop_path_component();
67-
}
68-
pop_path = true;
46+
loop {
47+
if pop_path {
48+
delegate.pop_path_component();
49+
}
50+
pop_path = true;
6951

70-
match (lhs_entries.next(), rhs_entries.next()) {
71-
(None, None) => {
72-
match state.trees.pop_front() {
73-
Some((None, Some(rhs), relation_to_propagate)) => {
74-
delegate.pop_front_tracked_path_and_set_current();
75-
relation = relation_to_propagate;
76-
rhs_entries = peekable(objects.find_tree_iter(&rhs, &mut state.buf2)?);
77-
}
78-
Some((Some(lhs), Some(rhs), relation_to_propagate)) => {
79-
delegate.pop_front_tracked_path_and_set_current();
80-
lhs_entries = peekable(objects.find_tree_iter(&lhs, &mut state.buf1)?);
81-
rhs_entries = peekable(objects.find_tree_iter(&rhs, &mut state.buf2)?);
82-
relation = relation_to_propagate;
83-
}
84-
Some((Some(lhs), None, relation_to_propagate)) => {
85-
delegate.pop_front_tracked_path_and_set_current();
86-
lhs_entries = peekable(objects.find_tree_iter(&lhs, &mut state.buf1)?);
87-
relation = relation_to_propagate;
88-
}
89-
Some((None, None, _)) => unreachable!("BUG: it makes no sense to fill the stack with empties"),
90-
None => return Ok(()),
91-
};
92-
pop_path = false;
93-
}
94-
(Some(lhs), Some(rhs)) => {
95-
use std::cmp::Ordering::*;
96-
let (lhs, rhs) = (lhs?, rhs?);
97-
match compare(&lhs, &rhs) {
98-
Equal => handle_lhs_and_rhs_with_equal_filenames(
99-
lhs,
100-
rhs,
101-
&mut state.trees,
102-
&mut state.change_id,
103-
relation,
104-
delegate,
105-
)?,
106-
Less => catchup_lhs_with_rhs(
107-
&mut lhs_entries,
108-
lhs,
109-
rhs,
110-
&mut state.trees,
111-
&mut state.change_id,
112-
relation,
113-
delegate,
114-
)?,
115-
Greater => catchup_rhs_with_lhs(
116-
&mut rhs_entries,
117-
lhs,
118-
rhs,
119-
&mut state.trees,
120-
&mut state.change_id,
121-
relation,
122-
delegate,
123-
)?,
52+
match (lhs_entries.next(), rhs_entries.next()) {
53+
(None, None) => {
54+
match state.trees.pop_front() {
55+
Some((None, Some(rhs), relation_to_propagate)) => {
56+
delegate.pop_front_tracked_path_and_set_current();
57+
relation = relation_to_propagate;
58+
rhs_entries = peekable(objects.find_tree_iter(&rhs, &mut state.buf2)?);
12459
}
60+
Some((Some(lhs), Some(rhs), relation_to_propagate)) => {
61+
delegate.pop_front_tracked_path_and_set_current();
62+
lhs_entries = peekable(objects.find_tree_iter(&lhs, &mut state.buf1)?);
63+
rhs_entries = peekable(objects.find_tree_iter(&rhs, &mut state.buf2)?);
64+
relation = relation_to_propagate;
65+
}
66+
Some((Some(lhs), None, relation_to_propagate)) => {
67+
delegate.pop_front_tracked_path_and_set_current();
68+
lhs_entries = peekable(objects.find_tree_iter(&lhs, &mut state.buf1)?);
69+
relation = relation_to_propagate;
70+
}
71+
Some((None, None, _)) => unreachable!("BUG: it makes no sense to fill the stack with empties"),
72+
None => return Ok(()),
73+
};
74+
pop_path = false;
75+
}
76+
(Some(lhs), Some(rhs)) => {
77+
use std::cmp::Ordering::*;
78+
let (lhs, rhs) = (lhs?, rhs?);
79+
match compare(&lhs, &rhs) {
80+
Equal => handle_lhs_and_rhs_with_equal_filenames(
81+
lhs,
82+
rhs,
83+
&mut state.trees,
84+
&mut state.change_id,
85+
relation,
86+
delegate,
87+
)?,
88+
Less => catchup_lhs_with_rhs(
89+
&mut lhs_entries,
90+
lhs,
91+
rhs,
92+
&mut state.trees,
93+
&mut state.change_id,
94+
relation,
95+
delegate,
96+
)?,
97+
Greater => catchup_rhs_with_lhs(
98+
&mut rhs_entries,
99+
lhs,
100+
rhs,
101+
&mut state.trees,
102+
&mut state.change_id,
103+
relation,
104+
delegate,
105+
)?,
125106
}
126-
(Some(lhs), None) => {
127-
let lhs = lhs?;
128-
delete_entry_schedule_recursion(lhs, &mut state.trees, &mut state.change_id, relation, delegate)?;
129-
}
130-
(None, Some(rhs)) => {
131-
let rhs = rhs?;
132-
add_entry_schedule_recursion(rhs, &mut state.trees, &mut state.change_id, relation, delegate)?;
133-
}
107+
}
108+
(Some(lhs), None) => {
109+
let lhs = lhs?;
110+
delete_entry_schedule_recursion(lhs, &mut state.trees, &mut state.change_id, relation, delegate)?;
111+
}
112+
(None, Some(rhs)) => {
113+
let rhs = rhs?;
114+
add_entry_schedule_recursion(rhs, &mut state.trees, &mut state.change_id, relation, delegate)?;
134115
}
135116
}
136117
}
@@ -150,7 +131,7 @@ fn delete_entry_schedule_recursion(
150131
queue: &mut VecDeque<TreeInfoTuple>,
151132
change_id: &mut ChangeId,
152133
relation_to_propagate: Option<Relation>,
153-
delegate: &mut impl tree::Visit,
134+
delegate: &mut impl Visit,
154135
) -> Result<(), Error> {
155136
delegate.push_path_component(entry.filename);
156137
let relation = relation_to_propagate.or_else(|| {
@@ -182,7 +163,7 @@ fn add_entry_schedule_recursion(
182163
queue: &mut VecDeque<TreeInfoTuple>,
183164
change_id: &mut ChangeId,
184165
relation_to_propagate: Option<Relation>,
185-
delegate: &mut impl tree::Visit,
166+
delegate: &mut impl Visit,
186167
) -> Result<(), Error> {
187168
delegate.push_path_component(entry.filename);
188169
let relation = relation_to_propagate.or_else(|| {
@@ -210,13 +191,13 @@ fn add_entry_schedule_recursion(
210191
}
211192

212193
fn catchup_rhs_with_lhs(
213-
rhs_entries: &mut IteratorType<gix_object::TreeRefIter<'_>>,
194+
rhs_entries: &mut IteratorType<TreeRefIter<'_>>,
214195
lhs: EntryRef<'_>,
215196
rhs: EntryRef<'_>,
216197
queue: &mut VecDeque<TreeInfoTuple>,
217198
change_id: &mut ChangeId,
218199
relation_to_propagate: Option<Relation>,
219-
delegate: &mut impl tree::Visit,
200+
delegate: &mut impl Visit,
220201
) -> Result<(), Error> {
221202
use std::cmp::Ordering::*;
222203
add_entry_schedule_recursion(rhs, queue, change_id, relation_to_propagate, delegate)?;
@@ -259,13 +240,13 @@ fn catchup_rhs_with_lhs(
259240
}
260241

261242
fn catchup_lhs_with_rhs(
262-
lhs_entries: &mut IteratorType<gix_object::TreeRefIter<'_>>,
243+
lhs_entries: &mut IteratorType<TreeRefIter<'_>>,
263244
lhs: EntryRef<'_>,
264245
rhs: EntryRef<'_>,
265246
queue: &mut VecDeque<TreeInfoTuple>,
266247
change_id: &mut ChangeId,
267248
relation_to_propagate: Option<Relation>,
268-
delegate: &mut impl tree::Visit,
249+
delegate: &mut impl Visit,
269250
) -> Result<(), Error> {
270251
use std::cmp::Ordering::*;
271252
delete_entry_schedule_recursion(lhs, queue, change_id, relation_to_propagate, delegate)?;
@@ -313,7 +294,7 @@ fn handle_lhs_and_rhs_with_equal_filenames(
313294
queue: &mut VecDeque<TreeInfoTuple>,
314295
change_id: &mut ChangeId,
315296
relation_to_propagate: Option<Relation>,
316-
delegate: &mut impl tree::Visit,
297+
delegate: &mut impl Visit,
317298
) -> Result<(), Error> {
318299
match (lhs.mode.is_tree(), rhs.mode.is_tree()) {
319300
(true, true) => {
@@ -413,7 +394,7 @@ fn handle_lhs_and_rhs_with_equal_filenames(
413394
Ok(())
414395
}
415396

416-
type IteratorType<I> = std::mem::ManuallyDrop<std::iter::Peekable<I>>;
397+
type IteratorType<I> = std::iter::Peekable<I>;
417398

418399
fn to_child(r: Option<Relation>) -> Option<Relation> {
419400
r.map(|r| match r {
@@ -423,7 +404,7 @@ fn to_child(r: Option<Relation>) -> Option<Relation> {
423404
}
424405

425406
fn peekable<I: Iterator>(iter: I) -> IteratorType<I> {
426-
std::mem::ManuallyDrop::new(iter.peekable())
407+
iter.peekable()
427408
}
428409

429410
#[cfg(test)]

‎gix-diff/src/tree/mod.rs

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,21 @@
11
use crate::tree::visit::Relation;
22
use bstr::BStr;
33
use gix_hash::ObjectId;
4-
use gix_object::{bstr::BString, TreeRefIter};
4+
use gix_object::bstr::BString;
55
use std::collections::VecDeque;
66

7+
/// The error returned by [`tree()`](super::tree()).
8+
#[derive(Debug, thiserror::Error)]
9+
#[allow(missing_docs)]
10+
pub enum Error {
11+
#[error(transparent)]
12+
Find(#[from] gix_object::find::existing_iter::Error),
13+
#[error("The delegate cancelled the operation")]
14+
Cancelled,
15+
#[error(transparent)]
16+
EntriesDecode(#[from] gix_object::decode::Error),
17+
}
18+
719
/// A trait to allow responding to a traversal designed to figure out the [changes](visit::Change)
820
/// to turn tree A into tree B.
921
pub trait Visit {
@@ -21,7 +33,7 @@ pub trait Visit {
2133
fn visit(&mut self, change: visit::Change) -> visit::Action;
2234
}
2335

24-
/// The state required to visit [Changes] to be instantiated with `State::default()`.
36+
/// The state required to run [tree-diffs](super::tree()).
2537
#[derive(Default, Clone)]
2638
pub struct State {
2739
buf1: Vec<u8>,
@@ -41,20 +53,7 @@ impl State {
4153
}
4254
}
4355

44-
/// An iterator over changes of a tree, instantiated using `Changes::from(…)`.
45-
pub struct Changes<'a>(Option<TreeRefIter<'a>>);
46-
47-
impl<'a, T> From<T> for Changes<'a>
48-
where
49-
T: Into<Option<TreeRefIter<'a>>>,
50-
{
51-
fn from(v: T) -> Self {
52-
Changes(v.into())
53-
}
54-
}
55-
56-
///
57-
pub mod changes;
56+
pub(super) mod function;
5857

5958
///
6059
pub mod visit;

‎gix-diff/tests/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ rust-version = "1.65"
1414
[[test]]
1515
doctest = false
1616
name = "diff"
17-
path = "diff.rs"
17+
path = "diff/main.rs"
1818

1919
[dev-dependencies]
2020
gix-diff = { path = ".." }

‎gix-diff/tests/blob/mod.rs renamed to ‎gix-diff/tests/diff/blob/mod.rs

File renamed without changes.

‎gix-diff/tests/blob/pipeline.rs renamed to ‎gix-diff/tests/diff/blob/pipeline.rs

File renamed without changes.

‎gix-diff/tests/blob/platform.rs renamed to ‎gix-diff/tests/diff/blob/platform.rs

File renamed without changes.

‎gix-diff/tests/diff.rs renamed to ‎gix-diff/tests/diff/main.rs

File renamed without changes.

‎gix-diff/tests/rewrites/mod.rs renamed to ‎gix-diff/tests/diff/rewrites/mod.rs

File renamed without changes.

‎gix-diff/tests/rewrites/tracker.rs renamed to ‎gix-diff/tests/diff/rewrites/tracker.rs

File renamed without changes.

0 commit comments

Comments
 (0)