From 6121166b1495566c8c7970e249b83bceff0b984a 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 | 26 ++- cli/tests/test_next_prev_commands.rs | 300 +++++++++++++++++++++++++++ 2 files changed, 317 insertions(+), 9 deletions(-) diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index da66fe82bb..b952bdaf27 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -121,17 +121,25 @@ impl Direction { start_revset: &Rc, args: &MovementArgsInternal, ) -> Result, CommandError> { - let target_revset = match (self, args.conflict) { - (Direction::Next, true) => start_revset + let target_revset = match (self, args.should_edit, args.conflict) { + (Direction::Next, true, false) => start_revset.descendants_at(args.offset), + (Direction::Next, true, true) => start_revset + .descendants_at(args.offset) + .descendants() + .filtered(RevsetFilterPredicate::HasConflict) + .roots(), + (Direction::Next, false, false) => start_revset .children() + .minus(working_revset) + .descendants_at(args.offset - 1), + (Direction::Next, false, true) => start_revset + .children() + .minus(working_revset) + .descendants_at(args.offset - 1) .descendants() .filtered(RevsetFilterPredicate::HasConflict) - .roots() - .minus(working_revset), - (Direction::Next, false) => start_revset - .descendants_at(args.offset) - .minus(working_revset), - (Direction::Prev, true) => + .roots(), + (Direction::Prev, _, true) => // If people desire to move to the root conflict, replace the `heads()` below // with `roots(). But let's wait for feedback. { @@ -141,7 +149,7 @@ impl Direction { .filtered(RevsetFilterPredicate::HasConflict) .heads() } - (Direction::Prev, false) => start_revset.ancestors_at(args.offset), + (Direction::Prev, _, false) => start_revset.ancestors_at(args.offset), }; Ok(target_revset) diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 84d51b7bc5..48d3f50de4 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -1112,6 +1112,306 @@ 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + test_env.jj_cmd_ok(&repo_path, &["next", "4"]); + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + 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 + "###); + + test_env.jj_cmd_ok(&repo_path, &["edit", "description(first)"]); + 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.jj_cmd_ok(&repo_path, &["next", "4"]); + 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])