From 7f6e5770dcb396142cec67ef8286512fad4a73c4 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 20 Jun 2024 17:07:01 -0700 Subject: [PATCH 1/5] next/prev: include graph in `--conflict` tests, highlighting bugs There are several bugs in both the tests and in the implementation that are made more clear by showing the log output before and after running the command. --- cli/tests/test_next_prev_commands.rs | 102 +++++++++++++++++++-------- 1 file changed, 72 insertions(+), 30 deletions(-) diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 7ddba11509..77c1ad6ad5 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -584,14 +584,25 @@ 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"]); + // TODO: We now should be a child of `third`. + 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,14 +621,21 @@ 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 "###); } @@ -638,13 +656,25 @@ fn test_next_conflict() { test_env.jj_cmd_ok(&repo_path, &["new", "description(second)"]); 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 the setup + // TODO: This test doesn't seem to test what it's supposed to (we're already on + // the second commit) + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ yqosqzytrlsw conflict + │ ◉ mzvwutvlkqwt conflict third + ├─╯ + ◉ rlvkpnrzqnoo conflict second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz + "###); + test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ yostqsxwqrlt conflict + │ ◉ mzvwutvlkqwt conflict third + ├─╯ + ◉ rlvkpnrzqnoo conflict second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz "###); } @@ -664,18 +694,30 @@ fn test_next_conflict_editing() { 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"]); + // Test the setup + // TODO: This test doesn't seem to test what it's supposed to (we're already on + // top of the third commit) + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ mzvwutvlkqwt conflict + ◉ kkmpptxzrspx conflict + ◉ rlvkpnrzqnoo second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz + "###); + // TODO: The command should be an error since there is no conflict after the + // current one + test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]); // We now should be editing the third commit. - insta::assert_snapshot!(stdout, @""); - 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 + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ yqosqzytrlsw conflict + ◉ kkmpptxzrspx conflict + ◉ rlvkpnrzqnoo second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz "###); } 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]) } From ce9699a7742d43ce59ee629c23859e72f8b83483 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 20 Jun 2024 17:51:17 -0700 Subject: [PATCH 2/5] next: add a test for `jj next --edit` on a head --- cli/tests/test_next_prev_commands.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 77c1ad6ad5..dd91e0460f 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -704,8 +704,6 @@ fn test_next_conflict_editing() { ◉ qpvuntsmwlqt first ◉ zzzzzzzzzzzz "###); - // TODO: The command should be an error since there is no conflict after the - // current one test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]); // We now should be editing the third commit. insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @@ -717,6 +715,30 @@ fn test_next_conflict_editing() { "###); } +#[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 + "###); + // TODO: The command should be an error since there is no conflict after the + // current one + test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]); + // TODO: The command should be an error since there is no conflict after the + // current one + test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]); +} + fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { let template = r#"separate(" ", change_id.short(), local_branches, if(conflict, "conflict"), description)"#; test_env.jj_cmd_success(cwd, &["log", "-T", template]) From 6ac0ac2f4fa7a04370adb63b673a2a869a5763fc Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 20 Jun 2024 19:09:11 -0700 Subject: [PATCH 3/5] next: make test cases test what they were supposed to --- cli/tests/test_next_prev_commands.rs | 40 +++++++++++----------------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index dd91e0460f..05a4a45ae8 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -641,38 +641,35 @@ fn test_prev_conflict_editing() { #[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)"]); + 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 - // TODO: This test doesn't seem to test what it's supposed to (we're already on - // the second commit) insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ yqosqzytrlsw conflict - │ ◉ mzvwutvlkqwt conflict third + @ royxmykxtrkr + │ ◉ kkmpptxzrspx conflict third + │ ◉ rlvkpnrzqnoo second ├─╯ - ◉ rlvkpnrzqnoo conflict second ◉ qpvuntsmwlqt first ◉ zzzzzzzzzzzz "###); test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ yostqsxwqrlt conflict - │ ◉ mzvwutvlkqwt conflict third - ├─╯ - ◉ rlvkpnrzqnoo conflict second + @ vruxwmqvtpmx conflict + ◉ kkmpptxzrspx conflict third + ◉ rlvkpnrzqnoo second ◉ qpvuntsmwlqt first ◉ zzzzzzzzzzzz "###); @@ -693,22 +690,17 @@ 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", "@+"]); + test_env.jj_cmd_ok(&repo_path, &["edit", "@-"]); // Test the setup - // TODO: This test doesn't seem to test what it's supposed to (we're already on - // top of the third commit) insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ mzvwutvlkqwt conflict ◉ kkmpptxzrspx conflict ◉ rlvkpnrzqnoo second - ◉ qpvuntsmwlqt first + @ qpvuntsmwlqt first ◉ zzzzzzzzzzzz "###); test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]); - // We now should be editing the third commit. insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ yqosqzytrlsw conflict - ◉ kkmpptxzrspx conflict + @ kkmpptxzrspx conflict ◉ rlvkpnrzqnoo second ◉ qpvuntsmwlqt first ◉ zzzzzzzzzzzz From 70fd2c2b409cfdc4f470d4f0fba29808703767f0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 20 Jun 2024 20:05:57 -0700 Subject: [PATCH 4/5] next/prev: fix a few bugs in `--conflict` --- cli/src/commands/next.rs | 6 ++++-- cli/src/commands/prev.rs | 3 +-- cli/tests/test_next_prev_commands.rs | 17 +++++++++-------- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 31fc22fc5b..6a78a790b6 100644 --- a/cli/src/commands/next.rs +++ b/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 = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 52c71e625e..f5905cce45 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -90,10 +90,9 @@ pub(crate) fn cmd_prev( // If people desire to move to the root conflict, replace the `heads()` below // with `roots(). But let's wait for feedback. target_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) diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 05a4a45ae8..00d674f41c 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -594,12 +594,11 @@ fn test_prev_conflict() { ◉ zzzzzzzzzzzz "###); test_env.jj_cmd_ok(&repo_path, &["prev", "--conflict"]); - // TODO: We now should be a child of `third`. insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ yostqsxwqrlt conflict │ ◉ royxmykxtrkr conflict fourth - │ ◉ kkmpptxzrspx conflict third ├─╯ + ◉ kkmpptxzrspx conflict third ◉ rlvkpnrzqnoo conflict second ◉ qpvuntsmwlqt first ◉ zzzzzzzzzzzz @@ -723,12 +722,14 @@ fn test_next_conflict_head() { @ rlvkpnrzqnoo conflict ◉ zzzzzzzzzzzz "###); - // TODO: The command should be an error since there is no conflict after the - // current one - test_env.jj_cmd_ok(&repo_path, &["next", "--conflict"]); - // TODO: The command should be an error since there is no conflict after the - // current one - test_env.jj_cmd_ok(&repo_path, &["next", "--conflict", "--edit"]); + 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###" + Error: No descendant found 1 commit forward + "###); } fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { From b64741957d20237f26c9f56170ce249b4aa5ed69 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 20 Jun 2024 20:44:13 -0700 Subject: [PATCH 5/5] prev: make revset code more similar to `next` --- cli/src/commands/prev.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index f5905cce45..659f116e6c 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -79,23 +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) .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())?