From 13a8c43a7eabae3a3481e2c884af1f65513e0465 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; 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 Change-Id: I8dafa2358d039ad1c07e9a512c1d10fed5845738 --- CHANGELOG.md | 5 +++++ cli/tests/test_working_copy.rs | 2 +- lib/src/settings.rs | 23 +++++++++-------------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4c28ebcf7..208a90e48a 100644 --- a/CHANGELOG.md +++ b/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 `snapshot.max-new-file-size="0"` and + `snapshot.max-new-file-size=0` are now equivalent. + ## [0.16.0] - 2024-04-03 ### Deprecations diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs index e54437b28b..461125b7f6 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 1eb572469b..475f091f4c 100644 --- a/lib/src/settings.rs +++ b/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(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)?; - Ok(HumanByteSize(bytes)) + let res = v.parse::(); + 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) } }