Skip to content

Commit

Permalink
cli git push: clearer user-facing messages
Browse files Browse the repository at this point in the history 
"Move forward" instead of "Move", "Move sideways" or "Move backward"
instead of (now misleading) "Force...".
  • Loading branch information
@ilyagr
ilyagr committed Jun 2, 2024
1 parent 3090adf commit e1cccee
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 45 deletions.
68 changes: 40 additions & 28 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::HashSet;
use std::collections::{HashMap, HashSet};
use std::io::Write;
use std::path::{Path, PathBuf};
use std::sync::Arc;
Expand All @@ -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,
Expand Down Expand Up @@ -779,6 +778,13 @@ fn do_git_clone(
Ok((workspace_command, stats))
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum BranchMoveDirection {
Forward,
Backward,
Sideways,
}

fn cmd_git_push(
ui: &mut Ui,
command: &CommandHelper,
Expand Down Expand Up @@ -899,20 +905,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) {
BranchMoveDirection::Forward
} else if repo.index().is_ancestor(new_target, old_target) {
BranchMoveDirection::Backward
} else {
BranchMoveDirection::Sideways
},
);
}
}
}
Expand Down Expand Up @@ -964,21 +972,25 @@ 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)
)?;
}
let old = short_commit_hash(old_target);
let new = 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)".
let msg = match branch_push_direction.get(branch_name).unwrap() {
BranchMoveDirection::Forward => {
format!("Move forward branch {branch_name} from {old} to {new}")
}
BranchMoveDirection::Backward => {
format!("Move backward branch {branch_name} from {old} to {new}")
}
BranchMoveDirection::Sideways => {
format!("Move sideways branch {branch_name} from {old} to {new}")
}
};
writeln!(ui.status(), " {msg}")?;
}
(Some(old_target), None) => {
writeln!(
Expand Down
61 changes: 44 additions & 17 deletions cli/tests/test_git_push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,15 @@ 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.
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
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###"
Expand All @@ -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]
Expand All @@ -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
"###);
}

Expand Down Expand Up @@ -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"]);
Expand Down Expand Up @@ -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.
"###);
Expand Down Expand Up @@ -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.
"###);
Expand Down Expand Up @@ -348,7 +375,7 @@ fn test_git_push_unexpectedly_deleted() {
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 1ebe27ba04bf
Move sideways branch branch1 from 45a3aa29e907 to 1ebe27ba04bf
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.
"###);
Expand Down Expand Up @@ -425,7 +452,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
"###);
}

Expand Down Expand Up @@ -454,7 +481,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.
"###);
Expand Down Expand Up @@ -499,7 +526,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.
"###);

Expand All @@ -521,7 +548,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###"
Expand Down Expand Up @@ -564,7 +591,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
Expand All @@ -573,7 +600,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
Expand All @@ -585,7 +612,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
Expand Down Expand Up @@ -614,7 +641,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###"
Expand Down Expand Up @@ -948,7 +975,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
Expand All @@ -959,7 +986,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
"###);
}

Expand Down

0 comments on commit e1cccee

Please sign in to comment.