From d8c209c82a57d9e69bdeccceb92c3dcb6a609689 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 22 Apr 2021 22:45:58 -0700 Subject: [PATCH] revsets: give parents/children operators higher precedence than range operators --- lib/src/revset.pest | 20 +++++---- lib/src/revset.rs | 88 ++++++++++++++++++++++++---------------- lib/tests/test_revset.rs | 2 +- 3 files changed, 68 insertions(+), 42 deletions(-) diff --git a/lib/src/revset.pest b/lib/src/revset.pest index 601fe8dc3..e127cf1e0 100644 --- a/lib/src/revset.pest +++ b/lib/src/revset.pest @@ -20,13 +20,13 @@ symbol = { literal_string = { "\"" ~ (!"\"" ~ ANY)+ ~ "\"" } whitespace = _{ " " } +// We could use the same rule name for the shared operators if we can +// think of a good name. parents_op = { ":" } -ancestors_op = { ",," } -prefix_op = _{ parents_op | ancestors_op } - children_op = { ":" } + +ancestors_op = { ",," } descendants_op = { ",," } -postfix_op = _{ children_op | descendants_op } union_op = { "|" } intersection_op = { "&" } @@ -45,12 +45,18 @@ primary = { | symbol } -prefix_expression = { prefix_op* ~ primary } +parents_expression = { parents_op* ~ primary } -postfix_expression = { prefix_expression ~ postfix_op* } +children_expression = { parents_expression ~ children_op* } + +range_expression = { + ancestors_op ~ children_expression + | children_expression ~ descendants_op + | children_expression +} infix_expression = { - whitespace* ~ postfix_expression ~ whitespace* ~ (infix_op ~ whitespace* ~ postfix_expression ~ whitespace*)* + whitespace* ~ range_expression ~ whitespace* ~ (infix_op ~ whitespace* ~ range_expression ~ whitespace*)* } expression = { diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 70665f33a..fd2269ad9 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -153,9 +153,9 @@ fn parse_expression_rule(mut pairs: Pairs) -> Result, ) -> Result { - let mut expression1 = parse_postfix_expression_rule(pairs.next().unwrap().into_inner())?; + let mut expression1 = parse_range_expression_rule(pairs.next().unwrap().into_inner())?; while let Some(operator) = pairs.next() { - let expression2 = parse_postfix_expression_rule(pairs.next().unwrap().into_inner())?; + let expression2 = parse_range_expression_rule(pairs.next().unwrap().into_inner())?; match operator.as_rule() { Rule::union_op => { expression1 = RevsetExpression::Union(Rc::new(expression1), Rc::new(expression2)) @@ -179,10 +179,44 @@ fn parse_infix_expression_rule( Ok(expression1) } -fn parse_postfix_expression_rule( +fn parse_range_expression_rule( mut pairs: Pairs, ) -> Result { - let mut expression = parse_prefix_expression_rule(pairs.next().unwrap().into_inner())?; + let first = pairs.next().unwrap(); + match first.as_rule() { + Rule::ancestors_op => { + return Ok(RevsetExpression::Ancestors(Rc::new( + parse_children_expression_rule(pairs.next().unwrap().into_inner())?, + ))); + } + Rule::children_expression => { + // Fall through + } + _ => { + panic!("unxpected revset range operator rule {:?}", first.as_rule()); + } + } + let mut expression = parse_children_expression_rule(first.into_inner())?; + if let Some(next) = pairs.next() { + match next.as_rule() { + Rule::descendants_op => { + expression = RevsetExpression::DagRange { + roots: Rc::new(expression), + heads: RevsetExpression::non_obsolete_heads(), + }; + } + _ => { + panic!("unxpected revset range operator rule {:?}", next.as_rule()); + } + } + } + Ok(expression) +} + +fn parse_children_expression_rule( + mut pairs: Pairs, +) -> Result { + let mut expression = parse_parents_expression_rule(pairs.next().unwrap().into_inner())?; for operator in pairs { match operator.as_rule() { Rule::children_op => { @@ -191,15 +225,9 @@ fn parse_postfix_expression_rule( heads: RevsetExpression::non_obsolete_heads(), }; } - Rule::descendants_op => { - expression = RevsetExpression::DagRange { - roots: Rc::new(expression), - heads: RevsetExpression::non_obsolete_heads(), - }; - } _ => { panic!( - "unxpected revset postfix operator rule {:?}", + "unxpected revset children operator rule {:?}", operator.as_rule() ); } @@ -208,21 +236,18 @@ fn parse_postfix_expression_rule( Ok(expression) } -fn parse_prefix_expression_rule( +fn parse_parents_expression_rule( mut pairs: Pairs, ) -> Result { let first = pairs.next().unwrap(); match first.as_rule() { Rule::primary => parse_primary_rule(first.into_inner()), Rule::parents_op => Ok(RevsetExpression::Parents(Rc::new( - parse_prefix_expression_rule(pairs)?, - ))), - Rule::ancestors_op => Ok(RevsetExpression::Ancestors(Rc::new( - parse_prefix_expression_rule(pairs)?, + parse_parents_expression_rule(pairs)?, ))), _ => { panic!( - "unxpected revset prefix operator rule {:?}", + "unxpected revset parents operator rule {:?}", first.as_rule() ); } @@ -934,8 +959,12 @@ mod tests { heads: RevsetExpression::non_obsolete_heads() }) ); - // Parse combinations of prefix and postfix operators. They all currently have - // the same precedence, so ",,foo:" means "(,,foo):" and not ",,(foo:)". + // Parse repeated "ancestors"/"descendants" operators + assert_matches!(parse(",,foo,,"), Err(RevsetParseError::SyntaxError(_))); + assert_matches!(parse(",,,,foo"), Err(RevsetParseError::SyntaxError(_))); + assert_matches!(parse("foo,,,,"), Err(RevsetParseError::SyntaxError(_))); + // Parse combinations of "parents"/"children" operators and the range operators. + // The former bind more strongly. assert_eq!( parse(":foo:"), Ok(RevsetExpression::Children { @@ -945,15 +974,6 @@ mod tests { heads: RevsetExpression::non_obsolete_heads(), }) ); - assert_eq!( - parse(",,foo,,"), - Ok(RevsetExpression::DagRange { - roots: Rc::new(RevsetExpression::Ancestors(Rc::new( - RevsetExpression::Symbol("foo".to_string()) - ))), - heads: RevsetExpression::non_obsolete_heads(), - }) - ); assert_eq!( parse(":foo,,"), Ok(RevsetExpression::DagRange { @@ -965,12 +985,12 @@ mod tests { ); assert_eq!( parse(",,foo:"), - Ok(RevsetExpression::Children { - roots: Rc::new(RevsetExpression::Ancestors(Rc::new( - RevsetExpression::Symbol("foo".to_string()) - ))), - heads: RevsetExpression::non_obsolete_heads(), - }) + Ok(RevsetExpression::Ancestors(Rc::new( + RevsetExpression::Children { + roots: Rc::new(RevsetExpression::Symbol("foo".to_string())), + heads: RevsetExpression::non_obsolete_heads() + } + ),)) ); } diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index cacb3a0d1..620a24e2d 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -312,7 +312,7 @@ fn test_evaluate_expression_parents(use_git: bool) { assert_eq!( resolve_commit_ids( mut_repo.as_repo_ref(), - &format!(":,,{}", commit2.id().hex()) + &format!(":({} | {})", commit1.id().hex(), commit2.id().hex()) ), vec![commit1.id().clone(), root_commit.id().clone()] );