From 75ebdf69a17ed16ff7b230006ae19aa51026fc5f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 23 Aug 2023 15:48:45 +0900 Subject: [PATCH] revset: parse @ like operator (but without alias substitution) This is what I proposed in #2095. @ is now an operator to concatenate symbols. Unlike the other operators, lhs/rhs of @ is not a target of alias substitution. 'x' in 'x@y' doesn't look like a named variable, though it's technically possible to allow definition of an alias expanded to a symbol of specific remote or vice versa. This will probably apply to the kind:pattern syntax, where aliases are expanded due to the current implementation restriction. I've added a TODO comment about that. --- CHANGELOG.md | 10 ++-- lib/src/revset.pest | 8 ++- lib/src/revset.rs | 122 +++++++++++++++++++++++++++++++++----------- 3 files changed, 106 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8861c1a1d..7dc26faf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,10 +42,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Older binaries may not warn user when attempting to `git push` commits with such signatures. -* In revsets, the working-copy symbols (such as `@` and `workspace_id@`) can - no longer be quoted and will not be resolved as a revset aliases or revset - function parameter. For example, `author(foo@)` is now an error, and - a revset alias `'revset-aliases.foo@' = '@'` will be ignored. +* In revsets, the working-copy or remote symbols (such as `@`, `workspace_id@`, + and `branch@remote`) can no longer be quoted as a unit. If a workspace or + branch name contains whitespace, quote the name like `"branch name"@remote`. + Also, these symbols will not be resolved as revset aliases or function + parameters. For example, `author(foo@)` is now an error, and the revset alias + `'revset-aliases.foo@' = '@'` will be failed to parse. * `jj git push` will now push all branches in the range `remote_branches()..@` instead of only branches pointing to `@` or `@-`. diff --git a/lib/src/revset.pest b/lib/src/revset.pest index 58d613a19..ac1be23db 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -identifier_part = @{ (ASCII_ALPHANUMERIC | "_" | "@" | "/")+ } +identifier_part = @{ (ASCII_ALPHANUMERIC | "_" | "/")+ } identifier = @{ identifier_part ~ ("." | "-" | "+" ) ~ identifier | identifier_part @@ -24,6 +24,8 @@ symbol = { literal_string = { "\"" ~ (!"\"" ~ ANY)* ~ "\"" } whitespace = _{ " " | "\t" | "\r" | "\n" | "\x0c" } +at_op = { "@" } + parents_op = { "-" } children_op = { "+" } compat_parents_op = { "^" } @@ -65,7 +67,11 @@ formal_parameters = { primary = { function_name ~ "(" ~ whitespace* ~ function_arguments ~ whitespace* ~ ")" | "(" ~ whitespace* ~ expression ~ whitespace* ~ ")" + // "@" operator cannot be nested + | symbol ~ at_op ~ symbol + | symbol ~ at_op | symbol + | at_op } neighbors_expression = _{ primary ~ (parents_op | children_op | compat_parents_op)* } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 2e17b0bf7..b338e547b 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -879,13 +879,36 @@ fn parse_primary_rule( let arguments_pair = pairs.next().unwrap(); parse_function_expression(first, arguments_pair, state, span) } - Rule::symbol => parse_symbol_rule(first.into_inner(), state), + // Symbol without "@" may be substituted by aliases. Primary expression including "@" + // is considered an indecomposable unit, and no alias substitution would be made. + Rule::symbol if pairs.peek().is_none() => parse_symbol_rule(first.into_inner(), state), + Rule::symbol => { + let name = parse_symbol_rule_as_literal(first.into_inner())?; + assert_eq!(pairs.next().unwrap().as_rule(), Rule::at_op); + if let Some(second) = pairs.next() { + // infix "@" + assert_eq!(second.as_rule(), Rule::symbol); + let remote = parse_symbol_rule_as_literal(second.into_inner())?; + Ok(RevsetExpression::symbol(format!("{name}@{remote}"))) // TODO + } else { + // postfix "@" + Ok(RevsetExpression::working_copy(WorkspaceId::new(name))) + } + } + Rule::at_op => { + // unary "@" + let ctx = state.workspace_ctx.as_ref().ok_or_else(|| { + RevsetParseError::new(RevsetParseErrorKind::WorkingCopyWithoutWorkspace) + })?; + Ok(RevsetExpression::working_copy(ctx.workspace_id.clone())) + } _ => { panic!("unexpected revset parse rule: {:?}", first.as_str()); } } } +/// Parses symbol to expression, expands aliases as needed. fn parse_symbol_rule( mut pairs: Pairs, state: ParseState, @@ -894,20 +917,7 @@ fn parse_symbol_rule( match first.as_rule() { Rule::identifier => { let name = first.as_str(); - if name.ends_with('@') { - let workspace_id = if name == "@" { - if let Some(ctx) = state.workspace_ctx { - ctx.workspace_id.clone() - } else { - return Err(RevsetParseError::new( - RevsetParseErrorKind::WorkingCopyWithoutWorkspace, - )); - } - } else { - WorkspaceId::new(name.strip_suffix('@').unwrap().to_string()) - }; - Ok(RevsetExpression::working_copy(workspace_id)) - } else if let Some(expr) = state.locals.get(name) { + if let Some(expr) = state.locals.get(name) { Ok(expr.clone()) } else if let Some((id, defn)) = state.aliases_map.get_symbol(name) { let locals = HashMap::new(); // Don't spill out the current scope @@ -925,6 +935,18 @@ fn parse_symbol_rule( } } +/// Parses part of compound symbol to string without alias substitution. +fn parse_symbol_rule_as_literal(mut pairs: Pairs) -> Result { + let first = pairs.next().unwrap(); + match first.as_rule() { + Rule::identifier => Ok(first.as_str().to_owned()), + Rule::literal_string => parse_string_literal(first), + _ => { + panic!("unexpected symbol parse rule: {:?}", first.as_str()); + } + } +} + // TODO: Add support for \-escape syntax fn parse_string_literal(pair: Pair) -> Result { assert_eq!(pair.as_rule(), Rule::literal_string); @@ -2634,11 +2656,38 @@ mod tests { parse_with_workspace("main@", &other_workspace_id), Ok(main_wc) ); - // Quoted "@" is not interpreted as a working copy + assert_eq!( + parse("main@origin"), + Ok(RevsetExpression::symbol("main@origin".to_string())) + ); + // Quoted component in @ expression + assert_eq!( + parse(r#""foo bar"@"#), + Ok(RevsetExpression::working_copy(WorkspaceId::new( + "foo bar".to_string() + ))) + ); + assert_eq!( + parse(r#""foo bar"@origin"#), + Ok(RevsetExpression::symbol("foo bar@origin".to_string())) + ); + assert_eq!( + parse(r#"main@"foo bar""#), + Ok(RevsetExpression::symbol("main@foo bar".to_string())) + ); + // Quoted "@" is not interpreted as a working copy or remote symbol assert_eq!( parse(r#""@""#), Ok(RevsetExpression::symbol("@".to_string())) ); + assert_eq!( + parse(r#""main@""#), + Ok(RevsetExpression::symbol("main@".to_string())) + ); + assert_eq!( + parse(r#""main@origin""#), + Ok(RevsetExpression::symbol("main@origin".to_string())) + ); // "@" in function argument must be quoted assert_eq!( parse("author(foo@)"), @@ -2810,9 +2859,22 @@ mod tests { ); } + #[test] + fn test_parse_revset_alias_symbol_decl() { + let mut aliases_map = RevsetAliasesMap::new(); + // Working copy or remote symbol cannot be used as an alias name. + assert!(aliases_map.insert("@", "none()").is_err()); + assert!(aliases_map.insert("a@", "none()").is_err()); + assert!(aliases_map.insert("a@b", "none()").is_err()); + } + #[test] fn test_parse_revset_alias_formal_parameter() { let mut aliases_map = RevsetAliasesMap::new(); + // Working copy or remote symbol cannot be used as an parameter name. + assert!(aliases_map.insert("f(@)", "none()").is_err()); + assert!(aliases_map.insert("f(a@)", "none()").is_err()); + assert!(aliases_map.insert("f(a@b)", "none()").is_err()); // Trailing comma isn't allowed for empty parameter assert!(aliases_map.insert("f(,)", "none()").is_err()); // Trailing comma is allowed for the last parameter @@ -3044,12 +3106,6 @@ mod tests { parse_with_aliases(r#"A|"A""#, [("A", "a")]).unwrap(), parse("a|A").unwrap() ); - // Working copy symbol should not be substituted with alias. - // TODO: Make it an error instead of being ignored - assert_eq!( - parse_with_aliases(r#"a@"#, [("a@", "a")]).unwrap(), - parse("a@").unwrap() - ); // Alias can be substituted to string literal. assert_eq!( @@ -3067,11 +3123,26 @@ mod tests { parse_with_aliases("author(A)", [("A", "exact:a")]).unwrap(), parse("author(exact:a)").unwrap() ); + // TODO: Substitution of part of a string pattern seems confusing. Disable it. assert_eq!( parse_with_aliases("author(exact:A)", [("A", "a")]).unwrap(), parse("author(exact:a)").unwrap() ); + // Part of @ symbol cannot be substituted. + assert_eq!( + parse_with_aliases("A@", [("A", "a")]).unwrap(), + parse("A@").unwrap() + ); + assert_eq!( + parse_with_aliases("A@b", [("A", "a")]).unwrap(), + parse("A@b").unwrap() + ); + assert_eq!( + parse_with_aliases("a@B", [("B", "b")]).unwrap(), + parse("a@B").unwrap() + ); + // Multi-level substitution. assert_eq!( parse_with_aliases("A", [("A", "BC"), ("BC", "b|C"), ("C", "c")]).unwrap(), @@ -3126,13 +3197,6 @@ mod tests { parse("(x|(x|a))&y").unwrap() ); - // Working copy symbol cannot be used as parameter name - // TODO: Make it an error instead of being ignored - assert_eq!( - parse_with_aliases("F(x)", [("F(a@)", "a@|y")]).unwrap(), - parse("a@|y").unwrap() - ); - // Function parameter should precede the symbol alias. assert_eq!( parse_with_aliases("F(a)|X", [("F(X)", "X"), ("X", "x")]).unwrap(),