Skip to content

Commit

Permalink
undo: always preserve git_refs by default
Browse files Browse the repository at this point in the history 
The result is equivalent to jj-vcs#1541
but with many more tests
  • Loading branch information
@ilyagr
ilyagr committed Jul 3, 2023
1 parent c3e2ec0 commit 072436e
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 50 deletions.
51 changes: 25 additions & 26 deletions src/commands/operation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,35 +207,34 @@ fn view_with_desired_portions_restored(
new_view
}

fn process_what_arg(what_arg: &[UndoWhatToRestore], colocated: bool) -> Vec<UndoWhatToRestore> {
fn process_what_arg(what_arg: &[UndoWhatToRestore], _colocated: bool) -> Vec<UndoWhatToRestore> {
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
}
}
Expand Down
34 changes: 27 additions & 7 deletions tests/test_git_import_export.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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###"
[
Expand All @@ -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]
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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###"
[
(
Expand All @@ -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`)
"###);
}

Expand Down
46 changes: 29 additions & 17 deletions tests/test_undo.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,28 +92,22 @@ 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
"###);
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
"###);
}

Expand Down Expand Up @@ -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
"###);
}

Expand Down

0 comments on commit 072436e

Please sign in to comment.