ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

revset: disable parsing rules of legacy dag range operator

Browse source 
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.
This commit is contained in:
Yuya Nishihara 2024-02-13 14:46:21 +09:00
parent 2905a70b18
commit 815437598f
7 changed files with 92 additions and 97 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

@ -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: `||`, `&&`, `!`

7
cli/src/cli_util.rs
View file

@ -408,7 +408,12 @@ impl From<RevsetParseError> 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,

47
cli/tests/test_log_command.rs
View file

@ -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();

11
cli/tests/test_revset_output.rs
View file

@ -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

2
docs/revsets.md
View file

@ -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

13
lib/src/revset.pest
View file

@ -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 = { "~" }

107
lib/src/revset.rs
View file

@ -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<Rule>) -> 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<RevsetExpression>) -> Rc<RevsetExpression> {
self.ancestors_range(GENERATION_RANGE_FULL)
}
fn legacy_ancestors(self: &Rc<RevsetExpression>) -> Rc<RevsetExpression> {
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<RevsetExpression>, generation: u64) -> Rc<RevsetExpression> {
@ -531,13 +523,6 @@ impl RevsetExpression {
is_legacy: false,
})
}
fn legacy_descendants(self: &Rc<RevsetExpression>) -> Rc<RevsetExpression> {
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<RevsetExpression>, generation: u64) -> Rc<RevsetExpression> {
@ -834,6 +819,8 @@ struct ParseState<'a> {
locals: &'a HashMap<&'a str, Rc<RevsetExpression>>,
user_email: &'a str,
workspace_ctx: &'a Option<RevsetWorkspaceContext<'a>>,
/// 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<Rule>,
state: ParseState,
) -> Result<Rc<RevsetExpression>, RevsetParseError> {
fn not_prefix_op(
op: &Pair<Rule>,
similar_op: impl Into<String>,
description: impl Into<String>,
) -> 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<Rule>,
similar_op: impl Into<String>,
@ -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<T: FromStr>(
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 {