next/prev: fix a few bugs in --conflict

cli/src/commands/next.rs

@@ -128,12 +128,14 @@ pub(crate) fn cmd_next(
 
     let target_revset = if args.conflict {
         start_revset
+            .children()
             .descendants()
             .filtered(RevsetFilterPredicate::HasConflict)
             .roots()
     } else {
-        start_revset.descendants_at(args.offset).minus(&wc_revset)
-    };
+        start_revset.descendants_at(args.offset)
+    }
+    .minus(&wc_revset);
 
     let targets: Vec<Commit> = target_revset
         .evaluate_programmatic(workspace_command.repo().as_ref())?

cli/src/commands/prev.rs

@@ -79,24 +79,25 @@ pub(crate) fn cmd_prev(
             .view()
             .heads()
             .contains(current_wc_id);
+    let wc_revset = RevsetExpression::commit(current_wc_id.clone());
     // If we're editing, start at the working-copy commit. Otherwise, start from
     // its direct parent(s).
-    let target_revset = if edit {
-        RevsetExpression::commit(current_wc_id.clone())
+    let start_revset = if edit {
+        wc_revset.clone()
     } else {
-        RevsetExpression::commit(current_wc_id.clone()).parents()
+        wc_revset.parents()
     };
+
     let target_revset = if args.conflict {
         // If people desire to move to the root conflict, replace the `heads()` below
         // with `roots(). But let's wait for feedback.
-        target_revset
+        start_revset
+            .parents()
             .ancestors()
             .filtered(RevsetFilterPredicate::HasConflict)
-            // We need to filter out empty commits to not land on empty working-copies lying around.
-            .minus(&RevsetExpression::is_empty())
             .heads()
     } else {
-        target_revset.ancestors_at(args.offset)
+        start_revset.ancestors_at(args.offset)
     };
     let targets: Vec<_> = target_revset
         .evaluate_programmatic(workspace_command.repo().as_ref())?

cli/tests/test_next_prev_commands.rs

@@ -584,14 +584,24 @@ fn test_prev_conflict() {
     std::fs::write(&file_path, "first+1").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["new", "description(third)"]);
     test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "fourth"]);
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
-    // We now should be a child of `fourth`.
-    insta::assert_snapshot!(stdout, @"");
-    insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: vruxwmqv b1ea981a (conflict) (empty) (no description set)
-    Parent commit      : rlvkpnrz c26675ba (conflict) second
-    There are unresolved conflicts at these paths:
-    content.txt    2-sided conflict
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  yqosqzytrlsw conflict
+    ◉  royxmykxtrkr conflict fourth
+    ◉  kkmpptxzrspx conflict third
+    ◉  rlvkpnrzqnoo conflict second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  yostqsxwqrlt conflict
+    │ ◉  royxmykxtrkr conflict fourth
+    ├─╯
+    ◉  kkmpptxzrspx conflict third
+    ◉  rlvkpnrzqnoo conflict second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
     "###);
 }
 
@@ -610,41 +620,57 @@ fn test_prev_conflict_editing() {
     test_env.jj_cmd_ok(&repo_path, &["edit", "description(first)"]);
     std::fs::write(&file_path, "first text").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["new", "description(third)"]);
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict", "--edit"]);
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  royxmykxtrkr conflict
+    ◉  kkmpptxzrspx conflict third
+    ◉  rlvkpnrzqnoo second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict", "--edit"]);
     // We now should be editing the third commit.
-    insta::assert_snapshot!(stdout, @"");
-    insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: kkmpptxz 26b1439f (conflict) third
-    Parent commit      : rlvkpnrz 55b5d11a (empty) second
-    There are unresolved conflicts at these paths:
-    content.txt    2-sided conflict
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  kkmpptxzrspx conflict third
+    ◉  rlvkpnrzqnoo second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
     "###);
 }
 
 #[test]
 fn test_next_conflict() {
-    // There is a conflict in the second commit, so after next it should be the new
+    // There is a conflict in the third commit, so after next it should be the new
     // parent.
     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 file_path = repo_path.join("content.txt");
+    std::fs::write(&file_path, "first").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]);
-    std::fs::write(&file_path, "second").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]);
-    // Create a conflict in the second commit.
-    test_env.jj_cmd_ok(&repo_path, &["edit", "description(first)"]);
-    std::fs::write(&file_path, "first").unwrap();
-    test_env.jj_cmd_ok(&repo_path, &["new", "description(second)"]);
+    // Create a conflict in the third commit.
+    std::fs::write(&file_path, "third").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]);
-    test_env.jj_cmd_ok(&repo_path, &["new", "description(second)"]);
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
-    insta::assert_snapshot!(stdout, @"");
-    insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: vruxwmqv b69eca51 (conflict) (empty) (no description set)
-    Parent commit      : rlvkpnrz fa43d820 (conflict) second
-    There are unresolved conflicts at these paths:
-    content.txt    2-sided conflict
+    test_env.jj_cmd_ok(&repo_path, &["new", "description(first)"]);
+    std::fs::write(&file_path, "first v2").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["squash", "--into", "description(third)"]);
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  royxmykxtrkr
+    │ ◉  kkmpptxzrspx conflict third
+    │ ◉  rlvkpnrzqnoo second
+    ├─╯
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  vruxwmqvtpmx conflict
+    ◉  kkmpptxzrspx conflict third
+    ◉  rlvkpnrzqnoo second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
     "###);
 }
 
@@ -663,19 +689,50 @@ fn test_next_conflict_editing() {
     std::fs::write(&file_path, "third").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["edit", "description(second)"]);
     std::fs::write(&file_path, "modified second").unwrap();
-    test_env.jj_cmd_ok(&repo_path, &["new", "@+"]);
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]);
-    // We now should be editing the third commit.
-    insta::assert_snapshot!(stdout, @"");
+    test_env.jj_cmd_ok(&repo_path, &["edit", "@-"]);
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ◉  kkmpptxzrspx conflict
+    ◉  rlvkpnrzqnoo second
+    @  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  kkmpptxzrspx conflict
+    ◉  rlvkpnrzqnoo second
+    ◉  qpvuntsmwlqt first
+    ◉  zzzzzzzzzzzz
+    "###);
+}
+
+#[test]
+fn test_next_conflict_head() {
+    // When editing a head with conflicts, `jj next --conflict [--edit]` errors out.
+    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 file_path = repo_path.join("file");
+    std::fs::write(&file_path, "first").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["new"]);
+    std::fs::write(&file_path, "second").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["abandon", "@-"]);
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  rlvkpnrzqnoo conflict
+    ◉  zzzzzzzzzzzz
+    "###);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: No descendant found 1 commit forward
+    "###);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict", "--edit"]);
     insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: royxmykx 08fda952 (conflict) (empty) (no description set)
-    Parent commit      : kkmpptxz 69ff337c (conflict) (no description set)
-    There are unresolved conflicts at these paths:
-    content.txt    2-sided conflict
+    Error: No descendant found 1 commit forward
     "###);
 }
 
 fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
-    let template = r#"separate(" ", change_id.short(), local_branches, description)"#;
+    let template = r#"separate(" ", change_id.short(), local_branches, if(conflict, "conflict"), description)"#;
     test_env.jj_cmd_success(cwd, &["log", "-T", template])
 }