( )⚙ D6271 rust-filepatterns: add a Rust implementation of pattern-related utils

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6271

rust-filepatterns: add a Rust implementation of pattern-related utils
ClosedPublic

Authored by Alphare on Apr 18 2019, 8:10 AM.

Details

Reviewers
kevincox
durin42
Group Reviewers
hg-reviewers
Commits
rHGe8f3740cc067: rust-filepatterns: add a Rust implementation of pattern-related utils
rHGc2803da26110: rust-filepatterns: add a Rust implementation of pattern-related utils
rHGb05c1041de8f: rust-filepatterns: add a Rust implementation of pattern-related utils
Summary

This change introduces Rust implementations of two functions related to
pattern handling, all located in match.py:

  • _regex
  • readpatternfile

These utils are useful in the long-term effort to improve hg status's
performance using Rust. Experimental work done by Valentin Gatien-Baron
shows very promising improvements, but is too different from the current
Mercurial core code structure to be used "as-is".
This is the first - albeit very small - step towards the code revamp
needed down the line.

Two dependencies were added: regex and lazy_static. Both of them
will be useful for a majority of the Rust code that will be written,
are well known and maintained either by the Rust core team, or by
very frequent contributors.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Alphare created this revision.Apr 18 2019, 8:10 AM
kevincox requested changes to this revision.Apr 20 2019, 4:21 PM
kevincox added inline comments.
rust/hg-core/src/filepatterns.rs
19

Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about performance you can use https://docs.rs/regex-syntax/0.6.6/regex_syntax/fn.escape_into.html which is quite fast.

22

It is worth putting a comment that these are matched in order.

30

Put doc comments before the thing that they are documenting and use ///

52

I suspect this .iter() isn't required.

62
input.iter().skip(1).position(|b| *b == b']')
227

This function appears to be way more complicated then it needs to be. I have tried to highlight the changes in comments below but I have a full version at https://rust.godbolt.org/z/08MrDP. I haven't actually run the code but I think my version has the same functionality as this one.

240

If you are worried about performance you can use a buffer outside the loop and replace it with line.replace_range(.., line_str). This saves continuously reallocating the string.

241

This copy is unnecessary and having the lifetime start so early is confusing. The only time this is used is as a temporary in the syntax: case and to set line_syntax where it is always unchanged from the default. This variable should just be removed.

242

This is also unnecessary. Because of the above comment.

250

You can just call str methods on String because they implement Deref<str>.

line = line.as_ref().trim_end().to_string();
260

rel_syntax: &'static str so this can be Cow::Borrowed. Actually current_syntax is only ever set to static lifetime strings so you don't have to use Cow at all.

268

This clone is unnecessary it is always a reference to line from herein so you can just use &str.

283

Slight preference for format!(). In theory it can do clever things like checking argument sizes to only allocate once. Either way I think it looks nice.

287

I'm pretty sure this is redundant because the only time syntax is modified is in the syntax: case which A) Sets current_syntax and B) continues.

300

Mapping to an always-ok result is odd. You should just map to the parse_pattern_file_contents(). Then you don't need to use ? to unwrap the extra layer. However you then loose the error conversion which you can add back with .map_err(|e| e.into()).

https://rust.godbolt.org/z/Mflpga

Even better is just use the try for the read and call parse_pattern_file_contents at the function level.

f.read_to_string(&mut contents)?;
Ok(parse_pattern_file_contents(&contents, &file_path, warn))
rust/hg-core/src/lib.rs
53

This seems like overkill to me. I would either remove it or use pub struct LineNumber(pub usize) if you want to do this for type safety. However I don't think type safety is really needed here so I would just remove the alias.

This revision now requires changes to proceed.Apr 20 2019, 4:21 PM
Alphare marked 12 inline comments as done.Apr 24 2019, 5:49 AM
Alphare updated this revision to Diff 14904.
Alphare added inline comments.Apr 24 2019, 5:49 AM
rust/hg-core/src/filepatterns.rs
19

I think I hit the same problem as the person in this issue: https://github.com/rust-lang/regex/issues/451. It's a hassle to escape bytes using the regex crate, and this felt like a good enough replacement.

52

It is required since repl is &&[u8], which is not an iterator.

62

This has a different behavior, since position expects a boolean return value in the closure for each element of the iterator.

227

While your version is just plain better, removing the regex-based comment escape breaks support for comments in test-hgignore.t, so this will still be necessary.

rust/hg-core/src/lib.rs
53

I would argue it's here for readability's sake. I used to have usize, but was advised to put an alias here because it would lessen mental overhead to read the function signatures. I'll remove it If you disagree.

kevincox accepted this revision.Apr 24 2019, 5:58 AM

Thanks for the changes.

rust/hg-core/src/filepatterns.rs
19

Ah, I understand. This make sense then.

52

It is probably more clear to do *repl then to show that you are dereferencing it.

