Avoid evaluating large revsets just to check if they're immutable

cli/src/cli_util.rs

@@ -27,7 +27,6 @@ use std::mem;
 use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
-use std::slice;
 use std::str::FromStr;
 use std::sync::Arc;
 use std::time::SystemTime;
@@ -915,30 +914,31 @@ impl WorkspaceCommandEnvironment {
     fn find_immutable_commit(
         &self,
         repo: &dyn Repo,
-        commit_ids: &[CommitId],
+        to_rewrite_expr: &Arc<ResolvedRevsetExpression>,
     ) -> Result<Option<CommitId>, CommandError> {
-        if self.command.global_args().ignore_immutable {
-            let root_id = repo.store().root_commit_id();
-            return Ok(commit_ids.iter().find(|id| *id == root_id).cloned());
-        }
+        let immutable_expression = if self.command.global_args().ignore_immutable {
+            UserRevsetExpression::root()
+        } else {
+            self.immutable_expression()
+        };
 
         // Not using self.id_prefix_context() because the disambiguation data
         // must not be calculated and cached against arbitrary repo. It's also
         // unlikely that the immutable expression contains short hashes.
         let id_prefix_context = IdPrefixContext::new(self.command.revset_extensions().clone());
-        let to_rewrite_revset = RevsetExpression::commits(commit_ids.to_vec());
-        let mut expression = RevsetExpressionEvaluator::new(
+        let immutable_expr = RevsetExpressionEvaluator::new(
             repo,
             self.command.revset_extensions().clone(),
             &id_prefix_context,
-            self.immutable_expression(),
-        );
-        expression.intersect_with(&to_rewrite_revset);
-
-        let mut commit_id_iter = expression.evaluate_to_commit_ids().map_err(|e| {
-            config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e)
-        })?;
+            immutable_expression,
+        )
+        .resolve()
+        .map_err(|e| config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e))?;
 
+        let mut commit_id_iter = immutable_expr
+            .intersection(to_rewrite_expr)
+            .evaluate(repo)?
+            .iter();
         Ok(commit_id_iter.next().transpose()?)
     }
 
