From 23431aa28b00f8c213f6a2aaae30d77d69ab2640 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 | 52 ++++++++++++++++----------------- tests/test_git_fetch.rs | 9 +++--- tests/test_git_import_export.rs | 34 ++++++++++++++++----- tests/test_undo.rs | 35 ++++++++++++---------- 4 files changed, 75 insertions(+), 55 deletions(-) diff --git a/src/commands/operation.rs b/src/commands/operation.rs index fd0162417c..953b471757 100644 --- a/src/commands/operation.rs +++ b/src/commands/operation.rs @@ -208,37 +208,35 @@ fn view_with_desired_portions_restored( fn process_what_arg( what_arg: &[UndoWhatToRestore], - colocated: bool, + _colocated: bool, ) -> BTreeSet { if !what_arg.is_empty() { what_arg.iter().cloned().collect() } else { - let mut default_what = - btreeset!(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.insert(UndoWhatToRestore::GitTracking); - } + let default_what = btreeset!(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_fetch.rs b/tests/test_git_fetch.rs index 4cd7c86f2a..357c75d793 100644 --- a/tests/test_git_fetch.rs +++ b/tests/test_git_fetch.rs @@ -668,12 +668,11 @@ fn test_git_fetch_undo() { "###); // Now try to fetch just one branch let stdout = test_env.jj_cmd_success(&target_jj_repo_path, &["git", "fetch", "--branch", "b"]); - insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stdout, @r###" + Nothing changed. + "###); insta::assert_snapshot!(get_log_output(&test_env, &target_jj_repo_path), @r###" - ◉ c7d4bdcbc215 descr_for_b b - ◉ ff36dc55760e descr_for_trunk1 - │ @ 230dd059e1b0 - ├─╯ + @ 230dd059e1b0 ◉ 000000000000 "###); } diff --git a/tests/test_git_import_export.rs b/tests/test_git_import_export.rs index 678dca1d17..165a280fa1 100644 --- a/tests/test_git_import_export.rs +++ b/tests/test_git_import_export.rs @@ -104,6 +104,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###" [ @@ -118,7 +119,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] @@ -149,11 +152,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 @@ -169,6 +180,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) @@ -246,7 +258,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###" [ ( @@ -260,9 +276,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 00d1834ab0..5cf1b9ec37 100644 --- a/tests/test_undo.rs +++ b/tests/test_undo.rs @@ -85,20 +85,14 @@ fn test_git_push_undo() { // | |tracking| // ------------------------------------------ // local `main` | BB | -- | -- - // remote-tracking | AA | AA | BB + // remote-tracking | AA | BB | BB 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: This should probably not be considered a conflict. 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 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 "###); } @@ -141,18 +135,26 @@ fn test_git_push_undo_with_import() { // | |tracking| // ------------------------------------------ // local `main` | BB | -- | -- - // remote-tracking | AA | AA | BB + // remote-tracking | AA | BB | BB - // 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 + + // 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 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"]); @@ -160,7 +162,8 @@ fn test_git_push_undo_with_import() { // 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 + @git (ahead by 1 commits, behind by 1 commits): 8c05de152218 BB + @origin (ahead by 1 commits, behind by 1 commits): 0cffb6146141 AA "###); }