# HG changeset patch # User Mitchell Kember <mkember@janestreet.com> # Date 1740500964 18000 # Tue Feb 25 11:29:24 2025 -0500 # Node ID 08500079488831c37eec8c08ffb8930c55c02a37 # Parent d81714a1c88d5143d3999afaa6e4eab4f32f590e # EXP-Topic rust-whitespace rust-utils: remove CleanWhitespace::None Instead we use Option<CleanWhitespace>. This is in preparation for a follow-up change that reimplements clean_whitespace, where the None case felt awkward. diff --git a/rust/hg-core/src/operations/annotate.rs b/rust/hg-core/src/operations/annotate.rs --- a/rust/hg-core/src/operations/annotate.rs +++ b/rust/hg-core/src/operations/annotate.rs @@ -26,7 +26,7 @@ pub struct AnnotateOptions { pub treat_binary_as_text: bool, pub follow_copies: bool, - pub whitespace: CleanWhitespace, + pub whitespace: Option<CleanWhitespace>, } /// The final result of annotating a file. @@ -72,11 +72,14 @@ /// Cleans `data` based on `whitespace` and then splits into lines. fn split( data: Vec<u8>, - whitespace: CleanWhitespace, + whitespace: Option<CleanWhitespace>, ) -> Result<Self, HgError> { - let data = match clean_whitespace(&data, whitespace) { - Cow::Borrowed(_) => data, - Cow::Owned(data) => data, + let data = match whitespace { + None => data, + Some(ws) => match clean_whitespace(&data, ws) { + Cow::Borrowed(_) => data, + Cow::Owned(data) => data, + }, }; Self::try_new(data, |data| bdiff::split_lines(data)) } @@ -323,11 +326,8 @@ // Step 3: Read files and split lines. Do the base file with and without // whitespace cleaning. Do the rest of the files in parallel with rayon. let base_file_original_lines = match options.whitespace { - CleanWhitespace::None => None, - _ => Some(OwnedLines::split( - base_file_data.clone(), - CleanWhitespace::None, - )?), + None => None, + _ => Some(OwnedLines::split(base_file_data.clone(), None)?), }; graph[base_id].file = AnnotatedFileState::Read(OwnedLines::split( base_file_data, diff --git a/rust/hg-core/src/utils/strings.rs b/rust/hg-core/src/utils/strings.rs --- a/rust/hg-core/src/utils/strings.rs +++ b/rust/hg-core/src/utils/strings.rs @@ -304,8 +304,6 @@ /// Options for [`clean_whitespace`]. #[derive(Copy, Clone)] pub enum CleanWhitespace { - /// Do nothing. - None, /// Remove whitespace at ends of lines. AtEol, /// Collapse consecutive whitespace characters into a single space. @@ -326,7 +324,6 @@ Regex::new(r"[ \t\r]+").expect("valid regex"); } let replacement: &[u8] = match how { - CleanWhitespace::None => return Cow::Borrowed(text), CleanWhitespace::AtEol => return AT_EOL.replace_all(text, b""), CleanWhitespace::Collapse => b" ", CleanWhitespace::All => b"", diff --git a/rust/rhg/src/commands/annotate.rs b/rust/rhg/src/commands/annotate.rs --- a/rust/rhg/src/commands/annotate.rs +++ b/rust/rhg/src/commands/annotate.rs @@ -161,15 +161,15 @@ treat_binary_as_text: args.get_flag("text"), follow_copies: !args.get_flag("no-follow"), whitespace: if args.get_flag("ignore-all-space") { - CleanWhitespace::All + Some(CleanWhitespace::All) } else if args.get_flag("ignore-space-change") { - CleanWhitespace::Collapse + Some(CleanWhitespace::Collapse) } else if args.get_flag("ignore-space-at-eol") { - CleanWhitespace::AtEol + Some(CleanWhitespace::AtEol) } else { // We ignore the --ignore-blank-lines flag (present for consistency // with other commands) since it has no effect on annotate. - CleanWhitespace::None + None }, }; # HG changeset patch # User Mitchell Kember <mkember@janestreet.com> # Date 1740500698 18000 # Tue Feb 25 11:24:58 2025 -0500 # Node ID 6c894c2129a814576f9a54b5260e730e75034750 # Parent 08500079488831c37eec8c08ffb8930c55c02a37 # EXP-Topic rust-whitespace rust-utils: make clean_whitespace mutate input This makes clean_whitespace take a &mut Vec<u8> instead of returning Cow<[u8]>. Where before it returned Cow::Borrowed, now it is a no-op. This is in preparation for a follow-up change that reimplements clean_whitespace without regexes. diff --git a/rust/hg-core/src/operations/annotate.rs b/rust/hg-core/src/operations/annotate.rs --- a/rust/hg-core/src/operations/annotate.rs +++ b/rust/hg-core/src/operations/annotate.rs @@ -1,5 +1,3 @@ -use std::borrow::Cow; - use crate::{ bdiff::{self, Lines}, errors::HgError, @@ -71,16 +69,12 @@ impl OwnedLines { /// Cleans `data` based on `whitespace` and then splits into lines. fn split( - data: Vec<u8>, + mut data: Vec<u8>, whitespace: Option<CleanWhitespace>, ) -> Result<Self, HgError> { - let data = match whitespace { - None => data, - Some(ws) => match clean_whitespace(&data, ws) { - Cow::Borrowed(_) => data, - Cow::Owned(data) => data, - }, - }; + if let Some(ws) = whitespace { + clean_whitespace(&mut data, ws); + } Self::try_new(data, |data| bdiff::split_lines(data)) } diff --git a/rust/hg-core/src/utils/strings.rs b/rust/hg-core/src/utils/strings.rs --- a/rust/hg-core/src/utils/strings.rs +++ b/rust/hg-core/src/utils/strings.rs @@ -313,8 +313,7 @@ } /// Normalizes whitespace in text so that it won't apppear in diffs. -/// Returns `Cow::Borrowed(text)` if the result is unchanged. -pub fn clean_whitespace(text: &[u8], how: CleanWhitespace) -> Cow<[u8]> { +pub fn clean_whitespace(text: &mut Vec<u8>, how: CleanWhitespace) { lazy_static! { // To match wsclean in mdiff.py, this includes "\f". static ref AT_EOL: Regex = @@ -324,24 +323,23 @@ Regex::new(r"[ \t\r]+").expect("valid regex"); } let replacement: &[u8] = match how { - CleanWhitespace::AtEol => return AT_EOL.replace_all(text, b""), + CleanWhitespace::AtEol => { + replace_all_mut(&AT_EOL, text, b""); + return; + } CleanWhitespace::Collapse => b" ", CleanWhitespace::All => b"", }; - let text = MULTIPLE.replace_all(text, replacement); - replace_all_cow(&AT_EOL, text, b"") + replace_all_mut(&MULTIPLE, text, replacement); + replace_all_mut(&AT_EOL, text, b""); } -/// Helper to call [`Regex::replace_all`] with `Cow` as input and output. -fn replace_all_cow<'a>( - regex: &Regex, - haystack: Cow<'a, [u8]>, - replacement: &[u8], -) -> Cow<'a, [u8]> { - match haystack { - Cow::Borrowed(haystack) => regex.replace_all(haystack, replacement), - Cow::Owned(haystack) => { - Cow::Owned(regex.replace_all(&haystack, replacement).into_owned()) +/// Helper to make [`Regex::replace_all`] mutate a vector. +fn replace_all_mut(regex: &Regex, haystack: &mut Vec<u8>, replacement: &[u8]) { + match regex.replace_all(haystack, replacement) { + Cow::Borrowed(_) => {} + Cow::Owned(result) => { + *haystack = result; } } } # HG changeset patch # User Mitchell Kember <mkember@janestreet.com> # Date 1741017527 18000 # Mon Mar 03 10:58:47 2025 -0500 # Node ID 422c8b0a17ab172938a878959b02e5f3219eaef6 # Parent 6c894c2129a814576f9a54b5260e730e75034750 # EXP-Topic rust-whitespace rust-utils: add tests for clean_whitespace This is in preparation for a follow-up change that reimplements the function. diff --git a/rust/hg-core/src/utils/strings.rs b/rust/hg-core/src/utils/strings.rs --- a/rust/hg-core/src/utils/strings.rs +++ b/rust/hg-core/src/utils/strings.rs @@ -373,4 +373,57 @@ assert_eq!(short_user(b"First Last <user@example.com>"), b"user"); assert_eq!(short_user(b"First Last <user.name@example.com>"), b"user"); } + + fn clean_ws(text: &[u8], how: CleanWhitespace) -> Vec<u8> { + let mut vec = text.to_vec(); + clean_whitespace(&mut vec, how); + vec + } + + #[test] + fn test_clean_whitespace_at_eol() { + // To match wsclean in mdiff.py, CleanWhitespace::AtEol only removes + // the final line's trailing whitespace if it ends in \n. + use CleanWhitespace::AtEol; + assert_eq!(clean_ws(b"", AtEol), b""); + assert_eq!(clean_ws(b" ", AtEol), b" "); + assert_eq!(clean_ws(b" ", AtEol), b" "); + assert_eq!(clean_ws(b"A", AtEol), b"A"); + assert_eq!(clean_ws(b"\n\n\n", AtEol), b"\n\n\n"); + assert_eq!(clean_ws(b" \n", AtEol), b"\n"); + assert_eq!(clean_ws(b"A \n", AtEol), b"A\n"); + assert_eq!(clean_ws(b"A B C\t\r\n", AtEol), b"A B C\n"); + assert_eq!(clean_ws(b"A \tB C\r\nD ", AtEol), b"A \tB C\nD "); + assert_eq!(clean_ws(b"A\x0CB\x0C\n", AtEol), b"A\x0CB\n"); + } + + #[test] + fn test_clean_whitespace_collapse() { + use CleanWhitespace::Collapse; + assert_eq!(clean_ws(b"", Collapse), b""); + assert_eq!(clean_ws(b" ", Collapse), b" "); + assert_eq!(clean_ws(b" ", Collapse), b" "); + assert_eq!(clean_ws(b"A", Collapse), b"A"); + assert_eq!(clean_ws(b"\n\n\n", Collapse), b"\n\n\n"); + assert_eq!(clean_ws(b" \n", Collapse), b"\n"); + assert_eq!(clean_ws(b"A \n", Collapse), b"A\n"); + assert_eq!(clean_ws(b"A B C\t\r\n", Collapse), b"A B C\n"); + assert_eq!(clean_ws(b"A \tB C\r\nD ", Collapse), b"A B C\nD "); + assert_eq!(clean_ws(b"A\x0CB\x0C\n", Collapse), b"A\x0CB\x0C\n"); + } + + #[test] + fn test_clean_whitespace_all() { + use CleanWhitespace::All; + assert_eq!(clean_ws(b"", All), b""); + assert_eq!(clean_ws(b" ", All), b""); + assert_eq!(clean_ws(b" ", All), b""); + assert_eq!(clean_ws(b"A", All), b"A"); + assert_eq!(clean_ws(b"\n\n\n", All), b"\n\n\n"); + assert_eq!(clean_ws(b" \n", All), b"\n"); + assert_eq!(clean_ws(b"A \n", All), b"A\n"); + assert_eq!(clean_ws(b"A B C\t\r\n", All), b"ABC\n"); + assert_eq!(clean_ws(b"A \tB C\r\nD ", All), b"ABC\nD"); + assert_eq!(clean_ws(b"A\x0CB\x0C\n", All), b"A\x0CB\x0C\n"); + } } # HG changeset patch # User Mitchell Kember <mkember@janestreet.com> # Date 1741017530 18000 # Mon Mar 03 10:58:50 2025 -0500 # Node ID cc30bead2d6153a7477785aa1495e5f6f155d62e # Parent 422c8b0a17ab172938a878959b02e5f3219eaef6 # EXP-Topic rust-whitespace rust-utils: reimplement clean_whitespace without regex This is similar to fixws in mercurial/cext/bdiff.c, except in addition to --ignore-all-space (-w) and --ignore-space-change (-b), it also handles --ignore-ws-at-eol (-Z). Unlike the regex, it operates in-place. For files with large histories such as mercurial/commands.py, it brings the penalty of -w and -b from 25% down to around 10%. For -Z, and for files with small histories, it makes little difference. Here are some poulpe benchmarks on the mercurial repo for the stack of four changes in this merge request (!1299): ### benchmark.name = hg.command.annotate # data-env-vars.name = mercurial-devel # bin-env-vars.hg.flavor = rhg ## benchmark.variants.files = 100-random-files ## benchmark.variants.flags = default 0471bf04ddf1: 0.647526 ~~~~~ 366d3433bed2: 0.633503 (-2.17%, -0.01) ## benchmark.variants.files = 100-random-files ## benchmark.variants.flags = -Z 0471bf04ddf1: 0.649229 ~~~~~ 366d3433bed2: 0.642296 (-1.07%, -0.01) ## benchmark.variants.files = 100-random-files ## benchmark.variants.flags = -b 0471bf04ddf1: 0.675751 ~~~~~ 366d3433bed2: 0.645437 (-4.49%, -0.03) ## benchmark.variants.files = 100-random-files ## benchmark.variants.flags = -w 0471bf04ddf1: 0.678141 ~~~~~ 366d3433bed2: 0.645375 (-4.83%, -0.03) ## benchmark.variants.files = file-with-long-history.a ## benchmark.variants.flags = default 0471bf04ddf1: 1.700064 ~~~~~ 366d3433bed2: 1.695099 ## benchmark.variants.files = file-with-long-history.a ## benchmark.variants.flags = -Z 0471bf04ddf1: 1.682014 ~~~~~ 366d3433bed2: 1.697669 ## benchmark.variants.files = file-with-long-history.a ## benchmark.variants.flags = -b 0471bf04ddf1: 2.140711 ~~~~~ 366d3433bed2: 1.857253 (-13.24%, -0.28) ## benchmark.variants.files = file-with-long-history.a ## benchmark.variants.flags = -w 0471bf04ddf1: 2.127745 ~~~~~ 366d3433bed2: 1.814402 (-14.73%, -0.31) ## benchmark.variants.files = file-with-long-history.b ## benchmark.variants.flags = default 0471bf04ddf1: 0.263050 ~~~~~ 366d3433bed2: 0.263603 ## benchmark.variants.files = file-with-long-history.b ## benchmark.variants.flags = -Z 0471bf04ddf1: 0.261817 ~~~~~ 366d3433bed2: 0.263896 ## benchmark.variants.files = file-with-long-history.b ## benchmark.variants.flags = -b 0471bf04ddf1: 0.286191 ~~~~~ 366d3433bed2: 0.276698 (-3.32%, -0.01) ## benchmark.variants.files = file-with-long-history.b ## benchmark.variants.flags = -w 0471bf04ddf1: 0.282997 ~~~~~ 366d3433bed2: 0.275538 (-2.64%, -0.01) ## benchmark.variants.files = file-with-long-history.c ## benchmark.variants.flags = default 0471bf04ddf1: 0.318708 ~~~~~ 366d3433bed2: 0.318006 ## benchmark.variants.files = file-with-long-history.c ## benchmark.variants.flags = -Z 0471bf04ddf1: 0.323910 ~~~~~ 366d3433bed2: 0.318743 (-1.60%, -0.01) ## benchmark.variants.files = file-with-long-history.c ## benchmark.variants.flags = -b 0471bf04ddf1: 0.390691 ~~~~~ 366d3433bed2: 0.372053 (-4.77%, -0.02) ## benchmark.variants.files = file-with-long-history.c ## benchmark.variants.flags = -w 0471bf04ddf1: 0.407243 ~~~~~ 366d3433bed2: 0.403874 ## benchmark.variants.files = file-with-long-history.d ## benchmark.variants.flags = default 0471bf04ddf1: 0.295730 ~~~~~ 366d3433bed2: 0.269190 (-8.97%, -0.03) ## benchmark.variants.files = file-with-long-history.d ## benchmark.variants.flags = -Z 0471bf04ddf1: 0.276715 ~~~~~ 366d3433bed2: 0.271512 (-1.88%, -0.01) ## benchmark.variants.files = file-with-long-history.d ## benchmark.variants.flags = -b 0471bf04ddf1: 0.283624 ~~~~~ 366d3433bed2: 0.278003 (-1.98%, -0.01) ## benchmark.variants.files = file-with-long-history.d ## benchmark.variants.flags = -w 0471bf04ddf1: 0.286407 ~~~~~ 366d3433bed2: 0.276957 (-3.30%, -0.01) ## benchmark.variants.files = file-with-long-history.e ## benchmark.variants.flags = default 0471bf04ddf1: 0.349047 ~~~~~ 366d3433bed2: 0.331948 (-4.90%, -0.02) ## benchmark.variants.files = file-with-long-history.e ## benchmark.variants.flags = -Z 0471bf04ddf1: 0.359457 ~~~~~ 366d3433bed2: 0.334011 (-7.08%, -0.03) ## benchmark.variants.files = file-with-long-history.e ## benchmark.variants.flags = -b 0471bf04ddf1: 0.373671 ~~~~~ 366d3433bed2: 0.347020 (-7.13%, -0.03) ## benchmark.variants.files = file-with-long-history.e ## benchmark.variants.flags = -w 0471bf04ddf1: 0.374186 ~~~~~ 366d3433bed2: 0.347367 (-7.17%, -0.03) diff --git a/rust/hg-core/src/utils/strings.rs b/rust/hg-core/src/utils/strings.rs --- a/rust/hg-core/src/utils/strings.rs +++ b/rust/hg-core/src/utils/strings.rs @@ -1,9 +1,7 @@ //! Contains string-related utilities. use crate::utils::hg_path::HgPath; -use lazy_static::lazy_static; -use regex::bytes::Regex; -use std::{borrow::Cow, cell::Cell, fmt, io::Write as _, ops::Deref as _}; +use std::{cell::Cell, fmt, io::Write as _, ops::Deref as _}; /// Useful until rust/issues/56345 is stable /// @@ -314,34 +312,61 @@ /// Normalizes whitespace in text so that it won't apppear in diffs. pub fn clean_whitespace(text: &mut Vec<u8>, how: CleanWhitespace) { - lazy_static! { - // To match wsclean in mdiff.py, this includes "\f". - static ref AT_EOL: Regex = - Regex::new(r"(?m)[ \t\r\f]+$").expect("valid regex"); - // To match fixws in cext/bdiff.c, this does *not* include "\f". - static ref MULTIPLE: Regex = - Regex::new(r"[ \t\r]+").expect("valid regex"); - } - let replacement: &[u8] = match how { + // We copy text[i] to text[w], where w advances more slowly than i. + let mut w = 0; + match how { + // To match wsclean in mdiff.py, this removes "\f" (0xC). CleanWhitespace::AtEol => { - replace_all_mut(&AT_EOL, text, b""); - return; + let mut newline_dest = 0; + for i in 0..text.len() { + let char = text[i]; + match char { + b' ' | b'\t' | b'\r' | 0xC => {} + _ => { + if char == b'\n' { + w = newline_dest; + } + newline_dest = w + 1; + } + } + text[w] = char; + w += 1; + } } - CleanWhitespace::Collapse => b" ", - CleanWhitespace::All => b"", - }; - replace_all_mut(&MULTIPLE, text, replacement); - replace_all_mut(&AT_EOL, text, b""); -} - -/// Helper to make [`Regex::replace_all`] mutate a vector. -fn replace_all_mut(regex: &Regex, haystack: &mut Vec<u8>, replacement: &[u8]) { - match regex.replace_all(haystack, replacement) { - Cow::Borrowed(_) => {} - Cow::Owned(result) => { - *haystack = result; + // To match fixws in cext/bdiff.c, CleanWhitespace::Collapse and + // CleanWhitespace::All do *not* remove "\f". + CleanWhitespace::Collapse => { + for i in 0..text.len() { + match text[i] { + b' ' | b'\t' | b'\r' => { + if w == 0 || text[w - 1] != b' ' { + text[w] = b' '; + w += 1; + } + } + b'\n' if w > 0 && text[w - 1] == b' ' => { + text[w - 1] = b'\n'; + } + char => { + text[w] = char; + w += 1; + } + } + } + } + CleanWhitespace::All => { + for i in 0..text.len() { + match text[i] { + b' ' | b'\t' | b'\r' => {} + char => { + text[w] = char; + w += 1; + } + } + } } } + text.truncate(w); } #[cfg(test)]