62

I don't understand. The example I showed would give a boolean. The only difference I see is that the returned value would be 1 less (because .position() wouldn't count the skipped element) but this should be easy to handle in the code and I think would be more clear overall.

227

Oops, I should have pointed out that I just commented that out because I don't have access to the regex crate on the website. It should still be included in the code.

This looks good now.

Alphare marked 13 inline comments as done.Apr 24 2019, 8:31 AM
Alphare updated this revision to Diff 14905.
Alphare added inline comments.Apr 24 2019, 8:32 AM
rust/hg-core/src/filepatterns.rs
62

You're right, I was chasing the wrong problem. This is indeed more semantic.

durin42 accepted this revision.May 15 2019, 2:10 PM

Meta-comment: we should extract an hgignore crate (with minimal other deps) and publish that, because that'd be a step towards automatic hgignore support in ripgrep (burntsushi has previously indicated that if there was a maintained hgignore crate that it'd be a doable feature).

This revision is now accepted and ready to land.May 15 2019, 2:10 PM
durin42 added a comment.May 15 2019, 2:11 PM

Ugh, but I got conflicts in Cargo.lock - could you rebase and let me know when you're ready?

durin42 requested changes to this revision.May 15 2019, 2:11 PM

(forgot to tag as needing changes)

This revision now requires changes to proceed.May 15 2019, 2:11 PM
Alphare updated this revision to Diff 15141.May 16 2019, 11:21 AM
Closed by commit rHGb05c1041de8f: rust-filepatterns: add a Rust implementation of pattern-related utils (authored by Alphare). · Explain WhyMay 16 2019, 1:12 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.May 17 2019, 1:47 AM
martinvonz added inline comments.
rust/hg-core/src/lib.rs
21

I get the following from HGWITHRUSTEXT= make local:

error[E0583]: file not found for module `filepatterns`
  --> hg-core/src/lib.rs:21:5
   |
21 | mod filepatterns;
   |     ^^^^^^^^^^^^
   |
   = help: name the file either filepatterns.rs or filepatterns/mod.rs inside the directory "hg-core/src"

error: aborting due to previous error

For more information about this error, try `rustc --explain E0583`.
error: Could not compile `hg-core`.

Also, the command modifies the rust/Cargo.lock, which is really annoying. Is there a way to fix that? Or does it only happen when the build fails, so it's not really a problem?

martinvonz added inline comments.May 17 2019, 1:51 AM
rust/hg-core/src/lib.rs
21

I guess I'll drop this patch until that's fixed. Sorry.

Alphare added inline comments.May 17 2019, 4:54 AM
rust/hg-core/src/lib.rs
21

It looks like the filepatterns.rs file was dropped. Maybe during a histedit or other command, but it was definitely there when I sent my patch, and I can't reproduce this issue locally, the file exists. Do you know what happened?

martinvonz added inline comments.May 17 2019, 9:11 AM
rust/hg-core/src/lib.rs
21

Dropped from where? Where did the file use to be? It doesn't seem to be added by this patch here in Phabricator. I get the same compilation error on b05c1041de8f, which is the version before my histedit.

martinvonz added inline comments.May 17 2019, 9:14 AM
rust/hg-core/src/lib.rs
21

Ah, it was there before it got queued. I just found the History tab here. I'll see if I can convince phabread to get that version somehow.

Alphare added inline comments.May 17 2019, 9:15 AM
rust/hg-core/src/lib.rs
21

I'm not sure about how Phabricator handles this, but in https://phab.mercurial-scm.org/D6271?id=15141, the filepatterns.rs file existed.

Alphare updated this revision to Diff 15169.May 17 2019, 10:16 AM

Revision Contents
Changeset List

Diff 15141

rust/Cargo.lock

