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; I'm not sure why. Strangely, adding an `visit_i64`
method *did* work, but then requires handling of overflow, etc.

Instead, just don't bother with any of that, and only parse strings, and use
`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, considering how heavy-handed serde
can be for small use cases like this.

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

@@ -15,6 +15,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 `max-new-file-size="0"` and `max-new-file-size=0`
+  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

@@ -357,27 +357,22 @@ impl<'de> serde::Deserialize<'de> for HumanByteSize {
                 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)?;
-                Ok(HumanByteSize(bytes))
+                let res = u64::from_str_radix(v, 10);
+                match res {
+                    Ok(bytes) => Ok(HumanByteSize(bytes)),
+                    Err(_) => {
+                        let bytes = parse_human_byte_size(v).map_err(Error::custom)?;
+                        Ok(HumanByteSize(bytes))
+                    }
+                }
             }
         }
 
-        if deserializer.is_human_readable() {
-            deserializer.deserialize_any(Visitor)
-        } else {
-            deserializer.deserialize_u64(Visitor)
-        }
+        deserializer.deserialize_str(Visitor)
     }
 }