Skip to content

Commit

Permalink
cli git push: no-op refactor of message printing
Browse files Browse the repository at this point in the history 
Attempting to simplify the code a bit.
  • Loading branch information
@ilyagr
ilyagr committed Jun 1, 2024
1 parent 80ad672 commit f2ed9ac
Showing 1 changed file with 65 additions and 65 deletions.
130 changes: 65 additions & 65 deletions cli/src/commands/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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::{
Expand Down Expand Up @@ -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)?,
}
Expand All @@ -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)?,
}
Expand All @@ -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)?,
}
Expand Down Expand Up @@ -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}",
Expand All @@ -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)?,
}
Expand All @@ -884,37 +885,23 @@ 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()
),
&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"
},
);
}
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1076,11 +1033,16 @@ impl From<RejectedBranchUpdateReason> 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<Option<BranchPushUpdate>, RejectedBranchUpdateReason> {
index: &dyn Index,
) -> Result<Option<(BranchPushUpdate, BranchPushLabel)>, RejectedBranchUpdateReason> {
let push_action = classify_branch_push_action(targets);
match push_action {
BranchPushAction::AlreadyMatches => Ok(None),
Expand All @@ -1100,7 +1062,45 @@ 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)) => {
let old = short_commit_hash(old_target);
let new = short_commit_hash(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}")
} else if index.is_ancestor(new_target, old_target) {
format!("Move backward branch {branch_name} from {old} to {new}")
} else {
format!("Move sideways branch {branch_name} from {old} to {new}")
}
}
(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!("old_target and new_target should not be equal, but both are None")
}
},
))),
}
}

Expand Down

0 comments on commit f2ed9ac

Please sign in to comment.