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.

Sign up for GitHub

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

git-push: fix bad combination of --change and --branch #3348

Merged
merged 5 commits into from
Mar 24, 2024
Merged

git-push: fix bad combination of --change and --branch #3348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
88df08a
tests: run "git push" with both --change and --branch arguments
yuja Mar 17, 2024
60d5a74
git-push: check duplicated --change branch prior to recording branch_…
yuja Mar 17, 2024
4027c64
git-push: extract function that updates push-{change_id} branches
yuja Mar 17, 2024
1cbfc2d
git-push: filter out duplicated branch names by caller
yuja Mar 17, 2024
fc87044
git-push: process --change branches first to avoid pushing old branch…
yuja Mar 17, 2024
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
127 changes: 63 additions & 64 deletions cli/src/commands/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use maplit::hashset;
use crate::cli_util::{
parse_string_pattern, print_trackable_remote_branches, short_change_hash, short_commit_hash,
start_repo_transaction, CommandHelper, RevisionArg, WorkspaceCommandHelper,
WorkspaceCommandTransaction,
};
use crate::command_error::{
user_error, user_error_with_hint, user_error_with_message, CommandError,
Expand Down Expand Up @@ -797,12 +798,6 @@ fn cmd_git_push(
};

let repo = workspace_command.repo().clone();
let change_commits: Vec<_> = args
.change
.iter()
.map(|change_str| workspace_command.resolve_single_rev(change_str))
.try_collect()?;

let mut tx = workspace_command.start_transaction();
let tx_description;
let mut branch_updates = vec![];
Expand Down Expand Up @@ -840,63 +835,29 @@ fn cmd_git_push(
}
tx_description = format!("push all deleted branches to git remote {remote}");
} else {
let mut seen_branches = hashset! {};
let branches_by_name =
find_branches_to_push(repo.view(), &args.branch, &remote, &mut seen_branches)?;
for (branch_name, targets) in branches_by_name {
match classify_branch_update(branch_name, &remote, targets) {
Ok(Some(update)) => branch_updates.push((branch_name.to_owned(), update)),
Ok(None) => writeln!(
ui.stderr(),
"Branch {branch_name}@{remote} already matches {branch_name}",
)?,
Err(reason) => return Err(reason.into()),
}
}
let mut seen_branches: HashSet<&str> = HashSet::new();

for (change_str, commit) in std::iter::zip(args.change.iter(), change_commits) {
let mut branch_name = format!(
"{}{}",
command.settings().push_branch_prefix(),
commit.change_id().hex()
);
if !seen_branches.insert(branch_name.clone()) {
continue;
}
let view = tx.base_repo().view();
if view.get_local_branch(&branch_name).is_absent() {
// A local branch with the full change ID doesn't exist already, so use the
// short ID if it's not ambiguous (which it shouldn't be most of the time).
let short_change_id = short_change_hash(commit.change_id());
if tx
.base_workspace_helper()
.resolve_single_rev(&short_change_id)
.is_ok()
{
// Short change ID is not ambiguous, so update the branch name to use it.
branch_name = format!(
"{}{}",
command.settings().push_branch_prefix(),
short_change_id
);
};
}
if view.get_local_branch(&branch_name).is_absent() {
writeln!(
ui.stderr(),
"Creating branch {} for revision {}",
branch_name,
change_str.deref()
)?;
}
tx.mut_repo()
.set_local_branch_target(&branch_name, RefTarget::normal(commit.id().clone()));
// Process --change branches first because matching branches can be moved.
let change_branch_names = update_change_branches(
ui,
&mut tx,
&args.change,
&command.settings().push_branch_prefix(),
)?;
let change_branches = change_branch_names.iter().map(|branch_name| {
let targets = LocalAndRemoteRef {
local_target: tx.repo().view().get_local_branch(&branch_name),
remote_ref: tx.repo().view().get_remote_branch(&branch_name, &remote),
local_target: tx.repo().view().get_local_branch(branch_name),
remote_ref: tx.repo().view().get_remote_branch(branch_name, &remote),
};
match classify_branch_update(&branch_name, &remote, targets) {
Ok(Some(update)) => branch_updates.push((branch_name.clone(), update)),
(branch_name.as_ref(), targets)
});
let branches_by_name = find_branches_to_push(repo.view(), &args.branch, &remote)?;
for (branch_name, targets) in change_branches.chain(branches_by_name.iter().copied()) {
if !seen_branches.insert(branch_name) {
continue;
}
match classify_branch_update(branch_name, &remote, targets) {
Ok(Some(update)) => branch_updates.push((branch_name.to_owned(), update)),
Ok(None) => writeln!(
ui.stderr(),
"Branch {branch_name}@{remote} already matches {branch_name}",
Expand All @@ -915,7 +876,7 @@ fn cmd_git_push(
use_default_revset,
)?;
for &(branch_name, targets) in &branches_targeted {
if !seen_branches.insert(branch_name.to_owned()) {
if !seen_branches.insert(branch_name) {
continue;
}
match classify_branch_update(branch_name, &remote, targets) {
Expand Down Expand Up @@ -1140,11 +1101,50 @@ fn classify_branch_update(
}
}

/// Creates or moves branches based on the change IDs.
fn update_change_branches(
ui: &Ui,
tx: &mut WorkspaceCommandTransaction,
changes: &[RevisionArg],
branch_prefix: &str,
) -> Result<Vec<String>, CommandError> {
let mut branch_names = Vec::new();
for change_str in changes {
let workspace_command = tx.base_workspace_helper();
let commit = workspace_command.resolve_single_rev(change_str)?;
let mut branch_name = format!("{branch_prefix}{}", commit.change_id().hex());
let view = tx.base_repo().view();
if view.get_local_branch(&branch_name).is_absent() {
// A local branch with the full change ID doesn't exist already, so use the
// short ID if it's not ambiguous (which it shouldn't be most of the time).
let short_change_id = short_change_hash(commit.change_id());
if workspace_command
.resolve_single_rev(&short_change_id)
.is_ok()
{
// Short change ID is not ambiguous, so update the branch name to use it.
branch_name = format!("{branch_prefix}{short_change_id}");
};
}
if view.get_local_branch(&branch_name).is_absent() {
writeln!(
ui.stderr(),
"Creating branch {} for revision {}",
branch_name,
change_str.deref()
)?;
}
tx.mut_repo()
.set_local_branch_target(&branch_name, RefTarget::normal(commit.id().clone()));
branch_names.push(branch_name);
}
Ok(branch_names)
}

fn find_branches_to_push<'a>(
view: &'a View,
branch_patterns: &[StringPattern],
remote_name: &str,
seen_branches: &mut HashSet<String>,
) -> Result<Vec<(&'a str, LocalAndRemoteRef<'a>)>, CommandError> {
let mut matching_branches = vec![];
let mut unmatched_patterns = vec![];
Expand All @@ -1160,8 +1160,7 @@ fn find_branches_to_push<'a>(
if matches.peek().is_none() {
unmatched_patterns.push(pattern);
}
matching_branches
.extend(matches.filter(|&(name, _)| seen_branches.insert(name.to_owned())));
matching_branches.extend(matches);
}
match &unmatched_patterns[..] {
[] => Ok(matching_branches),
Expand Down
53 changes: 51 additions & 2 deletions cli/tests/test_git_push.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,55 @@ fn test_git_push_changes() {
Branch changes to push to origin:
Force branch push-yostqsxwqrlt from 48d8c7948133 to b5f030322b1d
"###);

// specifying the same branch with --change/--branch doesn't break things
std::fs::write(workspace_root.join("file"), "modified4").unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "-c=@", "-b=push-yostqsxwqrlt"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Branch changes to push to origin:
Force branch push-yostqsxwqrlt from b5f030322b1d to 4df62cec2ee4
"###);

// try again with --change that moves the branch forward
std::fs::write(workspace_root.join("file"), "modified5").unwrap();
test_env.jj_cmd_ok(
&workspace_root,
&[
"branch",
"set",
"-r=@-",
"--allow-backwards",
"push-yostqsxwqrlt",
],
);
let stdout = test_env.jj_cmd_success(&workspace_root, &["status"]);
insta::assert_snapshot!(stdout, @r###"
Working copy changes:
M file
Working copy : yostqsxw 3e2ce808 bar
Parent commit: yqosqzyt fa16a141 push-yostqsxwqrlt* push-yqosqzytrlsw | foo
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "-c=@", "-b=push-yostqsxwqrlt"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Branch changes to push to origin:
Force branch push-yostqsxwqrlt from 4df62cec2ee4 to 3e2ce808759b
"###);
let stdout = test_env.jj_cmd_success(&workspace_root, &["status"]);
insta::assert_snapshot!(stdout, @r###"
Working copy changes:
M file
Working copy : yostqsxw 3e2ce808 push-yostqsxwqrlt | bar
Parent commit: yqosqzyt fa16a141 push-yqosqzytrlsw | foo
"###);

// Test changing `git.push-branch-prefix`. It causes us to push again.
let (stdout, stderr) = test_env.jj_cmd_ok(
&workspace_root,
Expand All @@ -436,7 +485,7 @@ fn test_git_push_changes() {
insta::assert_snapshot!(stderr, @r###"
Creating branch test-yostqsxwqrlt for revision @
Branch changes to push to origin:
Add branch test-yostqsxwqrlt to b5f030322b1d
Add branch test-yostqsxwqrlt to 3e2ce808759b
"###);
}

Expand Down Expand Up @@ -531,8 +580,8 @@ fn test_git_push_mixed() {
insta::assert_snapshot!(stderr, @r###"
Creating branch push-yqosqzytrlsw for revision @--
Branch changes to push to origin:
Add branch branch-1 to 7decc7932d9c
Add branch push-yqosqzytrlsw to fa16a14170fb
Add branch branch-1 to 7decc7932d9c
Add branch branch-2a to 1b45449e18d0
Add branch branch-2b to 1b45449e18d0
"###);
Expand Down