escape bookmark and remote names in trunk() config

CHANGELOG.md

@@ -163,6 +163,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   now materialized in a way that can be parsed correctly.
   [#3968](https://github.com/jj-vcs/jj/issues/3968)
 
+* Bookmark and remote names written by `jj git clone` to
+  `revset-aliases.'trunk()'` are now escaped if necessary.
+  [#5359](https://github.com/jj-vcs/jj/issues/5359)
+
 ## [0.25.0] - 2025-01-01
 
 ### Release highlights

cli/src/commands/git/mod.rs

@@ -28,6 +28,7 @@ use jj_lib::config::ConfigFile;
 use jj_lib::config::ConfigSource;
 use jj_lib::git;
 use jj_lib::git::UnexpectedGitBackendError;
+use jj_lib::revset;
 use jj_lib::store::Store;
 
 use self::clone::cmd_git_clone;
@@ -123,6 +124,8 @@ fn write_repository_level_trunk_alias(
     branch: &str,
 ) -> Result<(), CommandError> {
     let mut file = ConfigFile::load_or_empty(ConfigSource::Repo, repo_path.join("config.toml"))?;
+    let branch = revset::format_symbol(branch);
+    let remote = revset::format_symbol(remote);
     file.set_value(["revset-aliases", "trunk()"], format!("{branch}@{remote}"))
         .expect("initial repo config shouldn't have invalid values");
     file.save()?;

cli/tests/test_git_clone.rs

@@ -582,11 +582,11 @@ fn test_git_clone_remote_default_bookmark(subprocess: bool) {
     }
     insta::allow_duplicates! {
     insta::assert_snapshot!(
-        get_bookmark_output(&test_env, &test_env.env_root().join("clone2")), @r###"
-    feature1@origin: mzyxwzks 9f01a0e0 message
-    main: mzyxwzks 9f01a0e0 message
+        get_bookmark_output(&test_env, &test_env.env_root().join("clone3")), @r"
+    feature1: mzyxwzks 9f01a0e0 message
       @origin: mzyxwzks 9f01a0e0 message
-    "###);
+    main@origin: mzyxwzks 9f01a0e0 message
+    ");
     }
 
     // "trunk()" alias should be set to new default bookmark "feature1"
@@ -601,6 +601,49 @@ fn test_git_clone_remote_default_bookmark(subprocess: bool) {
     }
 }
 
+// A branch with a strange name should get quoted in the config. Windows doesn't
+// like the strange name, so we don't run the test there.
+#[cfg(unix)]
+#[test_case(false; "use git2 for remote calls")]
+#[test_case(true; "spawn a git subprocess for remote calls")]
+fn test_git_clone_remote_default_bookmark_with_escape(subprocess: bool) {
+    let test_env = TestEnvironment::default();
+    if subprocess {
+        test_env.add_config("git.subprocess = true");
+    }
+    let git_repo_path = test_env.env_root().join("source");
+    let git_repo = git2::Repository::init(git_repo_path).unwrap();
+    set_up_non_empty_git_repo(&git_repo);
+    // Rename the main branch to something that needs to be escaped
+    git_repo
+        .find_reference("refs/heads/main")
+        .unwrap()
+        .rename("refs/heads/\"", false, "")
+        .unwrap();
+
+    let (_stdout, stderr) =
+        test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "source", "clone"]);
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(stderr, @r#"
+    Fetching into new repo in "$TEST_ENV/clone"
+    bookmark: "@origin [new] untracked
+    Setting the revset alias "trunk()" to ""\""@origin"
+    Working copy now at: sqpuoqvx cad212e1 (empty) (no description set)
+    Parent commit      : mzyxwzks 9f01a0e0 " | message
+    Added 1 files, modified 0 files, removed 0 files
+    "#);
+    }
+
+    // "trunk()" alias should be escaped and quoted
+    let stdout = test_env.jj_cmd_success(
+        &test_env.env_root().join("clone"),
+        &["config", "list", "--repo", "revset-aliases.'trunk()'"],
+    );
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(stdout, @r#"revset-aliases.'trunk()' = '"\""@origin'"#);
+    }
+}
+
 #[test_case(false; "use git2 for remote calls")]
 #[test_case(true; "spawn a git subprocess for remote calls")]
 fn test_git_clone_ignore_working_copy(subprocess: bool) {

lib/src/dsl_util.rs

@@ -15,6 +15,7 @@
 //! Domain-specific language helpers.
 
 use std::array;
+use std::ascii;
 use std::collections::HashMap;
 use std::fmt;
 use std::slice;
@@ -429,6 +430,28 @@ impl<R: RuleType> StringLiteralParser<R> {
     }
 }
 
+/// Escape special characters in the input
+pub fn escape_string(unescaped: &str) -> String {
+    let mut escaped = String::with_capacity(unescaped.len());
+    for c in unescaped.chars() {
+        match c {
+            '"' => escaped.push_str(r#"\""#),
+            '\\' => escaped.push_str(r#"\\"#),
+            '\t' => escaped.push_str(r#"\t"#),
+            '\r' => escaped.push_str(r#"\r"#),
+            '\n' => escaped.push_str(r#"\n"#),
+            '\0' => escaped.push_str(r#"\0"#),
+            c if c.is_ascii_control() => {
+                for b in ascii::escape_default(c as u8) {
+                    escaped.push(b as char);
+                }
+            }
+            c => escaped.push(c),
+        }
+    }
+    escaped
+}
+
 /// Helper to parse function call.
 #[derive(Debug)]
 pub struct FunctionCallParser<R> {

lib/src/revset.rs

@@ -2686,6 +2686,15 @@ pub struct RevsetWorkspaceContext<'a> {
     pub workspace_id: &'a WorkspaceId,
 }
 
+/// Formats a string as symbol by quoting and escaping it if necessary
+pub fn format_symbol(literal: &str) -> String {
+    if revset_parser::is_identifier(literal) {
+        literal.to_string()
+    } else {
+        format!(r#""{}""#, dsl_util::escape_string(literal))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use std::path::PathBuf;
@@ -4327,4 +4336,31 @@ mod tests {
         }
         "###);
     }
+
+    #[test]
+    fn test_escape_string_literal() {
+        // Valid identifiers don't need quoting
+        assert_eq!(format_symbol("foo"), "foo");
+        assert_eq!(format_symbol("foo.bar"), "foo.bar");
+
+        // Invalid identifiers need quoting
+        assert_eq!(format_symbol("foo@bar"), r#""foo@bar""#);
+        assert_eq!(format_symbol("foo bar"), r#""foo bar""#);
+        assert_eq!(format_symbol(" foo "), r#"" foo ""#);
+        assert_eq!(format_symbol("(foo)"), r#""(foo)""#);
+        assert_eq!(format_symbol("all:foo"), r#""all:foo""#);
+
+        // Some characters also need escaping
+        assert_eq!(format_symbol("foo\"bar"), r#""foo\"bar""#);
+        assert_eq!(format_symbol("foo\\bar"), r#""foo\\bar""#);
+        assert_eq!(format_symbol("foo\\\"bar"), r#""foo\\\"bar""#);
+        assert_eq!(format_symbol("foo\nbar"), r#""foo\nbar""#);
+
+        // Some characters don't technically need escaping, but we escape them for
+        // clarity
+        assert_eq!(format_symbol("foo\"bar"), r#""foo\"bar""#);
+        assert_eq!(format_symbol("foo\\bar"), r#""foo\\bar""#);
+        assert_eq!(format_symbol("foo\\\"bar"), r#""foo\\\"bar""#);
+        assert_eq!(format_symbol("foo \x01 bar"), r#""foo \x01 bar""#);
+    }
 }

lib/src/revset_parser.rs

@@ -681,6 +681,14 @@ fn parse_as_string_literal(pair: Pair<Rule>) -> String {
     }
 }
 
+/// Checks if the text is a valid identifier
+pub fn is_identifier(text: &str) -> bool {
+    match RevsetParser::parse(Rule::identifier, text) {
+        Ok(mut pairs) => pairs.next().unwrap().as_span().end() == text.len(),
+        Err(_) => false,
+    }
+}
+
 pub type RevsetAliasesMap = AliasesMap<RevsetAliasParser, String>;
 
 #[derive(Clone, Debug, Default)]