From 815437598ff022faec36f482d94e8a2ac37f98bf Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 13 Feb 2024 14:46:21 +0900 Subject: [PATCH] revset: disable parsing rules of legacy dag range operator The legacy parsing rules are turned into compatibility errors. The x:y rule is temporarily enabled when parsing string patterns. It's weird, but we can't isolate the parsing function because a string pattern may be defined in an alias. --- CHANGELOG.md | 2 + cli/src/cli_util.rs | 7 ++- cli/tests/test_log_command.rs | 47 -------------- cli/tests/test_revset_output.rs | 11 ++++ docs/revsets.md | 2 - lib/src/revset.pest | 13 ++-- lib/src/revset.rs | 107 ++++++++++++++++++++------------ 7 files changed, 92 insertions(+), 97 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 593fb0149..45693ab27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Dropped support for the "legacy" graph-drawing style. Use "ascii" for a very similar result. +* Dropped support for the deprecated `:` revset operator. Use `::` instead. + ### New features * Templates now support logical operators: `||`, `&&`, `!` diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index bcf4beeee..416afb3d9 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -408,7 +408,12 @@ impl From for CommandError { let message = iter::successors(Some(&err), |e| e.origin()).join("\n"); // Only for the top-level error as we can't attach hint to inner errors let hint = match err.kind() { - RevsetParseErrorKind::NotPostfixOperator { + RevsetParseErrorKind::NotPrefixOperator { + op: _, + similar_op, + description, + } + | RevsetParseErrorKind::NotPostfixOperator { op: _, similar_op, description, diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 043b68004..6721d29ef 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -28,53 +28,6 @@ fn test_log_with_empty_revision() { "###); } -#[test] -fn test_log_legacy_range_operator() { - let test_env = TestEnvironment::default(); - test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); - let repo_path = test_env.env_root().join("repo"); - - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r=@:"]); - insta::assert_snapshot!(stdout, @r###" - @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 230dd059 - │ (empty) (no description set) - ~ - "###); - insta::assert_snapshot!(stderr, @r###" - The `:` revset operator is deprecated. Please switch to `::`. - "###); - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r=:@"]); - insta::assert_snapshot!(stdout, @r###" - @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 230dd059 - │ (empty) (no description set) - ◉ zzzzzzzz root() 00000000 - "###); - insta::assert_snapshot!(stderr, @r###" - The `:` revset operator is deprecated. Please switch to `::`. - "###); - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r=root():@"]); - insta::assert_snapshot!(stdout, @r###" - @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 230dd059 - │ (empty) (no description set) - ◉ zzzzzzzz root() 00000000 - "###); - insta::assert_snapshot!(stderr, @r###" - The `:` revset operator is deprecated. Please switch to `::`. - "###); - let (stdout, stderr) = test_env.jj_cmd_ok( - &repo_path, - &["log", "-r=x", "--config-toml", "revset-aliases.x = '@:'"], - ); - insta::assert_snapshot!(stdout, @r###" - @ qpvuntsm test.user@example.com 2001-02-03 04:05:07.000 +07:00 230dd059 - │ (empty) (no description set) - ~ - "###); - insta::assert_snapshot!(stderr, @r###" - The `:` revset operator is deprecated. Please switch to `::`. - "###); -} - #[test] fn test_log_with_or_without_diff() { let test_env = TestEnvironment::default(); diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 2e0322e58..f87a240d0 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -20,6 +20,17 @@ fn test_syntax_error() { test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); let repo_path = test_env.env_root().join("repo"); + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", ":x"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: --> 1:1 + | + 1 | :x + | ^ + | + = ':' is not a prefix operator + Hint: Did you mean '::' for ancestors? + "###); + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "x &"]); insta::assert_snapshot!(stderr, @r###" Error: Failed to parse revset: --> 1:4 diff --git a/docs/revsets.md b/docs/revsets.md index db8e0dd54..9578ba20a 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -59,8 +59,6 @@ only symbols. * `x::y`: Descendants of `x` that are also ancestors of `y`. Equivalent to `x:: & ::y`. This is what `git log` calls `--ancestry-path x..y`. * `::`: All visible commits in the repo. Equivalent to `all()`. -* `:x`, `x:`, and `x:y`: Deprecated versions of `::x`, `x::`, and `x::y` We - plan to delete them in jj 0.15+. * `x..y`: Ancestors of `y` that are not also ancestors of `x`. Equivalent to `::y ~ ::x`. This is what `git log` calls `x..y` (i.e. the same as we call it). * `..x`: Ancestors of `x`, including the commits in `x` itself, but excluding diff --git a/lib/src/revset.pest b/lib/src/revset.pest index 7703a6197..abbc82d3d 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -34,17 +34,16 @@ dag_range_op = { "::" } dag_range_pre_op = { "::" } dag_range_post_op = { "::" } dag_range_all_op = { "::" } -// TODO: Drop support for these in 0.15+ -legacy_dag_range_op = { ":" } -legacy_dag_range_pre_op = { ":" } -legacy_dag_range_post_op = { ":" } +compat_dag_range_op = { ":" } +compat_dag_range_pre_op = { ":" } +compat_dag_range_post_op = { ":" } range_op = { ".." } range_pre_op = { ".." } range_post_op = { ".." } range_all_op = { ".." } -range_ops = _{ dag_range_op | legacy_dag_range_op | range_op } -range_pre_ops = _{ dag_range_pre_op | legacy_dag_range_pre_op | range_pre_op } -range_post_ops = _{ dag_range_post_op | legacy_dag_range_post_op | range_post_op } +range_ops = _{ dag_range_op | compat_dag_range_op | range_op } +range_pre_ops = _{ dag_range_pre_op | compat_dag_range_pre_op | range_pre_op } +range_post_ops = _{ dag_range_post_op | compat_dag_range_post_op | range_post_op } range_all_ops = _{ dag_range_all_op | range_all_op } negate_op = { "~" } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 61655b024..2f14a0e31 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -82,17 +82,12 @@ impl Rule { fn is_compat(&self) -> bool { matches!( self, - Rule::compat_parents_op | Rule::compat_add_op | Rule::compat_sub_op - ) - } - - /// Whether this is a deprecated rule to be removed in later version. - fn is_legacy(&self) -> bool { - matches!( - self, - Rule::legacy_dag_range_op - | Rule::legacy_dag_range_pre_op - | Rule::legacy_dag_range_post_op + Rule::compat_parents_op + | Rule::compat_dag_range_op + | Rule::compat_dag_range_pre_op + | Rule::compat_dag_range_post_op + | Rule::compat_add_op + | Rule::compat_sub_op ) } @@ -112,9 +107,9 @@ impl Rule { | Rule::dag_range_pre_op | Rule::dag_range_post_op | Rule::dag_range_all_op => Some("::"), - Rule::legacy_dag_range_op - | Rule::legacy_dag_range_pre_op - | Rule::legacy_dag_range_post_op => Some(":"), + Rule::compat_dag_range_op + | Rule::compat_dag_range_pre_op + | Rule::compat_dag_range_post_op => Some(":"), Rule::range_op => Some(".."), Rule::range_pre_op | Rule::range_post_op | Rule::range_all_op => Some(".."), Rule::range_ops => None, @@ -155,6 +150,12 @@ pub struct RevsetParseError { pub enum RevsetParseErrorKind { #[error("Syntax error")] SyntaxError, + #[error("'{op}' is not a prefix operator")] + NotPrefixOperator { + op: String, + similar_op: String, + description: String, + }, #[error("'{op}' is not a postfix operator")] NotPostfixOperator { op: String, @@ -282,13 +283,11 @@ fn rename_rules_in_pest_error(mut err: pest::error::Error) -> pest::error: return err; }; - // Remove duplicated symbols. Legacy or compat symbols are also removed from the + // Remove duplicated symbols. Compat symbols are also removed from the // (positive) suggestion. let mut known_syms = HashSet::new(); positives.retain(|rule| { - !rule.is_compat() - && !rule.is_legacy() - && rule.to_symbol().map_or(true, |sym| known_syms.insert(sym)) + !rule.is_compat() && rule.to_symbol().map_or(true, |sym| known_syms.insert(sym)) }); let mut known_syms = HashSet::new(); negatives.retain(|rule| rule.to_symbol().map_or(true, |sym| known_syms.insert(sym))); @@ -489,13 +488,6 @@ impl RevsetExpression { pub fn ancestors(self: &Rc) -> Rc { self.ancestors_range(GENERATION_RANGE_FULL) } - fn legacy_ancestors(self: &Rc) -> Rc { - Rc::new(RevsetExpression::Ancestors { - heads: self.clone(), - generation: GENERATION_RANGE_FULL, - is_legacy: true, - }) - } /// Ancestors of `self`, including `self` until `generation` back. pub fn ancestors_at(self: &Rc, generation: u64) -> Rc { @@ -531,13 +523,6 @@ impl RevsetExpression { is_legacy: false, }) } - fn legacy_descendants(self: &Rc) -> Rc { - Rc::new(RevsetExpression::Descendants { - roots: self.clone(), - generation: GENERATION_RANGE_FULL, - is_legacy: true, - }) - } /// Descendants of `self`, including `self` until `generation` ahead. pub fn descendants_at(self: &Rc, generation: u64) -> Rc { @@ -834,6 +819,8 @@ struct ParseState<'a> { locals: &'a HashMap<&'a str, Rc>, user_email: &'a str, workspace_ctx: &'a Option>, + /// Whether or not `kind:"pattern"` syntax is allowed. + allow_string_pattern: bool, } impl ParseState<'_> { @@ -859,6 +846,7 @@ impl ParseState<'_> { locals, user_email: self.user_email, workspace_ctx: self.workspace_ctx, + allow_string_pattern: self.allow_string_pattern, }; f(expanding_state).map_err(|e| { RevsetParseError::with_span_and_origin( @@ -883,6 +871,21 @@ fn parse_expression_rule( pairs: Pairs, state: ParseState, ) -> Result, RevsetParseError> { + fn not_prefix_op( + op: &Pair, + similar_op: impl Into, + description: impl Into, + ) -> RevsetParseError { + RevsetParseError::with_span( + RevsetParseErrorKind::NotPrefixOperator { + op: op.as_str().to_owned(), + similar_op: similar_op.into(), + description: description.into(), + }, + op.as_span(), + ) + } + fn not_postfix_op( op: &Pair, similar_op: impl Into, @@ -923,13 +926,13 @@ fn parse_expression_rule( .op(Op::prefix(Rule::negate_op)) // Ranges can't be nested without parentheses. Associativity doesn't matter. .op(Op::infix(Rule::dag_range_op, Assoc::Left) - | Op::infix(Rule::legacy_dag_range_op, Assoc::Left) + | Op::infix(Rule::compat_dag_range_op, Assoc::Left) | Op::infix(Rule::range_op, Assoc::Left)) .op(Op::prefix(Rule::dag_range_pre_op) - | Op::prefix(Rule::legacy_dag_range_pre_op) + | Op::prefix(Rule::compat_dag_range_pre_op) | Op::prefix(Rule::range_pre_op)) .op(Op::postfix(Rule::dag_range_post_op) - | Op::postfix(Rule::legacy_dag_range_post_op) + | Op::postfix(Rule::compat_dag_range_post_op) | Op::postfix(Rule::range_post_op)) // Neighbors .op(Op::postfix(Rule::parents_op) @@ -948,13 +951,13 @@ fn parse_expression_rule( .map_prefix(|op, rhs| match op.as_rule() { Rule::negate_op => Ok(rhs?.negated()), Rule::dag_range_pre_op => Ok(rhs?.ancestors()), - Rule::legacy_dag_range_pre_op => Ok(rhs?.legacy_ancestors()), + Rule::compat_dag_range_pre_op => Err(not_prefix_op(&op, "::", "ancestors")), Rule::range_pre_op => Ok(RevsetExpression::root().range(&rhs?)), r => panic!("unexpected prefix operator rule {r:?}"), }) .map_postfix(|lhs, op| match op.as_rule() { Rule::dag_range_post_op => Ok(lhs?.descendants()), - Rule::legacy_dag_range_post_op => Ok(lhs?.legacy_descendants()), + Rule::compat_dag_range_post_op => Err(not_postfix_op(&op, "::", "descendants")), Rule::range_post_op => Ok(lhs?.range(&RevsetExpression::visible_heads())), Rule::parents_op => Ok(lhs?.parents()), Rule::children_op => Ok(lhs?.children()), @@ -968,7 +971,13 @@ fn parse_expression_rule( Rule::difference_op => Ok(lhs?.minus(&rhs?)), Rule::compat_sub_op => Err(not_infix_op(&op, "~", "difference")), Rule::dag_range_op => Ok(lhs?.dag_range_to(&rhs?)), - Rule::legacy_dag_range_op => Ok(lhs?.legacy_dag_range_to(&rhs?)), + Rule::compat_dag_range_op => { + if state.allow_string_pattern { + Ok(lhs?.legacy_dag_range_to(&rhs?)) + } else { + Err(not_infix_op(&op, "::", "DAG range")) + } + } Rule::range_op => Ok(lhs?.range(&rhs?)), r => panic!("unexpected infix operator rule {r:?}"), }) @@ -1471,7 +1480,11 @@ fn parse_function_argument_to_string_pattern( ) }; let make_type_error = || make_error("Expected function argument of string pattern".to_owned()); - let expression = parse_expression_rule(pair.into_inner(), state)?; + let expression = { + let mut inner_state = state; + inner_state.allow_string_pattern = true; + parse_expression_rule(pair.into_inner(), inner_state)? + }; let pattern = match expression.as_ref() { RevsetExpression::CommitRef(RevsetCommitRef::Symbol(symbol)) => { let needle = symbol.to_owned(); @@ -1515,7 +1528,12 @@ fn parse_function_argument_as_literal( span, ) }; - let expression = parse_expression_rule(pair.into_inner(), state)?; + let expression = { + // Don't suggest :: operator for :, which is invalid in this context. + let mut inner_state = state; + inner_state.allow_string_pattern = true; + parse_expression_rule(pair.into_inner(), inner_state)? + }; match expression.as_ref() { RevsetExpression::CommitRef(RevsetCommitRef::Symbol(symbol)) => { symbol.parse().map_err(|_| make_error()) @@ -1534,6 +1552,7 @@ pub fn parse( locals: &HashMap::new(), user_email: &context.user_email, workspace_ctx: &context.workspace, + allow_string_pattern: false, }; parse_program(revset_str, state) } @@ -2960,6 +2979,14 @@ mod tests { #[test] fn test_parse_revset_compat_operator() { + assert_eq!( + parse(":foo"), + Err(RevsetParseErrorKind::NotPrefixOperator { + op: ":".to_owned(), + similar_op: "::".to_owned(), + description: "ancestors".to_owned(), + }) + ); assert_eq!( parse("foo^"), Err(RevsetParseErrorKind::NotPostfixOperator {