Skip to content

Commit

Permalink
cli: git push: exclude new bookmarks tracking other remotes if no --r…
Browse files Browse the repository at this point in the history
…emote set

If you have multiple remotes to push to, you might want to keep some changes
in your private fork. Git CLI has one upstream remote per branch, but jj
supports multiple tracking remotes, and therefore "jj git push" can start
tracking new remotes automatically.

This patch makes new bookmarks not eligible for push if tracked bookmarks
already exists on other remotes. I considered adding a warning, but it's not
always possible to interrupt the push shortly after a warning is emitted. This
check can be turned off by specifying --remote=NAME explicitly. Another stricter
(and simpler in terms of implementation) idea is to force user to pass
--new-bookmark or something to push any local bookmark to new remote.

jj-vcs#1278
  • Loading branch information
yuja committed Oct 30, 2024
1 parent 377c3b5 commit fd934f9
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 6 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
* Default operation log template now shows end times of operations instead of
start times.

* `jj git push` no longer pushes tracking bookmarks to new remotes. Specify
`--remote=NAME` to bypass this restriction.

### Deprecations

### New features
Expand Down
52 changes: 46 additions & 6 deletions cli/src/commands/git/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,15 @@ pub fn cmd_git_push(
} else {
get_default_push_remote(ui, command.settings(), &git_repo)?
};
let track_new = args.remote.is_some();

let mut tx = workspace_command.start_transaction();
let view = tx.repo().view();
let tx_description;
let mut bookmark_updates = vec![];
if args.all {
for (bookmark_name, targets) in view.local_remote_bookmarks(&remote) {
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand All @@ -174,7 +175,7 @@ pub fn cmd_git_push(
if !targets.remote_ref.is_tracking() {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand All @@ -186,7 +187,7 @@ pub fn cmd_git_push(
if targets.local_target.is_present() {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand Down Expand Up @@ -223,7 +224,7 @@ pub fn cmd_git_push(
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => writeln!(
ui.status(),
Expand All @@ -246,7 +247,7 @@ pub fn cmd_git_push(
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, track_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand Down Expand Up @@ -491,9 +492,11 @@ impl From<RejectedBookmarkUpdateReason> for CommandError {
}

fn classify_bookmark_update(
view: &View,
bookmark_name: &str,
remote_name: &str,
targets: LocalAndRemoteRef,
track_new: bool,
) -> Result<Option<BookmarkPushUpdate>, RejectedBookmarkUpdateReason> {
let push_action = classify_bookmark_push_action(targets);
match push_action {
Expand All @@ -516,7 +519,44 @@ fn classify_bookmark_update(
bookmark."
)),
}),
BookmarkPushAction::Update(update) => Ok(Some(update)),
BookmarkPushAction::Update(update) => {
if !targets.remote_ref.is_tracking() && !track_new {
reject_tracking_bookmark(view, bookmark_name, remote_name)?;
}
Ok(Some(update))
}
}
}

fn reject_tracking_bookmark(
view: &View,
bookmark_name: &str,
remote_name: &str,
) -> Result<(), RejectedBookmarkUpdateReason> {
let tracking_remote_names = view
.remote_bookmarks_matching(
&StringPattern::exact(bookmark_name),
&StringPattern::everything(),
)
.filter(|&((_, remote), _)| remote != git::REMOTE_NAME_FOR_LOCAL_GIT_REPO)
.filter(|(_, remote_ref)| remote_ref.is_tracking())
.map(|((_, remote), _)| remote)
.collect_vec();
match &*tracking_remote_names {
[] => Ok(()),
[tracking_remote, ..] => {
let remote_bookmarks = tracking_remote_names
.iter()
.map(|remote| format!("{bookmark_name}@{remote}"))
.join(", ");
Err(RejectedBookmarkUpdateReason {
message: format!("Bookmark {bookmark_name} is already tracking {remote_bookmarks}"),
hint: Some(format!(
"Use --remote={tracking_remote} to update the remote bookmark. Use \
--remote={remote_name} to push to new remote."
)),
})
}
}
}

Expand Down
99 changes: 99 additions & 0 deletions cli/tests/test_git_push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,105 @@ fn test_git_push_moved_sideways_untracked() {
"###);
}

#[test]
fn test_git_push_already_tracking() {
let test_env = TestEnvironment::default();
// Set up colocated local workspace, which has the @git remote.
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "--colocate", "local"]);
let workspace_root = test_env.env_root().join("local");
for remote in ["origin", "other"] {
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", remote]);
let path = test_env
.env_root()
.join(PathBuf::from_iter([remote, ".jj", "repo", "store", "git"]));
test_env.jj_cmd_ok(
&workspace_root,
&["git", "remote", "add", remote, path.to_str().unwrap()],
);
}

// Push newly-created bookmark to default
test_env.jj_cmd_ok(&workspace_root, &["commit", "-m1"]);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "foo"]);
let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Add bookmark foo to 8ee5f4bfcc8f
"#);

// Push newly-created bookmark to other
test_env.jj_cmd_ok(&workspace_root, &["commit", "-m2"]);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "bar"]);
let (_stdout, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "--remote=other", "--bookmark=bar"],
);
insta::assert_snapshot!(stderr, @r#"
Changes to push to other:
Add bookmark bar to 6836627dd409
"#);

// Try to push tracking and newly-created bookmarks to default
test_env.jj_cmd_ok(&workspace_root, &["commit", "-m2"]);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "move", "--to=@-", "foo"]);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "-r@-", "baz"]);
let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--dry-run"]);
insta::assert_snapshot!(stderr, @r#"
Warning: Bookmark bar is already tracking bar@other
Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
Changes to push to origin:
Add bookmark baz to 8a0af1306e25
Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
Dry-run requested, not pushing.
"#);

// Try with explicit --remote argument, which bypasses the protection.
let (_stdout, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "--remote=origin", "--dry-run"],
);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Add bookmark bar to 6836627dd409
Add bookmark baz to 8a0af1306e25
Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
Dry-run requested, not pushing.
"#);

// Try with --all, which doesn't bypass the protection right now.
// TODO: Maybe okay to push all bookmarks because --all is an explicit flag?
let (_stdout, stderr) =
test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--all", "--dry-run"]);
insta::assert_snapshot!(stderr, @r#"
Warning: Bookmark bar is already tracking bar@other
Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
Changes to push to origin:
Add bookmark baz to 8a0af1306e25
Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
Dry-run requested, not pushing.
"#);

// Try with --tracked, which effectively silences the warning.
let (_stdout, stderr) =
test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--tracked", "--dry-run"]);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Move forward bookmark foo from 8ee5f4bfcc8f to 8a0af1306e25
Dry-run requested, not pushing.
"#);

// Try with --bookmark=NAME, which is an error. Git users might expect that
// the remote to push to is chosen by the tracking bookmark.
let stderr = test_env.jj_cmd_failure(
&workspace_root,
&["git", "push", "--bookmark=bar", "--dry-run"],
);
insta::assert_snapshot!(stderr, @r#"
Error: Bookmark bar is already tracking bar@other
Hint: Use --remote=other to update the remote bookmark. Use --remote=origin to push to new remote.
"#);
}

#[test]
// TODO: This test fails with libgit2 v1.8.1 on Windows.
#[cfg(not(target_os = "windows"))]
Expand Down

0 comments on commit fd934f9

Please sign in to comment.