cli: branch: assume deleted tracking branch name is still allocated

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.

https://github.com/martinvonz/jj/discussions/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.
This commit is contained in:
Yuya Nishihara 2024-06-14 18:16:23 +09:00
parent 06813d9462
commit af986a58ed
4 changed files with 82 additions and 10 deletions

View file

@ -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;
@ -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 recreate the local branch. Run `jj branch untrack \
'glob:{name}@*'` to disassociate them."
),
));
}
}
if branch_names.len() > 1 {

View file

@ -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(),
"Tracked remote branches for branch {new_branch} exist."
)?;
writeln!(
ui.hint_default(),
"Run `jj branch untrack 'glob:{new_branch}@*'` to disassociate them."
)?;
}
Ok(())
}

View file

@ -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;
@ -50,7 +50,9 @@ 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 a branch is absent locally but is still tracking remote branches,
// we are resurrecting the local branch, not "creating" a new branch.
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()) {

View file

@ -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
@ -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 recreate the 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]
@ -357,6 +399,12 @@ fn test_branch_rename() {
Warning: Tracked remote branches for branch bremote 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: Tracked remote branches for branch bremote exist.
Hint: Run `jj branch untrack 'glob:bremote@*'` to disassociate them.
"###);
}
#[test]