( )⚙ D11722 rhg: implement the debugignorerhg subcommand

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11722

rhg: implement the debugignorerhg subcommand
ClosedPublic

Authored by aalekseyev on Oct 27 2021, 7:42 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG6d4daf51283c: rhg: implement the debugignorerhg subcommand
Summary

This can be used to inspect the generated pattern, but also to benchmark
the time it takes to parse hgignore.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

aalekseyev created this revision.Oct 27 2021, 7:42 AM
aalekseyev retitled this revision from rhg: implement the rhg-debugignore subcommand to rhg: implement the debugignorerhg subcommand.Oct 27 2021, 7:44 AM
aalekseyev added a comment.EditedOct 27 2021, 7:52 AM

<a comment posted in the wrong thread by accident>

aalekseyev updated this revision to Diff 30993.Oct 27 2021, 7:56 AM
aalekseyev updated this revision to Diff 30997.Oct 27 2021, 2:48 PM
Alphare added a subscriber: Alphare.Oct 28 2021, 5:40 AM

I'll look at these patches and take them after 6.0 is out (this includes the one I've already accepted). It's been delayed quite a bit due to Windows issues.

Alphare requested changes to this revision.Nov 24 2021, 4:48 AM

Thanks for waiting for so long. 6.0 is out (it was delayed a lot for Windows reasons), so I'll be back to reviewing patches for default.

rust/hg-core/src/matchers.rs
605

It would be nice to have a type alias for this (and the one below). We already have one in hg-core/src/dirstate/status.rs, probably should be refactored.

rust/rhg/src/commands/debugignorerhg.rs
18

nit: we prefer new commands to have dashes in them now, but it's really not a big deal since this is temporary anyway

This revision now requires changes to proceed.Nov 24 2021, 4:48 AM
aalekseyev edited the summary of this revision. (Show Details)Nov 26 2021, 2:35 PM
aalekseyev updated this revision to Diff 31169.
aalekseyev updated this revision to Diff 31170.Nov 26 2021, 2:42 PM
aalekseyev updated this revision to Diff 31171.Nov 26 2021, 2:45 PM
aalekseyev updated this revision to Diff 31172.Nov 26 2021, 2:49 PM
aalekseyev added inline comments.Nov 26 2021, 2:49 PM
rust/hg-core/src/matchers.rs
605

I used the alias from status.rs now.

rust/rhg/src/commands/debugignorerhg.rs
18

I'm not sure how to do that, actually. I tried renaming the command to debugignorerhg, but got an error:

468 |     debugignore-rhg
    |                ^ no rules expected this token in macro call
Alphare accepted this revision.Nov 29 2021, 6:08 AM
Alphare added inline comments.
rust/rhg/src/commands/debugignorerhg.rs
18

Ah right, we'd need a smarter macro, this is well out of scope

This revision is now accepted and ready to land.Nov 29 2021, 6:08 AM
Alphare requested changes to this revision.Nov 29 2021, 6:18 AM

Actually this also results in a folded commit, please re-send

This revision now requires changes to proceed.Nov 29 2021, 6:18 AM
aalekseyev edited the summary of this revision. (Show Details)Nov 29 2021, 8:03 AM
aalekseyev updated this revision to Diff 31185.
aalekseyev added a comment.Nov 29 2021, 8:05 AM

@Alphare, folded commit seems better than the separate ones in this case, though, since the second commit is simply incorporating review comments, not really an independent change.
I ran hg fold --from .^ followed by hg phabsend . to make this less confusing.

If you were thinking of a certain logical split of this commit into two that I'm not aware of, please let me know.

Alphare added a comment.Nov 29 2021, 8:37 AM
In D11722#181171, @aalekseyev wrote:

@Alphare, folded commit seems better than the separate ones in this case, though, since the second commit is simply incorporating review comments, not really an independent change.
I ran hg fold --from .^ followed by hg phabsend . to make this less confusing.
If you were thinking of a certain logical split of this commit into two that I'm not aware of, please let me know.

