From 39276dc5b41fa5e756ebb8ae30b4d2ac91995be7 Mon Sep 17 00:00:00 2001
From: Florian Klink <flokli@flokli.de>
Date: Sat, 6 Apr 2024 01:20:22 +0300
Subject: [PATCH] feat(tvix/store/proto): avoid clone in PathInfo::validate()

Have this return a StorePathRef<'_>, rather than a StorePath, and leave
it up to the caller to possibly convert it to a owned StorePath.

This avoids some allocations, if we only want to validate.

Change-Id: I5cf8e246fe02bd4e631f46a5cb86d3f77a728a0d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11361
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>
---
 tvix/store/src/proto/mod.rs            | 14 +++++---------
 tvix/store/src/proto/tests/pathinfo.rs | 17 ++++++++---------
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/tvix/store/src/proto/mod.rs b/tvix/store/src/proto/mod.rs
index 10fde3352..dd3c18d77 100644
--- a/tvix/store/src/proto/mod.rs
+++ b/tvix/store/src/proto/mod.rs
@@ -74,23 +74,20 @@ pub enum ValidatePathInfoError {
 
 /// Parses a root node name.
 ///
-/// On success, this returns the parsed [store_path::StorePath].
+/// On success, this returns the parsed [store_path::StorePathRef].
 /// On error, it returns an error generated from the supplied constructor.
 fn parse_node_name_root<E>(
     name: &[u8],
     err: fn(Vec<u8>, store_path::Error) -> E,
-) -> Result<store_path::StorePath, E> {
-    match store_path::StorePath::from_bytes(name) {
-        Ok(np) => Ok(np),
-        Err(e) => Err(err(name.to_vec(), e)),
-    }
+) -> Result<store_path::StorePathRef<'_>, E> {
+    store_path::StorePathRef::from_bytes(name).map_err(|e| err(name.to_vec(), e))
 }
 
 impl PathInfo {
     /// validate performs some checks on the PathInfo struct,
     /// Returning either a [store_path::StorePath] of the root node, or a
     /// [ValidatePathInfoError].
-    pub fn validate(&self) -> Result<store_path::StorePath, ValidatePathInfoError> {
+    pub fn validate(&self) -> Result<store_path::StorePathRef<'_>, ValidatePathInfoError> {
         // ensure the references have the right number of bytes.
         for (i, reference) in self.references.iter().enumerate() {
             if reference.len() != store_path::DIGEST_SIZE {
@@ -154,8 +151,7 @@ impl PathInfo {
                 // converting to this field.
                 if let Some(deriver) = &narinfo.deriver {
                     store_path::StorePathRef::from_name_and_digest(&deriver.name, &deriver.digest)
-                        .map_err(ValidatePathInfoError::InvalidDeriverField)?
-                        .to_owned();
+                        .map_err(ValidatePathInfoError::InvalidDeriverField)?;
                 }
             }
         }
diff --git a/tvix/store/src/proto/tests/pathinfo.rs b/tvix/store/src/proto/tests/pathinfo.rs
index dca74dc92..2806a8ee1 100644
--- a/tvix/store/src/proto/tests/pathinfo.rs
+++ b/tvix/store/src/proto/tests/pathinfo.rs
@@ -3,8 +3,7 @@ use crate::tests::fixtures::*;
 use bytes::Bytes;
 use data_encoding::BASE64;
 use nix_compat::nixbase32;
-use nix_compat::store_path::{self, StorePath, StorePathRef};
-use std::str::FromStr;
+use nix_compat::store_path::{self, StorePathRef};
 use test_case::test_case;
 use tvix_castore::proto as castorepb;
 
@@ -20,7 +19,7 @@ use tvix_castore::proto as castorepb;
 )]
 fn validate_no_node(
     t_node: Option<castorepb::Node>,
-    t_result: Result<StorePath, ValidatePathInfoError>,
+    t_result: Result<StorePathRef, ValidatePathInfoError>,
 ) {
     // construct the PathInfo object
     let p = PathInfo {
@@ -36,7 +35,7 @@ fn validate_no_node(
         digest: DUMMY_DIGEST.clone().into(),
         size: 0,
     },
-    Ok(StorePath::from_str(DUMMY_NAME).expect("must succeed"));
+    Ok(StorePathRef::from_bytes(DUMMY_NAME.as_bytes()).expect("must succeed"));
     "ok"
 )]
 #[test_case(
@@ -62,7 +61,7 @@ fn validate_no_node(
 )]
 fn validate_directory(
     t_directory_node: castorepb::DirectoryNode,
-    t_result: Result<StorePath, ValidatePathInfoError>,
+    t_result: Result<StorePathRef, ValidatePathInfoError>,
 ) {
     // construct the PathInfo object
     let p = PathInfo {
@@ -81,7 +80,7 @@ fn validate_directory(
         size: 0,
         executable: false,
     },
-    Ok(StorePath::from_str(DUMMY_NAME).expect("must succeed"));
+    Ok(StorePathRef::from_bytes(DUMMY_NAME.as_bytes()).expect("must succeed"));
     "ok"
 )]
 #[test_case(
@@ -107,7 +106,7 @@ fn validate_directory(
 )]
 fn validate_file(
     t_file_node: castorepb::FileNode,
-    t_result: Result<StorePath, ValidatePathInfoError>,
+    t_result: Result<StorePathRef, ValidatePathInfoError>,
 ) {
     // construct the PathInfo object
     let p = PathInfo {
@@ -124,7 +123,7 @@ fn validate_file(
         name: DUMMY_NAME.into(),
         target: "foo".into(),
     },
-    Ok(StorePath::from_str(DUMMY_NAME).expect("must succeed"));
+    Ok(StorePathRef::from_bytes(DUMMY_NAME.as_bytes()).expect("must succeed"));
     "ok"
 )]
 #[test_case(
@@ -140,7 +139,7 @@ fn validate_file(
 )]
 fn validate_symlink(
     t_symlink_node: castorepb::SymlinkNode,
-    t_result: Result<StorePath, ValidatePathInfoError>,
+    t_result: Result<StorePathRef, ValidatePathInfoError>,
 ) {
     // construct the PathInfo object
     let p = PathInfo {