cli: allow snapshot.max-new-file-size to be a raw u64

Previously, this command would work:

    jj --config-toml='snapshot.max-new-file-size="1"' st

And is equivalent to this:

    jj --config-toml='snapshot.max-new-file-size="1B"' st

But this would not work, despite looking like it should:

    jj --config-toml='snapshot.max-new-file-size=1' st

This is extremely confusing for users.

This config value is deserialized via serde; and while the `HumanByteSize`
struct allegedly implemented Serde's `visit_u64` method, it was not called by
the deserialize visitor. Strangely, adding an `visit_i64` method *did* work, but
then requires handling of overflow, etc. This is likely because TOML integers
are naturally specified in `i64`.

Instead, just don't bother with any of that; implement a `TryFrom<String>`
instance for `HumanByteSize` that uses `u64::from_str` to try parsing the string
immediately; *then* fall back to `parse_human_byte_size` if that doesn't work.
This not only fixes the behavior but, IMO, is much simpler to reason about; we
get our `Deserialize` instance for free from the `TryFrom` instance.

Finally, this adjusts the test for `max-new-file-size` to now use a raw integer
literal, to ensure it doesn't regress. (There are already in-crate tests for
parsing the human readable strings.)

Signed-off-by: Austin Seipp <[email protected]>
Change-Id: I8dafa2358d039ad1c07e9a512c1d10fed5845738

CHANGELOG.md

@@ -18,6 +18,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed bugs
 
+* The `snapshot.max-new-file-size` option can now handle raw integer literals,
+  interpreted as a number of bytes, where previously it could only handle string
+  literals. This means that `snapshot.max-new-file-size="1"` and
+  `snapshot.max-new-file-size=1` are now equivalent.
+
 ## [0.16.0] - 2024-04-03
 
 ### Deprecations

cli/tests/test_working_copy.rs

@@ -20,7 +20,7 @@ fn test_snapshot_large_file() {
     test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
     let repo_path = test_env.env_root().join("repo");
 
-    test_env.add_config(r#"snapshot.max-new-file-size = "10""#);
+    test_env.add_config(r#"snapshot.max-new-file-size = 10"#);
     std::fs::write(repo_path.join("large"), "a lot of text").unwrap();
     let stderr = test_env.jj_cmd_failure(&repo_path, &["files"]);
     insta::assert_snapshot!(stderr, @r###"

lib/src/settings.rs

@@ -331,7 +331,8 @@ impl<T> ConfigResultExt<T> for Result<T, config::ConfigError> {
 }
 
 /// A size in bytes optionally formatted/serialized with binary prefixes
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, serde::Deserialize)]
+#[serde(try_from = "String")]
 pub struct HumanByteSize(pub u64);
 
 impl std::fmt::Display for HumanByteSize {
@@ -341,43 +342,18 @@ impl std::fmt::Display for HumanByteSize {
     }
 }
 
-impl<'de> serde::Deserialize<'de> for HumanByteSize {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: serde::Deserializer<'de>,
-    {
-        use serde::de::Error;
+impl TryFrom<String> for HumanByteSize {
+    type Error = String;
 
-        struct Visitor;
-
-        impl<'de> serde::de::Visitor<'de> for Visitor {
-            type Value = HumanByteSize;
-
-            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
-                write!(formatter, "a size in bytes with an optional binary unit")
-            }
-
-            fn visit_u64<E>(self, v: u64) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                Ok(HumanByteSize(v))
-            }
-
-            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                let bytes = parse_human_byte_size(v).map_err(Error::custom)?;
+    fn try_from(value: String) -> Result<Self, Self::Error> {
+        let res = value.parse::<u64>();
+        match res {
+            Ok(bytes) => Ok(HumanByteSize(bytes)),
+            Err(_) => {
+                let bytes = parse_human_byte_size(&value)?;
                 Ok(HumanByteSize(bytes))
             }
         }
-
-        if deserializer.is_human_readable() {
-            deserializer.deserialize_any(Visitor)
-        } else {
-            deserializer.deserialize_u64(Visitor)
-        }
     }
 }