version = "0.1.0" version = "0.1.0"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]] [[package]]
name = "hg-core" name = "hg-core"
version = "0.1.0" version = "0.1.0"
dependencies = [ dependencies = [
"byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)",
"lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
"memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
"rand 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)",
"rand_pcg 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "rand_pcg 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
"regex 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
] ]
[[package]] [[package]]
name = "hg-cpython" name = "hg-cpython"
version = "0.1.0" version = "0.1.0"
dependencies = [ dependencies = [
"cpython 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "cpython 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)",
"hg-core 0.1.0", "hg-core 0.1.0",
"libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)",
"python27-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "python27-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)",
"python3-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "python3-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)",
] ]
[[package]] [[package]]
name = "hgdirectffi" name = "hgdirectffi"
version = "0.1.0" version = "0.1.0"
dependencies = [ dependencies = [
"hg-core 0.1.0", "hg-core 0.1.0",
"libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)",
] ]
[[package]] [[package]]
name = "lazy_static" name = "lazy_static"
version = "1.2.0" version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]] [[package]]
name = "libc" name = "libc"
version = "0.2.45" version = "0.2.45"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]] [[package]]
version = "0.7.0" version = "0.7.0"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]] [[package]]
name = "thread_local" name = "thread_local"
version = "0.3.6" version = "0.3.6"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [ dependencies = [
"lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
] ]
[[package]] [[package]]
name = "ucd-util" name = "ucd-util"
version = "0.1.3" version = "0.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]] [[package]]
[metadata] [metadata]
"checksum aho-corasick 0.6.9 (registry+https://github.com/rust-lang/crates.io-index)" = "1e9a933f4e58658d7b12defcf96dc5c720f20832deebe3e0a19efd3b6aaeeb9e" "checksum aho-corasick 0.6.9 (registry+https://github.com/rust-lang/crates.io-index)" = "1e9a933f4e58658d7b12defcf96dc5c720f20832deebe3e0a19efd3b6aaeeb9e"
"checksum autocfg 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "a6d640bee2da49f60a4068a7fae53acde8982514ab7bae8b8cea9e88cbcfd799" "checksum autocfg 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "a6d640bee2da49f60a4068a7fae53acde8982514ab7bae8b8cea9e88cbcfd799"
"checksum bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "228047a76f468627ca71776ecdebd732a3423081fcf5125585bcd7c49886ce12" "checksum bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "228047a76f468627ca71776ecdebd732a3423081fcf5125585bcd7c49886ce12"
"checksum byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a019b10a2a7cdeb292db131fc8113e57ea2a908f6e7894b0c3c671893b65dbeb" "checksum byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a019b10a2a7cdeb292db131fc8113e57ea2a908f6e7894b0c3c671893b65dbeb"
"checksum cloudabi 0.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ddfc5b9aa5d4507acaf872de71051dfd0e309860e88966e1051e462a077aac4f" "checksum cloudabi 0.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ddfc5b9aa5d4507acaf872de71051dfd0e309860e88966e1051e462a077aac4f"
"checksum cpython 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "b489034e723e7f5109fecd19b719e664f89ef925be785885252469e9822fa940" "checksum cpython 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "b489034e723e7f5109fecd19b719e664f89ef925be785885252469e9822fa940"
"checksum fuchsia-cprng 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "81f7f8eb465745ea9b02e2704612a9946a59fa40572086c6fd49d6ddcf30bf31" "checksum fuchsia-cprng 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "81f7f8eb465745ea9b02e2704612a9946a59fa40572086c6fd49d6ddcf30bf31"
"checksum lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "a374c89b9db55895453a74c1e38861d9deec0b01b405a82516e9d5de4820dea1" "checksum lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "bc5729f27f159ddd61f4df6228e827e86643d4d3e7c32183cb30a1c08f604a14"
"checksum libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)" = "2d2857ec59fadc0773853c664d2d18e7198e83883e7060b63c924cb077bd5c74" "checksum libc 0.2.45 (registry+https://github.com/rust-lang/crates.io-index)" = "2d2857ec59fadc0773853c664d2d18e7198e83883e7060b63c924cb077bd5c74"
"checksum memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "2efc7bc57c883d4a4d6e3246905283d8dae951bb3bd32f49d6ef297f546e1c39" "checksum memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "2efc7bc57c883d4a4d6e3246905283d8dae951bb3bd32f49d6ef297f546e1c39"
"checksum num-traits 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)" = "0b3a5d7cc97d6d30d8b9bc8fa19bf45349ffe46241e8816f50f62f6d6aaabee1" "checksum num-traits 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)" = "0b3a5d7cc97d6d30d8b9bc8fa19bf45349ffe46241e8816f50f62f6d6aaabee1"
"checksum python27-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "56114c37d4dca82526d74009df7782a28c871ac9d36b19d4cb9e67672258527e" "checksum python27-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "56114c37d4dca82526d74009df7782a28c871ac9d36b19d4cb9e67672258527e"
"checksum python3-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "61e4aac43f833fd637e429506cb2ac9d7df672c4b68f2eaaa163649b7fdc0444" "checksum python3-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "61e4aac43f833fd637e429506cb2ac9d7df672c4b68f2eaaa163649b7fdc0444"
"checksum rand 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)" = "6d71dacdc3c88c1fde3885a3be3fbab9f35724e6ce99467f7d9c5026132184ca" "checksum rand 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)" = "6d71dacdc3c88c1fde3885a3be3fbab9f35724e6ce99467f7d9c5026132184ca"
"checksum rand_chacha 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "556d3a1ca6600bfcbab7c7c91ccb085ac7fbbcd70e008a98742e7847f4f7bcef" "checksum rand_chacha 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "556d3a1ca6600bfcbab7c7c91ccb085ac7fbbcd70e008a98742e7847f4f7bcef"
"checksum rand_core 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "7a6fdeb83b075e8266dcc8762c22776f6877a63111121f5f8c7411e5be7eed4b" "checksum rand_core 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "7a6fdeb83b075e8266dcc8762c22776f6877a63111121f5f8c7411e5be7eed4b"

