From f27f52984ea5a284a3849b780b47cb5d8a14cdc7 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 24 Nov 2023 10:12:34 -0800 Subject: [PATCH] revset: rename `resolve()` to `resolve_programmatic()` `RevsetExpression::resolve()` is meant for programmatically created expressions. In particular, it may not contain symbols. Let's try to clarify that by renaming the function and documenting it. --- cli/src/commands/new.rs | 8 ++++---- cli/src/commands/next.rs | 2 +- cli/src/commands/prev.rs | 2 +- cli/src/commands/rebase.rs | 6 +++--- lib/src/git.rs | 2 +- lib/src/repo.rs | 6 +++--- lib/src/revset.rs | 11 +++++++++-- lib/src/rewrite.rs | 4 ++-- lib/tests/test_revset.rs | 6 +++--- 9 files changed, 27 insertions(+), 20 deletions(-) diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index b3f2713c6..e547f22a5 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -116,7 +116,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", let new_parents = new_children.parents(); if let Some(commit_id) = new_children .dag_range_to(&new_parents) - .resolve(tx.repo())? + .resolve_programmatic(tx.repo())? .evaluate(tx.repo())? .iter() .next() @@ -128,7 +128,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", ))); } let mut new_parents_commits: Vec = new_parents - .resolve(tx.repo())? + .resolve_programmatic(tx.repo())? .evaluate(tx.repo())? .iter() .commits(tx.repo().store()) @@ -163,7 +163,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", // Exclude children that are ancestors of the new commit let to_rebase = old_parents.children().minus(&old_parents.ancestors()); to_rebase - .resolve(tx.base_repo().as_ref())? + .resolve_programmatic(tx.base_repo().as_ref())? .evaluate(tx.base_repo().as_ref())? .iter() .commits(tx.base_repo().store()) @@ -184,7 +184,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", let commit_parents = RevsetExpression::commits(child_commit.parent_ids().to_owned()); let new_parents = commit_parents.minus(&old_parents); let mut new_parent_commits: Vec = new_parents - .resolve(tx.base_repo().as_ref())? + .resolve_programmatic(tx.base_repo().as_ref())? .evaluate(tx.base_repo().as_ref())? .iter() .commits(tx.base_repo().store()) diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index d6b0e16eb..74e068070 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -90,7 +90,7 @@ pub(crate) fn cmd_next( descendant_expression.minus(&RevsetExpression::commit(current_wc_id.clone()).descendants()) }; let targets: Vec = target_expression - .resolve(workspace_command.repo().as_ref())? + .resolve_programmatic(workspace_command.repo().as_ref())? .evaluate(workspace_command.repo().as_ref())? .iter() .commits(workspace_command.repo().store()) diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index bed00d10b..5fb5c5ba6 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -88,7 +88,7 @@ pub(crate) fn cmd_prev( ancestor_expression.minus(&RevsetExpression::commit(current_wc_id.clone())) }; let targets: Vec<_> = target_revset - .resolve(workspace_command.repo().as_ref())? + .resolve_programmatic(workspace_command.repo().as_ref())? .evaluate(workspace_command.repo().as_ref())? .iter() .commits(workspace_command.repo().store()) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 8d3680835..dc26005e5 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -236,7 +236,7 @@ fn rebase_branch( .range(&RevsetExpression::commits(branch_commit_ids)) .roots(); let root_commits: IndexSet<_> = roots_expression - .resolve(workspace_command.repo().as_ref()) + .resolve_programmatic(workspace_command.repo().as_ref()) .unwrap() .evaluate(workspace_command.repo().as_ref()) .unwrap() @@ -312,7 +312,7 @@ fn rebase_revision( let children_expression = RevsetExpression::commit(old_commit.id().clone()).children(); let child_commits: Vec<_> = children_expression - .resolve(workspace_command.repo().as_ref()) + .resolve_programmatic(workspace_command.repo().as_ref()) .unwrap() .evaluate(workspace_command.repo().as_ref()) .unwrap() @@ -353,7 +353,7 @@ fn rebase_revision( .ancestors(), ); let new_child_parents: Vec = new_child_parents_expression - .resolve(tx.base_repo().as_ref()) + .resolve_programmatic(tx.base_repo().as_ref()) .unwrap() .evaluate(tx.base_repo().as_ref()) .unwrap() diff --git a/lib/src/git.rs b/lib/src/git.rs index 4ed14a0a6..163bc68c4 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -371,7 +371,7 @@ fn abandon_unreachable_commits( .range(&RevsetExpression::commits(hidable_git_heads)) .intersection(&RevsetExpression::visible_heads().ancestors()); let abandoned_commits = revset::optimize(abandoned_expression) - .resolve(mut_repo) + .resolve_programmatic(mut_repo) .unwrap() .evaluate(mut_repo) .unwrap() diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 1c15f05d2..5dbd6853b 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -265,7 +265,7 @@ impl ReadonlyRepo { .change_id_index .get_or_init(|| { let revset: Box> = RevsetExpression::all() - .resolve(self) + .resolve_programmatic(self) .unwrap() .evaluate(self) .unwrap(); @@ -1326,7 +1326,7 @@ impl Repo for MutableRepo { fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { let revset = RevsetExpression::all() - .resolve(self) + .resolve_programmatic(self) .unwrap() .evaluate(self) .unwrap(); @@ -1336,7 +1336,7 @@ impl Repo for MutableRepo { fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> usize { let revset = RevsetExpression::all() - .resolve(self) + .resolve_programmatic(self) .unwrap() .evaluate(self) .unwrap(); diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 9f959fcab..2ad857348 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -633,7 +633,12 @@ impl RevsetExpression { Rc::new(RevsetExpression::Difference(self.clone(), other.clone())) } - pub fn resolve( + /// Resolve a programmatically created revset expression. In particular, the + /// expression must not contain any symbols (branches, 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()`. + pub fn resolve_programmatic( self: Rc, repo: &dyn Repo, ) -> Result { @@ -642,6 +647,8 @@ impl RevsetExpression { .map(|expression| resolve_visibility(repo, &expression)) } + /// Resolve a user-provided expression. Symbols will be resolved using the + /// provided `SymbolResolver`. pub fn resolve_user_expression( self: Rc, repo: &dyn Repo, @@ -1932,7 +1939,7 @@ pub fn walk_revs<'index>( ) -> Result + 'index>, RevsetEvaluationError> { RevsetExpression::commits(unwanted.to_vec()) .range(&RevsetExpression::commits(wanted.to_vec())) - .resolve(repo) + .resolve_programmatic(repo) .unwrap() .evaluate(repo) } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index bf6538dd2..4b7f1b95f 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -289,7 +289,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .parents() .minus(&old_commits_expression); let heads_to_add = heads_to_add_expression - .resolve(mut_repo) + .resolve_programmatic(mut_repo) .unwrap() .evaluate(mut_repo) .unwrap() @@ -298,7 +298,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { let to_visit_expression = old_commits_expression.descendants(); let to_visit_revset = to_visit_expression - .resolve(mut_repo) + .resolve_programmatic(mut_repo) .unwrap() .evaluate(mut_repo) .unwrap(); diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 2b9acff4a..b54713398 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -350,7 +350,7 @@ fn test_resolve_working_copy() { // Cannot resolve a working-copy commit for an unknown workspace assert_matches!( - RevsetExpression::working_copy(ws1.clone()).resolve(mut_repo), + RevsetExpression::working_copy(ws1.clone()).resolve_programmatic(mut_repo), Err(RevsetResolutionError::WorkspaceMissingWorkingCopy { name }) if name == "ws1" ); @@ -363,7 +363,7 @@ fn test_resolve_working_copy() { .unwrap(); let resolve = |ws_id: WorkspaceId| -> Vec { RevsetExpression::working_copy(ws_id) - .resolve(mut_repo) + .resolve_programmatic(mut_repo) .unwrap() .evaluate(mut_repo) .unwrap() @@ -2637,7 +2637,7 @@ fn test_evaluate_expression_file() { let expression = RevsetExpression::filter(RevsetFilterPredicate::File(Some(vec![file_path.clone()]))); let revset = expression - .resolve(mut_repo) + .resolve_programmatic(mut_repo) .unwrap() .evaluate(mut_repo) .unwrap();