Skip to content

Commit

Permalink
str_util: support case‐insensitive string patterns
Browse files Browse the repository at this point in the history
Partially resolve a 1.5‐year‐old TODO comment.

Add opt‐in syntax for case‐insensitive matching, suffixing the
pattern kind with `-i`. Not every context supports case‐insensitive
patterns (e.g. Git branch fetch settings). It may make sense to make
this the default in at least some contexts (e.g. the commit signature
and description revsets), but it would require some thought to avoid
more confusing context‐sensitivity.

Make `mine()` match case‐insensitively unconditionally, since email
addresses are conventionally case‐insensitive and it doesn’t take
a pattern anyway.

This currently only handles ASCII case folding, due to the complexities
of case‐insensitive Unicode comparison and the `glob` crate’s lack
of support for it. This is unlikely to matter for email addresses,
which very rarely contain non‐ASCII characters, but is unfortunate
for names and descriptions. However, the current matching behaviour is
already seriously deficient for non‐ASCII text due to the lack of any
normalization, so this hopefully shouldn’t be a blocker to adding the
interface. An expository comment has been left in the code for anyone
who wants to try and address this (perhaps a future version of myself).
  • Loading branch information
emilazy committed Jul 8, 2024
1 parent 3aa7561 commit d8438ce
Show file tree
Hide file tree
Showing 7 changed files with 138 additions and 27 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

* `jj squash` now accepts a `--keep-emptied` option to keep the source commit.

* String patterns now support case‐insensitive matching by suffixing any
pattern kind with `-i`. `mine()` uses case‐insensitive matching on your email
address unconditionally. Only ASCII case folding is currently implemented,
but this will likely change in the future.

### Fixed bugs