Sure, most of the time, changes can be incorporated into the same changeset, but for ones that move a lot of code around (like refactor suggestions), we usually like to split them. This has a two-fold advantage of making the review of the actual change more obvious, and to make sure a mistake didn't appear in the refactor.

In this case, I should have been more explicit: I saw two concatenated commit messages and figured you already had two changesets, so I asked you to send them as separate.

Basically: one changeset == one phab diff

aalekseyev added a comment.Nov 29 2021, 8:43 AM

@Alphare, Ok. I understood you before, but those commits were not suitable for independent submission.

I only have one commit now. Would you like me to split something out into a separate commit?

Alphare requested changes to this revision.Nov 29 2021, 8:52 AM

Yes, I think in this case a separate changeset for the type refactor (of the old code) would make sense.

Also I was sure I added a comment about the println! usage, but I must have forgotten!

rust/rhg/src/commands/debugignorerhg.rs
38

This should use ui.write_stdout, which would also remove the need for the potentially lossy utf8 conversion.

This revision now requires changes to proceed.Nov 29 2021, 8:52 AM
aalekseyev added a parent revision: D11818: rhg: refactor to use IgnoreFnType alias more widely.Nov 29 2021, 9:09 AM
aalekseyev edited the summary of this revision. (Show Details)
aalekseyev updated this revision to Diff 31187.
aalekseyev marked an inline comment as done.Nov 29 2021, 9:11 AM

Done. The refactor part was extracted into https://phab.mercurial-scm.org/D11818.

Alphare accepted this revision.Nov 29 2021, 9:12 AM

Thanks, both look good.

This revision is now accepted and ready to land.Nov 29 2021, 9:12 AM
aalekseyev added a commit: rHG6d4daf51283c: rhg: implement the debugignorerhg subcommand.Nov 29 2021, 10:44 AM
Closed by commit rHG6d4daf51283c: rhg: implement the debugignorerhg subcommand (authored by aalekseyev). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 31170

rust/hg-core/src/matchers.rs

