Skip to content

Commit

Permalink
cli git push: clearer user-facing messages TODO: test backward
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 May 26, 2024
1 parent f438a21 commit e1511cf
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 44 deletions.
54 changes: 26 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 @@ -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"
},
);
}
}
}
Expand Down Expand Up @@ -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!(
Expand Down
32 changes: 16 additions & 16 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 Down Expand Up @@ -127,7 +127,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 +187,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 +232,7 @@ fn test_git_push_not_fast_forward() {
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 +272,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 @@ -370,7 +370,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 @@ -399,7 +399,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 @@ -444,7 +444,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 @@ -466,7 +466,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 @@ -509,7 +509,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 @@ -518,7 +518,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 @@ -530,7 +530,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 @@ -559,7 +559,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 @@ -893,7 +893,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 @@ -904,7 +904,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 e1511cf

Please sign in to comment.