Skip to content

Commit

Permalink
cli: make non-colocated repo also preserve git_refs on op undo/restore
Browse files Browse the repository at this point in the history 
Now we have a separate map for "git" tracking remote, we can always preserve
the last imported/exported git_refs. The option to restore git-tracking refs
has been removed. Perhaps, --what can be reorganized as --local and --remote
<NAME>.
  • Loading branch information
@yuja
yuja committed Oct 9, 2023
1 parent 8e952e0 commit b1da590
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 83 deletions.
67 changes: 9 additions & 58 deletions cli/src/commands/operation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,8 @@ pub struct OperationRestoreArgs {

/// What portions of the local state to restore (can be repeated)
///
/// Defaults to everything for non-colocated repos.
///
/// Defaults to `repo` and `remote-tracking` for colocated repos. This
/// ensures that the automatic `jj git export` succeeds.
///
/// This option is EXPERIMENTAL.
#[arg(long)]
#[arg(long, value_enum, default_values_t = DEFAULT_UNDO_WHAT)]
what: Vec<UndoWhatToRestore>,
}

Expand All @@ -78,13 +73,8 @@ pub struct OperationUndoArgs {

/// What portions of the local state to restore (can be repeated)
///
/// Defaults to everything for non-colocated repos.
///
/// Defaults to `repo` and `remote-tracking` for colocated repos. This
/// ensures that the automatic `jj git export` succeeds.
///
/// This option is EXPERIMENTAL.
#[arg(long)]
#[arg(long, value_enum, default_values_t = DEFAULT_UNDO_WHAT)]
what: Vec<UndoWhatToRestore>,
}

Expand All @@ -95,10 +85,11 @@ enum UndoWhatToRestore {
/// The remote-tracking branches. Do not restore these if you'd like to push
/// after the undo
RemoteTracking,
/// Remembered git repo state from the last `jj git import`
GitTracking,
}

const DEFAULT_UNDO_WHAT: [UndoWhatToRestore; 2] =
[UndoWhatToRestore::Repo, UndoWhatToRestore::RemoteTracking];

fn cmd_op_log(
ui: &mut Ui,
command: &CommandHelper,
Expand Down Expand Up @@ -179,13 +170,8 @@ fn view_with_desired_portions_restored(
current_view.clone()
};

let git_source_view = if what.contains(&UndoWhatToRestore::GitTracking) {
view_being_restored
} else {
current_view
};
new_view.git_refs = git_source_view.git_refs.clone();
new_view.git_head = git_source_view.git_head.clone();
new_view.git_refs = current_view.git_refs.clone();
new_view.git_head = current_view.git_head.clone();

if what.contains(&UndoWhatToRestore::RemoteTracking) == what.contains(&UndoWhatToRestore::Repo)
{
Expand Down Expand Up @@ -229,47 +215,13 @@ fn view_with_desired_portions_restored(
new_view
}

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);
}
default_what
}
}

pub fn cmd_op_undo(
ui: &mut Ui,
command: &CommandHelper,
args: &OperationUndoArgs,
) -> 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"));
Expand All @@ -287,7 +239,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, repo_is_colocated),
&args.what,
);
tx.mut_repo().set_view(new_view);
tx.finish(ui)?;
Expand All @@ -302,13 +254,12 @@ 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, repo_is_colocated),
&args.what,
);
tx.mut_repo().set_view(new_view);
tx.finish(ui)?;
Expand Down
43 changes: 18 additions & 25 deletions cli/tests/test_git_import_export.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,14 @@ fn test_git_export_undo() {
a: qpvuntsm 230dd059 (empty) (no description set)
"###);
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @"");
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-ra@git"]), @r###"
@ qpvuntsm [email protected] 2001-02-03 04:05:07.000 +07:00 a 230dd059
│ (empty) (no description set)
~
"###);

// "git export" can't be undone.
// Exported refs won't be removed by undoing the export, but the git-tracking
// branch is. This is the same as remote-tracking branches.
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "undo"]), @r###"
"###);
insta::assert_debug_snapshot!(get_git_repo_refs(&git_repo), @r###"
Expand All @@ -102,10 +108,18 @@ fn test_git_export_undo() {
),
]
"###);
insta::assert_snapshot!(test_env.jj_cmd_failure(&repo_path, &["log", "-ra@git"]), @r###"
Error: Revision "a@git" doesn't exist
Hint: Did you mean "a"?
"###);

// 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.
// This would re-export branch "a" and create git-tracking branch.
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @"");
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-ra@git"]), @r###"
@ qpvuntsm [email protected] 2001-02-03 04:05:07.000 +07:00 a 230dd059
│ (empty) (no description set)
~
"###);
}

#[test]
Expand All @@ -132,7 +146,7 @@ fn test_git_import_undo() {
a: qpvuntsm 230dd059 (empty) (no description set)
"###);

// "git import" can be undone by default in non-colocated repositories.
// "git import" can be undone by default.
let stdout = test_env.jj_cmd_success(&repo_path, &["op", "restore", &base_operation_id]);
insta::assert_snapshot!(stdout, @r###"
"###);
Expand All @@ -142,27 +156,6 @@ fn test_git_import_undo() {
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
a: qpvuntsm 230dd059 (empty) (no description set)
"###);

// Even if we don't restore the git_refs, "git import" can re-import the
// local branch "a".
// TODO: This will be the default "op restore" mode.
let stdout = test_env.jj_cmd_success(
&repo_path,
&[
"op",
"restore",
&base_operation_id,
"--what=repo",
"--what=remote-tracking",
],
);
insta::assert_snapshot!(stdout, @r###"
"###);
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @"");
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
a: qpvuntsm 230dd059 (empty) (no description set)
"###);
}

#[test]
Expand Down

0 comments on commit b1da590

Please sign in to comment.