This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11389

rhg: Port Python’s `ui.configlist` as `Config::get_list`
ClosedPublic

Authored by SimonSapin on Sep 3 2021, 10:38 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG6961eca0b3ee: rhg: Port Python’s `ui.configlist` as `Config::get_list`
Summary

This new method is not used yet outside of its own unit tests,
so this changeset should make no observable change.

The Rust parser implementation attempts to exactly replicate the behavior of
the Python one, even in edge cases where that behavior is… surprising.
New unit tests capture some of these edge cases.

This started as a line-by-line port. The main changes are:

  • Pass around a parser mode enum instead of parser functions
  • Inline the whole parser into one function
  • Use [u8]::get which returns an Option, instead of indexing after explicitly checking the length.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Sep 3 2021, 10:38 AM
SimonSapin added a comment.Sep 3 2021, 10:52 AM

Dear reviewer, when trying to find cases where this code has different behavior than that of mercurial/utils/stringutil.py (or convince yourself that it’s the same), I advise for your own mental well-being not to question too much what that behavior is.

Some of it is arguably bugs, but this patch does not get into what existing config file could potentially rely on those bugs and would be broken if fix the parsing algorithm.

SimonSapin added a comment.Sep 3 2021, 11:07 AM

Inline the whole parser into one function

Almost, I had to split back up the recursive part. But _parse_plain and _parse_quote are still inlined, which simplifies things slightly.

SimonSapin updated this revision to Diff 30183.Sep 4 2021, 3:25 AM
SimonSapin added a comment.Sep 4 2021, 3:27 AM

Updated manually rather than trough Baymax since the windows-py3-pyox job in https://foss.heptapod.net/octobus/mercurial-devel/-/pipelines/26551 failed 4 times in a row with different error messages.

Alphare accepted this revision.Sep 8 2021, 8:48 AM
Alphare added a subscriber: Alphare.

The Rust replication seems good to me, I don't think the performance aspect is too important either. I'm not glad we have to have this, but I don't have a better solution!

rust/hg-core/src/config/values.rs
241

nit: I find that these are more readable by using r#"this kind of string"#

This revision is now accepted and ready to land.Sep 8 2021, 8:48 AM
SimonSapin added inline comments.Sep 9 2021, 9:48 AM
rust/hg-core/src/config/values.rs
241

Yes, not having to mentally do backslash-unescaping would certainly be easier for human readers. However these raw strings literals do not support any backslash-escaping so \t and \x0b would have to be replaced by their literal characters whose difference is less visible to human readers. We could have only those test cases use non-raw literals, but keeping all test cases consistent with each other feels more valuable to me.

Revision Contents
Changeset List

Diff 30195

