From 2ffa6c11efd9dfa30221fc853d39459ea0430529 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 25 Jan 2025 22:38:43 -0800 Subject: [PATCH 1/3] cli: tests: fix a bad copy&paste --- cli/tests/test_git_clone.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index 94a0ac2b44..030a4cf39b 100644 --- a/cli/tests/test_git_clone.rs +++ b/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" From 2632ea1688b51ac43cb60ca49be35fc743f370d9 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 25 Jan 2025 22:38:43 -0800 Subject: [PATCH 2/3] revset: add a function for escaping a symbol When we have e.g. a bookmark name or a remote name and we want to produce a revset from it, we may need to quote and escape it. This patch implements a function for that. Perhaps we'll want to more generally be able to format a whole `RevsetExpression` later. --- lib/src/dsl_util.rs | 23 +++++++++++++++++++++++ lib/src/revset.rs | 36 ++++++++++++++++++++++++++++++++++++ lib/src/revset_parser.rs | 8 ++++++++ 3 files changed, 67 insertions(+) diff --git a/lib/src/dsl_util.rs b/lib/src/dsl_util.rs index 6ea2165023..9e1f6f0601 100644 --- a/lib/src/dsl_util.rs +++ b/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 StringLiteralParser { } } +/// 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 { diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 543e12d1ee..84179f0347 100644 --- a/lib/src/revset.rs +++ b/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""#); + } } diff --git a/lib/src/revset_parser.rs b/lib/src/revset_parser.rs index 4d9e252c7c..59202f7f0d 100644 --- a/lib/src/revset_parser.rs +++ b/lib/src/revset_parser.rs @@ -681,6 +681,14 @@ fn parse_as_string_literal(pair: Pair) -> 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; #[derive(Clone, Debug, Default)] From ac8ec0b8820d7d920d5cdc8a7c3a493a6afcaf20 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 25 Jan 2025 22:38:43 -0800 Subject: [PATCH 3/3] cli: git: escape bookmark and remote names in `trunk()` config Closes #5359. --- CHANGELOG.md | 4 ++++ cli/src/commands/git/mod.rs | 3 +++ cli/tests/test_git_clone.rs | 43 +++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 857eeaf711..79d1034f3a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cli/src/commands/git/mod.rs b/cli/src/commands/git/mod.rs index 3772e3ebbc..844e363417 100644 --- a/cli/src/commands/git/mod.rs +++ b/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()?; diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index 030a4cf39b..da191cea8e 100644 --- a/cli/tests/test_git_clone.rs +++ b/cli/tests/test_git_clone.rs @@ -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) {