rust/hg-core/Cargo.toml

[package] [package]
name = "hg-core" name = "hg-core"
version = "0.1.0" version = "0.1.0"
authors = ["Georges Racinet <gracinet@anybox.fr>"] authors = ["Georges Racinet <gracinet@anybox.fr>"]
description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)" description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)"
[lib] [lib]
name = "hg" name = "hg"
[dev-dependencies] [dev-dependencies]
rand = "*" rand = "*"
rand_pcg = "*" rand_pcg = "*"
[dependencies] [dependencies]
memchr = "2.2.0" memchr = "2.2.0"
byteorder = "1.3.1" byteorder = "1.3.1"
No newline at end of file lazy_static = "1.3.0"
regex = "^1.1"
No newline at end of file

rust/hg-core/src/ancestors.rs

// ancestors.rs // ancestors.rs
// //
// Copyright 2018 Georges Racinet <gracinet@anybox.fr> // Copyright 2018 Georges Racinet <gracinet@anybox.fr>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Rust versions of generic DAG ancestors algorithms for Mercurial //! Rust versions of generic DAG ancestors algorithms for Mercurial
use super::{Graph, GraphError, Revision, NULL_REVISION}; use super::{Graph, GraphError, Revision, NULL_REVISION};
use crate::dagops;
use std::cmp::max; use std::cmp::max;
use std::collections::{BinaryHeap, HashSet}; use std::collections::{BinaryHeap, HashSet};
use crate::dagops;
/// Iterator over the ancestors of a given list of revisions /// Iterator over the ancestors of a given list of revisions
/// This is a generic type, defined and implemented for any Graph, so that /// This is a generic type, defined and implemented for any Graph, so that
/// it's easy to /// it's easy to
/// ///
/// - unit test in pure Rust /// - unit test in pure Rust
/// - bind to main Mercurial code, potentially in several ways and have these /// - bind to main Mercurial code, potentially in several ways and have these
/// bindings evolve over time /// bindings evolve over time

rust/hg-core/src/filepatterns.rs

  • This file was added.
use crate::{LineNumber, PatternError, PatternFileError};
use regex::Regex;
use std::collections::HashMap;
use std::fs::File;
use std::io::Read;
use std::vec::Vec;
lazy_static! {
static ref reescape: Vec<Vec<u8>> = {
let mut v: Vec<Vec<u8>> = (0..=255).map(|byte| vec![byte]).collect();
let to_escape = b"()[]{}?*+-|^$\\.&~# \t\n\r\x0b\x0c";
for byte in to_escape {
v[*byte as usize].insert(0, b'\\');
}
v
};
}
/// These are matched in order
kevincoxUnsubmitted
Done

Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about performance you can use https://docs.rs/regex-syntax/0.6.6/regex_syntax/fn.escape_into.html which is quite fast.

kevincox: Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about…
AlphareAuthorUnsubmitted
Done

I think I hit the same problem as the person in this issue: https://github.com/rust-lang/regex/issues/451. It's a hassle to escape bytes using the regex crate, and this felt like a good enough replacement.