* `jj git push` now ignores immutable commits when checking whether a
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/git/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ fn find_branches_targeted_by_revisions<'a>(
};
let current_branches_expression = RevsetExpression::remote_branches(
StringPattern::everything(),
StringPattern::Exact(remote_name.to_owned()),
StringPattern::exact(remote_name),
)
.range(&RevsetExpression::commit(wc_commit_id))
.intersection(&RevsetExpression::branches(StringPattern::everything()));
Expand Down
3 changes: 3 additions & 0 deletions docs/revsets.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ Functions that perform string matching support the following pattern syntax:
* `glob:"pattern"`: Matches strings with Unix-style shell [wildcard
`pattern`](https://docs.rs/glob/latest/glob/struct.Pattern.html).

You can append `-i` after the kind to match case‐insensitively (e.g.
`glob-i:"fix*jpeg*"`).

## Aliases

New symbols and functions can be defined in the config file, by using any
Expand Down
3 changes: 1 addition & 2 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1052,8 +1052,7 @@ fn build_predicate_fn(
RevsetFilterPredicate::Author(pattern) => {
let pattern = pattern.clone();
// TODO: Make these functions that take a needle to search for accept some
// syntax for specifying whether it's a regex and whether it's
// case-sensitive.
// syntax for specifying whether it's a regex.
box_pure_predicate_fn(move |index, pos| {
let entry = index.entry_by_pos(pos);
let commit = store.get_commit(&entry.commit_id()).unwrap();
Expand Down
7 changes: 5 additions & 2 deletions lib/src/revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -687,8 +687,11 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
});
map.insert("mine", |function, context| {
function.expect_no_arguments()?;
// Email address domains are inherently case‐insensitive, and the local‐parts
// are generally (although not universally) treated as case‐insensitive too, so
// we use a case‐insensitive match here.
Ok(RevsetExpression::filter(RevsetFilterPredicate::Author(
StringPattern::Exact(context.user_email.to_owned()),
StringPattern::exact_i(&context.user_email),
)))
});
map.insert("committer", |function, _context| {
Expand Down Expand Up @@ -2518,7 +2521,7 @@ mod tests {
assert!(parse("mine(foo)").is_err());
insta::assert_debug_snapshot!(
parse("mine()").unwrap(),
@r###"Filter(Author(Exact("[email protected]")))"###);
@r###"Filter(Author(ExactI("[email protected]")))"###);
insta::assert_debug_snapshot!(
parse_with_workspace("empty()", &WorkspaceId::default()).unwrap(),
@"NotIn(Filter(File(All)))");
Expand Down
124 changes: 103 additions & 21 deletions lib/src/str_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,26 @@ pub enum StringPatternParseError {
GlobPattern(glob::PatternError),
}

fn parse_glob(src: &str) -> Result<glob::Pattern, StringPatternParseError> {
glob::Pattern::new(src).map_err(StringPatternParseError::GlobPattern)
}

/// Pattern to be tested against string property like commit description or
/// branch name.
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum StringPattern {
/// Matches strings exactly equal to `string`.
/// Matches strings exactly.
Exact(String),
/// Unix-style shell wildcard pattern.
Glob(glob::Pattern),
/// Matches strings that contain `substring`.
/// Matches strings case‐insensitively.
ExactI(String),
/// Matches strings that contain a substring.
Substring(String),
/// Matches strings that case‐insensitively contain a substring.
SubstringI(String),
/// Matches with a Unix‐style shell wildcard pattern.
Glob(glob::Pattern),
/// Matches with a case‐insensitive Unix‐style shell wildcard pattern.
GlobI(glob::Pattern),
}

impl StringPattern {
Expand All @@ -50,11 +60,12 @@ impl StringPattern {
StringPattern::Substring(String::new())
}

/// Parses the given string as a `StringPattern`. Everything before the
/// first ":" is considered the string's prefix. If the prefix is "exact:",
/// "glob:", or "substring:", a pattern of the specified kind is returned.
/// Returns an error if the string has an unrecognized prefix. Otherwise, a
/// `StringPattern::Exact` is returned.
/// Parses the given string as a [`StringPattern`]. Everything before the
/// first ":" is considered the string's prefix. If the prefix is
/// "exact[-i]:", "glob[-i]:", or "substring[-i]:", a pattern of the
/// specified kind is returned. Returns an error if the string has an
/// unrecognized prefix. Otherwise, a `StringPattern::Exact` is
/// returned.
pub fn parse(src: &str) -> Result<StringPattern, StringPatternParseError> {
if let Some((kind, pat)) = src.split_once(':') {
StringPattern::from_str_kind(pat, kind)
Expand All @@ -63,26 +74,48 @@ impl StringPattern {
}
}

/// Creates pattern that matches exactly.
/// Constructs a pattern that matches exactly.
pub fn exact(src: impl Into<String>) -> Self {
StringPattern::Exact(src.into())
}

/// Parses the given string as glob pattern.
/// Constructs a pattern that matches case‐insensitively.
pub fn exact_i(src: impl Into<String>) -> Self {
StringPattern::ExactI(src.into())
}

/// Constructs a pattern that matches a substring.
pub fn substring(src: impl Into<String>) -> Self {
StringPattern::Substring(src.into())
}

/// Constructs a pattern that case‐insensitively matches a substring
pub fn substring_i(src: impl Into<String>) -> Self {
StringPattern::SubstringI(src.into())
}

/// Parses the given string as a glob pattern.
pub fn glob(src: &str) -> Result<Self, StringPatternParseError> {
// TODO: might be better to do parsing and compilation separately since
// not all backends would use the compiled pattern object.
// TODO: if no meta character found, it can be mapped to Exact.
let pattern = glob::Pattern::new(src).map_err(StringPatternParseError::GlobPattern)?;
Ok(StringPattern::Glob(pattern))
Ok(StringPattern::Glob(parse_glob(src)?))
}

/// Parses the given string as pattern of the specified `kind`.
/// Parses the given string as a case‐insensitive glob pattern.
pub fn glob_i(src: &str) -> Result<Self, StringPatternParseError> {
Ok(StringPattern::GlobI(parse_glob(src)?))
}

/// Parses the given string as a pattern of the specified `kind`.
pub fn from_str_kind(src: &str, kind: &str) -> Result<Self, StringPatternParseError> {
match kind {
"exact" => Ok(StringPattern::exact(src)),
"exact-i" => Ok(StringPattern::exact_i(src)),
"substring" => Ok(StringPattern::substring(src)),
"substring-i" => Ok(StringPattern::substring_i(src)),
"glob" => StringPattern::glob(src),
"substring" => Ok(StringPattern::Substring(src.to_owned())),
"glob-i" => StringPattern::glob_i(src),
_ => Err(StringPatternParseError::InvalidKind(kind.to_owned())),
}
}
Expand All @@ -96,41 +129,86 @@ impl StringPattern {
///
/// This can be used to optimize map lookup by exact key.
pub fn as_exact(&self) -> Option<&str> {
// TODO: Handle trivial case‐insensitive patterns here? It might make people
// expect they can use case‐insensitive patterns in contexts where they
// generally can’t.
match self {
StringPattern::Exact(literal) => Some(literal),
StringPattern::Glob(_) | StringPattern::Substring(_) => None,
_ => None,
}
}

/// Returns the original string of this pattern.
pub fn as_str(&self) -> &str {
match self {
StringPattern::Exact(literal) => literal,
StringPattern::Glob(pattern) => pattern.as_str(),
StringPattern::ExactI(literal) => literal,
StringPattern::Substring(needle) => needle,
StringPattern::SubstringI(needle) => needle,
StringPattern::Glob(pattern) => pattern.as_str(),
StringPattern::GlobI(pattern) => pattern.as_str(),
}
}

/// Converts this pattern to a glob string. Returns `None` if the pattern
/// can't be represented as a glob.
pub fn to_glob(&self) -> Option<Cow<'_, str>> {
// TODO: If we add Regex pattern, it will return None.
//
// TODO: Handle trivial case‐insensitive patterns here? It might make people
// expect they can use case‐insensitive patterns in contexts where they
// generally can’t.
match self {
StringPattern::Exact(literal) => Some(glob::Pattern::escape(literal).into()),
StringPattern::Glob(pattern) => Some(pattern.as_str().into()),
StringPattern::Substring(needle) if needle.is_empty() => Some("*".into()),
StringPattern::Substring(needle) => {
Some(format!("*{}*", glob::Pattern::escape(needle)).into())
if needle.is_empty() {
Some("*".into())
} else {
Some(format!("*{}*", glob::Pattern::escape(needle)).into())
}
}
StringPattern::Glob(pattern) => Some(pattern.as_str().into()),
StringPattern::ExactI(_) => None,
StringPattern::SubstringI(_) => None,
StringPattern::GlobI(_) => None,
}
}

/// Returns true if this pattern matches the `haystack`.
///
/// When matching against a case‐insensitive pattern, only ASCII case
/// differences are currently folded. This may change in the future.
pub fn matches(&self, haystack: &str) -> bool {
// TODO: Unicode case folding is complicated and can be locale‐specific. The
// `glob` crate and Gitoxide only deal with ASCII case folding, so we do
// the same here; a more elaborate case folding system will require
// making sure those behave in a matching manner where relevant.
//
// Care will need to be taken regarding normalization and the choice of an
// appropriate case‐insensitive comparison scheme (`toNFKC_Casefold`?) to ensure
// that it is compatible with the standard case‐insensitivity of haystack
// components (like internationalized domain names in email addresses). The
// availability of normalization and case folding schemes in database backends
// will also need to be considered. A locale‐specific case folding
// scheme would likely not be appropriate for Jujutsu.
//
// For some discussion of this topic, see:
// <https://github.com/unicode-org/icu4x/issues/3151>
match self {
StringPattern::Exact(literal) => haystack == literal,
StringPattern::Glob(pattern) => pattern.matches(haystack),
StringPattern::ExactI(literal) => haystack.eq_ignore_ascii_case(literal),
StringPattern::Substring(needle) => haystack.contains(needle),
StringPattern::SubstringI(needle) => haystack
.to_ascii_lowercase()
.contains(&needle.to_ascii_lowercase()),
StringPattern::Glob(pattern) => pattern.matches(haystack),
StringPattern::GlobI(pattern) => pattern.matches_with(
haystack,
glob::MatchOptions {
case_sensitive: false,
..glob::MatchOptions::new()
},
),
}
}

Expand Down Expand Up @@ -192,6 +270,10 @@ mod tests {
StringPattern::parse("substring:foo").unwrap(),
StringPattern::from_str_kind("foo", "substring").unwrap()
);
assert_eq!(
StringPattern::parse("substring-i:foo").unwrap(),
StringPattern::from_str_kind("foo", "substring-i").unwrap()
);

// Parse a pattern that contains a : itself.
assert_eq!(
Expand Down
21 changes: 20 additions & 1 deletion lib/tests/test_revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2398,6 +2398,15 @@ fn test_evaluate_expression_author() {
resolve_commit_ids(mut_repo, "author(\"email3\")"),
vec![commit3.id().clone()]
);
// Can match case‐insensitively
assert_eq!(
resolve_commit_ids(mut_repo, "author(substring-i:Name)"),
vec![
commit3.id().clone(),
commit2.id().clone(),
commit1.id().clone(),
]
);
// Searches only among candidates if specified
assert_eq!(
resolve_commit_ids(mut_repo, "visible_heads() & author(\"name2\")"),
Expand Down Expand Up @@ -2452,7 +2461,8 @@ fn test_evaluate_expression_mine() {
.set_parents(vec![commit2.id().clone()])
.set_author(Signature {
name: "name3".to_string(),
email: settings.user_email(),
// Test that matches are case‐insensitive
email: settings.user_email().to_ascii_uppercase(),
timestamp,
})
.write()
Expand Down Expand Up @@ -2539,6 +2549,15 @@ fn test_evaluate_expression_committer() {
resolve_commit_ids(mut_repo, "committer(\"email3\")"),
vec![commit3.id().clone()]
);
// Can match case‐insensitively
assert_eq!(
resolve_commit_ids(mut_repo, "committer(substring-i:Name)"),
vec![
commit3.id().clone(),
commit2.id().clone(),
commit1.id().clone(),
]
);
// Searches only among candidates if specified
assert_eq!(
resolve_commit_ids(mut_repo, "visible_heads() & committer(\"name2\")"),
Expand Down

0 comments on commit d8438ce

Please sign in to comment.