surf: improve Moved type for Diff's

In some cases a moved file can also be modified and in these cases
there will be a diff as well as new Oid's for the two sides of the
diff.

Add DiffFile to the Moved type, changing the insert_moved method to
accept the DiffFile's and the DiffContent.

When serializing a Moved, if the old and the new are the same then the
serialization of the diff files and content are skipped -- same as the
old behaviour. Otherwise, the two sides are serialized along with the
content.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
This commit is contained in:
Fintan Halpenny
2023-08-30 17:10:25 +01:00
parent bc06ee0773
commit 6ee2815cbf
3 changed files with 255 additions and 15 deletions
+27 -8
View File
@@ -129,12 +129,21 @@ impl Diff {
self.files.push(diff);
}
pub fn insert_moved(&mut self, old_path: PathBuf, new_path: PathBuf) {
pub fn insert_moved(
&mut self,
old_path: PathBuf,
new_path: PathBuf,
old: DiffFile,
new: DiffFile,
content: DiffContent,
) {
self.update_stats(&DiffContent::Empty);
let diff = FileDiff::Moved(Moved {
old_path,
new_path,
diff: DiffContent::Empty,
old,
new,
diff: content,
});
self.files.push(diff);
}
@@ -187,8 +196,10 @@ pub struct Deleted {
pub struct Moved {
/// The old path to this file, relative to the repository root.
pub old_path: PathBuf,
pub old: DiffFile,
/// The new path to this file, relative to the repository root.
pub new_path: PathBuf,
pub new: DiffFile,
pub diff: DiffContent,
}
@@ -198,12 +209,20 @@ impl Serialize for Moved {
where
S: Serializer,
{
let mut state = serializer.serialize_struct("Moved", 2)?;
state.serialize_field("oldPath", &self.old_path)?;
state.serialize_field("newPath", &self.new_path)?;
// `DiffContent` is not serialized yet for `Moved`, only
// to keep the serialization same as before.
state.end()
if self.old == self.new {
let mut state = serializer.serialize_struct("Moved", 2)?;
state.serialize_field("oldPath", &self.old_path)?;
state.serialize_field("newPath", &self.new_path)?;
state.end()
} else {
let mut state = serializer.serialize_struct("Moved", 5)?;
state.serialize_field("oldPath", &self.old_path)?;
state.serialize_field("newPath", &self.new_path)?;
state.serialize_field("old", &self.old)?;
state.serialize_field("new", &self.new)?;
state.serialize_field("diff", &self.diff)?;
state.end()
}
}
}
+23 -7
View File
@@ -235,7 +235,7 @@ impl<'a> TryFrom<git2::Diff<'a>> for Diff {
Delta::Added => created(&mut diff, &git_diff, idx, &delta)?,
Delta::Deleted => deleted(&mut diff, &git_diff, idx, &delta)?,
Delta::Modified => modified(&mut diff, &git_diff, idx, &delta)?,
Delta::Renamed => renamed(&mut diff, &delta)?,
Delta::Renamed => renamed(&mut diff, &git_diff, idx, &delta)?,
Delta::Copied => copied(&mut diff, &delta)?,
status => {
return Err(error::Diff::DeltaUnhandled(status));
@@ -323,17 +323,33 @@ fn modified(
}
}
fn renamed(diff: &mut Diff, delta: &git2::DiffDelta<'_>) -> Result<(), error::Diff> {
let old = delta
fn renamed(
diff: &mut Diff,
git_diff: &git2::Diff<'_>,
idx: usize,
delta: &git2::DiffDelta<'_>,
) -> Result<(), error::Diff> {
let old_path = delta
.old_file()
.path()
.ok_or(error::Diff::PathUnavailable)?;
let new = delta
.ok_or(error::Diff::PathUnavailable)?
.to_path_buf();
let new_path = delta
.new_file()
.path()
.ok_or(error::Diff::PathUnavailable)?;
.ok_or(error::Diff::PathUnavailable)?
.to_path_buf();
let patch = git2::Patch::from_diff(git_diff, idx)?;
let old = DiffFile::try_from(delta.old_file())?;
let new = DiffFile::try_from(delta.new_file())?;
diff.insert_moved(old.to_path_buf(), new.to_path_buf());
if let Some(patch) = patch {
diff.insert_moved(old_path, new_path, old, new, DiffContent::try_from(patch)?);
} else if delta.new_file().is_binary() {
diff.insert_moved(old_path, new_path, old, new, DiffContent::Binary);
} else {
diff.insert_moved(old_path, new_path, old, new, DiffContent::Empty);
}
Ok(())
}
+205
View File
@@ -358,6 +358,211 @@ fn test_diff_serde() -> Result<(), Error> {
Ok(())
}
#[test]
fn test_rename_with_changes() {
let buf = r"
diff --git a/radicle/src/node/tracking/config.rs b/radicle-node/src/service/tracking.rs
similarity index 96%
rename from radicle/src/node/tracking/config.rs
rename to radicle-node/src/service/tracking.rs
index 3f69208f3..cbc843c82 100644
--- a/radicle/src/node/tracking/config.rs
+++ b/radicle-node/src/service/tracking.rs
@@ -5,15 +5,17 @@ use std::ops;
use log::error;
use thiserror::Error;
-use crate::crypto::PublicKey;
-use crate::identity::IdentityError;
+use radicle::crypto::PublicKey;
+use radicle::identity::IdentityError;
+use radicle::storage::{Namespaces, ReadRepository as _, ReadStorage};
+
+use crate::prelude::Id;
+use crate::service::NodeId;
+
pub use crate::node::tracking::store;
pub use crate::node::tracking::store::Config as Store;
pub use crate::node::tracking::store::Error;
pub use crate::node::tracking::{Alias, Node, Policy, Repo, Scope};
-use crate::node::NodeId;
-use crate::prelude::Id;
-use crate::storage::{Namespaces, ReadRepository as _, ReadStorage};
#[derive(Debug, Error)]
pub enum NamespacesError {
";
let diff = git2::Diff::from_buffer(buf.as_bytes()).unwrap();
let diff = Diff::try_from(diff).unwrap();
let json = serde_json::json!(
{
"added": [],
"copied": [],
"deleted": [],
"modified": [],
"moved": [
{
"diff": {
"eof": "noneMissing",
"hunks": [
{
"header": "@@ -5,15 +5,17 @@ use std::ops;\n",
"lines": [
{
"line": "use log::error;\n",
"lineNoNew": 5,
"lineNoOld": 5,
"type": "context",
},
{
"line": "use thiserror::Error;\n",
"lineNoNew": 6,
"lineNoOld": 6,
"type": "context",
},
{
"line": "\n",
"lineNoNew": 7,
"lineNoOld": 7,
"type": "context",
},
{
"line": "use crate::crypto::PublicKey;\n",
"lineNo": 8,
"type": "deletion",
},
{
"line": "use crate::identity::IdentityError;\n",
"lineNo": 9,
"type": "deletion",
},
{
"line": "use radicle::crypto::PublicKey;\n",
"lineNo": 8,
"type": "addition",
},
{
"line": "use radicle::identity::IdentityError;\n",
"lineNo": 9,
"type": "addition",
},
{
"line": "use radicle::storage::{Namespaces, ReadRepository as _, ReadStorage};\n",
"lineNo": 10,
"type": "addition",
},
{
"line": "\n",
"lineNo": 11,
"type": "addition",
},
{
"line": "use crate::prelude::Id;\n",
"lineNo": 12,
"type": "addition",
},
{
"line": "use crate::service::NodeId;\n",
"lineNo": 13,
"type": "addition",
},
{
"line": "\n",
"lineNo": 14,
"type": "addition",
},
{
"line": "pub use crate::node::tracking::store;\n",
"lineNoNew": 15,
"lineNoOld": 10,
"type": "context",
},
{
"line": "pub use crate::node::tracking::store::Config as Store;\n",
"lineNoNew": 16,
"lineNoOld": 11,
"type": "context",
},
{
"line": "pub use crate::node::tracking::store::Error;\n",
"lineNoNew": 17,
"lineNoOld": 12,
"type": "context",
},
{
"line": "pub use crate::node::tracking::{Alias, Node, Policy, Repo, Scope};\n",
"lineNoNew": 18,
"lineNoOld": 13,
"type": "context",
},
{
"line": "use crate::node::NodeId;\n",
"lineNo": 14,
"type": "deletion",
},
{
"line": "use crate::prelude::Id;\n",
"lineNo": 15,
"type": "deletion",
},
{
"line": "use crate::storage::{Namespaces, ReadRepository as _, ReadStorage};\n",
"lineNo": 16,
"type": "deletion",
},
{
"line": "\n",
"lineNoNew": 19,
"lineNoOld": 17,
"type": "context",
},
{
"line": "#[derive(Debug, Error)]\n",
"lineNoNew": 20,
"lineNoOld": 18,
"type": "context",
},
{
"line": "pub enum NamespacesError {\n",
"lineNoNew": 21,
"lineNoOld": 19,
"type": "context",
},
],
"new": {
"end": 22,
"start": 5,
},
"old": {
"end": 20,
"start": 5,
},
},
],
"type": "plain",
},
"new": {
"mode": "blob",
"oid": "cbc843c820000000000000000000000000000000",
},
"newPath": "radicle-node/src/service/tracking.rs",
"old": {
"mode": "blob",
"oid": "3f69208f30000000000000000000000000000000",
},
"oldPath": "radicle/src/node/tracking/config.rs",
},
],
"stats": {
"deletions": 0,
"filesChanged": 1,
"insertions": 0,
},
});
assert_eq!(serde_json::to_value(diff).unwrap(), json);
}
// A possible false positive is being hit here for this clippy
// warning. Tracking issue:
// https://github.com/rust-lang/rust-clippy/issues/11402