From 8a04cf24a54e6d08e1ac1fea59c54f71c7d9c811 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 25 Jun 2023 21:05:54 -0700 Subject: [PATCH] undo: always preserve git_refs by default The result is equivalent to https://github.com/martinvonz/jj/pull/1541 but with many more tests --- src/commands/operation.rs | 51 ++++++++++++++++----------------- tests/test_git_import_export.rs | 34 +++++++++++++++++----- tests/test_undo.rs | 46 ++++++++++++++++++----------- 3 files changed, 81 insertions(+), 50 deletions(-) diff --git a/src/commands/operation.rs b/src/commands/operation.rs index 8ab79dc0d3..b2ee4dd7fd 100644 --- a/src/commands/operation.rs +++ b/src/commands/operation.rs @@ -207,35 +207,34 @@ fn view_with_desired_portions_restored( new_view } -fn process_what_arg(what_arg: &[UndoWhatToRestore], colocated: bool) -> Vec { +fn process_what_arg(what_arg: &[UndoWhatToRestore], _colocated: bool) -> Vec { if !what_arg.is_empty() { what_arg.to_vec() } else { - let mut default_what = vec![UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking]; - if !colocated { - // In a colocated repo, restoring the git-tracking refs is harmful - // (https://github.com/martinvonz/jj/issues/922). - // - // The issue is that `jj undo` does not directly change the local - // git repo's branches. Keeping those up to date the job of the - // automatic `jj git import` and `jj git export`, and they rely on the - // git-tracking refs matching the git repo's branches. - // - // Consider, for example, undoing a `jj branch set` command. If the - // git-tracking refs were restored by `undo`, they would no longer - // match the actual positions of branches in the git repo. So, the - // automatic `jj git export` would fail and the automatic `jj git - // import` would create a conflict, as demonstrated by the bug - // linked above. - // - // So, we have `undo` *not* move the git-tracking branches. After - // the undo, git-tracking refs will still match the actual positions - // of the git repo's branches (in the normal case where they matched - // before the undo). The automatic `jj git export` that happens - // immediately after the undo will successfully export whatever - // changes to branches `undo` caused. - default_what.push(UndoWhatToRestore::GitTracking); - } + let default_what = vec![UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking]; + // In a colocated repo, restoring the git-tracking refs is harmful + // (https://github.com/martinvonz/jj/issues/922). + // + // The issue is that `jj undo` does not directly change the local + // git repo's branches. Keeping those up to date the job of the + // automatic `jj git import` and `jj git export`, and they rely on the + // git-tracking refs matching the git repo's branches. + // + // Consider, for example, undoing a `jj branch set` command. If the + // git-tracking refs were restored by `undo`, they would no longer + // match the actual positions of branches in the git repo. So, the + // automatic `jj git export` would fail and the automatic `jj git + // import` would create a conflict, as demonstrated by the bug + // linked above. + // + // So, we have `undo` *not* move the git-tracking branches. After + // the undo, git-tracking refs will still match the actual positions + // of the git repo's branches (in the normal case where they matched + // before the undo). The automatic `jj git export` that happens + // immediately after the undo will successfully export whatever + // changes to branches `undo` caused. + // + // In a normal repo, it's less clear which behavior is best. default_what } } diff --git a/tests/test_git_import_export.rs b/tests/test_git_import_export.rs index f5c6fb75a8..427e716e2c 100644 --- a/tests/test_git_import_export.rs +++ b/tests/test_git_import_export.rs @@ -101,6 +101,7 @@ fn test_git_export_undo() { // "git export" can't be undone. insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "undo"]), @r###" + Nothing changed. "###); insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###" [ @@ -115,7 +116,9 @@ fn test_git_export_undo() { // This would re-export branch "a" as the internal state has been rolled back. // It might be better to preserve the state, and say "Nothing changed" here. - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @""); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @r###" + Nothing changed. + "###); } #[test] @@ -146,11 +149,19 @@ fn test_git_import_undo() { let stdout = test_env.jj_cmd_success(&repo_path, &["op", "restore", &base_operation_id]); insta::assert_snapshot!(stdout, @r###" "###); - insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); + insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" + a (forgotten) + @git: 230dd059e1b0 (no description set) + (this branch will be deleted from the underlying Git repo on the next `jj git export`) + "###); // Try "git import" again, which should re-import the branch "a". - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @""); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @r###" + Nothing changed. + "###); insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - a: 230dd059e1b0 (no description set) + a (forgotten) + @git: 230dd059e1b0 (no description set) + (this branch will be deleted from the underlying Git repo on the next `jj git export`) "###); // If we don't restore the git_refs, undoing the import removes the local branch @@ -166,6 +177,7 @@ fn test_git_import_undo() { ], ); insta::assert_snapshot!(stdout, @r###" + Nothing changed. "###); insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" a (forgotten) @@ -243,7 +255,11 @@ fn test_git_import_move_export_with_default_undo() { Working copy now at: 230dd059e1b0 (no description set) Parent commit : 000000000000 (no description set) "###); - insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); + insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" + a (forgotten) + @git: 096dc80da670 (no description set) + (this branch will be deleted from the underlying Git repo on the next `jj git export`) + "###); insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###" [ ( @@ -257,9 +273,13 @@ fn test_git_import_move_export_with_default_undo() { // The last branch "a" state is imported from git. No idea what's the most // intuitive result here. - insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @""); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @r###" + Nothing changed. + "###); insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - a: 096dc80da670 (no description set) + a (forgotten) + @git: 096dc80da670 (no description set) + (this branch will be deleted from the underlying Git repo on the next `jj git export`) "###); } diff --git a/tests/test_undo.rs b/tests/test_undo.rs index 90bc0ac9d9..2c887e776e 100644 --- a/tests/test_undo.rs +++ b/tests/test_undo.rs @@ -92,7 +92,7 @@ fn test_git_push_undo() { // | |tracking| // ------------------------------------------ // local `main` | BB | -- | -- - // remote-tracking | AA | AA | BB + // remote-tracking | AA | BB | BB insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" main: 8c05de152218 BB @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA @@ -100,20 +100,14 @@ fn test_git_push_undo() { test_env.advance_test_rng_seed_to_multiple_of(100_000); test_env.jj_cmd_success(&repo_path, &["describe", "-m", "CC"]); test_env.jj_cmd_success(&repo_path, &["git", "fetch"]); - // TODO: The user would probably not expect a conflict here. It currently is - // because the undo made us forget that the remote was at v2, so the fetch - // made us think it updated from v1 to v2 (instead of the no-op it could - // have been). + // Conflict is gone, though the refs are in a strange state // // One option to solve this would be to have undo not restore remote-tracking // branches, but that also has undersired consequences: the second fetch in `jj // git fetch && jj undo && jj git fetch` would become a no-op. insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - main (conflicted): - - 0cffb6146141 AA - + 0a3e99f08a48 CC - + 8c05de152218 BB - @origin (behind by 1 commits): 8c05de152218 BB + main: 0a3e99f08a48 CC + @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA "###); } @@ -163,33 +157,51 @@ fn test_git_push_undo_with_import() { // | |tracking| // ------------------------------------------ // local `main` | BB | -- | -- - // remote-tracking | AA | AA | BB + // remote-tracking | AA | BB | BB insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" main: 8c05de152218 BB @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA "###); - // PROBLEM: inserting this import changes the outcome compared to previous test - // TODO: decide if this is the better behavior, and whether import of - // remote-tracking branches should happen on every operation. + // import becomes a no-op test_env.jj_cmd_success(&repo_path, &["git", "import"]); // | jj refs | jj's | git // | | git | repo // | |tracking| // ------------------------------------------ // local `main` | BB | -- | -- - // remote-tracking | BB | BB | BB + // remote-tracking | AA | BB | BB insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" main: 8c05de152218 BB + @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA "###); + + // Export makes the git repo state more sane (though the test doesn't make it + // explicit) + test_env.jj_cmd_success(&repo_path, &["git", "export"]); + // | jj refs | jj's | git + // | | git | repo + // | |tracking| + // ------------------------------------------ + // local `main` | BB | -- | -- + // remote-tracking | AA | AA | AA + insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" + main: 8c05de152218 BB + @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA + "###); + test_env.advance_test_rng_seed_to_multiple_of(100_000); test_env.jj_cmd_success(&repo_path, &["describe", "-m", "CC"]); test_env.jj_cmd_success(&repo_path, &["git", "fetch"]); // There is not a conflict. This seems like a good outcome; undoing `git push` // was essentially a no-op. insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - main: 0a3e99f08a48 CC - @origin (ahead by 1 commits, behind by 1 commits): 8c05de152218 BB + main (conflicted): + - 0cffb6146141 AA + + 0a3e99f08a48 CC + + 8c05de152218 BB + @git (behind by 1 commits): 8c05de152218 BB + @origin (behind by 1 commits): 8c05de152218 BB "###); }