rust/hg-core/src/config/config.rs

) -> Option<impl Iterator<Item = &[u8]>> { ) -> Option<impl Iterator<Item = &[u8]>> {
self.get(section, item).map(|value| { self.get(section, item).map(|value| {
value value
.split(|&byte| byte == b',') .split(|&byte| byte == b',')
.map(|component| component.trim()) .map(|component| component.trim())
}) })
} }
/// If there is an `item` value in `section`, parse and return a list of
/// byte strings.
pub fn get_list(
&self,
section: &[u8],
item: &[u8],
) -> Option<Vec<Vec<u8>>> {
self.get(section, item).map(values::parse_list)
}
/// Returns the raw value bytes of the first one found, or `None`. /// Returns the raw value bytes of the first one found, or `None`.
pub fn get(&self, section: &[u8], item: &[u8]) -> Option<&[u8]> { pub fn get(&self, section: &[u8], item: &[u8]) -> Option<&[u8]> {
self.get_inner(section, item) self.get_inner(section, item)
.map(|(_, value)| value.bytes.as_ref()) .map(|(_, value)| value.bytes.as_ref())
} }
/// Returns the layer and the value of the first one found, or `None`. /// Returns the layer and the value of the first one found, or `None`.
fn get_inner( fn get_inner(

rust/hg-core/src/config/values.rs

//! Parsing functions for various type of configuration values. //! Parsing functions for various type of configuration values.
//! //!
//! Returning `None` indicates a syntax error. Using a `Result` would be more //! Returning `None` indicates a syntax error. Using a `Result` would be more
//! correct but would take more boilerplate for converting between error types, //! correct but would take more boilerplate for converting between error types,
//! compared to using `.ok()` on inner results of various error types to //! compared to using `.ok()` on inner results of various error types to
//! convert them all to options. The `Config::get_parse` method later converts //! convert them all to options. The `Config::get_parse` method later converts
//! those options to results with `ConfigValueParseError`, which contains //! those options to results with `ConfigValueParseError`, which contains
//! details about where the value came from (but omits details of what’s //! details about where the value came from (but omits details of what’s
//! invalid inside the value). //! invalid inside the value).
use crate::utils::SliceExt;
pub(super) fn parse_bool(v: &[u8]) -> Option<bool> { pub(super) fn parse_bool(v: &[u8]) -> Option<bool> {
match v.to_ascii_lowercase().as_slice() { match v.to_ascii_lowercase().as_slice() {
b"1" | b"yes" | b"true" | b"on" | b"always" => Some(true), b"1" | b"yes" | b"true" | b"on" | b"always" => Some(true),
b"0" | b"no" | b"false" | b"off" | b"never" => Some(false), b"0" | b"no" | b"false" | b"off" | b"never" => Some(false),
_ => None, _ => None,
} }
} }
} else { } else {
return None; return None;
} }
} }
} }
value.parse().ok() value.parse().ok()
} }
/// Parse a config value as a list of sub-values.
///
/// Ported from `parselist` in `mercurial/utils/stringutil.py`
// Note: keep behavior in sync with the Python one.
// Note: this could return `Vec<Cow<[u8]>>` instead and borrow `input` when
// possible (when there’s no backslash-escapes) but this is probably not worth
// the complexity as config is presumably not accessed inside
// preformance-sensitive loops.
pub(super) fn parse_list(input: &[u8]) -> Vec<Vec<u8>> {
// Port of Python’s `value.lstrip(b' ,\n')`
// TODO: is this really what we want?
let input =
input.trim_start_matches(|b| b == b' ' || b == b',' || b == b'\n');
parse_list_without_trim_start(input)
}
fn parse_list_without_trim_start(input: &[u8]) -> Vec<Vec<u8>> {
// Start of port of Python’s `_configlist`
let input = input.trim_end_matches(|b| b == b' ' || b == b',');
if input.is_empty() {
return Vec::new();
}
// Just to make “a string” less confusable with “a list of strings”.
type ByteString = Vec<u8>;
// These correspond to Python’s…
let mut mode = ParserMode::Plain; // `parser`
let mut values = Vec::new(); // `parts[:-1]`
let mut next_value = ByteString::new(); // `parts[-1]`
let mut offset = 0; // `offset`
// Setting `parser` to `None` is instead handled by returning immediately
enum ParserMode {
Plain,
Quoted,
}
loop {
match mode {
ParserMode::Plain => {
// Start of port of Python’s `_parse_plain`
let mut whitespace = false;
while let Some(&byte) = input.get(offset) {
if is_space(byte) || byte == b',' {
whitespace = true;
offset += 1;
} else {
break;
}
}
if let Some(&byte) = input.get(offset) {
if whitespace {
values.push(std::mem::take(&mut next_value))
}
if byte == b'"' && next_value.is_empty() {
mode = ParserMode::Quoted;
} else {
if byte == b'"' && next_value.ends_with(b"\\") {
next_value.pop();
}
next_value.push(byte);
}
offset += 1;
} else {
values.push(next_value);
return values;
}
}
ParserMode::Quoted => {
// Start of port of Python’s `_parse_quote`
if let Some(&byte) = input.get(offset) {
if byte == b'"' {
// The input contains a quoted zero-length value `""`
debug_assert_eq!(next_value, b"");
values.push(std::mem::take(&mut next_value));
offset += 1;
while let Some(&byte) = input.get(offset) {
if is_space(byte) || byte == b',' {
offset += 1;
} else {
break;
}
}
mode = ParserMode::Plain;
continue;
}
}
while let Some(&byte) = input.get(offset) {
if byte == b'"' {
break;
}
if byte == b'\\' && input.get(offset + 1) == Some(&b'"') {
next_value.push(b'"');
offset += 2;
} else {
next_value.push(byte);
offset += 1;
}
}
if offset >= input.len() {
// We didn’t find a closing double-quote,
// so treat the opening one as part of an unquoted value
// instead of delimiting the start of a quoted value.
// `next_value` may have had some backslash-escapes
// unescaped. TODO: shouldn’t we use a slice of `input`
// instead?
let mut real_values =
parse_list_without_trim_start(&next_value);
if let Some(first) = real_values.first_mut() {
first.insert(0, b'"');
// Drop `next_value`
values.extend(real_values)
} else {
next_value.push(b'"');
values.push(next_value);
}
return values;
}
// We’re not at the end of the input, which means the `while`
// loop above ended at at double quote. Skip
// over that.
offset += 1;
while let Some(&byte) = input.get(offset) {
if byte == b' ' || byte == b',' {
offset += 1;
} else {
break;
}
}
if offset >= input.len() {
values.push(next_value);
return values;
}
if offset + 1 == input.len() && input[offset] == b'"' {
next_value.push(b'"');
offset += 1;
} else {
values.push(std::mem::take(&mut next_value));
}
mode = ParserMode::Plain;
}
}
}
// https://docs.python.org/3/library/stdtypes.html?#bytes.isspace
fn is_space(byte: u8) -> bool {
if let b' ' | b'\t' | b'\n' | b'\r' | b'\x0b' | b'\x0c' = byte {
true
} else {
false
}
}
}
#[test]
fn test_parse_list() {
// Make `assert_eq` error messages nicer
fn as_strings(values: &[Vec<u8>]) -> Vec<String> {
values
.iter()
.map(|v| std::str::from_utf8(v.as_ref()).unwrap().to_owned())
.collect()
}
macro_rules! assert_parse_list {
( $input: expr => [ $( $output: expr ),* ] ) => {
assert_eq!(
as_strings(&parse_list($input)),
as_strings(&[ $( Vec::from(&$output[..]) ),* ]),
);
}
}
// Keep these Rust tests in sync with the Python ones in
// `tests/test-config-parselist.py`
assert_parse_list!(b"" => []);
assert_parse_list!(b"," => []);
assert_parse_list!(b"A" => [b"A"]);
assert_parse_list!(b"B,B" => [b"B", b"B"]);
assert_parse_list!(b", C, ,C," => [b"C", b"C"]);
assert_parse_list!(b"\"" => [b"\""]);
assert_parse_list!(b"\"\"" => [b"", b""]);
assert_parse_list!(b"D,\"" => [b"D", b"\""]);
assert_parse_list!(b"E,\"\"" => [b"E", b"", b""]);
AlphareUnsubmitted
Not Done

nit: I find that these are more readable by using r#"this kind of string"#

Alphare: nit: I find that these are more readable by using `r#"this kind of string"#`
SimonSapinAuthorUnsubmitted
Done

Yes, not having to mentally do backslash-unescaping would certainly be easier for human readers. However these raw strings literals do not support any backslash-escaping so \t and \x0b would have to be replaced by their literal characters whose difference is less visible to human readers. We could have only those test cases use non-raw literals, but keeping all test cases consistent with each other feels more valuable to me.

SimonSapin: Yes, not having to mentally do backslash-unescaping would certainly be easier for human readers.
assert_parse_list!(b"\"F,F\"" => [b"F,F"]);
assert_parse_list!(b"\"G,G" => [b"\"G", b"G"]);
assert_parse_list!(b"\"H \\\",\\\"H" => [b"\"H", b",", b"H"]);
assert_parse_list!(b"I,I\"" => [b"I", b"I\""]);
assert_parse_list!(b"J,\"J" => [b"J", b"\"J"]);
assert_parse_list!(b"K K" => [b"K", b"K"]);
assert_parse_list!(b"\"K\" K" => [b"K", b"K"]);
assert_parse_list!(b"L\tL" => [b"L", b"L"]);
assert_parse_list!(b"\"L\"\tL" => [b"L", b"", b"L"]);
assert_parse_list!(b"M\x0bM" => [b"M", b"M"]);
assert_parse_list!(b"\"M\"\x0bM" => [b"M", b"", b"M"]);
assert_parse_list!(b"\"N\" , ,\"" => [b"N\""]);
assert_parse_list!(b"\" ,O, " => [b"\"", b"O"]);
}
#[test] #[test]
fn test_parse_byte_size() { fn test_parse_byte_size() {
assert_eq!(parse_byte_size(b""), None); assert_eq!(parse_byte_size(b""), None);
assert_eq!(parse_byte_size(b"b"), None); assert_eq!(parse_byte_size(b"b"), None);
assert_eq!(parse_byte_size(b"12"), Some(12)); assert_eq!(parse_byte_size(b"12"), Some(12));
assert_eq!(parse_byte_size(b"12b"), Some(12)); assert_eq!(parse_byte_size(b"12b"), Some(12));
assert_eq!(parse_byte_size(b"12 b"), Some(12)); assert_eq!(parse_byte_size(b"12 b"), Some(12));

tests/test-config-parselist.py

  • This file was added.
"""
List-valued configuration keys have an ad-hoc microsyntax. From `hg help config`:
> List values are separated by whitespace or comma, except when values are
> placed in double quotation marks:
>
> allow_read = "John Doe, PhD", brian, betty
>
> Quotation marks can be escaped by prefixing them with a backslash. Only
> quotation marks at the beginning of a word is counted as a quotation
> (e.g., ``foo"bar baz`` is the list of ``foo"bar`` and ``baz``).
That help documentation is fairly light on details, the actual parser has many
other edge cases. This test tries to cover them.
"""
from mercurial.utils import stringutil
def assert_parselist(input, expected):
result = stringutil.parselist(input)
if result != expected:
raise AssertionError(
"parse_input(%r)\n got %r\nexpected %r"
% (input, result, expected)
)
# Keep these Python tests in sync with the Rust ones in `rust/hg-core/src/config/values.rs`
assert_parselist(b'', [])
assert_parselist(b',', [])
assert_parselist(b'A', [b'A'])
assert_parselist(b'B,B', [b'B', b'B'])
assert_parselist(b', C, ,C,', [b'C', b'C'])
assert_parselist(b'"', [b'"'])
assert_parselist(b'""', [b'', b''])
assert_parselist(b'D,"', [b'D', b'"'])
assert_parselist(b'E,""', [b'E', b'', b''])
assert_parselist(b'"F,F"', [b'F,F'])
assert_parselist(b'"G,G', [b'"G', b'G'])
assert_parselist(b'"H \\",\\"H', [b'"H', b',', b'H'])
assert_parselist(b'I,I"', [b'I', b'I"'])
assert_parselist(b'J,"J', [b'J', b'"J'])
assert_parselist(b'K K', [b'K', b'K'])
assert_parselist(b'"K" K', [b'K', b'K'])
assert_parselist(b'L\tL', [b'L', b'L'])
assert_parselist(b'"L"\tL', [b'L', b'', b'L'])
assert_parselist(b'M\x0bM', [b'M', b'M'])
assert_parselist(b'"M"\x0bM', [b'M', b'', b'M'])
assert_parselist(b'"N" , ,"', [b'N"'])
assert_parselist(b'" ,O, ', [b'"', b'O'])