@@ -1773,9 +1773,17 @@ to the current parents may contain changes from multiple commits.
         &self,
         commits: impl IntoIterator<Item = &'a CommitId>,
     ) -> Result<(), CommandError> {
-        let repo = self.repo().as_ref();
         let commit_ids = commits.into_iter().cloned().collect_vec();
-        let Some(commit_id) = self.env.find_immutable_commit(repo, &commit_ids)? else {
+        let to_rewrite_expr = RevsetExpression::commits(commit_ids);
+        self.check_rewritable_expr(&to_rewrite_expr)
+    }
+
+    pub fn check_rewritable_expr(
+        &self,
+        to_rewrite_expr: &Arc<ResolvedRevsetExpression>,
+    ) -> Result<(), CommandError> {
+        let repo = self.repo().as_ref();
+        let Some(commit_id) = self.env.find_immutable_commit(repo, to_rewrite_expr)? else {
             return Ok(());
         };
         let error = if &commit_id == repo.store().root_commit_id() {
@@ -1798,16 +1806,15 @@ to the current parents may contain changes from multiple commits.
             // find_immutable_commit().
             let id_prefix_context =
                 IdPrefixContext::new(self.env.command.revset_extensions().clone());
-            let to_rewrite_expr = RevsetExpression::commits(commit_ids);
             let (lower_bound, upper_bound) = RevsetExpressionEvaluator::new(
                 repo,
                 self.env.command.revset_extensions().clone(),
                 &id_prefix_context,
-                self.env
-                    .immutable_expression()
-                    .intersection(&to_rewrite_expr.descendants()),
+                self.env.immutable_expression(),
             )
-            .evaluate()?
+            .resolve()?
+            .intersection(&to_rewrite_expr.descendants())
+            .evaluate(repo)?
             .count_estimate()?;
             let exact = upper_bound == Some(lower_bound);
             let or_more = if exact { "" } else { " or more" };
@@ -2040,7 +2047,7 @@ See https://jj-vcs.github.io/jj/latest/working-copy/#stale-working-copy \
         for (name, wc_commit_id) in &tx.repo().view().wc_commit_ids().clone() {
             if self
                 .env
-                .find_immutable_commit(tx.repo(), slice::from_ref(wc_commit_id))?
+                .find_immutable_commit(tx.repo(), &RevsetExpression::commit(wc_commit_id.clone()))?
                 .is_some()
             {
                 let wc_commit = tx.repo().store().get_commit(wc_commit_id)?;

cli/src/commands/abandon.rs

@@ -74,16 +74,21 @@ pub(crate) fn cmd_abandon(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let to_abandon = {
-        let targets: Vec<_> = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
+        let target_expr = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
             workspace_command
                 .parse_union_revsets(ui, &[&*args.revisions_pos, &*args.revisions_opt].concat())?
         } else {
             workspace_command.parse_revset(ui, &RevisionArg::AT)?
         }
-        .evaluate_to_commit_ids()?
-        .try_collect()?;
-        let visible: IndexSet<_> = RevsetExpression::commits(targets.clone())
-            .intersection(&RevsetExpression::visible_heads().ancestors())
+        .resolve()?;
+        let visible_expr = target_expr.intersection(&RevsetExpression::visible_heads().ancestors());
+        workspace_command.check_rewritable_expr(&visible_expr)?;
+        let visible: IndexSet<_> = visible_expr
+            .evaluate(workspace_command.repo().as_ref())?
+            .iter()
+            .try_collect()?;
+
+        let targets: Vec<_> = target_expr
             .evaluate(workspace_command.repo().as_ref())?
             .iter()
             .try_collect()?;
@@ -100,7 +105,6 @@ pub(crate) fn cmd_abandon(
         writeln!(ui.status(), "No revisions to abandon.")?;
         return Ok(());
     }
-    workspace_command.check_rewritable(&to_abandon)?;
 
     let mut tx = workspace_command.start_transaction();
     let options = RewriteRefsOptions {

cli/src/commands/describe.rs

@@ -20,8 +20,9 @@ use std::iter;
 use clap_complete::ArgValueCompleter;
 use itertools::Itertools as _;
 use jj_lib::backend::Signature;
-use jj_lib::commit::CommitIteratorExt as _;
 use jj_lib::object_id::ObjectId as _;
+use jj_lib::repo::Repo as _;
+use jj_lib::revset::RevsetIteratorExt as _;
 use tracing::instrument;
 
 use crate::cli_util::CommandHelper;
@@ -138,19 +139,23 @@ pub(crate) fn cmd_describe(
         )?;
     }
     let mut workspace_command = command.workspace_helper(ui)?;
-    let commits: Vec<_> = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
+    let target_expr = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
         workspace_command
             .parse_union_revsets(ui, &[&*args.revisions_pos, &*args.revisions_opt].concat())?
     } else {
         workspace_command.parse_revset(ui, &RevisionArg::AT)?
     }
-    .evaluate_to_commits()?
-    .try_collect()?; // in reverse topological order
+    .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let commits: Vec<_> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
+        .commits(workspace_command.repo().store()) // in reverse topological order
+        .try_collect()?;
     if commits.is_empty() {
         writeln!(ui.status(), "No revisions to describe.")?;
         return Ok(());
     }
-    workspace_command.check_rewritable(commits.iter().ids())?;
     let text_editor = workspace_command.text_editor()?;
 
     let mut tx = workspace_command.start_transaction();

cli/src/commands/fix.rs

@@ -134,15 +134,18 @@ pub(crate) fn cmd_fix(
     let workspace_root = workspace_command.workspace_root().to_owned();
     let path_converter = workspace_command.path_converter().to_owned();
     let tools_config = get_tools_config(ui, workspace_command.settings())?;
-    let root_commits: Vec<CommitId> = if args.source.is_empty() {
+    let target_expr = if args.source.is_empty() {
         let revs = workspace_command.settings().get_string("revsets.fix")?;
         workspace_command.parse_revset(ui, &RevisionArg::from(revs))?
     } else {
         workspace_command.parse_union_revsets(ui, &args.source)?
     }
-    .evaluate_to_commit_ids()?
-    .try_collect()?;
-    workspace_command.check_rewritable(root_commits.iter())?;
+    .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let root_commits: Vec<CommitId> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
+        .try_collect()?;
     let matcher = workspace_command
         .parse_file_patterns(ui, &args.paths)?
         .to_matcher();

cli/src/commands/gerrit/upload.rs

@@ -167,17 +167,19 @@ pub fn cmd_gerrit_upload(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
 
-    let revisions: Vec<_> = workspace_command
+    let target_expr = workspace_command
         .parse_union_revsets(ui, &args.revisions)?
-        .evaluate_to_commit_ids()?
+        .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let revisions: Vec<_> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
         .try_collect()?;
     if revisions.is_empty() {
         writeln!(ui.status(), "No revisions to upload.")?;
         return Ok(());
     }
 
-    workspace_command.check_rewritable(revisions.iter())?;
-
     // If you have the changes main -> A -> B, and then run `jj gerrit upload -r B`,
     // then that uploads both A and B. Thus, we need to ensure that A also
     // has a Change-ID.

cli/src/commands/metaedit.rs

@@ -122,19 +122,22 @@ pub(crate) fn cmd_metaedit(
     args: &MetaeditArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let commit_ids: Vec<_> = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
+    let target_expr = if !args.revisions_pos.is_empty() || !args.revisions_opt.is_empty() {
         workspace_command
             .parse_union_revsets(ui, &[&*args.revisions_pos, &*args.revisions_opt].concat())?
     } else {
         workspace_command.parse_revset(ui, &RevisionArg::AT)?
     }
-    .evaluate_to_commit_ids()?
-    .try_collect()?;
+    .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let commit_ids: Vec<_> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
+        .try_collect()?;
     if commit_ids.is_empty() {
         writeln!(ui.status(), "No revisions to modify.")?;
         return Ok(());
     }
-    workspace_command.check_rewritable(commit_ids.iter())?;
 
     let mut tx = workspace_command.start_transaction();
     let tx_description = match commit_ids.as_slice() {

cli/src/commands/rebase.rs

@@ -425,11 +425,14 @@ fn plan_rebase_revisions(
     revisions: &[RevisionArg],
     rebase_destination: &RebaseDestinationArgs,
 ) -> Result<MoveCommitsLocation, CommandError> {
-    let target_commit_ids: Vec<_> = workspace_command
+    let target_expr = workspace_command
         .parse_union_revsets(ui, revisions)?
-        .evaluate_to_commit_ids()?
+        .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let target_commit_ids: Vec<_> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
         .try_collect()?; // in reverse topological order
-    workspace_command.check_rewritable(&target_commit_ids)?;
 
     let (new_parent_ids, new_child_ids) = compute_commit_location(
         ui,
@@ -519,12 +522,12 @@ fn plan_rebase_branch(
     let roots_expression = RevsetExpression::commits(new_parent_ids.clone())
         .range(&RevsetExpression::commits(branch_commit_ids))
         .roots();
+    workspace_command.check_rewritable_expr(&roots_expression)?;
     let root_commit_ids: Vec<_> = roots_expression
         .evaluate(workspace_command.repo().as_ref())
         .unwrap()
         .iter()
         .try_collect()?;
-    workspace_command.check_rewritable(&root_commit_ids)?;
     if rebase_destination.destination.is_some() && new_child_ids.is_empty() {
         for id in &root_commit_ids {
             let commit = workspace_command.repo().store().get_commit(id)?;

cli/src/commands/sign.rs

@@ -18,6 +18,7 @@ use itertools::Itertools as _;
 use jj_lib::commit::Commit;
 use jj_lib::commit::CommitIteratorExt as _;
 use jj_lib::repo::Repo as _;
+use jj_lib::revset::RevsetIteratorExt as _;
 use jj_lib::signing::SignBehavior;
 
 use crate::cli_util::CommandHelper;
@@ -76,11 +77,16 @@ pub fn cmd_sign(ui: &mut Ui, command: &CommandHelper, args: &SignArgs) -> Result
         workspace_command.parse_revset(ui, &RevisionArg::from(revset_string))?
     } else {
         workspace_command.parse_union_revsets(ui, &args.revisions)?
-    };
+    }
+    .resolve()?;
 
-    let to_sign: IndexSet<Commit> = revset_expression.evaluate_to_commits()?.try_collect()?;
+    workspace_command.check_rewritable_expr(&revset_expression)?;
 
-    workspace_command.check_rewritable(to_sign.iter().ids())?;
+    let to_sign: IndexSet<Commit> = revset_expression
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
+        .commits(workspace_command.repo().store())
+        .try_collect()?;
 
     let mut tx = workspace_command.start_transaction();
 

cli/src/commands/simplify_parents.rs

@@ -56,24 +56,23 @@ pub(crate) fn cmd_simplify_parents(
             .get_string("revsets.simplify-parents")?;
         workspace_command
             .parse_revset(ui, &RevisionArg::from(revs))?
-            .expression()
-            .clone()
+            .resolve()?
     } else {
         workspace_command
             .parse_union_revsets(ui, &args.source)?
-            .expression()
+            .resolve()?
             .descendants()
             .union(
-                workspace_command
+                &workspace_command
                     .parse_union_revsets(ui, &args.revisions)?
-                    .expression(),
+                    .resolve()?,
             )
     };
-    let commit_ids: Vec<_> = workspace_command
-        .attach_revset_evaluator(revs)
-        .evaluate_to_commit_ids()?
+    workspace_command.check_rewritable_expr(&revs)?;
+    let commit_ids: Vec<_> = revs
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
         .try_collect()?;
-    workspace_command.check_rewritable(&commit_ids)?;
     let commit_ids_set: HashSet<_> = commit_ids.iter().cloned().collect();
     let num_orig_commits = commit_ids.len();
 

cli/src/commands/unsign.rs

@@ -17,6 +17,8 @@ use indexmap::IndexSet;
 use itertools::Itertools as _;
 use jj_lib::commit::Commit;
 use jj_lib::commit::CommitIteratorExt as _;
+use jj_lib::repo::Repo as _;
+use jj_lib::revset::RevsetIteratorExt as _;
 use jj_lib::signing::SignBehavior;
 
 use crate::cli_util::CommandHelper;
@@ -50,13 +52,16 @@ pub fn cmd_unsign(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
 
-    let commits: IndexSet<Commit> = workspace_command
+    let target_expr = workspace_command
         .parse_union_revsets(ui, &args.revisions)?
-        .evaluate_to_commits()?
+        .resolve()?;
+    workspace_command.check_rewritable_expr(&target_expr)?;
+    let commits: IndexSet<Commit> = target_expr
+        .evaluate(workspace_command.repo().as_ref())?
+        .iter()
+        .commits(workspace_command.repo().store())
         .try_collect()?;
 
-    workspace_command.check_rewritable(commits.iter().ids())?;
-
     let to_unsign: IndexSet<Commit> = commits
         .into_iter()
         .filter(|commit| commit.is_signed())

cli/tests/test_immutable_commits.rs

@@ -609,3 +609,23 @@ fn test_rewrite_immutable_commands() {
     [exit status: 1]
     "#);
 }
+
+#[test]
+fn test_immutable_log() {
+    let test_env = TestEnvironment::default();
+    test_env.run_jj_in(".", ["git", "init", "repo"]).success();
+    let work_dir = test_env.work_dir("repo");
+    work_dir.run_jj(["new"]).success();
+    test_env.add_config(r#"revset-aliases."immutable_heads()" = "@-""#);
+    // The immutable commits should be indicated in the graph even with
+    // `--ignore-immutable`
+    let output = work_dir.run_jj(["log", "--ignore-immutable"]);
+    insta::assert_snapshot!(output, @r"
+    @  rlvkpnrz [email protected] 2001-02-03 08:05:08 43444d88
+    │  (empty) (no description set)
+    ◆  qpvuntsm [email protected] 2001-02-03 08:05:07 e8849ae1
+    │  (empty) (no description set)
+    ◆  zzzzzzzz root() 00000000
+    [EOF]
+    ");
+}