Alphare: I think I hit the same problem as the person in this issue: `https://github.com/rust…
kevincoxUnsubmitted
Done

Ah, I understand. This make sense then.

kevincox: Ah, I understand. This make sense then.
const GLOB_REPLACEMENTS: &[(&[u8], &[u8])] =
&[(b"*/", b"(?:.*/)?"), (b"*", b".*"), (b"", b"[^/]*")];
kevincoxUnsubmitted
Done

It is worth putting a comment that these are matched in order.

kevincox: It is worth putting a comment that these are matched in order.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum PatternSyntax {
Regexp,
/// Glob that matches at the front of the path
RootGlob,
/// Glob that matches at any suffix of the path (still anchored at slashes)
Glob,
Path,
kevincoxUnsubmitted
Done

Put doc comments before the thing that they are documenting and use ///

kevincox: Put doc comments before the thing that they are documenting and use `///`
RelPath,
RelGlob,
RelRegexp,
RootFiles,
}
/// Transforms a glob pattern into a regex
fn glob_to_re(pat: &[u8]) -> Vec<u8> {
let mut input = pat;
let mut res: Vec<u8> = vec![];
let mut group_depth = 0;
while let Some((c, rest)) = input.split_first() {
input = rest;
match c {
b'*' => {
for (source, repl) in GLOB_REPLACEMENTS {
if input.starts_with(source) {
input = &input[source.len()..];
res.extend(*repl);
break;
kevincoxUnsubmitted
Done

I suspect this .iter() isn't required.

kevincox: I suspect this `.iter()` isn't required.
AlphareAuthorUnsubmitted
Done

It is required since repl is &&[u8], which is not an iterator.

Alphare: It is required since `repl` is `&&[u8]`, which is not an iterator.
kevincoxUnsubmitted
Done

It is probably more clear to do *repl then to show that you are dereferencing it.

kevincox: It is probably more clear to do `*repl` then to show that you are dereferencing it.
}
}
}
b'?' => res.extend(b"."),
b'[' => {
match input.iter().skip(1).position(|b| *b == b']') {
None => res.extend(b"\\["),
Some(end) => {
// Account for the one we skipped
let end = end + 1;
kevincoxUnsubmitted
Done
input.iter().skip(1).position(|b| *b == b']')
kevincox: ``` input.iter().skip(1).position(|b| *b == b']') ```
AlphareAuthorUnsubmitted
Done

This has a different behavior, since position expects a boolean return value in the closure for each element of the iterator.

Alphare: This has a different behavior, since `position` expects a boolean return value in the closure…
kevincoxUnsubmitted
Done

I don't understand. The example I showed would give a boolean. The only difference I see is that the returned value would be 1 less (because .position() wouldn't count the skipped element) but this should be easy to handle in the code and I think would be more clear overall.

kevincox: I don't understand. The example I showed would give a boolean. The only difference I see is…
AlphareAuthorUnsubmitted
Not Done

You're right, I was chasing the wrong problem. This is indeed more semantic.

Alphare: You're right, I was chasing the wrong problem. This is indeed more semantic.
res.extend(b"[");
for (i, b) in input[..end].iter().enumerate() {
if *b == b'!' && i == 0 {
res.extend(b"^")
} else if *b == b'^' && i == 0 {
res.extend(b"\\^")
} else if *b == b'\\' {
res.extend(b"\\\\")
} else {
res.push(*b)
}
}
res.extend(b"]");
input = &input[end + 1..];
}
}
}
b'{' => {
group_depth += 1;
res.extend(b"(?:")
}
b'}' if group_depth > 0 => {
group_depth -= 1;
res.extend(b")");
}
b',' if group_depth > 0 => res.extend(b"|"),
b'\\' => {
let c = {
if let Some((c, rest)) = input.split_first() {
input = rest;
c
} else {
c
}
};
res.extend(&reescape[*c as usize])
}
_ => res.extend(&reescape[*c as usize]),
}
}
res
}
fn escape_pattern(pattern: &[u8]) -> Vec<u8> {
pattern
.iter()
.flat_map(|c| reescape[*c as usize].clone())
.collect()
}
fn parse_pattern_syntax(kind: &[u8]) -> Result<PatternSyntax, PatternError> {
match kind {
b"re" => Ok(PatternSyntax::Regexp),
b"path" => Ok(PatternSyntax::Path),
b"relpath" => Ok(PatternSyntax::RelPath),
b"rootfilesin" => Ok(PatternSyntax::RootFiles),
b"relglob" => Ok(PatternSyntax::RelGlob),
b"relre" => Ok(PatternSyntax::RelRegexp),
b"glob" => Ok(PatternSyntax::Glob),
b"rootglob" => Ok(PatternSyntax::RootGlob),
_ => Err(PatternError::UnsupportedSyntax(
String::from_utf8_lossy(kind).to_string(),
)),
}
}
/// Builds the regex that corresponds to the given pattern.
/// If within a `syntax: regexp` context, returns the pattern,
/// otherwise, returns the corresponding regex.
fn _build_single_regex(
syntax: PatternSyntax,
pattern: &[u8],
globsuffix: &[u8],
) -> Vec<u8> {
if pattern.is_empty() {
return vec![];
}
match syntax {
PatternSyntax::Regexp => pattern.to_owned(),
PatternSyntax::RelRegexp => {
if pattern[0] == b'^' {
return pattern.to_owned();
}
let mut res = b".*".to_vec();
res.extend(pattern);
res
}
PatternSyntax::Path | PatternSyntax::RelPath => {
if pattern == b"." {
return vec![];
}
let mut pattern = escape_pattern(pattern);
pattern.extend(b"(?:/|$)");
pattern
}
PatternSyntax::RootFiles => {
let mut res = if pattern == b"." {
vec![]
} else {
// Pattern is a directory name.
let mut as_vec: Vec<u8> = escape_pattern(pattern);
as_vec.push(b'/');
as_vec
};
// Anything after the pattern must be a non-directory.
res.extend(b"[^/]+$");
res
}
PatternSyntax::Glob
| PatternSyntax::RelGlob
| PatternSyntax::RootGlob => {
let mut res: Vec<u8> = vec![];
if syntax == PatternSyntax::RelGlob {
res.extend(b"(?:|.*/)");
}
res.extend(glob_to_re(pattern));
res.extend(globsuffix.iter());
res
}
}
}
const GLOB_SPECIAL_CHARACTERS: [u8; 7] =
[b'*', b'?', b'[', b']', b'{', b'}', b'\\'];
/// Wrapper function to `_build_single_regex` that short-circuits 'exact' globs
/// that don't need to be transformed into a regex.
pub fn build_single_regex(
kind: &str,
pat: &[u8],
globsuffix: &[u8],
) -> Result<Vec<u8>, PatternError> {
let enum_kind = parse_pattern_syntax(kind.as_bytes())?;
if enum_kind == PatternSyntax::RootGlob
&& pat.iter().all(|b| GLOB_SPECIAL_CHARACTERS.contains(b))
{
Ok(pat.to_vec())
} else {
Ok(_build_single_regex(enum_kind, pat, globsuffix))
}
}
lazy_static! {
static ref SYNTAXES: HashMap<&'static str, &'static str> = {
let mut m = HashMap::new();
m.insert("re", "relre:");
m.insert("regexp", "relre:");
m.insert("glob", "relglob:");
m.insert("rootglob", "rootglob:");
m.insert("include", "include");
m.insert("subinclude", "subinclude");
m
};
}
pub type PatternTuple = (String, LineNumber, String);
type WarningTuple = (String, String);
pub fn parse_pattern_file_contents(
lines: &str,
kevincoxUnsubmitted
Done

This function appears to be way more complicated then it needs to be. I have tried to highlight the changes in comments below but I have a full version at https://rust.godbolt.org/z/08MrDP. I haven't actually run the code but I think my version has the same functionality as this one.

kevincox: This function appears to be way more complicated then it needs to be. I have tried to highlight…
AlphareAuthorUnsubmitted
Done

While your version is just plain better, removing the regex-based comment escape breaks support for comments in test-hgignore.t, so this will still be necessary.

Alphare: While your version is just plain better, removing the regex-based comment escape breaks support…
kevincoxUnsubmitted
Done

Oops, I should have pointed out that I just commented that out because I don't have access to the regex crate on the website. It should still be included in the code.

This looks good now.

kevincox: Oops, I should have pointed out that I just commented that out because I don't have access to…
file_path: &str,
warn: bool,
) -> (Vec<PatternTuple>, Vec<WarningTuple>) {
let comment_regex = Regex::new(r"((?:^|[^\\])(?:\\\\)*)#.*").unwrap();
let mut inputs: Vec<PatternTuple> = vec![];
let mut warnings: Vec<WarningTuple> = vec![];
let mut current_syntax = "relre:";
let mut line = String::new();
for (line_number, line_str) in lines.split('\n').enumerate() {
let line_number = line_number + 1;
line.replace_range(.., line_str);
kevincoxUnsubmitted
Done

If you are worried about performance you can use a buffer outside the loop and replace it with line.replace_range(.., line_str). This saves continuously reallocating the string.

kevincox: If you are worried about performance you can use a buffer outside the loop and replace it with…
kevincoxUnsubmitted
Done

This copy is unnecessary and having the lifetime start so early is confusing. The only time this is used is as a temporary in the syntax: case and to set line_syntax where it is always unchanged from the default. This variable should just be removed.

kevincox: This copy is unnecessary and having the lifetime start so early is confusing. The only time…
if line.contains('#') {
kevincoxUnsubmitted
Done

This is also unnecessary. Because of the above comment.

kevincox: This is also unnecessary. Because of the above comment.
if let Some(cap) = comment_regex.captures(line.clone().as_ref()) {
line = line[..cap.get(1).unwrap().end()].to_string()
}
line = line.replace(r"\#", "#");
}
let mut line = line.trim_end();
kevincoxUnsubmitted
Done

You can just call str methods on String because they implement Deref<str>.

line = line.as_ref().trim_end().to_string();
kevincox: You can just call `str` methods on `String` because they implement `Deref<str>`. ```…
if line.is_empty() {
continue;
}
if line.starts_with("syntax:") {
let syntax = line["syntax:".len()..].trim();
if let Some(rel_syntax) = SYNTAXES.get(syntax) {
current_syntax = rel_syntax;
} else if warn {
kevincoxUnsubmitted
Done

rel_syntax: &'static str so this can be Cow::Borrowed. Actually current_syntax is only ever set to static lifetime strings so you don't have to use Cow at all.

kevincox: `rel_syntax: &'static str` so this can be `Cow::Borrowed`. Actually `current_syntax` is only…
warnings.push((file_path.to_string(), syntax.to_string()));
}
continue;
}
let mut line_syntax: &str = &current_syntax;
for (s, rels) in SYNTAXES.iter() {
kevincoxUnsubmitted
Done

This clone is unnecessary it is always a reference to line from herein so you can just use &str.

kevincox: This clone is unnecessary it is always a reference to line from herein so you can just use…
if line.starts_with(rels) {
line_syntax = rels;
line = &line[rels.len()..];
break;
} else if line.starts_with(&format!("{}:", s)) {
line_syntax = rels;
line = &line[s.len() + 1..];
break;
}
}
inputs.push((
format!("{}{}", line_syntax, line),
line_number,
line.to_string(),
kevincoxUnsubmitted
Done

Slight preference for format!(). In theory it can do clever things like checking argument sizes to only allocate once. Either way I think it looks nice.

kevincox: Slight preference for `format!()`. In theory it can do clever things like checking argument…
));
}
(inputs, warnings)
}
kevincoxUnsubmitted
Done

