cli: apply log --limit before --reversed

CHANGELOG.md

@@ -57,6 +57,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * `jj git remote add`/`set-url` now converts relative Git remote path to
   absolute path.
 
+* `jj log`/`op log` now applies `-n`/`--limit` *before* the items are reversed.
+  Rationale: It's more useful to see the N most recent commits/operations, and
+  is more performant. The old behavior can be achieved by `jj log .. | head`.
+  [#5403](https://github.com/jj-vcs/jj/issues/5403)
+
 * Upgraded `scm-record` from v0.4.0 to v0.5.0. See release notes at
   <https://github.com/arxanas/scm-record/releases/tag/v0.5.0>.
 

cli/src/commands/evolog.rs

@@ -53,6 +53,9 @@ pub(crate) struct EvologArgs {
     )]
     revision: RevisionArg,
     /// Limit number of revisions to show
+    ///
+    /// Applied after revisions are reordered topologically, but before being
+    /// reversed.
     #[arg(long, short = 'n')]
     limit: Option<usize>,
     /// Show revisions in the opposite order (older revisions first)

cli/src/commands/log.rs

@@ -14,6 +14,7 @@
 
 use clap_complete::ArgValueCandidates;
 use clap_complete::ArgValueCompleter;
+use itertools::Itertools as _;
 use jj_lib::backend::CommitId;
 use jj_lib::config::ConfigGetError;
 use jj_lib::config::ConfigGetResultExt as _;
@@ -77,14 +78,15 @@ pub(crate) struct LogArgs {
         add = ArgValueCompleter::new(complete::log_files),
     )]
     paths: Vec<String>,
-    /// Show revisions in the opposite order (older revisions first)
-    #[arg(long)]
-    reversed: bool,
     /// Limit number of revisions to show
     ///
-    /// Applied after revisions are filtered and reordered.
+    /// Applied after revisions are filtered and reordered topologically, but
+    /// before being reversed.
     #[arg(long, short = 'n')]
     limit: Option<usize>,
+    /// Show revisions in the opposite order (older revisions first)
+    #[arg(long)]
+    reversed: bool,
     /// Don't show the graph, show a flat list of revisions
     #[arg(long)]
     no_graph: bool,
@@ -177,8 +179,6 @@ pub(crate) fn cmd_log(
         let mut formatter = ui.stdout_formatter();
         let formatter = formatter.as_mut();
 
-        let limit = args.limit.unwrap_or(usize::MAX);
-
         if !args.no_graph {
             let mut raw_output = formatter.raw()?;
             let mut graph = get_graphlog(graph_style, raw_output.as_mut());
@@ -192,13 +192,16 @@ pub(crate) fn cmd_log(
                         forward_iter.prioritize_branch(id.clone());
                     }
                 }
+                // The input to TopoGroupedGraphIterator shouldn't be truncated
+                // because the prioritized commit must exist in the input set.
+                let forward_iter = forward_iter.take(args.limit.unwrap_or(usize::MAX));
                 if args.reversed {
                     Box::new(reverse_graph(forward_iter, |id| id)?.into_iter().map(Ok))
                 } else {
                     Box::new(forward_iter)
                 }
             };
