From 51be4620c65ac038f9b0f911c75e6c81ca1fd099 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Thu, 30 Nov 2023 22:37:16 -0800 Subject: [PATCH 1/2] test_abandon_command: show change ids in get_log_output Branches move around a little confusigly with `abandon`. We do want to keep them, to test their behavior, but we can show the change id to make things clearer. --- cli/tests/test_abandon_command.rs | 77 +++++++++++++++++-------------- 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/cli/tests/test_abandon_command.rs b/cli/tests/test_abandon_command.rs index 47bcf35c01..86a6f28b03 100644 --- a/cli/tests/test_abandon_command.rs +++ b/cli/tests/test_abandon_command.rs @@ -43,15 +43,15 @@ fn test_basics() { create_commit(&test_env, &repo_path, "e", &["a", "d"]); // Test the setup insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ e + @ [znk] e ├─╮ - │ ◉ d - │ ◉ c - │ │ ◉ b + │ ◉ [vru] d + │ ◉ [roy] c + │ │ ◉ [zsu] b ├───╯ - ◉ │ a + ◉ │ [rlv] a ├─╯ - ◉ + ◉ [zzz] "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "d"]); @@ -65,14 +65,14 @@ fn test_basics() { Added 0 files, modified 0 files, removed 1 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ e + @ [znk] e ├─╮ - │ ◉ c d - │ │ ◉ b + │ ◉ [roy] c d + │ │ ◉ [zsu] b ├───╯ - ◉ │ a + ◉ │ [rlv] a ├─╯ - ◉ + ◉ [zzz] "###); test_env.jj_cmd_ok(&repo_path, &["undo"]); @@ -85,14 +85,14 @@ fn test_basics() { Added 0 files, modified 0 files, removed 3 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ - │ ◉ b + @ [nkm] + │ ◉ [zsu] b ├─╯ - ◉ a e?? - │ ◉ d e?? - │ ◉ c + ◉ [rlv] a e?? + │ ◉ [vru] d e?? + │ ◉ [roy] c ├─╯ - ◉ + ◉ [zzz] "###); // Abandoning `a` would normally result in its descendant merge commit, `e`, @@ -109,12 +109,12 @@ fn test_basics() { Added 0 files, modified 0 files, removed 1 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ e - ◉ d - ◉ c - │ ◉ b + @ [znk] e + ◉ [vru] d + ◉ [roy] c + │ ◉ [zsu] b ├─╯ - ◉ a + ◉ [zzz] a "###); test_env.jj_cmd_ok(&repo_path, &["undo"]); @@ -130,11 +130,11 @@ fn test_basics() { Added 0 files, modified 0 files, removed 3 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ - │ ◉ b + @ [wvu] + │ ◉ [zsu] b ├─╯ - ◉ a e?? - ◉ c d e?? + ◉ [rlv] a e?? + ◉ [zzz] c d e?? "###); // Test abandoning the same commit twice directly @@ -145,13 +145,13 @@ fn test_basics() { Abandoned commit zsuskuln 1394f625 b | b "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ e + @ [znk] e ├─╮ - │ ◉ d - │ ◉ c - ◉ │ a b + │ ◉ [vru] d + │ ◉ [roy] c + ◉ │ [rlv] a b ├─╯ - ◉ + ◉ [zzz] "###); // Test abandoning the same commit twice indirectly @@ -169,10 +169,10 @@ fn test_basics() { Added 0 files, modified 0 files, removed 4 files "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ - │ ◉ c d e?? + @ [oup] + │ ◉ [roy] c d e?? ├─╯ - ◉ a b e?? + ◉ [zzz] a b e?? "###); } @@ -213,5 +213,12 @@ fn test_double_abandon() { } fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { - test_env.jj_cmd_success(repo_path, &["log", "-T", "branches"]) + test_env.jj_cmd_success( + repo_path, + &[ + "log", + "-T", + r#"separate(" ", "[" ++ change_id.short(3) ++ "]", branches)"#, + ], + ) } From eccd2667b0aaa79ca9b9e34616f1c5f80cb30470 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 19 Nov 2023 18:38:59 -0800 Subject: [PATCH 2/2] test_abandon_command: create another test for bug 2600 See comments inline for details. Cc #2600. In particular, I wanted to make sure these behaviors are not affected by #2646. They don't seem to be. The tests ended up weirder than expected because of https://github.com/martinvonz/jj/issues/2600#issuecomment-1835418824. Even though, right now, the behavior of tests is unaffected by that issue, the *expected* behavior is different. --- cli/tests/test_abandon_command.rs | 187 ++++++++++++++++++++++++++++++ 1 file changed, 187 insertions(+) diff --git a/cli/tests/test_abandon_command.rs b/cli/tests/test_abandon_command.rs index 86a6f28b03..51d692f23e 100644 --- a/cli/tests/test_abandon_command.rs +++ b/cli/tests/test_abandon_command.rs @@ -176,6 +176,193 @@ fn test_basics() { "###); } +// TODO(#2600): Make sure the results here become saner as #2600 is fixed. There +// is an simpler demo of #2600 at https://github.com/martinvonz/jj/pull/2655. +// However, fixing #2600 will likely change how `abandon` works. This test +// exists to track how that happens. See also the corresponding test in +// `test_rebase_command` +#[test] +fn test_bug_2600() { + 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"); + + // We will not touch "nottherootcommit". See the + // `test_bug_2600_rootcommit_special_case` for the one case where base being the + // child of the root commit changes the expected behavior. + create_commit(&test_env, &repo_path, "nottherootcommit", &[]); + create_commit(&test_env, &repo_path, "base", &["nottherootcommit"]); + create_commit(&test_env, &repo_path, "a", &["base"]); + create_commit(&test_env, &repo_path, "b", &["base", "a"]); + create_commit(&test_env, &repo_path, "c", &["b"]); + + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [vru] b + ├─╮ + │ ◉ [roy] a + ├─╯ + ◉ [zsu] base + ◉ [rlv] nottherootcommit + ◉ [zzz] + "###); + let setup_opid = test_env.current_operation_id(&repo_path); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "base"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Abandoned commit zsuskuln 73c929fc base | base + Rebased 3 descendant commits onto parents of abandoned commits + Working copy now at: znkkpsqq 510f8756 c | c + Parent commit : vruxwmqv 7301d9ab b | b + Added 0 files, modified 0 files, removed 1 files + "###); + // BUG. The user would expect + // @ c + // ├─╮ + // │ ◉ a + // ├─╯ + // ◉ base nottherootcommit + // ◉ + // This is likely caused by DescendantRebaser + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [vru] b + ◉ [roy] a + ◉ [rlv] base nottherootcommit + ◉ [zzz] + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Abandoned commit royxmykx 98f3b9ba a | a + Rebased 2 descendant commits onto parents of abandoned commits + Working copy now at: znkkpsqq 683b9435 c | c + Parent commit : vruxwmqv c10cb7b4 b | b + Added 0 files, modified 0 files, removed 1 files + "###); + // This is likely what the user will expect. + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [vru] b + ◉ [zsu] a base + ◉ [rlv] nottherootcommit + ◉ [zzz] + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "b"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Abandoned commit vruxwmqv 8c0dced0 b | b + Rebased 1 descendant commits onto parents of abandoned commits + Working copy now at: znkkpsqq 924bdd1c c | c + Parent commit : royxmykx 98f3b9ba a b?? | a + Added 0 files, modified 0 files, removed 1 files + "###); + // BUG. The user would expect + // @ c + // ├─╮ + // │ ◉ a + // ├─╯ + // ◉ base + // ◉ nottherootcommit + // ◉ + // This is likely caused by logic in `cmd_abandon`, not DescendantRebaser + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [roy] a b?? + ◉ [zsu] b?? base + ◉ [rlv] nottherootcommit + ◉ [zzz] + "###); + + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // ========= Reminder of the setup =========== + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [vru] b + ├─╮ + │ ◉ [roy] a + ├─╯ + ◉ [zsu] base + ◉ [rlv] nottherootcommit + ◉ [zzz] + "###); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "a", "b"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Abandoned the following commits: + royxmykx 98f3b9ba a | a + vruxwmqv 8c0dced0 b | b + Rebased 1 descendant commits onto parents of abandoned commits + Working copy now at: znkkpsqq 84fac1f8 c | c + Parent commit : zsuskuln 73c929fc a b base | base + Added 0 files, modified 0 files, removed 2 files + "###); + // This is likely what the user would expect + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [znk] c + ◉ [zsu] a b base + ◉ [rlv] nottherootcommit + ◉ [zzz] + "###); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "list", "b"]); + insta::assert_snapshot!(stdout, @r###" + b: zsuskuln 73c929fc base + "###); + insta::assert_snapshot!(stderr, @""); +} + +#[test] +fn test_bug_2600_rootcommit_special_case() { + 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"); + + // Set up like `test_bug_2600`, but without the `nottherootcommit` commit. + create_commit(&test_env, &repo_path, "base", &[]); + create_commit(&test_env, &repo_path, "a", &["base"]); + create_commit(&test_env, &repo_path, "b", &["base", "a"]); + create_commit(&test_env, &repo_path, "c", &["b"]); + + // Setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [vru] c + ◉ [roy] b + ├─╮ + │ ◉ [zsu] a + ├─╯ + ◉ [rlv] base + ◉ [zzz] + "###); + + // Now, the test + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["abandon", "base"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Abandoned commit rlvkpnrz 0c61db1b base | base + Rebased 3 descendant commits onto parents of abandoned commits + Working copy now at: vruxwmqv 73e9185c c | c + Parent commit : royxmykx 80dd9cba b | b + Added 0 files, modified 0 files, removed 1 files + "###); + // The current behavior is either correct or should be replaced with an error + // message. Even though the user would expect `b` to still be a descendant of + // `base`, it is impossible in the Git backend. + // See also https://github.com/martinvonz/jj/issues/2600#issuecomment-1835418824 + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ [vru] c + ◉ [roy] b + ◉ [zsu] a + ◉ [zzz] base + "###); +} + #[test] fn test_double_abandon() { let test_env = TestEnvironment::default();