From 4ca75ebb2e1c86505e997823916afe76eb481bd7 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 21 May 2024 20:32:45 -0700 Subject: [PATCH] cli `git push`: clearer user-facing messages "Move forward" instead of "Move", "Move sideways" or "Move backward" instead of (now misleading) "Force...". --- cli/src/commands/git.rs | 54 +++++++++++++++++----------------- cli/tests/test_git_push.rs | 59 +++++++++++++++++++++++++++----------- 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 994868fbf6..36601f7e98 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::HashSet; +use std::collections::{HashMap, HashSet}; use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -37,7 +37,6 @@ use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::str_util::StringPattern; use jj_lib::view::View; use jj_lib::workspace::Workspace; -use maplit::hashset; use crate::cli_util::{ print_trackable_remote_branches, short_change_hash, short_commit_hash, start_repo_transaction, @@ -899,20 +898,22 @@ fn cmd_git_push( } let mut new_heads = vec![]; - // Short-term TODO: `force_pushed_branches` no longer has an effect on how - // branches are actually pushed. Messages about "Moving" vs "Forcing" - // branches are now misleading. Change this logic so that we print whether - // the branch moved forward, backward, or sideways. - let mut force_pushed_branches = hashset! {}; + let mut branch_push_direction = HashMap::new(); for (branch_name, update) in &branch_updates { if let Some(new_target) = &update.new_target { new_heads.push(new_target.clone()); - let force = match &update.old_target { - None => false, - Some(old_target) => !repo.index().is_ancestor(old_target, new_target), - }; - if force { - force_pushed_branches.insert(branch_name.to_string()); + 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" + }, + ); } } } @@ -964,21 +965,18 @@ fn cmd_git_push( for (branch_name, update) in &branch_updates { match (&update.old_target, &update.new_target) { (Some(old_target), Some(new_target)) => { - if force_pushed_branches.contains(branch_name) { - writeln!( - ui.status(), - " Force branch {branch_name} from {} to {}", - short_commit_hash(old_target), - short_commit_hash(new_target) - )?; - } else { - writeln!( - ui.status(), - " Move branch {branch_name} from {} to {}", - short_commit_hash(old_target), - short_commit_hash(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!( diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 26191476cd..87776990a9 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -91,7 +91,7 @@ fn test_git_push_current_branch() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Move branch branch2 from 8476341eb395 to 10ee3363b259 + Move forward branch branch2 from 8476341eb395 to 10ee3363b259 Add branch my-branch to 10ee3363b259 Dry-run requested, not pushing. "###); @@ -99,7 +99,7 @@ fn test_git_push_current_branch() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Move branch branch2 from 8476341eb395 to 10ee3363b259 + Move forward branch branch2 from 8476341eb395 to 10ee3363b259 Add branch my-branch to 10ee3363b259 "###); insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###" @@ -110,6 +110,33 @@ fn test_git_push_current_branch() { my-branch: yostqsxw 10ee3363 (empty) foo @origin: yostqsxw 10ee3363 (empty) foo "###); + + // Try pushing backwards + test_env.jj_cmd_success( + &workspace_root, + &[ + "branch", + "set", + "branch2", + "-rbranch2-", + "--allow-backwards", + ], + ); + // This behavior is a strangeness of our definition of the default push revset. + // We could consider changing it. + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Warning: No branches found in the default push revset: remote_branches(remote=origin)..@ + Nothing changed. + "###); + // We can move a branch backwards + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-bbranch2"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Branch changes to push to origin: + Move backward branch branch2 from 10ee3363b259 to 8476341eb395 + "###); } #[test] @@ -127,7 +154,7 @@ fn test_git_push_parent_branch() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch branch1 from 45a3aa29e907 to d47326d59ee1 + Move sideways branch branch1 from 45a3aa29e907 to d47326d59ee1 "###); } @@ -187,7 +214,7 @@ fn test_git_push_other_remote_has_branch() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch branch1 from 45a3aa29e907 to 50421a29358a + Move sideways branch branch1 from 45a3aa29e907 to 50421a29358a "###); // Since it's already pushed to origin, nothing will happen if push again let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]); @@ -232,7 +259,7 @@ fn test_git_push_forward_unexpectedly_moved() { let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Move branch branch1 from 45a3aa29e907 to c35839cb8e8c + Move forward branch branch1 from 45a3aa29e907 to c35839cb8e8c Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1 Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again. "###); @@ -272,7 +299,7 @@ fn test_git_push_sideways_unexpectedly_moved() { let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch branch1 from 45a3aa29e907 to 0f8bf988588e + Move sideways branch branch1 from 45a3aa29e907 to 0f8bf988588e Error: Refusing to push a branch that unexpectedly moved on the remote. Affected refs: refs/heads/branch1 Hint: Try fetching from the remote, then make the branch point to where you want it to be, and push again. "###); @@ -370,7 +397,7 @@ fn test_git_push_locally_created_and_rewritten() { let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch my from fcc999921ce9 to bde1d2e44b2a + Move sideways branch my from fcc999921ce9 to bde1d2e44b2a "###); } @@ -399,7 +426,7 @@ fn test_git_push_multiple() { insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: Delete branch branch1 from 45a3aa29e907 - Force branch branch2 from 8476341eb395 to 15dcdaa4f12f + Move sideways branch branch2 from 8476341eb395 to 15dcdaa4f12f Add branch my-branch to 15dcdaa4f12f Dry-run requested, not pushing. "###); @@ -444,7 +471,7 @@ fn test_git_push_multiple() { insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: Delete branch branch1 from 45a3aa29e907 - Force branch branch2 from 8476341eb395 to 15dcdaa4f12f + Move sideways branch branch2 from 8476341eb395 to 15dcdaa4f12f Dry-run requested, not pushing. "###); @@ -466,7 +493,7 @@ fn test_git_push_multiple() { insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: Delete branch branch1 from 45a3aa29e907 - Force branch branch2 from 8476341eb395 to 15dcdaa4f12f + Move sideways branch branch2 from 8476341eb395 to 15dcdaa4f12f Add branch my-branch to 15dcdaa4f12f "###); insta::assert_snapshot!(get_branch_output(&test_env, &workspace_root), @r###" @@ -509,7 +536,7 @@ fn test_git_push_changes() { insta::assert_snapshot!(stderr, @r###" Creating branch push-yqosqzytrlsw for revision @- Branch changes to push to origin: - Force branch push-yostqsxwqrlt from 28d7620ea63a to 48d8c7948133 + Move sideways branch push-yostqsxwqrlt from 28d7620ea63a to 48d8c7948133 Add branch push-yqosqzytrlsw to fa16a14170fb "###); // specifying the same change twice doesn't break things @@ -518,7 +545,7 @@ fn test_git_push_changes() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch push-yostqsxwqrlt from 48d8c7948133 to b5f030322b1d + Move sideways branch push-yostqsxwqrlt from 48d8c7948133 to b5f030322b1d "###); // specifying the same branch with --change/--branch doesn't break things @@ -530,7 +557,7 @@ fn test_git_push_changes() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch push-yostqsxwqrlt from b5f030322b1d to 4df62cec2ee4 + Move sideways branch push-yostqsxwqrlt from b5f030322b1d to 4df62cec2ee4 "###); // try again with --change that moves the branch forward @@ -559,7 +586,7 @@ fn test_git_push_changes() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Branch changes to push to origin: - Force branch push-yostqsxwqrlt from 4df62cec2ee4 to 3e2ce808759b + Move sideways branch push-yostqsxwqrlt from 4df62cec2ee4 to 3e2ce808759b "###); let stdout = test_env.jj_cmd_success(&workspace_root, &["status"]); insta::assert_snapshot!(stdout, @r###" @@ -893,7 +920,7 @@ fn test_git_push_conflicting_branches() { Warning: Branch branch2 is conflicted Hint: Run `jj branch list` to inspect, and use `jj branch set` to fix it up. Branch changes to push to origin: - Move branch branch1 from 45a3aa29e907 to fd1d63e031ea + Move forward branch branch1 from 45a3aa29e907 to fd1d63e031ea "###); // --revisions shouldn't be blocked by conflicting branch @@ -904,7 +931,7 @@ fn test_git_push_conflicting_branches() { Warning: Branch branch2 is conflicted Hint: Run `jj branch list` to inspect, and use `jj branch set` to fix it up. Branch changes to push to origin: - Move branch branch1 from fd1d63e031ea to 8263cf992d33 + Move forward branch branch1 from fd1d63e031ea to 8263cf992d33 "###); }