I'm pretty sure this is redundant because the only time syntax is modified is in the syntax: case which A) Sets current_syntax and B) continues.

kevincox: I'm pretty sure this is redundant because the only time `syntax` is modified is in the `syntax…
pub fn read_pattern_file(
file_path: String,
warn: bool,
) -> Result<(Vec<PatternTuple>, Vec<WarningTuple>), PatternFileError> {
let mut f = File::open(&file_path)?;
let mut contents = String::new();
f.read_to_string(&mut contents)?;
Ok(parse_pattern_file_contents(&contents, &file_path, warn))
}
kevincoxUnsubmitted
Done

Mapping to an always-ok result is odd. You should just map to the parse_pattern_file_contents(). Then you don't need to use ? to unwrap the extra layer. However you then loose the error conversion which you can add back with .map_err(|e| e.into()).

https://rust.godbolt.org/z/Mflpga

Even better is just use the try for the read and call parse_pattern_file_contents at the function level.

f.read_to_string(&mut contents)?;
Ok(parse_pattern_file_contents(&contents, &file_path, warn))
kevincox: Mapping to an always-ok result is odd. You should just map to the `parse_pattern_file_contents…
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn escape_pattern_test() {
let untouched = br#"!"%',/0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ_`abcdefghijklmnopqrstuvwxyz"#;
assert_eq!(escape_pattern(untouched), untouched.to_vec());
// All escape codes
assert_eq!(
escape_pattern(br#"()[]{}?*+-|^$\\.&~# \t\n\r\v\f"#),
br#"\(\)\[\]\{\}\?\*\+\-\|\^\$\\\\\.\&\~\#\ \\t\\n\\r\\v\\f"#
.to_vec()
);
}
#[test]
fn glob_test() {
assert_eq!(glob_to_re(br#"?"#), br#"."#);
assert_eq!(glob_to_re(br#"*"#), br#"[^/]*"#);
assert_eq!(glob_to_re(br#"**"#), br#".*"#);
assert_eq!(glob_to_re(br#"**/a"#), br#"(?:.*/)?a"#);
assert_eq!(glob_to_re(br#"a/**/b"#), br#"a/(?:.*/)?b"#);
assert_eq!(glob_to_re(br#"[a*?!^][^b][!c]"#), br#"[a*?!^][\^b][^c]"#);
assert_eq!(glob_to_re(br#"{a,b}"#), br#"(?:a|b)"#);
assert_eq!(glob_to_re(br#".\*\?"#), br#"\.\*\?"#);
}
#[test]
fn test_parse_pattern_file_contents() {
let lines = "syntax: glob\n*.elc";
assert_eq!(
vec![("relglob:*.elc".to_string(), 2, "*.elc".to_string())],
parse_pattern_file_contents(lines, "file_path", false).0,
);
let lines = "syntax: include\nsyntax: glob";
assert_eq!(
parse_pattern_file_contents(lines, "file_path", false).0,
vec![]
);
}
}

rust/hg-core/src/lib.rs

// Copyright 2018 Georges Racinet <gracinet@anybox.fr> // Copyright 2018 Georges Racinet <gracinet@anybox.fr>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
extern crate byteorder; extern crate byteorder;
extern crate memchr; extern crate memchr;
#[macro_use]
extern crate lazy_static;
extern crate regex;
mod ancestors; mod ancestors;
pub mod dagops; pub mod dagops;
pub use ancestors::{AncestorsIterator, LazyAncestors, MissingAncestors}; pub use ancestors::{AncestorsIterator, LazyAncestors, MissingAncestors};
mod dirstate; mod dirstate;
pub mod discovery; pub mod discovery;
pub mod testing; // unconditionally built, for use from integration tests pub mod testing; // unconditionally built, for use from integration tests
pub use dirstate::{ pub use dirstate::{
pack_dirstate, parse_dirstate, CopyVec, CopyVecEntry, DirstateEntry, pack_dirstate, parse_dirstate, CopyVec, CopyVecEntry, DirstateEntry,
DirstateParents, DirstateVec, DirstateParents, DirstateVec,
}; };
mod filepatterns;
martinvonzUnsubmitted
Not Done

I get the following from HGWITHRUSTEXT= make local:

error[E0583]: file not found for module `filepatterns`
  --> hg-core/src/lib.rs:21:5
   |
21 | mod filepatterns;
   |     ^^^^^^^^^^^^
   |
   = help: name the file either filepatterns.rs or filepatterns/mod.rs inside the directory "hg-core/src"

error: aborting due to previous error

For more information about this error, try `rustc --explain E0583`.
error: Could not compile `hg-core`.

Also, the command modifies the rust/Cargo.lock, which is really annoying. Is there a way to fix that? Or does it only happen when the build fails, so it's not really a problem?

martinvonz: I get the following from `HGWITHRUSTEXT= make local`: ``` error[E0583]: file not found for…
martinvonzUnsubmitted
Not Done

I guess I'll drop this patch until that's fixed. Sorry.

martinvonz: I guess I'll drop this patch until that's fixed. Sorry.
AlphareAuthorUnsubmitted
Not Done

It looks like the filepatterns.rs file was dropped. Maybe during a histedit or other command, but it was definitely there when I sent my patch, and I can't reproduce this issue locally, the file exists. Do you know what happened?

Alphare: It looks like the `filepatterns.rs` file was dropped. Maybe during a `histedit` or other…
martinvonzUnsubmitted
Not Done

Dropped from where? Where did the file use to be? It doesn't seem to be added by this patch here in Phabricator. I get the same compilation error on b05c1041de8f, which is the version before my histedit.

martinvonz: Dropped from where? Where did the file use to be? It doesn't seem to be added by this patch…
AlphareAuthorUnsubmitted
Not Done

I'm not sure about how Phabricator handles this, but in https://phab.mercurial-scm.org/D6271?id=15141, the filepatterns.rs file existed.

Alphare: I'm not sure about how Phabricator handles this, but in https://phab.mercurial-scm.org/D6271?
martinvonzUnsubmitted
Not Done

Ah, it was there before it got queued. I just found the History tab here. I'll see if I can convince phabread to get that version somehow.

martinvonz: Ah, it was there before it got queued. I just found the History tab here. I'll see if I can…
pub use filepatterns::{
build_single_regex, read_pattern_file, PatternSyntax, PatternTuple,
};
/// Mercurial revision numbers /// Mercurial revision numbers
/// ///
/// As noted in revlog.c, revision numbers are actually encoded in /// As noted in revlog.c, revision numbers are actually encoded in
/// 4 bytes, and are liberally converted to ints, whence the i32 /// 4 bytes, and are liberally converted to ints, whence the i32
pub type Revision = i32; pub type Revision = i32;
/// Marker expressing the absence of a parent /// Marker expressing the absence of a parent
/// The simplest expression of what we need of Mercurial DAGs. /// The simplest expression of what we need of Mercurial DAGs.
pub trait Graph { pub trait Graph {
/// Return the two parents of the given `Revision`. /// Return the two parents of the given `Revision`.
/// ///
/// Each of the parents can be independently `NULL_REVISION` /// Each of the parents can be independently `NULL_REVISION`
fn parents(&self, Revision) -> Result<[Revision; 2], GraphError>; fn parents(&self, Revision) -> Result<[Revision; 2], GraphError>;
} }
pub type LineNumber = usize;
kevincoxUnsubmitted
Done

This seems like overkill to me. I would either remove it or use pub struct LineNumber(pub usize) if you want to do this for type safety. However I don't think type safety is really needed here so I would just remove the alias.

kevincox: This seems like overkill to me. I would either remove it or use `pub struct LineNumber(pub…
AlphareAuthorUnsubmitted
Done

I would argue it's here for readability's sake. I used to have usize, but was advised to put an alias here because it would lessen mental overhead to read the function signatures. I'll remove it If you disagree.

Alphare: I would argue it's here for readability's sake. I used to have `usize`, but was advised to put…
#[derive(Clone, Debug, PartialEq)] #[derive(Clone, Debug, PartialEq)]
pub enum GraphError { pub enum GraphError {
ParentOutOfRange(Revision), ParentOutOfRange(Revision),
WorkingDirectoryUnsupported, WorkingDirectoryUnsupported,
} }
#[derive(Clone, Debug, PartialEq)] #[derive(Clone, Debug, PartialEq)]
pub enum DirstateParseError { pub enum DirstateParseError {
} }
} }
impl From<std::io::Error> for DirstateParseError { impl From<std::io::Error> for DirstateParseError {
fn from(e: std::io::Error) -> Self { fn from(e: std::io::Error) -> Self {
DirstateParseError::CorruptedEntry(e.to_string()) DirstateParseError::CorruptedEntry(e.to_string())
} }
} }
#[derive(Debug)]
pub enum PatternError {
UnsupportedSyntax(String),
}
#[derive(Debug)]
pub enum PatternFileError {
IO(std::io::Error),
Pattern(PatternError, LineNumber),
}
impl From<std::io::Error> for PatternFileError {
fn from(e: std::io::Error) -> Self {
PatternFileError::IO(e)
}
}