From 43cb9543a9e49e4312aac41a8b3143cfd13c66dc Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 28 Sep 2024 19:20:06 +0900 Subject: [PATCH] revset: add at_operation(op, expression) This can be used in order to refer old working-copy commit, for example. If we find it's useful, maybe we can add an infix syntax later. Closes #1283 --- CHANGELOG.md | 3 + docs/revsets.md | 6 ++ lib/src/revset.rs | 129 ++++++++++++++++++++++++++++++++--- lib/tests/test_revset.rs | 143 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 268 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 270be32916..5a1d3036c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * New command `jj simplify-parents` will remove redundant parent edges. +* New `at_operation(op, expr)` revset can be used in order to query revisions + based on historical state. + ### Fixed bugs * Error on `trunk()` revset resolution is now handled gracefully. diff --git a/docs/revsets.md b/docs/revsets.md index bb9e900468..bbd6c29142 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -306,6 +306,12 @@ given [string pattern](#string-patterns). * `working_copies()`: The working copy commits across all the workspaces. +* `at_operation(op, x)`: Evaluates `x` at the specified [operation][]. For + example, `at_operation(@-, visible_heads())` will return all heads which were + visible at the previous operation. + +[operation]: glossary.md#operation + ??? examples Given this history: diff --git a/lib/src/revset.rs b/lib/src/revset.rs index e775abf19a..939c734922 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -46,7 +46,10 @@ use crate::object_id::HexPrefix; use crate::object_id::PrefixResolution; use crate::op_store::RemoteRefState; use crate::op_store::WorkspaceId; +use crate::op_walk; +use crate::repo::ReadonlyRepo; use crate::repo::Repo; +use crate::repo::RepoLoaderError; use crate::repo_path::RepoPathUiConverter; use crate::revset_parser; pub use crate::revset_parser::expect_literal; @@ -208,6 +211,13 @@ pub enum RevsetExpression { Filter(RevsetFilterPredicate), /// Marker for subtree that should be intersected as filter. AsFilter(Rc), + /// Resolves symbols and visibility at the specified operation. + AtOperation { + operation: String, + candidates: Rc, + /// Copy of `repo.view().heads()`, should be set by `resolve_symbols()`. + visible_heads: Option>, + }, Present(Rc), NotIn(Rc), Union(Rc, Rc), @@ -429,13 +439,17 @@ impl RevsetExpression { Rc::new(Self::Difference(self.clone(), other.clone())) } - /// Resolve a programmatically created revset expression. In particular, the - /// expression must not contain any symbols (bookmarks, tags, change/commit - /// prefixes). Callers must not include `RevsetExpression::symbol()` in - /// the expression, and should instead resolve symbols to `CommitId`s and - /// pass them into `RevsetExpression::commits()`. Similarly, the expression - /// must not contain any `RevsetExpression::remote_symbol()` or + /// Resolve a programmatically created revset expression. + /// + /// In particular, the expression must not contain any symbols (bookmarks, + /// tags, change/commit prefixes). Callers must not include + /// `RevsetExpression::symbol()` in the expression, and should instead + /// resolve symbols to `CommitId`s and pass them into + /// `RevsetExpression::commits()`. Similarly, the expression must not + /// contain any `RevsetExpression::remote_symbol()` or /// `RevsetExpression::working_copy()`, unless they're known to be valid. + /// The expression must not contain `RevsetExpression::AtOperation` even if + /// it's known to be valid. It can fail at loading operation data. pub fn resolve_programmatic(self: Rc, repo: &dyn Repo) -> ResolvedExpression { let symbol_resolver = FailingSymbolResolver; resolve_symbols(repo, self, &symbol_resolver) @@ -825,6 +839,20 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: let expression = lower_expression(diagnostics, arg, context)?; Ok(Rc::new(RevsetExpression::Present(expression))) }); + map.insert("at_operation", |diagnostics, function, context| { + let [op_arg, cand_arg] = function.expect_exact_arguments()?; + // TODO: Parse "opset" here if we add proper language support. + let operation = + revset_parser::expect_expression_with(diagnostics, op_arg, |_diagnostics, node| { + Ok(node.span.as_str().to_owned()) + })?; + let candidates = lower_expression(diagnostics, cand_arg, context)?; + Ok(Rc::new(RevsetExpression::AtOperation { + operation, + candidates, + visible_heads: None, + })) + }); map }); @@ -1132,6 +1160,17 @@ fn try_transform_expression( RevsetExpression::AsFilter(candidates) => { transform_rec(candidates, pre, post)?.map(RevsetExpression::AsFilter) } + RevsetExpression::AtOperation { + operation, + candidates, + visible_heads, + } => transform_rec(candidates, pre, post)?.map(|candidates| { + RevsetExpression::AtOperation { + operation: operation.clone(), + candidates, + visible_heads: visible_heads.clone(), + } + }), RevsetExpression::Present(candidates) => { transform_rec(candidates, pre, post)?.map(RevsetExpression::Present) } @@ -1486,6 +1525,24 @@ pub fn walk_revs<'index>( .evaluate_programmatic(repo) } +fn reload_repo_at_operation( + repo: &dyn Repo, + op_str: &str, +) -> Result, RevsetResolutionError> { + // TODO: Maybe we should ensure that the resolved operation is an ancestor + // of the current operation. If it weren't, there might be commits unknown + // to the outer repo. + let base_repo = repo.base_repo(); + let operation = op_walk::resolve_op_with_repo(base_repo, op_str) + .map_err(|err| RevsetResolutionError::Other(err.into()))?; + base_repo.reload_at(&operation).map_err(|err| match err { + RepoLoaderError::Backend(err) => RevsetResolutionError::StoreError(err), + RepoLoaderError::IndexRead(_) + | RepoLoaderError::OpHeadResolution(_) + | RepoLoaderError::OpStore(_) => RevsetResolutionError::Other(err.into()), + }) +} + fn resolve_remote_bookmark(repo: &dyn Repo, name: &str, remote: &str) -> Option> { let view = repo.view(); let target = match (name, remote) { @@ -1858,6 +1915,22 @@ fn resolve_symbols( Ok(try_transform_expression( &expression, |expression| match expression.as_ref() { + // 'at_operation(op, x)' switches symbol resolution contexts. + RevsetExpression::AtOperation { + operation, + candidates, + visible_heads: _, + } => { + let repo = reload_repo_at_operation(repo, operation)?; + let candidates = + resolve_symbols(repo.as_ref(), candidates.clone(), symbol_resolver)?; + let visible_heads = Some(repo.view().heads().iter().cloned().collect()); + Ok(Some(Rc::new(RevsetExpression::AtOperation { + operation: operation.clone(), + candidates, + visible_heads, + }))) + } // 'present(x)' opens new symbol resolution scope to map error to 'none()'. RevsetExpression::Present(candidates) => { resolve_symbols(repo, candidates.clone(), symbol_resolver) @@ -1898,9 +1971,6 @@ fn resolve_symbols( /// return type `ResolvedExpression` is stricter than `RevsetExpression`, /// and isn't designed for such transformation. fn resolve_visibility(repo: &dyn Repo, expression: &RevsetExpression) -> ResolvedExpression { - // If we add "operation" scope (#1283), visible_heads might be translated to - // `RevsetExpression::WithinOperation(visible_heads, expression)` node to - // evaluate filter predicates and "all()" against that scope. let context = VisibilityResolutionContext { visible_heads: &repo.view().heads().iter().cloned().collect_vec(), }; @@ -1969,6 +2039,17 @@ impl VisibilityResolutionContext<'_> { predicate: self.resolve_predicate(expression), } } + RevsetExpression::AtOperation { + operation: _, + candidates, + visible_heads, + } => { + let visible_heads = visible_heads + .as_ref() + .expect("visible_heads should have been resolved by caller"); + let context = VisibilityResolutionContext { visible_heads }; + context.resolve(candidates) + } RevsetExpression::Present(_) => { panic!("Expression '{expression:?}' should have been resolved by caller"); } @@ -2045,6 +2126,10 @@ impl VisibilityResolutionContext<'_> { ResolvedPredicateExpression::Filter(predicate.clone()) } RevsetExpression::AsFilter(candidates) => self.resolve_predicate(candidates), + // Filters should be intersected with all() within the at-op repo. + RevsetExpression::AtOperation { .. } => { + ResolvedPredicateExpression::Set(self.resolve(expression).into()) + } RevsetExpression::Present(_) => { panic!("Expression '{expression:?}' should have been resolved by caller") } @@ -3010,6 +3095,15 @@ mod tests { optimize(parse("present(bookmarks() & all())").unwrap()), @r###"Present(CommitRef(Bookmarks(Substring(""))))"###); + insta::assert_debug_snapshot!( + optimize(parse("at_operation(@-, bookmarks() & all())").unwrap()), @r#" + AtOperation { + operation: "@-", + candidates: CommitRef(Bookmarks(Substring(""))), + visible_heads: None, + } + "#); + insta::assert_debug_snapshot!( optimize(parse("~bookmarks() & all()").unwrap()), @r###"NotIn(CommitRef(Bookmarks(Substring(""))))"###); @@ -3480,6 +3574,23 @@ mod tests { Filter(Author(Substring("baz"))), ) "###); + + // Filter node shouldn't move across at_operation() boundary. + insta::assert_debug_snapshot!( + optimize(parse("author(foo) & bar & at_operation(@-, committer(baz))").unwrap()), + @r#" + Intersection( + Intersection( + CommitRef(Symbol("bar")), + AtOperation { + operation: "@-", + candidates: Filter(Committer(Substring("baz"))), + visible_heads: None, + }, + ), + Filter(Author(Substring("foo"))), + ) + "#); } #[test] diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index f4b0eeecfe..9de50e0e30 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -949,6 +949,13 @@ fn test_resolve_symbol_git_refs() { } fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec { + try_resolve_commit_ids(repo, revset_str).unwrap() +} + +fn try_resolve_commit_ids( + repo: &dyn Repo, + revset_str: &str, +) -> Result, RevsetResolutionError> { let settings = testutils::user_settings(); let aliases_map = RevsetAliasesMap::default(); let revset_extensions = RevsetExtensions::default(); @@ -961,10 +968,8 @@ fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec { ); let expression = optimize(parse(&mut RevsetDiagnostics::new(), revset_str, &context).unwrap()); let symbol_resolver = DefaultSymbolResolver::new(repo, revset_extensions.symbol_resolvers()); - let expression = expression - .resolve_user_expression(repo, &symbol_resolver) - .unwrap(); - expression.evaluate(repo).unwrap().iter().collect() + let expression = expression.resolve_user_expression(repo, &symbol_resolver)?; + Ok(expression.evaluate(repo).unwrap().iter().collect()) } fn resolve_commit_ids_in_workspace( @@ -2894,6 +2899,136 @@ fn test_evaluate_expression_committer() { ); } +#[test] +fn test_evaluate_expression_at_operation() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo0 = &test_repo.repo; + let root_commit = repo0.store().root_commit(); + + let mut tx = repo0.start_transaction(&settings); + let commit1_op1 = create_random_commit(tx.repo_mut(), &settings) + .set_description("commit1@op1") + .write() + .unwrap(); + let commit2_op1 = create_random_commit(tx.repo_mut(), &settings) + .set_description("commit2@op1") + .write() + .unwrap(); + tx.repo_mut() + .set_local_bookmark_target("commit1_ref", RefTarget::normal(commit1_op1.id().clone())); + let repo1 = tx.commit("test"); + + let mut tx = repo1.start_transaction(&settings); + let commit1_op2 = tx + .repo_mut() + .rewrite_commit(&settings, &commit1_op1) + .set_description("commit1@op2") + .write() + .unwrap(); + let commit3_op2 = create_random_commit(tx.repo_mut(), &settings) + .set_description("commit3@op2") + .write() + .unwrap(); + tx.repo_mut().rebase_descendants(&settings).unwrap(); + let repo2 = tx.commit("test"); + + let mut tx = repo2.start_transaction(&settings); + let _commit4_op3 = create_random_commit(tx.repo_mut(), &settings) + .set_description("commit4@op3") + .write() + .unwrap(); + + // Symbol resolution: + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@, commit1_ref)"), + vec![commit1_op2.id().clone()] + ); + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@-, commit1_ref)"), + vec![commit1_op1.id().clone()] + ); + assert_matches!( + try_resolve_commit_ids(repo2.as_ref(), "at_operation(@--, commit1_ref)"), + Err(RevsetResolutionError::NoSuchRevision { .. }) + ); + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "present(at_operation(@--, commit1_ref))"), + vec![] + ); + + // Visibility resolution: + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@, all())"), + vec![ + commit3_op2.id().clone(), + commit1_op2.id().clone(), + commit2_op1.id().clone(), + root_commit.id().clone(), + ] + ); + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@-, all())"), + vec![ + commit2_op1.id().clone(), + commit1_op1.id().clone(), + root_commit.id().clone(), + ] + ); + + // Operation is resolved relative to the outer ReadonlyRepo. + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@-, at_operation(@-, all()))"), + resolve_commit_ids(repo2.as_ref(), "at_operation(@--, all())") + ); + + // TODO: It might make more sense to resolve "@" to the current MutableRepo + // state. However, doing that isn't easy because there's no Operation object + // representing a MutableRepo state. For now, "@" is resolved to the base + // operation. + assert_eq!( + resolve_commit_ids(tx.repo(), "at_operation(@, all())"), + vec![ + commit3_op2.id().clone(), + commit1_op2.id().clone(), + commit2_op1.id().clone(), + root_commit.id().clone(), + ] + ); + + // Filter should be evaluated within the at-op repo. Note that this can + // populate hidden commits without explicitly referring them by commit refs. + // It might be better to intersect inner results again with the outer repo. + assert_eq!( + resolve_commit_ids(repo2.as_ref(), "at_operation(@-, description('commit'))"), + vec![commit2_op1.id().clone(), commit1_op1.id().clone()] + ); + // For the same reason, commit1_op1 isn't filtered out. The following query + // is effectively evaluated as "description('commit1') & commit1_op1". + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "description('commit1') & at_operation(@-, description('commit'))" + ), + vec![commit1_op1.id().clone()] + ); + // If we have an explicit ::visible_heads(), commit1_op1 is filtered out. + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "::visible_heads() & description('commit1') & at_operation(@-, description('commit'))" + ), + vec![] + ); + + // Bad operation: + // TODO: should we suppress NoSuchOperation error by present()? + assert_matches!( + try_resolve_commit_ids(repo2.as_ref(), "at_operation(000000000000-, all())"), + Err(RevsetResolutionError::Other(_)) + ); +} + #[test] fn test_evaluate_expression_union() { let settings = testutils::user_settings();