cli: refactor single/default_single revset evaluation helpers

cli/src/cli_util.rs

@@ -52,7 +52,7 @@ use jj_lib::repo::{
 };
 use jj_lib::repo_path::{FsPathParseError, RepoPath, RepoPathBuf};
 use jj_lib::revset::{
-    RevsetAliasesMap, RevsetCommitRef, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt,
+    RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt,
     RevsetParseContext, RevsetWorkspaceContext,
 };
 use jj_lib::rewrite::restore_tree;
@@ -750,112 +750,53 @@ impl WorkspaceCommandHelper {
     /// Resolve a revset to a single revision. Return an error if the revset is
     /// empty or has multiple revisions.
     pub fn resolve_single_rev(&self, revision_str: &str) -> Result<Commit, CommandError> {
-        self.resolve_single_rev_with_hint_about_all_prefix(revision_str, false)
-    }
-
-    /// Resolve a revset any number of revisions (including 0).
-    pub fn resolve_revset(&self, revision_str: &str) -> Result<Vec<Commit>, CommandError> {
-        Ok(self
-            .parse_revset(revision_str)?
-            .evaluate_to_commits()?
-            .try_collect()?)
-    }
-
-    /// Resolve a revset any number of revisions (including 0), but require the
-    /// user to indicate if they allow multiple revisions by prefixing the
-    /// expression with `all:`.
-    pub fn resolve_revset_default_single(
-        &self,
-        revision_str: &str,
-    ) -> Result<Vec<Commit>, CommandError> {
-        // TODO: Let pest parse the prefix too once we've dropped support for `:`
-        if let Some(revision_str) = revision_str.strip_prefix("all:") {
-            self.resolve_revset(revision_str)
-        } else {
-            self.resolve_single_rev_with_hint_about_all_prefix(revision_str, true)
-                .map(|commit| vec![commit])
-        }
+        let expression = self.parse_revset(revision_str)?;
+        let should_hint_about_all_prefix = false;
+        revset_util::evaluate_revset_to_single_commit(
+            revision_str,
+            &expression,
+            || self.commit_summary_template(),
+            should_hint_about_all_prefix,
+        )
     }
 
-    fn resolve_single_rev_with_hint_about_all_prefix(
+    /// Evaluates revset expressions to non-empty set of commits. The returned
+    /// set preserves the order of the input expressions.
+    ///
+    /// If an input expression is prefixed with `all:`, it may be evaluated to
+    /// any number of revisions (including 0.)
+    pub fn resolve_some_revsets_default_single(
         &self,
-        revision_str: &str,
-        should_hint_about_all_prefix: bool,
-    ) -> Result<Commit, CommandError> {
-        let revset_expression = self.parse_revset(revision_str)?;
-        let mut iter = revset_expression.evaluate_to_commits()?.fuse();
-        match (iter.next(), iter.next()) {
-            (Some(commit), None) => Ok(commit?),
-            (None, _) => Err(user_error(format!(
-                r#"Revset "{revision_str}" didn't resolve to any revisions"#
-            ))),
-            (Some(commit0), Some(commit1)) => {
-                let mut cmd_err = user_error(format!(
-                    r#"Revset "{revision_str}" resolved to more than one revision"#
-                ));
-                let mut iter = [commit0, commit1].into_iter().chain(iter);
-                let commits: Vec<_> = iter.by_ref().take(5).try_collect()?;
-                let elided = iter.next().is_some();
-                let write_commits_summary = |formatter: &mut dyn Formatter| {
-                    let template = self.commit_summary_template();
-                    for commit in &commits {
-                        write!(formatter, "  ")?;
-                        template.format(commit, formatter)?;
-                        writeln!(formatter)?;
-                    }
-                    if elided {
-                        writeln!(formatter, "  ...")?;
-                    }
-                    Ok(())
-                };
-                if commits[0].change_id() == commits[1].change_id() {
-                    // Separate hint if there's commits with same change id
-                    cmd_err.add_formatted_hint_with(|formatter| {
-                        writeln!(
-                            formatter,
-                            r#"The revset "{revision_str}" resolved to these revisions:"#
-                        )?;
-                        write_commits_summary(formatter)
-                    });
-                    cmd_err.add_hint(
-                        "Some of these commits have the same change id. Abandon one of them with \
-                         `jj abandon -r <REVISION>`.",
-                    );
-                } else if let RevsetExpression::CommitRef(RevsetCommitRef::Symbol(branch_name)) =
-                    revset_expression.expression().as_ref()
-                {
-                    // Separate hint if there's a conflicted branch
-                    cmd_err.add_formatted_hint_with(|formatter| {
-                        writeln!(
-                            formatter,
-                            "Branch {branch_name} resolved to multiple revisions because it's \
-                             conflicted."
-                        )?;
-                        writeln!(formatter, "It resolved to these revisions:")?;
-                        write_commits_summary(formatter)
-                    });
-                    cmd_err.add_hint(format!(
-                        "Set which revision the branch points to with `jj branch set \
-                         {branch_name} -r <REVISION>`.",
-                    ));
-                } else {
-                    cmd_err.add_formatted_hint_with(|formatter| {
-                        writeln!(
-                            formatter,
-                            r#"The revset "{revision_str}" resolved to these revisions:"#
-                        )?;
-                        write_commits_summary(formatter)
-                    });
-                    if should_hint_about_all_prefix {
-                        cmd_err.add_hint(format!(
-                            "Prefix the expression with 'all:' to allow any number of revisions \
-                             (i.e. 'all:{revision_str}')."
-                        ));
-                    }
-                };
-                Err(cmd_err)
+        revision_args: &[RevisionArg],
+    ) -> Result<IndexSet<Commit>, CommandError> {
+        let mut all_commits = IndexSet::new();
+        for revision_str in revision_args {
+            let (expression, all) = self.parse_revset_with_all_prefix(revision_str)?;
+            if all {
+                for commit in expression.evaluate_to_commits()? {
+                    all_commits.insert(commit?);
+                }
+            } else {
+                let should_hint_about_all_prefix = true;
+                let commit = revset_util::evaluate_revset_to_single_commit(
+                    revision_str,
+                    &expression,
+                    || self.commit_summary_template(),
+                    should_hint_about_all_prefix,
+                )?;
+                let commit_hash = short_commit_hash(commit.id());
+                if !all_commits.insert(commit) {
+                    return Err(user_error(format!(
+                        r#"More than one revset resolved to revision {commit_hash}"#,
+                    )));
+                }
             }
         }
+        if all_commits.is_empty() {
+            Err(user_error("Empty revision set"))
+        } else {
+            Ok(all_commits)
+        }
     }
 
     pub fn parse_revset(
@@ -866,6 +807,18 @@ impl WorkspaceCommandHelper {
         self.attach_revset_evaluator(expression)
     }
 
+    fn parse_revset_with_all_prefix(
+        &self,
+        revision_str: &str,
+    ) -> Result<(RevsetExpressionEvaluator<'_>, bool), CommandError> {
+        // TODO: Let pest parse the prefix too once we've dropped support for `:`
+        if let Some(revision_str) = revision_str.strip_prefix("all:") {
+            Ok((self.parse_revset(revision_str)?, true))
+        } else {
+            Ok((self.parse_revset(revision_str)?, false))
+        }
+    }
+
     /// Parses the given revset expressions and concatenates them all.
     pub fn parse_union_revsets(
         &self,
@@ -1665,29 +1618,6 @@ pub fn print_trackable_remote_branches(ui: &Ui, view: &View) -> io::Result<()> {
     Ok(())
 }
 
-pub fn resolve_multiple_nonempty_revsets_default_single(
-    workspace_command: &WorkspaceCommandHelper,
-    revisions: &[RevisionArg],
-) -> Result<IndexSet<Commit>, CommandError> {
-    let mut all_commits = IndexSet::new();
-    for revision_str in revisions {
-        let commits = workspace_command.resolve_revset_default_single(revision_str)?;
-        for commit in commits {
-            let commit_hash = short_commit_hash(commit.id());
-            if !all_commits.insert(commit) {
-                return Err(user_error(format!(
-                    r#"More than one revset resolved to revision {commit_hash}"#,
-                )));
-            }
-        }
-    }
-    if all_commits.is_empty() {
-        Err(user_error("Empty revision set"))
-    } else {
-        Ok(all_commits)
-    }
-}
-
 pub fn update_working_copy(
     repo: &Arc<ReadonlyRepo>,
     workspace: &mut Workspace,

cli/src/commands/new.rs

@@ -22,9 +22,7 @@ use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
 use jj_lib::rewrite::{merge_commit_trees, rebase_commit};
 use tracing::instrument;
 
-use crate::cli_util::{
-    resolve_multiple_nonempty_revsets_default_single, short_commit_hash, CommandHelper, RevisionArg,
-};
+use crate::cli_util::{short_commit_hash, CommandHelper, RevisionArg};
 use crate::command_error::{user_error, CommandError};
 use crate::description_util::join_message_paragraphs;
 use crate::ui::Ui;
@@ -96,10 +94,10 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
         ));
     }
     let mut workspace_command = command.workspace_helper(ui)?;
-    let target_commits =
-        resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.revisions)?
-            .into_iter()
-            .collect_vec();
+    let target_commits = workspace_command
+        .resolve_some_revsets_default_single(&args.revisions)?
+        .into_iter()
+        .collect_vec();
     let target_ids = target_commits.iter().map(|c| c.id().clone()).collect_vec();
     let mut tx = workspace_command.start_transaction();
     let mut num_rebased;

cli/src/commands/rebase.rs

@@ -30,8 +30,8 @@ use jj_lib::settings::UserSettings;
 use tracing::instrument;
 
 use crate::cli_util::{
-    resolve_multiple_nonempty_revsets_default_single, short_commit_hash, CommandHelper,
-    RevisionArg, WorkspaceCommandHelper, WorkspaceCommandTransaction,
+    short_commit_hash, CommandHelper, RevisionArg, WorkspaceCommandHelper,
+    WorkspaceCommandTransaction,
 };
 use crate::command_error::{user_error, CommandError};
 use crate::formatter::Formatter;
@@ -193,10 +193,10 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
         simplify_ancestor_merge: false,
     };
     let mut workspace_command = command.workspace_helper(ui)?;
-    let new_parents =
-        resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.destination)?
-            .into_iter()
-            .collect_vec();
+    let new_parents = workspace_command
+        .resolve_some_revsets_default_single(&args.destination)?
+        .into_iter()
+        .collect_vec();
     if let Some(rev_str) = &args.revision {
         assert_eq!(
             // In principle, `-r --skip-empty` could mean to abandon the `-r`
@@ -221,8 +221,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
             rev_str,
         )?;
     } else if !args.source.is_empty() {
-        let source_commits =
-            resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.source)?;
+        let source_commits = workspace_command.resolve_some_revsets_default_single(&args.source)?;
         rebase_descendants_transaction(
             ui,
             command.settings(),
@@ -235,7 +234,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
         let branch_commits = if args.branch.is_empty() {
             IndexSet::from([workspace_command.resolve_single_rev("@")?])
         } else {
-            resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.branch)?
+            workspace_command.resolve_some_revsets_default_single(&args.branch)?
         };
         rebase_branch(
             ui,

cli/src/commands/squash.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools as _;
 use jj_lib::commit::Commit;
 use jj_lib::matchers::Matcher;
 use jj_lib::object_id::ObjectId;
@@ -80,10 +81,13 @@ pub(crate) fn cmd_squash(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
 
-    let mut sources;
+    let mut sources: Vec<Commit>;
     let destination;
     if args.from.is_some() || args.into.is_some() {
-        sources = workspace_command.resolve_revset(args.from.as_deref().unwrap_or("@"))?;
+        sources = workspace_command
+            .parse_revset(args.from.as_deref().unwrap_or("@"))?
+            .evaluate_to_commits()?
+            .try_collect()?;
         destination = workspace_command.resolve_single_rev(args.into.as_deref().unwrap_or("@"))?;
         if sources.iter().any(|source| source.id() == destination.id()) {
             return Err(user_error("Source and destination cannot be the same"));

cli/src/commands/workspace.rs

@@ -29,9 +29,8 @@ use jj_lib::workspace::Workspace;
 use tracing::instrument;
 
 use crate::cli_util::{
-    check_stale_working_copy, print_checkout_stats,
-    resolve_multiple_nonempty_revsets_default_single, short_commit_hash, CommandHelper,
-    RevisionArg, WorkingCopyFreshness, WorkspaceCommandHelper,
+    check_stale_working_copy, print_checkout_stats, short_commit_hash, CommandHelper, RevisionArg,
+    WorkingCopyFreshness, WorkspaceCommandHelper,
 };
 use crate::command_error::{internal_error_with_message, user_error, CommandError};
 use crate::ui::Ui;
@@ -203,7 +202,8 @@ fn cmd_workspace_add(
             vec![tx.repo().store().root_commit()]
         }
     } else {
-        resolve_multiple_nonempty_revsets_default_single(&old_workspace_command, &args.revision)?
+        old_workspace_command
+            .resolve_some_revsets_default_single(&args.revision)?
             .into_iter()
             .collect_vec()
     };