diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 36601f7e98..147b6abe09 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -25,6 +25,7 @@ use jj_lib::file_util; use jj_lib::git::{ self, parse_gitmodules, GitBranchPushTargets, GitFetchError, GitFetchStats, GitPushError, }; +use jj_lib::index::Index; use jj_lib::object_id::ObjectId; use jj_lib::op_store::RefTarget; use jj_lib::refs::{ @@ -795,11 +796,11 @@ fn cmd_git_push( let repo = workspace_command.repo().clone(); let mut tx = workspace_command.start_transaction(); let tx_description; - let mut branch_updates = vec![]; + let mut labeled_branch_updates = vec![]; if args.all { for (branch_name, targets) in repo.view().local_remote_branches(&remote) { - match classify_branch_update(branch_name, &remote, targets) { - Ok(Some(update)) => branch_updates.push((branch_name.to_owned(), update)), + match classify_branch_update(branch_name, &remote, targets, repo.index()) { + Ok(Some(update)) => labeled_branch_updates.push((branch_name.to_owned(), update)), Ok(None) => {} Err(reason) => reason.print(ui)?, } @@ -810,8 +811,8 @@ fn cmd_git_push( if !targets.remote_ref.is_tracking() { continue; } - match classify_branch_update(branch_name, &remote, targets) { - Ok(Some(update)) => branch_updates.push((branch_name.to_owned(), update)), + match classify_branch_update(branch_name, &remote, targets, repo.index()) { + Ok(Some(update)) => labeled_branch_updates.push((branch_name.to_owned(), update)), Ok(None) => {} Err(reason) => reason.print(ui)?, } @@ -822,8 +823,8 @@ fn cmd_git_push( if targets.local_target.is_present() { continue; } - match classify_branch_update(branch_name, &remote, targets) { - Ok(Some(update)) => branch_updates.push((branch_name.to_owned(), update)), + match classify_branch_update(branch_name, &remote, targets, repo.index()) { + Ok(Some(update)) => labeled_branch_updates.push((branch_name.to_owned(), update)), Ok(None) => {} Err(reason) => reason.print(ui)?, } @@ -851,8 +852,8 @@ fn cmd_git_push( 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)), + match classify_branch_update(branch_name, &remote, targets, repo.index()) { + Ok(Some(update)) => labeled_branch_updates.push((branch_name.to_owned(), update)), Ok(None) => writeln!( ui.status(), "Branch {branch_name}@{remote} already matches {branch_name}", @@ -874,8 +875,8 @@ fn cmd_git_push( 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)), + match classify_branch_update(branch_name, &remote, targets, repo.index()) { + Ok(Some(update)) => labeled_branch_updates.push((branch_name.to_owned(), update)), Ok(None) => {} Err(reason) => reason.print(ui)?, } @@ -884,7 +885,7 @@ fn cmd_git_push( tx_description = format!( "push {} to git remote {}", make_branch_term( - &branch_updates + &labeled_branch_updates .iter() .map(|(branch, _)| branch.as_str()) .collect_vec() @@ -892,29 +893,15 @@ fn cmd_git_push( &remote ); } - if branch_updates.is_empty() { + if labeled_branch_updates.is_empty() { writeln!(ui.status(), "Nothing changed.")?; return Ok(()); } let mut new_heads = vec![]; - let mut branch_push_direction = HashMap::new(); - for (branch_name, update) in &branch_updates { + for (_branch_name, (update, _message)) in &labeled_branch_updates { if let Some(new_target) = &update.new_target { new_heads.push(new_target.clone()); - if let Some(old_target) = &update.old_target { - assert_ne!(old_target, new_target); - branch_push_direction.insert( - branch_name.to_string(), - if repo.index().is_ancestor(old_target, new_target) { - "forward" - } else if repo.index().is_ancestor(new_target, old_target) { - "backward" - } else { - "sideways" - }, - ); - } } } @@ -962,40 +949,10 @@ fn cmd_git_push( } writeln!(ui.status(), "Branch changes to push to {}:", &remote)?; - for (branch_name, update) in &branch_updates { - match (&update.old_target, &update.new_target) { - (Some(old_target), Some(new_target)) => { - // TODO(ilyagr): Add color. Once there is color, "Move branch ... sideways" may - // read more naturally than "Move sideways branch ...". Without color, it's hard - // to see at a glance if one branch among many was moved sideways (say). - // TODO: People on Discord suggest "Move branch ... forward by n commits", - // possibly "Move branch ... sideways (X forward, Y back)". - writeln!( - ui.status(), - " Move {dir} branch {branch_name} from {old} to {new}", - dir = branch_push_direction.get(branch_name).unwrap(), - old = short_commit_hash(old_target), - new = short_commit_hash(new_target) - )?; - } - (Some(old_target), None) => { - writeln!( - ui.status(), - " Delete branch {branch_name} from {}", - short_commit_hash(old_target) - )?; - } - (None, Some(new_target)) => { - writeln!( - ui.status(), - " Add branch {branch_name} to {}", - short_commit_hash(new_target) - )?; - } - (None, None) => { - panic!("Not pushing any change to branch {branch_name}"); - } - } + let mut branch_updates = vec![]; + for (branch_name, (update, message)) in labeled_branch_updates { + writeln!(ui.status(), " {}", message)?; + branch_updates.push((branch_name, update)); } if args.dry_run { @@ -1076,11 +1033,16 @@ impl From for CommandError { } } +// TODO: This could become a formatted string or a function that takes a labeled +// output as a parameter if we add color to these messages. +type BranchPushLabel = String; + fn classify_branch_update( branch_name: &str, remote_name: &str, targets: LocalAndRemoteRef, -) -> Result, RejectedBranchUpdateReason> { + index: &dyn Index, +) -> Result, RejectedBranchUpdateReason> { let push_action = classify_branch_push_action(targets); match push_action { BranchPushAction::AlreadyMatches => Ok(None), @@ -1100,7 +1062,55 @@ fn classify_branch_update( "Run `jj branch track {branch_name}@{remote_name}` to import the remote branch." )), }), - BranchPushAction::Update(update) => Ok(Some(update)), + BranchPushAction::Update(update) => Ok(Some(( + update.clone(), + // TODO(ilyagr): Add color. Once there is color, "Move branch ... + // sideways" may read more naturally than "Move sideways branch + // ...". Without color, it's hard to see at a glance if one branch + // among many was moved sideways (say). + // TODO: People on Discord suggest "Move branch ... forward by n + // commits", possibly "Move branch ... sideways (X forward, Y + // back)". + match (&update.old_target, &update.new_target) { + (Some(old_target), Some(new_target)) => { + assert_ne!(old_target, new_target); + if index.is_ancestor(old_target, new_target) { + format!( + "Move forward branch {branch_name} from {old} to {new}", + old = short_commit_hash(old_target), + new = short_commit_hash(new_target) + ) + } else if index.is_ancestor(new_target, old_target) { + format!( + "Move backward branch {branch_name} from {old} to {new}", + old = short_commit_hash(old_target), + new = short_commit_hash(new_target) + ) + } else { + format!( + "Move sideways branch {branch_name} from {old} to {new}", + old = short_commit_hash(old_target), + new = short_commit_hash(new_target) + ) + } + } + (Some(old_target), None) => { + format!( + "Delete branch {branch_name} from {}", + short_commit_hash(old_target) + ) + } + (None, Some(new_target)) => { + format!( + "Add branch {branch_name} to {}", + short_commit_hash(new_target) + ) + } + (None, None) => { + panic!("Not pushing any change to branch {branch_name}") + } + }, + ))), } }