feat(tvix/castore/proto): add owned conv to castore::Directory

Replace the hand-rolled code comparing names with a try_fold. Also, make
it slightly stricter here, detecting duplicates in the same fields
earlier.

Change-Id: I9c560838ece88c3b8b339249a8ecbf3b05969538
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12226
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: edef <edef@edef.eu>
Tested-by: BuildkiteCI
This commit is contained in:
Florian Klink 2024-08-17 18:23:44 +03:00 committed by clbot
parent 96832c0411
commit 0cfe2aaf6a
2 changed files with 86 additions and 67 deletions

View file

@ -1,4 +1,4 @@
use std::str; use std::{cmp::Ordering, str};
use prost::Message; use prost::Message;
@ -66,79 +66,76 @@ impl Directory {
} }
} }
/// Accepts a name, and a mutable reference to the previous name.
/// If the passed name is larger than the previous one, the reference is updated.
/// If it's not, an error is returned.
fn update_if_lt_prev<'n>(prev_name: &mut &'n [u8], name: &'n [u8]) -> Result<(), DirectoryError> {
if *name < **prev_name {
return Err(DirectoryError::WrongSorting(bytes::Bytes::copy_from_slice(
name,
)));
}
*prev_name = name;
Ok(())
}
// TODO: add a proper owned version here that moves various fields
impl TryFrom<Directory> for crate::Directory { impl TryFrom<Directory> for crate::Directory {
type Error = DirectoryError; type Error = DirectoryError;
fn try_from(value: Directory) -> Result<Self, Self::Error> { fn try_from(value: Directory) -> Result<Self, Self::Error> {
(&value).try_into() // Check directories, files and symlinks are sorted
} // We'll notice duplicates across all three fields when constructing the Directory.
} // FUTUREWORK: use is_sorted() once stable, and/or implement the producer for
// [crate::Directory::try_from_iter] iterating over all three and doing all checks inline.
impl TryFrom<&Directory> for crate::Directory { value
type Error = DirectoryError; .directories
.iter()
fn try_from(directory: &Directory) -> Result<crate::Directory, DirectoryError> { .try_fold(&b""[..], |prev_name, e| {
let mut dir = crate::Directory::new(); match e.name.as_ref().cmp(prev_name) {
Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
let mut last_file_name: &[u8] = b""; Ordering::Equal => {
Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
// TODO: this currently loops over all three types separately, rather }
// than peeking and picking from where would be the next. Ordering::Greater => Ok(e.name.as_ref()),
}
for file in directory.files.iter().map(move |file| { })?;
update_if_lt_prev(&mut last_file_name, &file.name).map(|()| file.clone()) value.files.iter().try_fold(&b""[..], |prev_name, e| {
}) { match e.name.as_ref().cmp(prev_name) {
let file = file?; Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
Ordering::Equal => {
let (name, node) = Node { Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
node: Some(node::Node::File(file)), }
Ordering::Greater => Ok(e.name.as_ref()),
} }
.into_name_and_node()?; })?;
value.symlinks.iter().try_fold(&b""[..], |prev_name, e| {
dir.add(name, node)?; match e.name.as_ref().cmp(prev_name) {
} Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
let mut last_directory_name: &[u8] = b""; Ordering::Equal => {
for directory in directory.directories.iter().map(move |directory| { Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
update_if_lt_prev(&mut last_directory_name, &directory.name).map(|()| directory.clone()) }
}) { Ordering::Greater => Ok(e.name.as_ref()),
let directory = directory?;
let (name, node) = Node {
node: Some(node::Node::Directory(directory)),
} }
.into_name_and_node()?; })?;
dir.add(name, node)?; let mut elems: Vec<(PathComponent, crate::Node)> =
} Vec::with_capacity(value.directories.len() + value.files.len() + value.symlinks.len());
let mut last_symlink_name: &[u8] = b"";
for symlink in directory.symlinks.iter().map(move |symlink| {
update_if_lt_prev(&mut last_symlink_name, &symlink.name).map(|()| symlink.clone())
}) {
let symlink = symlink?;
let (name, node) = Node { for e in value.directories {
node: Some(node::Node::Symlink(symlink)), elems.push(
} Node {
.into_name_and_node()?; node: Some(node::Node::Directory(e)),
}
dir.add(name, node)?; .into_name_and_node()?,
);
} }
Ok(dir) for e in value.files {
elems.push(
Node {
node: Some(node::Node::File(e)),
}
.into_name_and_node()?,
)
}
for e in value.symlinks {
elems.push(
Node {
node: Some(node::Node::Symlink(e)),
}
.into_name_and_node()?,
)
}
crate::Directory::try_from_iter(elems)
} }
} }

View file

@ -155,7 +155,7 @@ fn validate_invalid_names() {
{ {
let d = Directory { let d = Directory {
directories: vec![DirectoryNode { directories: vec![DirectoryNode {
name: "".into(), name: b"\0"[..].into(),
digest: DUMMY_DIGEST.to_vec().into(), digest: DUMMY_DIGEST.to_vec().into(),
size: 42, size: 42,
}], }],
@ -163,7 +163,7 @@ fn validate_invalid_names() {
}; };
match crate::Directory::try_from(d).expect_err("must fail") { match crate::Directory::try_from(d).expect_err("must fail") {
DirectoryError::InvalidName(n) => { DirectoryError::InvalidName(n) => {
assert_eq!(n.as_ref(), b"") assert_eq!(n.as_ref(), b"\0")
} }
_ => panic!("unexpected error"), _ => panic!("unexpected error"),
}; };
@ -282,7 +282,7 @@ fn validate_sorting() {
} }
} }
// "a" exists twice, bad. // "a" exists twice (same types), bad.
{ {
let d = Directory { let d = Directory {
directories: vec![ directories: vec![
@ -307,6 +307,28 @@ fn validate_sorting() {
} }
} }
// "a" exists twice (different types), bad.
{
let d = Directory {
directories: vec![DirectoryNode {
name: "a".into(),
digest: DUMMY_DIGEST.to_vec().into(),
size: 42,
}],
symlinks: vec![SymlinkNode {
name: "a".into(),
target: "b".into(),
}],
..Default::default()
};
match crate::Directory::try_from(d).expect_err("must fail") {
DirectoryError::DuplicateName(s) => {
assert_eq!(s.as_ref(), b"a");
}
_ => panic!("unexpected error"),
}
}
// "a" comes before "b", all good. // "a" comes before "b", all good.
{ {
let d = Directory { let d = Directory {