From d64f5dca33ef9252cf4f3e1900b645aa1f59bb80 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 31 May 2023 20:55:46 -0700 Subject: [PATCH] undo: preserve git-tracking refs in colocated repos by default --- CHANGELOG.md | 4 ++++ src/commands/operation.rs | 39 +++++++++++++++++++++++++++++-------- tests/test_git_colocated.rs | 6 ++---- tests/test_undo.rs | 20 ++++++++++++------- 4 files changed, 50 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab40682716..559f1fa5bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -168,6 +168,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#697](https://github.com/martinvonz/jj/issues/697), [#1608](https://github.com/martinvonz/jj/issues/1608) +* In colocated repos, a bug causing conflicts when undoing branch moves (#922) + has been fixed. Some surprising behaviors related to undoing `jj git push` or + `jj git fetch` remain. + ## [0.7.0] - 2023-02-16 ### Breaking changes diff --git a/src/commands/operation.rs b/src/commands/operation.rs index c8b1366b1d..8ab79dc0d3 100644 --- a/src/commands/operation.rs +++ b/src/commands/operation.rs @@ -207,15 +207,36 @@ fn view_with_desired_portions_restored( new_view } -fn process_what_arg(what_arg: &[UndoWhatToRestore]) -> Vec { +fn process_what_arg(what_arg: &[UndoWhatToRestore], colocated: bool) -> Vec { if !what_arg.is_empty() { what_arg.to_vec() } else { - vec![ - UndoWhatToRestore::Repo, - UndoWhatToRestore::RemoteTracking, - UndoWhatToRestore::GitTracking, - ] + 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); + } + default_what } } @@ -226,6 +247,7 @@ pub fn cmd_op_undo( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let bad_op = workspace_command.resolve_single_op(&args.operation)?; + let repo_is_colocated = workspace_command.working_copy_shared_with_git(); let parent_ops = bad_op.parents(); if parent_ops.len() > 1 { return Err(user_error("Cannot undo a merge operation")); @@ -243,7 +265,7 @@ pub fn cmd_op_undo( let new_view = view_with_desired_portions_restored( tx.repo().view().store_view(), tx.base_repo().view().store_view(), - &process_what_arg(&args.what), + &process_what_arg(&args.what, repo_is_colocated), ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; @@ -258,12 +280,13 @@ fn cmd_op_restore( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let target_op = workspace_command.resolve_single_op(&args.operation)?; + let repo_is_colocated = workspace_command.working_copy_shared_with_git(); let mut tx = workspace_command .start_transaction(&format!("restore to operation {}", target_op.id().hex())); let new_view = view_with_desired_portions_restored( target_op.view().store_view(), tx.base_repo().view().store_view(), - &process_what_arg(&args.what), + &process_what_arg(&args.what, repo_is_colocated), ); tx.mut_repo().set_view(new_view); tx.finish(ui)?; diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index 1a00c5b83f..7f68053ec1 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -371,10 +371,8 @@ fn test_git_colocated_squash_undo() { // TODO: There should be no divergence here; 2f376ea1478c should be hidden // (#922) insta::assert_snapshot!(get_log_output_divergence(&test_env, &repo_path), @r###" - ◉ qpvuntsmwlqt 2f376ea1478c A master !divergence! - │ @ rlvkpnrzqnoo 8f71e3b6a3be - │ ◉ qpvuntsmwlqt a86754f975f9 A HEAD@git !divergence! - ├─╯ + @ rlvkpnrzqnoo 8f71e3b6a3be + ◉ qpvuntsmwlqt a86754f975f9 A master HEAD@git ◉ zzzzzzzzzzzz 000000000000 "###); } diff --git a/tests/test_undo.rs b/tests/test_undo.rs index 1bcd0b566a..90bc0ac9d9 100644 --- a/tests/test_undo.rs +++ b/tests/test_undo.rs @@ -235,30 +235,36 @@ fn test_git_push_undo_colocated() { // Undo the push test_env.jj_cmd_success(&repo_path, &["op", "restore", &pre_push_opid]); - // === Before auto-import ==== + // === Before auto-export ==== // | jj refs | jj's | git // | | git | repo // | |tracking| // ------------------------------------------ // local `main` | BB | BB | BB - // remote-tracking | AA | AA | BB - // === After automatic `jj git import` ==== + // remote-tracking | AA | BB | BB + // === After automatic `jj git export` ==== // | jj refs | jj's | git // | | git | repo // | |tracking| // ------------------------------------------ // local `main` | BB | BB | BB - // remote-tracking | BB | BB | 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"]); - // This currently gives an identical result to `test_git_push_undo_import` + // We have the same conflict as `test_git_push_undo`. TODO: why did we get the + // same result in a seemingly different way? 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): 0a3e99f08a48 CC + @origin (behind by 1 commits): 8c05de152218 BB "###); }