use crate::filepatterns::normalize_path_bytes; use crate::filepatterns::normalize_path_bytes;
use std::borrow::ToOwned; use std::borrow::ToOwned;
use std::collections::HashSet; use std::collections::HashSet;
use std::fmt::{Display, Error, Formatter}; use std::fmt::{Display, Error, Formatter};
use std::iter::FromIterator; use std::iter::FromIterator;
use std::ops::Deref; use std::ops::Deref;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use crate::dirstate::status::IgnoreFnType;
use micro_timer::timed; use micro_timer::timed;
#[derive(Debug, PartialEq)] #[derive(Debug, PartialEq)]
pub enum VisitChildrenSet<'a> { pub enum VisitChildrenSet<'a> {
/// Don't visit anything /// Don't visit anything
Empty, Empty,
/// Only visit this directory /// Only visit this directory
/// /// /// ///
/// assert_eq!(matcher.matches(HgPath::new(b"testing")), false); /// assert_eq!(matcher.matches(HgPath::new(b"testing")), false);
/// assert_eq!(matcher.matches(HgPath::new(b"this should work")), true); /// assert_eq!(matcher.matches(HgPath::new(b"this should work")), true);
/// assert_eq!(matcher.matches(HgPath::new(b"this also")), true); /// assert_eq!(matcher.matches(HgPath::new(b"this also")), true);
/// assert_eq!(matcher.matches(HgPath::new(b"but not this")), false); /// assert_eq!(matcher.matches(HgPath::new(b"but not this")), false);
/// ``` /// ```
pub struct IncludeMatcher<'a> { pub struct IncludeMatcher<'a> {
patterns: Vec<u8>, patterns: Vec<u8>,
match_fn: Box<dyn for<'r> Fn(&'r HgPath) -> bool + 'a + Sync>, match_fn: IgnoreFnType<'a>,
/// Whether all the patterns match a prefix (i.e. recursively) /// Whether all the patterns match a prefix (i.e. recursively)
prefix: bool, prefix: bool,
roots: HashSet<HgPathBuf>, roots: HashSet<HgPathBuf>,
dirs: HashSet<HgPathBuf>, dirs: HashSet<HgPathBuf>,
parents: HashSet<HgPathBuf>, parents: HashSet<HgPathBuf>,
} }
impl<'a> Matcher for IncludeMatcher<'a> { impl<'a> Matcher for IncludeMatcher<'a> {
.build() .build()
.map_err(|e| PatternError::UnsupportedSyntax(e.to_string()))?; .map_err(|e| PatternError::UnsupportedSyntax(e.to_string()))?;
Ok(move |path: &HgPath| re.is_match(path.as_bytes())) Ok(move |path: &HgPath| re.is_match(path.as_bytes()))
} }
/// Returns the regex pattern and a function that matches an `HgPath` against /// Returns the regex pattern and a function that matches an `HgPath` against
/// said regex formed by the given ignore patterns. /// said regex formed by the given ignore patterns.
fn build_regex_match( fn build_regex_match<'a, 'b>(
ignore_patterns: &[IgnorePattern], ignore_patterns: &'a[IgnorePattern],
) -> PatternResult<(Vec<u8>, Box<dyn Fn(&HgPath) -> bool + Sync>)> { ) -> PatternResult<(Vec<u8>, IgnoreFnType<'b>)> {
let mut regexps = vec![]; let mut regexps = vec![];
let mut exact_set = HashSet::new(); let mut exact_set = HashSet::new();
for pattern in ignore_patterns { for pattern in ignore_patterns {
if let Some(re) = build_single_regex(pattern)? { if let Some(re) = build_single_regex(pattern)? {
regexps.push(re); regexps.push(re);
} else { } else {
let exact = normalize_path_bytes(&pattern.pattern); let exact = normalize_path_bytes(&pattern.pattern);
exact_set.insert(HgPathBuf::from_bytes(&exact)); exact_set.insert(HgPathBuf::from_bytes(&exact));
} }
} }
let full_regex = regexps.join(&b'|'); let full_regex = regexps.join(&b'|');
// An empty pattern would cause the regex engine to incorrectly match the // An empty pattern would cause the regex engine to incorrectly match the
// (empty) root directory // (empty) root directory
let func = if !(regexps.is_empty()) { let func = if !(regexps.is_empty()) {
let matcher = re_matcher(&full_regex)?; let matcher = re_matcher(&full_regex)?;
let func = move |filename: &HgPath| { let func = move |filename: &HgPath| {
exact_set.contains(filename) || matcher(filename) exact_set.contains(filename) || matcher(filename)
}; };
Box::new(func) as Box<dyn Fn(&HgPath) -> bool + Sync> Box::new(func) as IgnoreFnType
} else { } else {
let func = move |filename: &HgPath| exact_set.contains(filename); let func = move |filename: &HgPath| exact_set.contains(filename);
Box::new(func) as Box<dyn Fn(&HgPath) -> bool + Sync> Box::new(func) as IgnoreFnType
}; };
Ok((full_regex, func)) Ok((full_regex, func))
} }
/// Returns roots and directories corresponding to each pattern. /// Returns roots and directories corresponding to each pattern.
/// ///
/// This calculates the roots and directories exactly matching the patterns and /// This calculates the roots and directories exactly matching the patterns and
parents, parents,
}) })
} }
/// Returns a function that checks whether a given file (in the general sense) /// Returns a function that checks whether a given file (in the general sense)
/// should be matched. /// should be matched.
fn build_match<'a, 'b>( fn build_match<'a, 'b>(
ignore_patterns: Vec<IgnorePattern>, ignore_patterns: Vec<IgnorePattern>,
) -> PatternResult<(Vec<u8>, Box<dyn Fn(&HgPath) -> bool + 'b + Sync>)> { ) -> PatternResult<(Vec<u8>, IgnoreFnType<'b>)> {
let mut match_funcs: Vec<Box<dyn Fn(&HgPath) -> bool + Sync>> = vec![]; let mut match_funcs: Vec<IgnoreFnType<'b>> = vec![];
// For debugging and printing // For debugging and printing
let mut patterns = vec![]; let mut patterns = vec![];
let (subincludes, ignore_patterns) = filter_subincludes(ignore_patterns)?; let (subincludes, ignore_patterns) = filter_subincludes(ignore_patterns)?;
if !subincludes.is_empty() { if !subincludes.is_empty() {
// Build prefix-based matcher functions for subincludes // Build prefix-based matcher functions for subincludes
let mut submatchers = FastHashMap::default(); let mut submatchers = FastHashMap::default();
}), }),
) )
}) })
} }
/// Parses all "ignore" files with their recursive includes and returns a /// Parses all "ignore" files with their recursive includes and returns a
/// function that checks whether a given file (in the general sense) should be /// function that checks whether a given file (in the general sense) should be
/// ignored. /// ignored.
pub fn get_ignore_function<'a>( pub fn get_ignore_matcher<'a>(
mut all_pattern_files: Vec<PathBuf>, mut all_pattern_files: Vec<PathBuf>,
root_dir: &Path, root_dir: &Path,
inspect_pattern_bytes: &mut impl FnMut(&[u8]), inspect_pattern_bytes: &mut impl FnMut(&[u8]),
) -> PatternResult<( ) -> PatternResult<(IncludeMatcher<'a>, Vec<PatternFileWarning>)> {
Box<dyn for<'r> Fn(&'r HgPath) -> bool + Sync + 'a>,
Vec<PatternFileWarning>,
)> {
let mut all_patterns = vec![]; let mut all_patterns = vec![];
let mut all_warnings = vec![]; let mut all_warnings = vec![];
// Sort to make the ordering of calls to `inspect_pattern_bytes` // Sort to make the ordering of calls to `inspect_pattern_bytes`
// deterministic even if the ordering of `all_pattern_files` is not (such // deterministic even if the ordering of `all_pattern_files` is not (such
// as when a iteration order of a Python dict or Rust HashMap is involved). // as when a iteration order of a Python dict or Rust HashMap is involved).
// Sort by "string" representation instead of the default by component // Sort by "string" representation instead of the default by component
// (with a Rust-specific definition of a component) // (with a Rust-specific definition of a component)
all_pattern_files all_pattern_files
.sort_unstable_by(|a, b| a.as_os_str().cmp(b.as_os_str())); .sort_unstable_by(|a, b| a.as_os_str().cmp(b.as_os_str()));
for pattern_file in &all_pattern_files { for pattern_file in &all_pattern_files {
let (patterns, warnings) = get_patterns_from_file( let (patterns, warnings) = get_patterns_from_file(
pattern_file, pattern_file,
root_dir, root_dir,
inspect_pattern_bytes, inspect_pattern_bytes,
)?; )?;
all_patterns.extend(patterns.to_owned()); all_patterns.extend(patterns.to_owned());
all_warnings.extend(warnings); all_warnings.extend(warnings);
} }
let matcher = IncludeMatcher::new(all_patterns)?; let matcher = IncludeMatcher::new(all_patterns)?;
Ok(( Ok((matcher, all_warnings))
Box::new(move |path: &HgPath| matcher.matches(path)), }
all_warnings,
)) /// Parses all "ignore" files with their recursive includes and returns a
/// function that checks whether a given file (in the general sense) should be
/// ignored.
pub fn get_ignore_function<'a>(
all_pattern_files: Vec<PathBuf>,
root_dir: &Path,
inspect_pattern_bytes: &mut impl FnMut(&[u8]),
) -> PatternResult<(
IgnoreFnType<'a>,
AlphareUnsubmitted
Not Done

It would be nice to have a type alias for this (and the one below). We already have one in hg-core/src/dirstate/status.rs, probably should be refactored.

Alphare: It would be nice to have a type alias for this (and the one below). We already have one in `hg…
aalekseyevAuthorUnsubmitted
Done

I used the alias from status.rs now.

aalekseyev: I used the alias from status.rs now.
Vec<PatternFileWarning>,
)> {
let res =
get_ignore_matcher(all_pattern_files, root_dir, inspect_pattern_bytes);
res.map(|(matcher, all_warnings)| {
let res: IgnoreFnType<'a> =
Box::new(move |path: &HgPath| matcher.matches(path));
(res, all_warnings)
})
} }
impl<'a> IncludeMatcher<'a> { impl<'a> IncludeMatcher<'a> {
pub fn new(ignore_patterns: Vec<IgnorePattern>) -> PatternResult<Self> { pub fn new(ignore_patterns: Vec<IgnorePattern>) -> PatternResult<Self> {
let RootsDirsAndParents { let RootsDirsAndParents {
roots, roots,
dirs, dirs,
parents, parents,
// TODO cache // TODO cache
let thing = self let thing = self
.dirs .dirs
.iter() .iter()
.chain(self.roots.iter()) .chain(self.roots.iter())
.chain(self.parents.iter()); .chain(self.parents.iter());
DirsChildrenMultiset::new(thing, Some(&self.parents)) DirsChildrenMultiset::new(thing, Some(&self.parents))
} }
pub fn debug_get_patterns(&self) -> &[u8] {
self.patterns.as_ref()
}
} }
impl<'a> Display for IncludeMatcher<'a> { impl<'a> Display for IncludeMatcher<'a> {
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {
// XXX What about exact matches? // XXX What about exact matches?
// I'm not sure it's worth it to clone the HashSet and keep it // I'm not sure it's worth it to clone the HashSet and keep it
// around just in case someone wants to display the matcher, plus // around just in case someone wants to display the matcher, plus
// it's going to be unreadable after a few entries, but we need to // it's going to be unreadable after a few entries, but we need to

rust/rhg/src/commands/debugignorerhg.rs

  • This file was added.
use crate::error::CommandError;
use clap::SubCommand;
use hg;
use hg::matchers::get_ignore_matcher;
use hg::StatusError;
use log::warn;
pub const HELP_TEXT: &str = "
Show effective hgignore patterns used by rhg.
This is a pure Rust version of `hg debugignore`.
Some options might be missing, check the list below.
";
pub fn args() -> clap::App<'static, 'static> {
SubCommand::with_name("debugignorerhg").about(HELP_TEXT)
}
AlphareUnsubmitted
Not Done

nit: we prefer new commands to have dashes in them now, but it's really not a big deal since this is temporary anyway

Alphare: nit: we prefer new commands to have dashes in them now, but it's really not a big deal since…
aalekseyevAuthorUnsubmitted
Done

I'm not sure how to do that, actually. I tried renaming the command to debugignorerhg, but got an error:

468 |     debugignore-rhg
    |                ^ no rules expected this token in macro call
aalekseyev: I'm not sure how to do that, actually. I tried renaming the command to debugignorerhg, but got…
AlphareUnsubmitted
Not Done

Ah right, we'd need a smarter macro, this is well out of scope

Alphare: Ah right, we'd need a smarter macro, this is well out of scope
pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
let repo = invocation.repo?;
let ignore_file = repo.working_directory_vfs().join(".hgignore"); // TODO hardcoded
let (ignore_matcher, warnings) = get_ignore_matcher(
vec![ignore_file],
&repo.working_directory_path().to_owned(),
&mut |_pattern_bytes| (),
)
.map_err(|e| StatusError::from(e))?;
if !warnings.is_empty() {
warn!("Pattern warnings: {:?}", &warnings);
}
let patterns = ignore_matcher.debug_get_patterns();
println!("{}", String::from_utf8_lossy(patterns));
Ok(())
AlphareUnsubmitted
Done

This should use ui.write_stdout, which would also remove the need for the potentially lossy utf8 conversion.

Alphare: This should use `ui.write_stdout`, which would also remove the need for the potentially lossy…
}

rust/rhg/src/main.rs

} }
}; };
} }
subcommands! { subcommands! {
cat cat
debugdata debugdata
debugrequirements debugrequirements
debugignorerhg
files files
root root
config config
status status
} }
pub struct CliInvocation<'a> { pub struct CliInvocation<'a> {
ui: &'a Ui, ui: &'a Ui,