Skip to content

Commit

Permalink
cli: branch: assume deleted tracking branch name is still allocated
Browse files Browse the repository at this point in the history
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

Renaming to deleted tracking branch is still allowed (with warning) because the
"rename" command can't handle tracked remotes very well. If it were banned, bad
rename couldn't be reverted by using "jj branch rename". It would be confusing
if "rename a b" succeeded with warning, but the following "rename b a" failed.
  • Loading branch information
yuja committed Jun 28, 2024
1 parent ea22243 commit 0f8a93a
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 10 deletions.
25 changes: 17 additions & 8 deletions cli/src/commands/branch/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use clap::builder::NonEmptyStringValueParser;
use jj_lib::object_id::ObjectId as _;
use jj_lib::op_store::RefTarget;

use super::has_tracked_remote_branches;
use crate::cli_util::{CommandHelper, RevisionArg};
use crate::command_error::{user_error_with_hint, CommandError};
use crate::ui::Ui;
Expand All @@ -42,14 +43,22 @@ pub 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}"),
format!(
"Use `jj branch set` to reset local branch. Run `jj branch untrack \
'glob:{name}@*'` to disassociate them."
),
));
}
}

if branch_names.len() > 1 {
Expand Down
13 changes: 13 additions & 0 deletions cli/src/commands/branch/rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ pub fn cmd_branch_rename(
`jj git push --all` would also be sufficient."
)?;
}
if has_tracked_remote_branches(view, new_branch) {
// This isn't an error because branch renaming can't be propagated to
// the remote immediately. "rename old new && rename new old" should be
// allowed even if the original old branch had tracked remotes.
writeln!(
ui.warning_default(),
"Branch {new_branch} has existing tracked remote branches."
)?;
writeln!(
ui.hint_default(),
"Run `jj branch untrack 'glob:{new_branch}@*'` to disassociate them."
)?;
}

Ok(())
}
4 changes: 2 additions & 2 deletions cli/src/commands/branch/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use clap::builder::NonEmptyStringValueParser;
use jj_lib::object_id::ObjectId as _;
use jj_lib::op_store::RefTarget;

use super::is_fast_forward;
use super::{has_tracked_remote_branches, is_fast_forward};
use crate::cli_util::{CommandHelper, RevisionArg};
use crate::command_error::{user_error_with_hint, CommandError};
use crate::ui::Ui;
Expand Down Expand Up @@ -50,7 +50,7 @@ pub fn cmd_branch_set(
let mut new_branch_names: Vec<&str> = Vec::new();
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) {
new_branch_names.push(name);
}
if !args.allow_backwards && !is_fast_forward(repo, old_target, target_commit.id()) {
Expand Down
48 changes: 48 additions & 0 deletions cli/tests/test_branch_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,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", "move", "foo"]);
insta::assert_snapshot!(stderr, @r###"
Error: No such branch: foo
Expand Down Expand Up @@ -150,6 +158,40 @@ fn test_branch_move() {
&["branch", "move", "--to=@-", "--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 29a62310 (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 reset local branch. Run `jj branch untrack 'glob:foo@*'` to disassociate them.
"###);

// 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 d5f17aba (empty) (no description set)
@origin (behind by 1 commits): qpvuntsm 29a62310 (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 (_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 d5f17aba (empty) (no description set)
foo@origin: qpvuntsm 29a62310 (empty) commit
"###);
}

#[test]
Expand Down Expand Up @@ -357,6 +399,12 @@ fn test_branch_rename() {
Warning: Branch bremote has tracked 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.
"###);
let (_stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["branch", "rename", "bremote2", "bremote"]);
insta::assert_snapshot!(stderr, @r###"
Warning: Branch bremote has existing tracked remote branches.
Hint: Run `jj branch untrack 'glob:bremote@*'` to disassociate them.
"###);
}

#[test]
Expand Down

0 comments on commit 0f8a93a

Please sign in to comment.