Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli git push: clearer user-facing messages #3765

Merged
merged 1 commit into from
Jun 2, 2024
Merged

cli git push: clearer user-facing messages #3765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 40 additions & 28 deletions cli/src/commands/git.rs
View file
Open in desktop
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 {
ilyagr marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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