From 480016b6609e63ffebf8b5f96d2e9192f86825ae Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 14 Jun 2024 18:16:23 +0900 Subject: [PATCH] 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. --- cli/src/commands/branch/create.rs | 26 +++++++++++------ cli/src/commands/branch/rename.rs | 13 +++++++++ cli/src/commands/branch/set.rs | 4 +-- cli/tests/test_branch_command.rs | 48 +++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 11 deletions(-) diff --git a/cli/src/commands/branch/create.rs b/cli/src/commands/branch/create.rs index 42cd8ee955d..2b6df9cf7f4 100644 --- a/cli/src/commands/branch/create.rs +++ b/cli/src/commands/branch/create.rs @@ -16,7 +16,7 @@ use clap::builder::NonEmptyStringValueParser; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::RefTarget; -use super::make_branch_term; +use super::{has_tracked_remote_branches, make_branch_term}; use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::{user_error_with_hint, CommandError}; use crate::ui::Ui; @@ -43,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 { diff --git a/cli/src/commands/branch/rename.rs b/cli/src/commands/branch/rename.rs index f83edc86e4b..4c80bb32713 100644 --- a/cli/src/commands/branch/rename.rs +++ b/cli/src/commands/branch/rename.rs @@ -76,6 +76,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(()) } diff --git a/cli/src/commands/branch/set.rs b/cli/src/commands/branch/set.rs index 3757fc8106d..6efcf8c7490 100644 --- a/cli/src/commands/branch/set.rs +++ b/cli/src/commands/branch/set.rs @@ -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, make_branch_term}; +use super::{has_tracked_remote_branches, is_fast_forward, make_branch_term}; use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::{user_error_with_hint, CommandError}; use crate::ui::Ui; @@ -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()) { diff --git a/cli/tests/test_branch_command.rs b/cli/tests/test_branch_command.rs index 6f4dacd921f..25479cf3fbd 100644 --- a/cli/tests/test_branch_command.rs +++ b/cli/tests/test_branch_command.rs @@ -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 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] @@ -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]