Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: branch: on create/set/rename, assume deleted tracking local branch still exists #3884

Merged
merged 3 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 recreate the local branch. Run `jj branch untrack \
'glob:{name}@*'` to disassociate them."
),
));
}
}

if branch_names.len() > 1 {
Expand Down
7 changes: 7 additions & 0 deletions cli/src/commands/branch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,13 @@ fn find_remote_branches<'a>(
}
}

/// Whether or not the `branch` has any tracked remotes (i.e. is a tracking
/// local branch.)
fn has_tracked_remote_branches(view: &View, branch: &str) -> bool {
view.remote_branches_matching(&StringPattern::exact(branch), &StringPattern::everything())
.any(|(_, remote_ref)| remote_ref.is_tracking())
}

fn is_fast_forward(repo: &dyn Repo, old_target: &RefTarget, new_target_id: &CommitId) -> bool {
if old_target.is_present() {
// Strictly speaking, "all" old targets should be ancestors, but we allow
Expand Down
27 changes: 17 additions & 10 deletions cli/src/commands/branch/rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
// limitations under the License.

use jj_lib::op_store::RefTarget;
use jj_lib::str_util::StringPattern;

use super::has_tracked_remote_branches;
use crate::cli_util::CommandHelper;
use crate::command_error::{user_error, CommandError};
use crate::ui::Ui;
Expand Down Expand Up @@ -57,24 +57,31 @@ pub fn cmd_branch_rename(
tx.finish(ui, format!("rename branch {old_branch} to {new_branch}"))?;

let view = workspace_command.repo().view();
if view
.remote_branches_matching(
&StringPattern::exact(old_branch),
&StringPattern::everything(),
)
.any(|(_, remote_ref)| remote_ref.is_tracking())
{
if has_tracked_remote_branches(view, old_branch) {
writeln!(
ui.warning_default(),
"Branch {old_branch} has tracking remote branches which were not renamed."
"Tracked remote branches for branch {old_branch} were not renamed.",
)?;
writeln!(
ui.hint_default(),
"to rename the branch on the remote, you can `jj git push --branch {old_branch}` \
"To rename the branch on the remote, you can `jj git push --branch {old_branch}` \
first (to delete it on the remote), and then `jj git push --branch {new_branch}`. \
`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(())
}
6 changes: 4 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,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) {
yuja marked this conversation as resolved.
Show resolved Hide resolved
new_branch_names.push(name);
}
if !args.allow_backwards && !is_fast_forward(repo, old_target, target_commit.id()) {
Expand Down
52 changes: 50 additions & 2 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 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]
Expand Down Expand Up @@ -354,8 +396,14 @@ fn test_branch_rename() {
let (_stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["branch", "rename", "bremote", "bremote2"]);
insta::assert_snapshot!(stderr, @r###"
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.
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.
"###);
}

Expand Down