-            for node in iter.take(limit) {
+            for node in iter {
                 let (commit_id, edges) = node?;
 
                 // The graph is keyed by (CommitId, is_synthetic)
@@ -278,13 +281,16 @@ pub(crate) fn cmd_log(
                 }
             }
         } else {
-            let iter: Box<dyn Iterator<Item = Result<CommitId, RevsetEvaluationError>>> =
+            let iter: Box<dyn Iterator<Item = Result<CommitId, RevsetEvaluationError>>> = {
+                let forward_iter = revset.iter().take(args.limit.unwrap_or(usize::MAX));
                 if args.reversed {
-                    Box::new(revset.iter().reversed()?)
+                    let entries: Vec<_> = forward_iter.try_collect()?;
+                    Box::new(entries.into_iter().rev().map(Ok))
                 } else {
-                    Box::new(revset.iter())
-                };
-            for commit_or_error in iter.commits(store).take(limit) {
+                    Box::new(forward_iter)
+                }
+            };
+            for commit_or_error in iter.commits(store) {
                 let commit = commit_or_error?;
                 with_content_format
                     .write(formatter, |formatter| template.format(&commit, formatter))?;

cli/src/commands/operation/log.rs

@@ -50,7 +50,8 @@ use crate::ui::Ui;
 pub struct OperationLogArgs {
     /// Limit number of operations to show
     ///
-    /// Applied after operations are reordered.
+    /// Applied after operations are reordered topologically, but before being
+    /// reversed.
     #[arg(long, short = 'n')]
     limit: Option<usize>,
     /// Show operations in the opposite order (older operations first)
@@ -196,8 +197,8 @@ fn do_op_log(
     ui.request_pager();
     let mut formatter = ui.stdout_formatter();
     let formatter = formatter.as_mut();
-    let limit = args.limit.unwrap_or(usize::MAX);
-    let iter = op_walk::walk_ancestors(slice::from_ref(current_op));
+    let iter =
+        op_walk::walk_ancestors(slice::from_ref(current_op)).take(args.limit.unwrap_or(usize::MAX));
 
     if !args.no_graph {
         let mut raw_output = formatter.raw()?;
@@ -213,7 +214,7 @@ fn do_op_log(
         } else {
             Box::new(iter)
         };
-        for node in iter_nodes.take(limit) {
+        for node in iter_nodes {
             let (op, edges) = node?;
             let mut buffer = vec![];
             let within_graph = with_content_format.sub_width(graph.width(op.id(), &edges));
@@ -241,7 +242,7 @@ fn do_op_log(
         } else {
             Box::new(iter)
         };
-        for op in iter.take(limit) {
+        for op in iter {
             let op = op?;
             with_content_format.write(formatter, |formatter| template.format(&op, formatter))?;
             if let Some(show) = &maybe_show_op_diff {

cli/tests/cli-reference@.md.snap

@@ -805,6 +805,8 @@ Lists the previous commits which a change has pointed to. The current commit of
 
   Default value: `@`
 * `-n`, `--limit <LIMIT>` — Limit number of revisions to show
+
+   Applied after revisions are reordered topologically, but before being reversed.
 * `--reversed` — Show revisions in the opposite order (older revisions first)
 * `--no-graph` — Don't show the graph, show a flat list of revisions
 * `-T`, `--template <TEMPLATE>` — Render each revision using the given template
@@ -1382,10 +1384,10 @@ The working-copy commit is indicated by a `@` symbol in the graph. Immutable rev
 * `-r`, `--revisions <REVSETS>` — Which revisions to show
 
    If no paths nor revisions are specified, this defaults to the `revsets.log` setting.
-* `--reversed` — Show revisions in the opposite order (older revisions first)
 * `-n`, `--limit <LIMIT>` — Limit number of revisions to show
 
-   Applied after revisions are filtered and reordered.
+   Applied after revisions are filtered and reordered topologically, but before being reversed.
+* `--reversed` — Show revisions in the opposite order (older revisions first)
 * `--no-graph` — Don't show the graph, show a flat list of revisions
 * `-T`, `--template <TEMPLATE>` — Render each revision using the given template
 
@@ -1571,7 +1573,7 @@ Like other commands, `jj op log` snapshots the current working-copy changes and
 
 * `-n`, `--limit <LIMIT>` — Limit number of operations to show
 
-   Applied after operations are reordered.
+   Applied after operations are reordered topologically, but before being reversed.
 * `--reversed` — Show operations in the opposite order (older operations first)
 * `--no-graph` — Don't show the graph, show a flat list of operations
 * `-T`, `--template <TEMPLATE>` — Render each operation using the given template

cli/tests/test_evolog_command.rs

@@ -370,6 +370,17 @@ fn test_evolog_reversed_no_graph() {
     qpvuntsm [email protected] 2001-02-03 08:05:10 5cb22a87
     (empty) c
     ");
+
+    let stdout = test_env.jj_cmd_success(
+        &repo_path,
+        &["evolog", "--limit=2", "--reversed", "--no-graph"],
+    );
+    insta::assert_snapshot!(stdout, @r"
+    qpvuntsm hidden [email protected] 2001-02-03 08:05:09 b4584f54
+    (empty) b
+    qpvuntsm [email protected] 2001-02-03 08:05:10 5cb22a87
+    (empty) c
+    ");
 }
 
 #[test]
@@ -415,4 +426,17 @@ fn test_evolog_reverse_with_graph() {
     ○  qpvuntsm [email protected] 2001-02-03 08:05:13 a177c2f2
        (empty) c+d+e
     ");
+
+    let stdout = test_env.jj_cmd_success(
+        &repo_path,
+        &["evolog", "-rdescription(c+d+e)", "--limit=3", "--reversed"],
+    );
+    insta::assert_snapshot!(stdout, @r"
+    ○  mzvwutvl hidden [email protected] 2001-02-03 08:05:11 280cbb6e
+    │  (empty) d
+    │ ○  royxmykx hidden [email protected] 2001-02-03 08:05:12 031df638
+    ├─╯  (empty) e
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:13 a177c2f2
+       (empty) c+d+e
+    ");
 }

cli/tests/test_log_command.rs

@@ -968,17 +968,18 @@ fn test_log_limit() {
     c
     "###);
 
-    // Applied on reversed DAG
+    // Applied on reversed DAG: Because the node "a" is omitted, "b" and "c" are
+    // rendered as roots.
     let stdout = test_env.jj_cmd_success(
         &repo_path,
         &["log", "-T", "description", "--limit=3", "--reversed"],
     );
-    insta::assert_snapshot!(stdout, @r###"
-    ◆
-    ○    a
-    ├─╮
-    │ ○  c
-    "###);
+    insta::assert_snapshot!(stdout, @r"
+    ○  c
+    │ ○  b
+    ├─╯
+    @  d
+    ");
     let stdout = test_env.jj_cmd_success(
         &repo_path,
         &[
@@ -990,10 +991,11 @@ fn test_log_limit() {
             "--no-graph",
         ],
     );
-    insta::assert_snapshot!(stdout, @r###"
-    a
+    insta::assert_snapshot!(stdout, @r"
     b
-    "###);
+    c
+    d
+    ");
 
     // Applied on filtered commits
     let stdout = test_env.jj_cmd_success(

cli/tests/test_operations.rs

@@ -250,12 +250,32 @@ fn test_op_log_reversed() {
     "#);
 
     // Should work correctly with `--limit`
-    let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log", "--reversed", "--limit=2"]);
-    insta::assert_snapshot!(stdout, @r#"
-    ○  000000000000 root()
-    ○    eac759b9ab75 [email protected] 2001-02-03 04:05:07.000 +07:00 - 2001-02-03 04:05:07.000 +07:00
-    ├─╮  add workspace 'default'
-    "#);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log", "--reversed", "--limit=3"]);
+    insta::assert_snapshot!(stdout, @r"
+    ○  8e3e726be123 [email protected] 2001-02-03 04:05:10.000 +07:00 - 2001-02-03 04:05:10.000 +07:00
+    │  describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    │  args: jj describe -m 'description 1' --at-op @-
+    │ ○  d009cfc04993 [email protected] 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00
+    ├─╯  describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    │    args: jj describe -m 'description 0'
+    @  e4538ffdc13d [email protected] 2001-02-03 04:05:11.000 +07:00 - 2001-02-03 04:05:11.000 +07:00
+       reconcile divergent operations
+       args: jj op log --reversed
+    ");
+
+    // Should work correctly with `--limit` and `--no-graph`
+    let stdout = test_env.jj_cmd_success(
+        &repo_path,
+        &["op", "log", "--reversed", "--limit=2", "--no-graph"],
+    );
+    insta::assert_snapshot!(stdout, @r"
+    d009cfc04993 [email protected] 2001-02-03 04:05:08.000 +07:00 - 2001-02-03 04:05:08.000 +07:00
+    describe commit 230dd059e1b059aefc0da06a2e5a7dbf22362f22
+    args: jj describe -m 'description 0'
+    e4538ffdc13d [email protected] 2001-02-03 04:05:11.000 +07:00 - 2001-02-03 04:05:11.000 +07:00
+    reconcile divergent operations
+    args: jj op log --reversed
+    ");
 }
 
 #[test]

lib/src/revset.rs

@@ -2566,7 +2566,6 @@ pub type RevsetContainingFn<'a> = dyn Fn(&CommitId) -> Result<bool, RevsetEvalua
 
 pub trait RevsetIteratorExt<'index, I> {
     fn commits(self, store: &Arc<Store>) -> RevsetCommitIterator<I>;
-    fn reversed(self) -> Result<ReverseRevsetIterator, RevsetEvaluationError>;
 }
 
 impl<I: Iterator<Item = Result<CommitId, RevsetEvaluationError>>> RevsetIteratorExt<'_, I> for I {
@@ -2576,11 +2575,6 @@ impl<I: Iterator<Item = Result<CommitId, RevsetEvaluationError>>> RevsetIterator
             store: store.clone(),
         }
     }
-
-    fn reversed(self) -> Result<ReverseRevsetIterator, RevsetEvaluationError> {
-        let entries: Vec<_> = self.into_iter().try_collect()?;
-        Ok(ReverseRevsetIterator { entries })
-    }
 }
 
 pub struct RevsetCommitIterator<I> {
@@ -2603,18 +2597,6 @@ impl<I: Iterator<Item = Result<CommitId, RevsetEvaluationError>>> Iterator
     }
 }
 
-pub struct ReverseRevsetIterator {
-    entries: Vec<CommitId>,
-}
-
-impl Iterator for ReverseRevsetIterator {
-    type Item = Result<CommitId, RevsetEvaluationError>;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        self.entries.pop().map(Ok)
-    }
-}
-
 /// A set of extensions for revset evaluation.
 pub struct RevsetExtensions {
     symbol_resolvers: Vec<Box<dyn SymbolResolverExtension>>,