From 85816f2c9bcf589406220fbff825314f11e9a51e Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sat, 30 Nov 2024 16:38:29 +0900
Subject: [PATCH] settings: explicitly convert integer to HumanByteSize

Since config::Value type was lax about data types, an integer value could be
deserialized through a string. This won't apply to new toml_edit-based value
types.
---
 cli/tests/test_working_copy.rs | 15 ++++++++++++++
 lib/src/settings.rs            | 38 +++++++++++++++++++++++++++-------
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs
index 826293b3d2..9f6b0fda89 100644
--- a/cli/tests/test_working_copy.rs
+++ b/cli/tests/test_working_copy.rs
@@ -54,6 +54,21 @@ fn test_snapshot_large_file() {
         This will increase the maximum file size allowed for new files, for this command only.
     "###);
 
+    // test invalid configuration
+    let stderr = test_env.jj_cmd_failure(
+        &repo_path,
+        &[
+            "file",
+            "list",
+            "--config-toml=snapshot.max-new-file-size = []",
+        ],
+    );
+    insta::assert_snapshot!(stderr, @r"
+    Config error: Invalid type or value for snapshot.max-new-file-size
+    Caused by: Expected a positive integer or a string in '<number><unit>' form
+    For help, see https://martinvonz.github.io/jj/latest/config/.
+    ");
+
     // No error if we disable auto-tracking of the path
     test_env.add_config(r#"snapshot.auto-track = 'none()'"#);
     let stdout = test_env.jj_cmd_success(&repo_path, &["file", "list"]);
diff --git a/lib/src/settings.rs b/lib/src/settings.rs
index 061baf8b63..8477385d5a 100644
--- a/lib/src/settings.rs
+++ b/lib/src/settings.rs
@@ -237,8 +237,8 @@ impl UserSettings {
 
     pub fn max_new_file_size(&self) -> Result<u64, ConfigGetError> {
         let cfg = self
-            .get::<HumanByteSize>("snapshot.max-new-file-size")
-            .map(|x| x.0);
+            .get_value_with("snapshot.max-new-file-size", TryInto::try_into)
+            .map(|HumanByteSize(x)| x);
         match cfg {
             Ok(0) => Ok(u64::MAX),
             x @ Ok(_) => x,
@@ -338,8 +338,7 @@ impl JJRng {
 }
 
 /// A size in bytes optionally formatted/serialized with binary prefixes
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, serde::Deserialize)]
-#[serde(try_from = "String")]
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
 pub struct HumanByteSize(pub u64);
 
 impl std::fmt::Display for HumanByteSize {
@@ -363,11 +362,18 @@ impl FromStr for HumanByteSize {
     }
 }
 
-impl TryFrom<String> for HumanByteSize {
+impl TryFrom<ConfigValue> for HumanByteSize {
     type Error = &'static str;
 
-    fn try_from(value: String) -> Result<Self, Self::Error> {
-        value.parse()
+    fn try_from(value: ConfigValue) -> Result<Self, Self::Error> {
+        if let Ok(n) = value.clone().into_int() {
+            let n = u64::try_from(n).map_err(|_| "Integer out of range")?;
+            Ok(HumanByteSize(n))
+        } else if let Ok(s) = value.into_string() {
+            s.parse()
+        } else {
+            Err("Expected a positive integer or a string in '<number><unit>' form")
+        }
     }
 }
 
@@ -398,6 +404,8 @@ fn parse_human_byte_size(v: &str) -> Result<u64, &'static str> {
 
 #[cfg(test)]
 mod tests {
+    use assert_matches::assert_matches;
+
     use super::*;
 
     #[test]
@@ -422,4 +430,20 @@ mod tests {
         );
         assert_eq!(parse_human_byte_size(""), Err("must start with a number"));
     }
+
+    #[test]
+    fn byte_size_from_config_value() {
+        assert_eq!(
+            HumanByteSize::try_from(ConfigValue::from(42)).unwrap(),
+            HumanByteSize(42)
+        );
+        assert_eq!(
+            HumanByteSize::try_from(ConfigValue::from("42K")).unwrap(),
+            HumanByteSize(42 * 1024)
+        );
+        assert_matches!(
+            HumanByteSize::try_from(ConfigValue::from(-1)),
+            Err("Integer out of range")
+        );
+    }
 }