From fd934f9252664703188f9ee5334960175717e41f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 28 Oct 2024 17:43:16 +0900 Subject: [PATCH] cli: git push: exclude new bookmarks tracking other remotes if no --remote 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. #1278 --- CHANGELOG.md | 3 ++ cli/src/commands/git/push.rs | 52 ++++++++++++++++--- cli/tests/test_git_push.rs | 99 ++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d97601187..8a76b0dcf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 45e956eb5f..1729e7077b 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -155,6 +155,7 @@ 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(); @@ -162,7 +163,7 @@ pub fn cmd_git_push( 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)?, @@ -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)?, @@ -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)?, @@ -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(), @@ -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)?, @@ -491,9 +492,11 @@ impl From for CommandError { } fn classify_bookmark_update( + view: &View, bookmark_name: &str, remote_name: &str, targets: LocalAndRemoteRef, + track_new: bool, ) -> Result, RejectedBookmarkUpdateReason> { let push_action = classify_bookmark_push_action(targets); match push_action { @@ -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." + )), + }) + } } } diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 4a34c22358..2142c7af33 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -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"))]