From c6568787f392964c2a6d58b5cf95c2e3ac443868 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 10 Oct 2024 17:34:07 +0900 Subject: [PATCH] cli: warn if trunk() alias cannot be resolved, fall back to none() This patch replaces all call sites with present(trunk()), and adds an explicit check for unresolvable trunk(). If we add coalesce() expression, maybe it can be rewritten to coalesce(present(trunk()), builtin_trunk()). Fixes #4616 --- CHANGELOG.md | 2 ++ cli/src/cli_util.rs | 1 + cli/src/config-schema.json | 4 ++-- cli/src/config/revsets.toml | 11 +++++++++-- cli/src/revset_util.rs | 32 ++++++++++++++++++++++++++++++++ cli/tests/test_git_clone.rs | 35 +++++++++++++++++++++++++++++++++++ cli/tests/test_operations.rs | 8 -------- docs/config.md | 6 +++--- docs/revsets.md | 16 ++++++++-------- 9 files changed, 92 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b6abbd72..270be3291 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed bugs +* Error on `trunk()` revset resolution is now handled gracefully. + [#4616](https://github.com/martinvonz/jj/issues/4616) ## [0.22.0] - 2024-10-02 diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d93665404..011b65579 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -382,6 +382,7 @@ impl CommandHelper { let op_head = self.resolve_operation(ui, workspace.repo_loader())?; let repo = workspace.repo_loader().load_at(&op_head)?; let env = self.workspace_environment(ui, &workspace)?; + revset_util::warn_unresolvable_trunk(ui, repo.as_ref(), &env.revset_parse_context())?; WorkspaceCommandHelper::new(ui, workspace, repo, env, self.is_at_head_operation()) } diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index b3faa02d7..bcb263143 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -389,7 +389,7 @@ "log": { "type": "string", "description": "Default set of revisions to show when no explicit revset is given for jj log and similar commands", - "default": "present(@) | ancestors(immutable_heads().., 2) | trunk()" + "default": "present(@) | ancestors(immutable_heads().., 2) | present(trunk())" }, "short-prefixes": { "type": "string", @@ -408,7 +408,7 @@ "immutable_heads()": { "type": "string", "description": "Revisions to consider immutable. Ancestors of these are also considered immutable. The root commit is always considered immutable.", - "default": "trunk() | tags() | untracked_remote_branches()" + "default": "present(trunk()) | tags() | untracked_remote_branches()" } }, "additionalProperties": { diff --git a/cli/src/config/revsets.toml b/cli/src/config/revsets.toml index b07b3cdfb..e036670dc 100644 --- a/cli/src/config/revsets.toml +++ b/cli/src/config/revsets.toml @@ -3,9 +3,14 @@ [revsets] fix = "reachable(@, mutable())" -log = "present(@) | ancestors(immutable_heads().., 2) | trunk()" +# log revset is also used as the default short-prefixes. If it failed to +# evaluate, lengthy warning messages would be printed. Use present(expr) to +# suppress symbol resolution error. +log = "present(@) | ancestors(immutable_heads().., 2) | present(trunk())" [revset-aliases] +# trunk() can be overridden as '@'. Use present(trunk()) if +# symbol resolution error should be suppressed. 'trunk()' = ''' latest( remote_bookmarks(exact:"main", exact:"origin") | @@ -18,7 +23,9 @@ latest( ) ''' -'builtin_immutable_heads()' = 'trunk() | tags() | untracked_remote_bookmarks()' +# If immutable_heads() failed to evaluate, many jj commands wouldn't work. Use +# present(expr) to suppress symbol resolution error. +'builtin_immutable_heads()' = 'present(trunk()) | tags() | untracked_remote_bookmarks()' 'immutable_heads()' = 'builtin_immutable_heads()' 'immutable()' = '::(immutable_heads() | root())' 'mutable()' = '~immutable()' diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index cdfe140e8..9391bc4e0 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -14,6 +14,7 @@ //! Utility for parsing and evaluating user-provided revset expressions. +use std::io; use std::rc::Rc; use std::sync::Arc; @@ -213,6 +214,37 @@ pub fn parse_immutable_heads_expression( Ok(heads.union(&RevsetExpression::root())) } +/// Prints warning if `trunk()` alias cannot be resolved. This alias could be +/// generated by `jj git init`/`clone`. +pub(super) fn warn_unresolvable_trunk( + ui: &Ui, + repo: &dyn Repo, + context: &RevsetParseContext, +) -> io::Result<()> { + let (_, _, revset_str) = context + .aliases_map() + .get_function("trunk", 0) + .expect("trunk() should be defined by default"); + let Ok(expression) = revset::parse(&mut RevsetDiagnostics::new(), revset_str, context) else { + // Parse error would have been reported. + return Ok(()); + }; + // Not using IdPrefixContext since trunk() revset shouldn't contain short + // prefixes. + let symbol_resolver = DefaultSymbolResolver::new(repo, context.symbol_resolvers()); + if let Err(err) = expression.resolve_user_expression(repo, &symbol_resolver) { + writeln!( + ui.warning_default(), + "Failed to resolve `revset-aliases.trunk()`: {err}" + )?; + writeln!( + ui.hint_default(), + "Use `jj config edit --repo` to adjust the `trunk()` alias." + )?; + } + Ok(()) +} + pub(super) fn evaluate_revset_to_single_commit<'a>( revision_str: &str, expression: &RevsetExpressionEvaluator<'_>, diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index a9022346b..0ce982e47 100644 --- a/cli/tests/test_git_clone.rs +++ b/cli/tests/test_git_clone.rs @@ -522,6 +522,41 @@ fn test_git_clone_with_remote_name() { "#); } +#[test] +fn test_git_clone_trunk_deleted() { + let test_env = TestEnvironment::default(); + 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); + let clone_path = test_env.env_root().join("clone"); + + let (stdout, stderr) = + test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "source", "clone"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Fetching into new repo in "$TEST_ENV/clone" + bookmark: main@origin [new] untracked + Setting the revset alias "trunk()" to "main@origin" + Working copy now at: sqpuoqvx cad212e1 (empty) (no description set) + Parent commit : mzyxwzks 9f01a0e0 main | message + Added 1 files, modified 0 files, removed 0 files + "#); + + test_env.jj_cmd_ok(&clone_path, &["bookmark", "forget", "main"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&clone_path, &["log"]); + insta::assert_snapshot!(stdout, @r#" + @ sqpuoqvx test.user@example.com 2001-02-03 08:05:07 cad212e1 + │ (empty) (no description set) + ○ mzyxwzks some.one@example.com 1970-01-01 11:00:00 9f01a0e0 + │ message + ◆ zzzzzzzz root() 00000000 + "#); + insta::assert_snapshot!(stderr, @r#" + Warning: Failed to resolve `revset-aliases.trunk()`: Revision "main@origin" doesn't exist + Hint: Use `jj config edit --repo` to adjust the `trunk()` alias. + "#); +} + fn get_bookmark_output(test_env: &TestEnvironment, repo_path: &Path) -> String { test_env.jj_cmd_success(repo_path, &["bookmark", "list", "--all-remotes"]) } diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index cdfd0019d..793f6fb0b 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -789,10 +789,6 @@ fn test_op_diff() { test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "git-repo", "repo"]); let repo_path = test_env.env_root().join("repo"); - // Suppress warning in the commit summary template. The configured "trunk()" - // can't be found in earlier operations. - test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); - // Overview of op log. let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(&stdout, @r#" @@ -1586,10 +1582,6 @@ fn test_op_show() { test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "git-repo", "repo"]); let repo_path = test_env.env_root().join("repo"); - // Suppress warning in the commit summary template. The configured "trunk()" - // can't be found in earlier operations. - test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); - // Overview of op log. let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(&stdout, @r#" diff --git a/docs/config.md b/docs/config.md index c1cf2155e..e4fa40661 100644 --- a/docs/config.md +++ b/docs/config.md @@ -266,8 +266,8 @@ diff-invocation-mode = "file-by-file" You can configure the set of immutable commits via `revset-aliases."immutable_heads()"`. The default set of immutable heads is -`trunk() | tags() | untracked_remote_bookmarks()`. For example, to prevent -rewriting commits on `main@origin` and commits authored by other users: +`present(trunk()) | tags() | untracked_remote_bookmarks()`. For example, to +prevent rewriting commits on `main@origin` and commits authored by other users: ```toml # The `main.. &` bit is an optimization to scan for non-`mine()` commits only @@ -290,7 +290,7 @@ revsets.log = "main@origin.." ``` The default value for `revsets.log` is -`'present(@) | ancestors(immutable_heads().., 2) | trunk()'`. +`'present(@) | ancestors(immutable_heads().., 2) | present(trunk())'`. ### Graph style diff --git a/docs/revsets.md b/docs/revsets.md index efd2c2d98..bb9e90046 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -429,15 +429,15 @@ for a comprehensive list. 'trunk()' = 'your-bookmark@your-remote' ``` -* `builtin_immutable_heads()`: Resolves to `trunk() | tags() | untracked_remote_bookmarks()`. - It is used as the default definition for `immutable_heads()` below. it is not - recommended to redefined this alias. Prefer to redefine `immutable_heads()` - instead. +* `builtin_immutable_heads()`: Resolves to + `present(trunk()) | tags() | untracked_remote_bookmarks()`. It is used as the + default definition for `immutable_heads()` below. it is not recommended to + redefined this alias. Prefer to redefine `immutable_heads()` instead. -* `immutable_heads()`: Resolves to `trunk() | tags() | untracked_remote_bookmarks()` - by default. It is actually defined as `builtin_immutable_heads()`, and can be - overridden as required. See [here](config.md#set-of-immutable-commits) for - details. +* `immutable_heads()`: Resolves to + `present(trunk()) | tags() | untracked_remote_bookmarks()` by default. It is + actually defined as `builtin_immutable_heads()`, and can be overridden as + required. See [here](config.md#set-of-immutable-commits) for details. * `immutable()`: The set of commits that `jj` treats as immutable. This is equivalent to `::(immutable_heads() | root())`. It is not recommended to redefine