From bb87fac1a459c8451a0b1666fd4256bc753a280d Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 31 Mar 2024 15:20:02 +0900 Subject: [PATCH] revset: parse "all:" prefix rule by pest I had to use negative lookahead !":" because we still support a dummy ":" operator to provide a suggestion. --- cli/src/cli_util.rs | 23 +++-- cli/tests/test_revset_output.rs | 12 +++ lib/src/revset.pest | 4 + lib/src/revset.rs | 168 ++++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 10 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 9ed7e49a0d..be57662d57 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -52,7 +52,7 @@ use jj_lib::repo::{ }; use jj_lib::repo_path::{FsPathParseError, RepoPath, RepoPathBuf}; use jj_lib::revset::{ - RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, + RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, RevsetModifier, RevsetParseContext, RevsetWorkspaceContext, }; use jj_lib::rewrite::restore_tree; @@ -771,7 +771,11 @@ impl WorkspaceCommandHelper { ) -> Result, CommandError> { let mut all_commits = IndexSet::new(); for revision_str in revision_args { - let (expression, all) = self.parse_revset_with_all_prefix(revision_str)?; + let (expression, modifier) = self.parse_revset_with_modifier(revision_str)?; + let all = match modifier { + Some(RevsetModifier::All) => true, + None => false, + }; if all { for commit in expression.evaluate_to_commits()? { all_commits.insert(commit?); @@ -807,16 +811,15 @@ impl WorkspaceCommandHelper { self.attach_revset_evaluator(expression) } - fn parse_revset_with_all_prefix( + // TODO: maybe better to parse all: prefix even if it is the default? It + // shouldn't be allowed in aliases, though. + fn parse_revset_with_modifier( &self, revision_str: &str, - ) -> Result<(RevsetExpressionEvaluator<'_>, bool), CommandError> { - // TODO: Let pest parse the prefix too once we've dropped support for `:` - if let Some(revision_str) = revision_str.strip_prefix("all:") { - Ok((self.parse_revset(revision_str)?, true)) - } else { - Ok((self.parse_revset(revision_str)?, false)) - } + ) -> Result<(RevsetExpressionEvaluator<'_>, Option), CommandError> { + let context = self.revset_parse_context(); + let (expression, modifier) = revset::parse_with_modifier(revision_str, &context)?; + Ok((self.attach_revset_evaluator(expression)?, modifier)) } /// Parses the given revset expressions and concatenates them all. diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 2d3bdd5afb..9fd915ccb9 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -66,6 +66,18 @@ fn test_syntax_error() { = '^' is not a postfix operator Hint: Did you mean '-' for parents? "###); + + // "jj new" supports "all:" prefix + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "ale:x"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: Modifier "ale" doesn't exist + Caused by: --> 1:1 + | + 1 | ale:x + | ^-^ + | + = Modifier "ale" doesn't exist + "###); } #[test] diff --git a/lib/src/revset.pest b/lib/src/revset.pest index e06ffd4c6f..a118701470 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -97,6 +97,10 @@ expression = { } program = _{ SOI ~ whitespace* ~ expression ~ whitespace* ~ EOI } +program_modifier = { identifier ~ pattern_kind_op ~ !":" } +program_with_modifier = _{ + SOI ~ whitespace* ~ (program_modifier ~ whitespace*)? ~ expression ~ whitespace* ~ EOI +} alias_declaration_part = _{ function_name ~ "(" ~ whitespace* ~ formal_parameters ~ whitespace* ~ ")" diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 0c0185a50f..dec825ba22 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -135,6 +135,8 @@ impl Rule { Rule::range_expression => None, Rule::expression => None, Rule::program => None, + Rule::program_modifier => None, + Rule::program_with_modifier => None, Rule::alias_declaration_part => None, Rule::alias_declaration => None, } @@ -171,6 +173,8 @@ pub enum RevsetParseErrorKind { similar_op: String, description: String, }, + #[error(r#"Modifier "{0}" doesn't exist"#)] + NoSuchModifier(String), #[error(r#"Function "{name}" doesn't exist"#)] NoSuchFunction { name: String, @@ -271,6 +275,17 @@ fn rename_rules_in_pest_error(mut err: pest::error::Error) -> pest::error: pub const GENERATION_RANGE_FULL: Range = 0..u64::MAX; pub const GENERATION_RANGE_EMPTY: Range = 0..0; +/// Global flag applied to the entire expression. +/// +/// The core revset engine doesn't use this value. It's up to caller to +/// interpret it to change the evaluation behavior. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum RevsetModifier { + /// Expression can be evaluated to multiple revisions even if a single + /// revision is expected by default. + All, +} + /// Symbol or function to be resolved to `CommitId`s. #[derive(Clone, Debug, Eq, PartialEq)] pub enum RevsetCommitRef { @@ -842,6 +857,39 @@ fn parse_program( parse_expression_rule(first.into_inner(), state) } +fn parse_program_with_modifier( + revset_str: &str, + state: ParseState, +) -> Result<(Rc, Option), RevsetParseError> { + let mut pairs = RevsetParser::parse(Rule::program_with_modifier, revset_str)?; + let first = pairs.next().unwrap(); + match first.as_rule() { + Rule::expression => { + let expression = parse_expression_rule(first.into_inner(), state)?; + Ok((expression, None)) + } + Rule::program_modifier => { + let (lhs, op) = first.into_inner().collect_tuple().unwrap(); + let rhs = pairs.next().unwrap(); + assert_eq!(lhs.as_rule(), Rule::identifier); + assert_eq!(op.as_rule(), Rule::pattern_kind_op); + assert_eq!(rhs.as_rule(), Rule::expression); + let modififer = match lhs.as_str() { + "all" => RevsetModifier::All, + name => { + return Err(RevsetParseError::with_span( + RevsetParseErrorKind::NoSuchModifier(name.to_owned()), + lhs.as_span(), + )); + } + }; + let expression = parse_expression_rule(rhs.into_inner(), state)?; + Ok((expression, Some(modififer))) + } + r => panic!("unexpected revset parse rule: {r:?}"), + } +} + fn parse_expression_rule( pairs: Pairs, state: ParseState, @@ -1525,6 +1573,15 @@ pub fn parse( parse_program(revset_str, state) } +pub fn parse_with_modifier( + revset_str: &str, + context: &RevsetParseContext, +) -> Result<(Rc, Option), RevsetParseError> { + let locals = HashMap::new(); + let state = ParseState::new(context, &locals); + parse_program_with_modifier(revset_str, state) +} + /// `Some` for rewritten expression, or `None` to reuse the original expression. type TransformedExpression = Option>; @@ -2570,6 +2627,29 @@ mod tests { super::parse(revset_str, &context).map_err(|e| e.kind) } + fn parse_with_modifier( + revset_str: &str, + ) -> Result<(Rc, Option), RevsetParseErrorKind> { + parse_with_aliases_and_modifier(revset_str, [] as [(&str, &str); 0]) + } + + fn parse_with_aliases_and_modifier( + revset_str: &str, + aliases: impl IntoIterator, impl Into)>, + ) -> Result<(Rc, Option), RevsetParseErrorKind> { + let mut aliases_map = RevsetAliasesMap::new(); + for (decl, defn) in aliases { + aliases_map.insert(decl, defn).unwrap(); + } + let context = RevsetParseContext { + aliases_map: &aliases_map, + user_email: "test.user@example.com".to_string(), + workspace: None, + }; + // Map error to comparable object + super::parse_with_modifier(revset_str, &context).map_err(|e| e.kind) + } + #[test] #[allow(clippy::redundant_clone)] // allow symbol.clone() fn test_revset_expression_building() { @@ -2902,6 +2982,83 @@ mod tests { assert!(parse("remote_branches(a,,remote=b)").is_err()); } + #[test] + fn test_parse_revset_with_modifier() { + let all_symbol = RevsetExpression::symbol("all".to_owned()); + let foo_symbol = RevsetExpression::symbol("foo".to_owned()); + + // all: is a program modifier, but all:: isn't + assert_eq!( + parse_with_modifier("all:"), + Err(RevsetParseErrorKind::SyntaxError) + ); + assert_eq!( + parse_with_modifier("all:foo"), + Ok((foo_symbol.clone(), Some(RevsetModifier::All))) + ); + assert_eq!( + parse_with_modifier("all::"), + Ok((all_symbol.descendants(), None)) + ); + assert_eq!( + parse_with_modifier("all::foo"), + Ok((all_symbol.dag_range_to(&foo_symbol), None)) + ); + + // all::: could be parsed as all:(::), but rejected for simplicity + assert_eq!( + parse_with_modifier("all:::"), + Err(RevsetParseErrorKind::SyntaxError) + ); + assert_eq!( + parse_with_modifier("all:::foo"), + Err(RevsetParseErrorKind::SyntaxError) + ); + + assert_eq!( + parse_with_modifier("all:(foo)"), + Ok((foo_symbol.clone(), Some(RevsetModifier::All))) + ); + assert_eq!( + parse_with_modifier("all:all::foo"), + Ok(( + all_symbol.dag_range_to(&foo_symbol), + Some(RevsetModifier::All) + )) + ); + assert_eq!( + parse_with_modifier("all:all | foo"), + Ok((all_symbol.union(&foo_symbol), Some(RevsetModifier::All))) + ); + + assert_eq!( + parse_with_modifier("all: ::foo"), + Ok((foo_symbol.ancestors(), Some(RevsetModifier::All))) + ); + assert_eq!( + parse_with_modifier(" all: foo"), + Ok((foo_symbol.clone(), Some(RevsetModifier::All))) + ); + assert_matches!( + parse_with_modifier("(all:foo)"), + Err(RevsetParseErrorKind::NotInfixOperator { .. }) + ); + assert_matches!( + parse_with_modifier("all :foo"), + Err(RevsetParseErrorKind::SyntaxError) + ); + assert_matches!( + parse_with_modifier("all:all:all"), + Err(RevsetParseErrorKind::NotInfixOperator { .. }) + ); + + // Top-level string pattern can't be parsed, which is an error anyway + assert_eq!( + parse_with_modifier(r#"exact:"foo""#), + Err(RevsetParseErrorKind::NoSuchModifier("exact".to_owned())) + ); + } + #[test] fn test_parse_whitespace() { let ascii_whitespaces: String = ('\x00'..='\x7f') @@ -3286,6 +3443,12 @@ mod tests { parse("a@B").unwrap() ); + // Modifier cannot be substituted. + assert_eq!( + parse_with_aliases_and_modifier("all:all", [("all", "ALL")]).unwrap(), + parse_with_modifier("all:ALL").unwrap() + ); + // Multi-level substitution. assert_eq!( parse_with_aliases("A", [("A", "BC"), ("BC", "b|C"), ("C", "c")]).unwrap(), @@ -3307,6 +3470,11 @@ mod tests { parse_with_aliases("A", [("A", "a(")]), Err(RevsetParseErrorKind::BadAliasExpansion("A".to_owned())) ); + // Modifier isn't allowed in alias definition. + assert_eq!( + parse_with_aliases_and_modifier("A", [("A", "all:a")]), + Err(RevsetParseErrorKind::BadAliasExpansion("A".to_owned())) + ); } #[test]