From 36d637b162cce36ad0714f0b545d4b9947e8c2e4 Mon Sep 17 00:00:00 2001
From: Essien Ita Essien <34972+essiene@users.noreply.github.com>
Date: Fri, 16 Aug 2024 01:10:44 +0100
Subject: [PATCH] Fetch descendants more correctly.

See context in [this discussion](https://github.com/martinvonz/jj/pull/3935#discussion_r1649520967)

Fixes #3947
---
 cli/src/movement_util.rs             |  32 +--
 cli/tests/test_next_prev_commands.rs | 399 +++++++++++++++++++++++++++
 2 files changed, 415 insertions(+), 16 deletions(-)

diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs
index da66fe82bb..ce197f7e6c 100644
--- a/cli/src/movement_util.rs
+++ b/cli/src/movement_util.rs
@@ -121,27 +121,27 @@ impl Direction {
         start_revset: &Rc<RevsetExpression>,
         args: &MovementArgsInternal,
     ) -> Result<Rc<RevsetExpression>, CommandError> {
-        let target_revset = match (self, args.conflict) {
-            (Direction::Next, true) => start_revset
+        let nth = match (self, args.should_edit) {
+            (Direction::Next, true) => start_revset.descendants_at(args.offset),
+            (Direction::Next, false) => start_revset
                 .children()
+                .minus(working_revset)
+                .descendants_at(args.offset - 1),
+            (Direction::Prev, _) => start_revset.ancestors_at(args.offset),
+        };
+
+        let target_revset = match (self, args.conflict) {
+            (_, false) => nth,
+            (Direction::Next, true) => nth
                 .descendants()
                 .filtered(RevsetFilterPredicate::HasConflict)
-                .roots()
-                .minus(working_revset),
-            (Direction::Next, false) => start_revset
-                .descendants_at(args.offset)
-                .minus(working_revset),
-            (Direction::Prev, true) =>
+                .roots(),
             // If people desire to move to the root conflict, replace the `heads()` below
             // with `roots(). But let's wait for feedback.
-            {
-                start_revset
-                    .parents()
-                    .ancestors()
-                    .filtered(RevsetFilterPredicate::HasConflict)
-                    .heads()
-            }
-            (Direction::Prev, false) => start_revset.ancestors_at(args.offset),
+            (Direction::Prev, true) => nth
+                .ancestors()
+                .filtered(RevsetFilterPredicate::HasConflict)
+                .heads(),
         };
 
         Ok(target_revset)
diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs
index 84d51b7bc5..07d9fd9128 100644
--- a/cli/tests/test_next_prev_commands.rs
+++ b/cli/tests/test_next_prev_commands.rs
@@ -1112,6 +1112,405 @@ fn test_movement_edit_mode_false() {
     "###);
 }
 
+#[test]
+fn test_movement_target_revset() {
+    let test_env = TestEnvironment::default();
+
+    test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+    let conflict_path = repo_path.join("conflict.txt");
+    let good_path = repo_path.join("good.txt");
+
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]);
+
+    std::fs::write(&conflict_path, "second").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]);
+
+    std::fs::write(&conflict_path, "third").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]);
+
+    std::fs::write(&conflict_path, "fourth").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "fourth"]);
+
+    std::fs::write(&conflict_path, "fifth").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "fifth"]);
+
+    std::fs::write(&conflict_path, "sixth").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "sixth"]);
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "seventh"]);
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "eight"]);
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "ninth"]);
+    test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "tenth"]);
+
+    // Create a conflict
+    test_env.jj_cmd_ok(&repo_path, &["edit", "description(fourth)"]);
+    std::fs::write(&conflict_path, "modified fourth").unwrap();
+
+    // Fix the conflict
+    test_env.jj_cmd_ok(&repo_path, &["edit", "description(seventh)"]);
+    std::fs::remove_file(&conflict_path).unwrap();
+    std::fs::write(&good_path, "good").unwrap();
+
+    test_env.jj_cmd_ok(&repo_path, &["edit", "description(tenth)"]);
+
+    // Confirm the final state of the tree.
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    test_env.add_config(r#"ui.movement.edit = false"#);
+
+    test_env.jj_cmd_ok(&repo_path, &["new", "description(second)"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  nkmrtpmomlro
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    │ ×  mzvwutvlkqwt conflict fifth
+    │ ○  zsuskulnrvyr fourth
+    │ ○  kkmpptxzrspx third
+    ├─╯
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=false, conflict=false, offset is implied (1)
+    test_env.jj_cmd_ok(&repo_path, &["next"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  xznxytknoqwo
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    │ ×  mzvwutvlkqwt conflict fifth
+    │ ○  zsuskulnrvyr fourth
+    ├─╯
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=false, offset is implied (1)
+    test_env.jj_cmd_ok(&repo_path, &["prev"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  nmzmmopxokps
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    │ ×  mzvwutvlkqwt conflict fifth
+    │ ○  zsuskulnrvyr fourth
+    │ ○  kkmpptxzrspx third
+    ├─╯
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=false, conflict=true, offset = ignored, wc no conflict
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  wvuyspvkupzz conflict
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    ├─╯
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=false, conflict=true, offset = ignored, wc conflict
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  pzsxstztnpkv conflict
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    ├─╯
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=false, offset is explicit
+    test_env.jj_cmd_ok(&repo_path, &["prev", "3"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  uuuvxpvwspwr
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    │ ×  mzvwutvlkqwt conflict fifth
+    │ ○  zsuskulnrvyr fourth
+    ├─╯
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=false, conflict=false, offset is explicit
+    test_env.jj_cmd_ok(&repo_path, &["next", "6"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  oupztwtkortx
+    │ ○  znkkpsqqskkl tenth
+    ├─╯
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=true, offset is ignored, conflict in ancestor
+    test_env.jj_cmd_ok(&repo_path, &["prev", "2"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  qwyusntzmkzw
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    ├─╯
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=true, offset is ignored, wc no conflict
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  zowrlwsvrkvk conflict
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    ├─╯
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=true, offset is ignored, wc conflict
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  rkoyqlrvkoto conflict
+    │ ○  znkkpsqqskkl tenth
+    │ ○  yostqsxwqrlt ninth
+    │ ○  vruxwmqvtpmx eight
+    │ ○  yqosqzytrlsw seventh
+    │ ×  royxmykxtrkr conflict sixth
+    ├─╯
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    test_env.add_config(r#"ui.movement.edit = true"#);
+
+    test_env.jj_cmd_ok(&repo_path, &["edit", "description(second)"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    @  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=true, conflict=false, offset is implied (1)
+    test_env.jj_cmd_ok(&repo_path, &["next"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    @  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=true, conflict=false, offset is implied (1)
+    test_env.jj_cmd_ok(&repo_path, &["prev"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    @  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=true, conflict=true, offset = ignored, wc no conflict
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    @  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=true, conflict=true, offset = ignored, wc conflict
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    @  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=false, offset is explicit
+    test_env.jj_cmd_ok(&repo_path, &["prev", "3"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    @  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // next, edit=true, conflict=false, offset is explicit
+    test_env.jj_cmd_ok(&repo_path, &["next", "6"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    @  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=true, conflict=true, offset is ignored, conflict in ancestor
+    test_env.jj_cmd_ok(&repo_path, &["prev", "2"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    @  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=true, conflict=true, offset is ignored, wc no conflict
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    @  royxmykxtrkr conflict sixth
+    ×  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+
+    // prev, edit=false, conflict=true, offset is ignored, wc conflict
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○  znkkpsqqskkl tenth
+    ○  yostqsxwqrlt ninth
+    ○  vruxwmqvtpmx eight
+    ○  yqosqzytrlsw seventh
+    ×  royxmykxtrkr conflict sixth
+    @  mzvwutvlkqwt conflict fifth
+    ○  zsuskulnrvyr fourth
+    ○  kkmpptxzrspx third
+    ○  rlvkpnrzqnoo second
+    ○  qpvuntsmwlqt first
+    ◆  zzzzzzzzzzzz
+    "###);
+}
+
 fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
     let template = r#"separate(" ", change_id.short(), local_bookmarks, if(conflict, "conflict"), description)"#;
     test_env.jj_cmd_success(cwd, &["log", "-T", template])