Skip to content

Commit

Permalink
cli: branch: on create/set/rename, assume deleted tracking branch sti…
Browse files Browse the repository at this point in the history
…ll exists

While explaining branch tracking behavior, I find it's bad UX that a deleted
branch can be re-"create"d with tracking state preserved. It's rather a "set"
operation. Since deleted tracking branch is still listed, I think it's better
to assume that the local branch name is reserved.

#3871

is_fast_forward() is restored from 8706fad "cli: inline check for
non-fast-forwardable branch move."
  • Loading branch information
yuja committed Jun 14, 2024
1 parent 3bbd298 commit 88ff355
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 14 deletions.
42 changes: 28 additions & 14 deletions cli/src/commands/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,19 @@ fn cmd_branch_create(
workspace_command.resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
let view = workspace_command.repo().view();
let branch_names = &args.names;
if let Some(branch_name) = branch_names
.iter()
.find(|&name| view.get_local_branch(name).is_present())
{
return Err(user_error_with_hint(
format!("Branch already exists: {branch_name}"),
"Use `jj branch set` to update it.",
));
for name in branch_names {
if view.get_local_branch(name).is_present() {
return Err(user_error_with_hint(
format!("Branch already exists: {name}"),
"Use `jj branch set` to update it.",
));
}
if has_tracked_remote_branches(view, name) {
return Err(user_error_with_hint(
format!("Tracked remote branches exist for deleted branch: {name}"),
"Use `jj branch set` to restore local branch.",
));
}
}

if branch_names.len() > 1 {
Expand Down Expand Up @@ -302,6 +307,11 @@ fn cmd_branch_rename(
if view.get_local_branch(new_branch).is_present() {
return Err(user_error(format!("Branch already exists: {new_branch}")));
}
if has_tracked_remote_branches(view, new_branch) {
return Err(user_error(format!(
"Tracked remote branches exist for deleted branch: {new_branch}"
)));
}

let mut tx = workspace_command.start_transaction();
tx.mut_repo()
Expand Down Expand Up @@ -346,16 +356,20 @@ fn cmd_branch_set(
workspace_command.resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
let repo = workspace_command.repo().as_ref();
let is_fast_forward = |old_target: &RefTarget| {
// Strictly speaking, "all" old targets should be ancestors, but we allow
// conflict resolution by setting branch to "any" of the old target descendants.
old_target
.added_ids()
.any(|old| repo.index().is_ancestor(old, target_commit.id()))
if old_target.is_present() {
// Strictly speaking, "all" old targets should be ancestors, but we allow
// conflict resolution by setting branch to "any" of the old target descendants.
old_target
.added_ids()
.any(|old| repo.index().is_ancestor(old, target_commit.id()))
} else {
true
}
};
let branch_names = &args.names;
for name in branch_names {
let old_target = repo.view().get_local_branch(name);
if old_target.is_absent() {
if old_target.is_absent() && !has_tracked_remote_branches(repo.view(), name) {
return Err(user_error_with_hint(
format!("No such branch: {name}"),
"Use `jj branch create` to create it.",
Expand Down
73 changes: 73 additions & 0 deletions cli/tests/test_branch_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ fn test_branch_move() {
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
let repo_path = test_env.env_root().join("repo");

// Set up remote
let git_repo_path = test_env.env_root().join("git-repo");
git2::Repository::init_bare(git_repo_path).unwrap();
test_env.jj_cmd_ok(
&repo_path,
&["git", "remote", "add", "origin", "../git-repo"],
);

let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "set", "foo"]);
insta::assert_snapshot!(stderr, @r###"
Error: No such branch: foo
Expand Down Expand Up @@ -126,6 +134,45 @@ fn test_branch_move() {
&["branch", "set", "-r@-", "--allow-backwards", "foo"],
);
insta::assert_snapshot!(stderr, @"");

// Delete branch locally, but is still tracking remote
test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-mcommit"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-r@-"]);
test_env.jj_cmd_ok(&repo_path, &["branch", "delete", "foo"]);
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
foo (deleted)
@origin: qpvuntsm 74205c97 (empty) commit
"###);

// Deleted tracking branch name should still be allocated
let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "create", "foo"]);
insta::assert_snapshot!(stderr, @r###"
Error: Tracked remote branches exist for deleted branch: foo
Hint: Use `jj branch set` to restore local branch.
"###);

// Restoring local target shouldn't invalidate tracking state
let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "set", "foo"]);
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
foo: mzvwutvl c3cc8247 (empty) (no description set)
@origin (behind by 1 commits): qpvuntsm 74205c97 (empty) commit
"###);

// Untracked remote branch shouldn't block creation of local branch
test_env.jj_cmd_ok(&repo_path, &["branch", "untrack", "foo@origin"]);
test_env.jj_cmd_ok(&repo_path, &["branch", "delete", "foo"]);
let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "set", "foo"]);
insta::assert_snapshot!(stderr, @r###"
Error: No such branch: foo
Hint: Use `jj branch create` to create it.
"###);
let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "create", "foo"]);
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
foo: mzvwutvl c3cc8247 (empty) (no description set)
foo@origin: qpvuntsm 74205c97 (empty) commit
"###);
}

#[test]
Expand Down Expand Up @@ -230,6 +277,32 @@ fn test_branch_rename() {
Warning: Branch bremote has tracking remote branches which were not renamed.
Hint: to rename the branch on the remote, you can `jj git push --branch bremote` first (to delete it on the remote), and then `jj git push --branch bremote2`. `jj git push --all` would also be sufficient.
"###);

insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
bexist: yqosqzyt 372e2731 (empty) commit-1
blocal1: qpvuntsm c177823f (empty) commit-0
bremote (deleted)
@origin: kpqxywon 1a9dae5e (empty) commit-2
bremote2: kpqxywon 1a9dae5e (empty) commit-2
"###);

// Deleted tracking branch name should still be allocated
let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "rename", "blocal1", "bremote"]);
insta::assert_snapshot!(stderr, @r###"
Error: Tracked remote branches exist for deleted branch: bremote
"###);

// Untracked remote branch shouldn't block rename
test_env.jj_cmd_ok(&repo_path, &["branch", "untrack", "bremote@origin"]);
let (_stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["branch", "rename", "blocal1", "bremote"]);
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###"
bexist: yqosqzyt 372e2731 (empty) commit-1
bremote: qpvuntsm c177823f (empty) commit-0
bremote@origin: kpqxywon 1a9dae5e (empty) commit-2
bremote2: kpqxywon 1a9dae5e (empty) commit-2
"###);
}

#[test]
Expand Down

0 comments on commit 88ff355

Please sign in to comment.