From ddfdf5e3572671c3d2559bfa0c19811720368744 Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Thu, 28 Mar 2024 15:45:31 -0500 Subject: [PATCH] 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` 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 Change-Id: I8dafa2358d039ad1c07e9a512c1d10fed5845738 --- CHANGELOG.md | 4 ++++ cli/tests/test_working_copy.rs | 2 +- lib/src/settings.rs | 44 ++++++++-------------------------- 3 files changed, 15 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1a673d3d..4ae03aa2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 rebase the children of the revision being split if they had other parents (i.e. if the child was a merge). +* 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 diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs index e54437b28..461125b7f 100644 --- a/cli/tests/test_working_copy.rs +++ b/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###" diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 1eb572469..380b305b4 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -331,7 +331,8 @@ impl ConfigResultExt for Result { } /// 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(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - use serde::de::Error; +impl TryFrom 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(self, v: u64) -> Result - where - E: Error, - { - Ok(HumanByteSize(v)) - } - - fn visit_str(self, v: &str) -> Result - where - E: Error, - { - let bytes = parse_human_byte_size(v).map_err(Error::custom)?; + fn try_from(value: String) -> Result { + let res = value.parse::(); + 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) - } } }