From 59ca16705e733ec961d3e273bad7f117d19feb12 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 17 Feb 2025 15:55:00 +0900
Subject: [PATCH] revset: add parsing function that rejects expression other
 than symbol name

---
 lib/src/revset.pest      |  2 ++
 lib/src/revset.rs        |  1 +
 lib/src/revset_parser.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/lib/src/revset.pest b/lib/src/revset.pest
index 86ba50a8f..26c7a2f49 100644
--- a/lib/src/revset.pest
+++ b/lib/src/revset.pest
@@ -122,6 +122,8 @@ program = _{
   SOI ~ whitespace* ~ (program_modifier ~ whitespace*)? ~ expression ~ whitespace* ~ EOI
 }
 
+symbol_name = _{ SOI ~ symbol ~ EOI }
+
 function_alias_declaration = {
   function_name ~ "(" ~ whitespace* ~ formal_parameters ~ whitespace* ~ ")"
 }
diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 8de019a36..ead231479 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -52,6 +52,7 @@ use crate::repo::RepoLoaderError;
 use crate::repo_path::RepoPathUiConverter;
 use crate::revset_parser;
 pub use crate::revset_parser::expect_literal;
+pub use crate::revset_parser::parse_symbol;
 pub use crate::revset_parser::BinaryOp;
 pub use crate::revset_parser::ExpressionKind;
 pub use crate::revset_parser::ExpressionNode;
diff --git a/lib/src/revset_parser.rs b/lib/src/revset_parser.rs
index 612151cc9..9809bce42 100644
--- a/lib/src/revset_parser.rs
+++ b/lib/src/revset_parser.rs
@@ -130,6 +130,7 @@ impl Rule {
             Rule::expression => None,
             Rule::program_modifier => None,
             Rule::program => None,
+            Rule::symbol_name => None,
             Rule::function_alias_declaration => None,
             Rule::alias_declaration => None,
         }
@@ -689,6 +690,22 @@ pub fn is_identifier(text: &str) -> bool {
     }
 }
 
+/// Parses the text as a revset symbol, rejects empty string.
+pub fn parse_symbol(text: &str) -> Result<String, RevsetParseError> {
+    let mut pairs = RevsetParser::parse(Rule::symbol_name, text)?;
+    let first = pairs.next().unwrap();
+    let span = first.as_span();
+    let name = parse_as_string_literal(first);
+    if name.is_empty() {
+        Err(RevsetParseError::expression(
+            "Expected non-empty string",
+            span,
+        ))
+    } else {
+        Ok(name)
+    }
+}
+
 pub type RevsetAliasesMap = AliasesMap<RevsetAliasParser, String>;
 
 #[derive(Clone, Debug, Default)]
@@ -1287,6 +1304,29 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_parse_symbol_explicitly() {
+        assert_matches!(parse_symbol("").as_deref(), Err(_));
+        // empty string could be a valid ref name, but it would be super
+        // confusing if identifier was empty.
+        assert_matches!(parse_symbol("''").as_deref(), Err(_));
+
+        assert_matches!(parse_symbol("foo.bar").as_deref(), Ok("foo.bar"));
+        assert_matches!(parse_symbol("foo@bar").as_deref(), Err(_));
+        assert_matches!(parse_symbol("foo bar").as_deref(), Err(_));
+
+        assert_matches!(parse_symbol("'foo bar'").as_deref(), Ok("foo bar"));
+        assert_matches!(parse_symbol(r#""foo\tbar""#).as_deref(), Ok("foo\tbar"));
+
+        // leading/trailing whitespace is NOT ignored.
+        assert_matches!(parse_symbol(" foo").as_deref(), Err(_));
+        assert_matches!(parse_symbol("foo ").as_deref(), Err(_));
+
+        // (foo) could be parsed as a symbol "foo", but is rejected because user
+        // might expect a literal "(foo)".
+        assert_matches!(parse_symbol("(foo)").as_deref(), Err(_));
+    }
+
     #[test]
     fn parse_at_workspace_and_remote_symbol() {
         // Parse "@" (the current working copy)