Skip to content

Commit

Permalink
cli: git push: do not push new bookmarks by default
Browse files Browse the repository at this point in the history 
If you have multiple remotes to push to, you might want to keep some changes
(such as security patches) 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 by default. I considered
adding a warning, but it's not always possible to interrupt the push shortly
after a warning is emitted.

--all implies --allow-new because otherwise it's equivalent to --tracked. It's
also easier to write a conflict rule with --all/--deleted/--tracked than with
two of them.

-c/--change doesn't require --allow-new because it is the flag to create new
tracking bookmark.

jj-vcs#1278
  • Loading branch information
@yuja
yuja committed Nov 15, 2024
1 parent 1ed2ef0 commit 35cbc1d
Show file tree
Hide file tree
Showing 10 changed files with 219 additions and 69 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### Breaking changes

* `jj git push` no longer pushes new bookmarks by default. Use `--allow-new` to
bypass this restriction.

### Deprecations

### New features
Expand Down
57 changes: 47 additions & 10 deletions cli/src/commands/git/push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ use crate::ui::Ui;

/// Push to a Git remote
///
/// By default, pushes any bookmarks pointing to
/// By default, pushes tracking bookmarks pointing to
/// `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific
/// bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate
/// bookmark names based on the change IDs of specific commits.
Expand Down Expand Up @@ -98,7 +98,7 @@ pub struct GitPushArgs {
add = ArgValueCandidates::new(complete::local_bookmarks),
)]
bookmark: Vec<StringPattern>,
/// Push all bookmarks (including deleted bookmarks)
/// Push all bookmarks (including new and deleted bookmarks)
#[arg(long)]
all: bool,
/// Push all tracked bookmarks (including deleted bookmarks)
Expand All @@ -115,6 +115,11 @@ pub struct GitPushArgs {
/// correspond to missing local bookmarks.
#[arg(long)]
deleted: bool,
/// Allow pushing new bookmarks
///
/// Newly-created remote bookmarks will be tracked automatically.
#[arg(long, conflicts_with = "what")]
allow_new: bool,
/// Allow pushing commits with empty descriptions
#[arg(long)]
allow_empty_description: bool,
Expand All @@ -127,8 +132,9 @@ pub struct GitPushArgs {
/// Push this commit by creating a bookmark based on its change ID (can be
/// repeated)
///
/// Use the `git.push-bookmark-prefix` setting to change the prefix for
/// generated names.
/// The created bookmark will be tracked automatically. Use the
/// `git.push-bookmark-prefix` setting to change the prefix for generated
/// names.
#[arg(long, short)]
change: Vec<RevisionArg>,
/// Only display what will change on the remote
Expand Down Expand Up @@ -172,7 +178,8 @@ 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) {
let allow_new = true; // implied by --all
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand All @@ -184,7 +191,8 @@ pub fn cmd_git_push(
if !targets.remote_ref.is_tracking() {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
let allow_new = false; // doesn't matter
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand All @@ -196,7 +204,8 @@ pub fn cmd_git_push(
if targets.local_target.is_present() {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
let allow_new = false; // doesn't matter
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand Down Expand Up @@ -228,12 +237,27 @@ pub fn cmd_git_push(
(bookmark_name.as_ref(), targets)
});
let view = tx.repo().view();
for (bookmark_name, targets) in change_bookmarks {
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
let allow_new = true; // --change implies creation of remote bookmark
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => writeln!(
ui.status(),
"Bookmark {bookmark_name}@{remote} already matches {bookmark_name}",
)?,
Err(reason) => return Err(reason.into()),
}
}

let bookmarks_by_name = find_bookmarks_to_push(view, &args.bookmark, &remote)?;
for (bookmark_name, targets) in change_bookmarks.chain(bookmarks_by_name.iter().copied()) {
for &(bookmark_name, targets) in &bookmarks_by_name {
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets) {
match classify_bookmark_update(bookmark_name, &remote, targets, args.allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => writeln!(
ui.status(),
Expand All @@ -256,7 +280,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(bookmark_name, &remote, targets, args.allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => {}
Err(reason) => reason.print(ui)?,
Expand Down Expand Up @@ -504,6 +528,7 @@ fn classify_bookmark_update(
bookmark_name: &str,
remote_name: &str,
targets: LocalAndRemoteRef,
allow_new: bool,
) -> Result<Option<BookmarkPushUpdate>, RejectedBookmarkUpdateReason> {
let push_action = classify_bookmark_push_action(targets);
match push_action {
Expand All @@ -526,6 +551,18 @@ fn classify_bookmark_update(
bookmark."
)),
}),
BookmarkPushAction::Update(update) if update.old_target.is_none() && !allow_new => {
Err(RejectedBookmarkUpdateReason {
message: format!(
"Refusing to create new remote bookmark {bookmark_name}@{remote_name}"
),
hint: Some(
"Use --allow-new to push new bookmark. Use --remote to specify the remote to \
push to."
.to_owned(),
),
})
}
BookmarkPushAction::Update(update) => Ok(Some(update)),
}
}
Expand Down
9 changes: 6 additions & 3 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ Create a new Git backed repo
Push to a Git remote
By default, pushes any bookmarks pointing to `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate bookmark names based on the change IDs of specific commits.
By default, pushes tracking bookmarks pointing to `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific bookmarks. Use `--all` to push all bookmarks. Use `--change` to generate bookmark names based on the change IDs of specific commits.
Before the command actually moves, creates, or deletes a remote bookmark, it makes several [safety checks]. If there is a problem, you may need to run `jj git fetch --remote <remote name>` and/or resolve some [bookmark conflicts].
Expand All @@ -1166,19 +1166,22 @@ Before the command actually moves, creates, or deletes a remote bookmark, it mak
* `-b`, `--bookmark <BOOKMARK>` — Push only this bookmark, or bookmarks matching a pattern (can be repeated)
By default, the specified name matches exactly. Use `glob:` prefix to select bookmarks by wildcard pattern. For details, see https://martinvonz.github.io/jj/latest/revsets#string-patterns.
* `--all` — Push all bookmarks (including deleted bookmarks)
* `--all` — Push all bookmarks (including new and deleted bookmarks)
* `--tracked` — Push all tracked bookmarks (including deleted bookmarks)
This usually means that the bookmark was already pushed to or fetched from the relevant remote. For details, see https://martinvonz.github.io/jj/latest/bookmarks#remotes-and-tracked-bookmarks
* `--deleted` — Push all deleted bookmarks
Only tracked bookmarks can be successfully deleted on the remote. A warning will be printed if any untracked bookmarks on the remote correspond to missing local bookmarks.
* `--allow-new` — Allow pushing new bookmarks
Newly-created remote bookmarks will be tracked automatically.
* `--allow-empty-description` — Allow pushing commits with empty descriptions
* `--allow-private` — Allow pushing commits that are private
* `-r`, `--revisions <REVISIONS>` — Push bookmarks pointing to these commits (can be repeated)
* `-c`, `--change <CHANGE>` — Push this commit by creating a bookmark based on its change ID (can be repeated)
Use the `git.push-bookmark-prefix` setting to change the prefix for generated names.
The created bookmark will be tracked automatically. Use the `git.push-bookmark-prefix` setting to change the prefix for generated names.
* `--dry-run` — Only display what will change on the remote
Expand Down
7 changes: 4 additions & 3 deletions cli/tests/test_bookmark_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ fn test_bookmark_move() {

// Delete bookmark locally, but is still tracking remote
test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-mcommit"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-r@-"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-r@-"]);
test_env.jj_cmd_ok(&repo_path, &["bookmark", "delete", "foo"]);
insta::assert_snapshot!(get_bookmark_output(&test_env, &repo_path), @r###"
foo (deleted)
Expand Down Expand Up @@ -441,7 +441,7 @@ fn test_bookmark_rename() {
test_env.jj_cmd_ok(&repo_path, &["new"]);
test_env.jj_cmd_ok(&repo_path, &["describe", "-m=commit-2"]);
test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "bremote"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-b=bremote"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-b=bremote"]);
let (_stdout, stderr) =
test_env.jj_cmd_ok(&repo_path, &["bookmark", "rename", "bremote", "bremote2"]);
insta::assert_snapshot!(stderr, @r###"
Expand Down Expand Up @@ -1081,7 +1081,7 @@ fn test_bookmark_track_conflict() {
);
test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "main"]);
test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "a"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-b", "main"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "--allow-new", "-b", "main"]);
test_env.jj_cmd_ok(&repo_path, &["bookmark", "untrack", "main@origin"]);
test_env.jj_cmd_ok(
&repo_path,
Expand Down Expand Up @@ -1757,6 +1757,7 @@ fn test_bookmark_list_tracked() {
&[
"git",
"push",
"--allow-new",
"--remote",
"upstream",
"--bookmark",
Expand Down
5 changes: 4 additions & 1 deletion cli/tests/test_completion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ fn test_bookmark_names() {
test_env.jj_cmd_ok(&repo_path, &["desc", "-r", "aaa-tracked", "-m", "x"]);
test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "bbb-tracked"]);
test_env.jj_cmd_ok(&repo_path, &["desc", "-r", "bbb-tracked", "-m", "x"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "--bookmark", "glob:*-tracked"]);
test_env.jj_cmd_ok(
&repo_path,
&["git", "push", "--allow-new", "--bookmark", "glob:*-tracked"],
);

test_env.jj_cmd_ok(&origin_path, &["bookmark", "create", "aaa-untracked"]);
test_env.jj_cmd_ok(&origin_path, &["desc", "-r", "aaa-untracked", "-m", "x"]);
Expand Down
25 changes: 20 additions & 5 deletions cli/tests/test_git_private_commits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,14 @@ fn set_up_remote_at_main(test_env: &TestEnvironment, workspace_root: &Path, remo
);
test_env.jj_cmd_ok(
workspace_root,
&["git", "push", "--remote", remote_name, "-b=main"],
&[
"git",
"push",
"--allow-new",
"--remote",
remote_name,
"-b=main",
],
);
}

Expand Down Expand Up @@ -155,7 +162,10 @@ fn test_git_private_commits_not_directly_in_line_block_pushing() {
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "bookmark1"]);

test_env.add_config(r#"git.private-commits = "description(glob:'private*')""#);
let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "-b=bookmark1"]);
let stderr = test_env.jj_cmd_failure(
&workspace_root,
&["git", "push", "--allow-new", "-b=bookmark1"],
);
insta::assert_snapshot!(stderr, @r###"
Error: Won't push commit f1253a9b1ea9 since it is private
"###);
Expand Down Expand Up @@ -192,8 +202,10 @@ fn test_git_private_commits_already_on_the_remote_do_not_block_push() {
test_env.jj_cmd_ok(&workspace_root, &["new", "main", "-m=private 1"]);
test_env.jj_cmd_ok(&workspace_root, &["new", "-m=public 3"]);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "set", "main"]);
let (_, stderr) =
test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=main", "-b=bookmark1"]);
let (_, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "--allow-new", "-b=main", "-b=bookmark1"],
);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Move forward bookmark main from 7eb97bf230ad to fbb352762352
Expand Down Expand Up @@ -223,7 +235,10 @@ fn test_git_private_commits_already_on_the_remote_do_not_block_push() {
&["new", "description('private 1')", "-m=public 4"],
);
test_env.jj_cmd_ok(&workspace_root, &["bookmark", "create", "bookmark2"]);
let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=bookmark2"]);
let (_, stderr) = test_env.jj_cmd_ok(
&workspace_root,
&["git", "push", "--allow-new", "-b=bookmark2"],
);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Add bookmark bookmark2 to ee5b808b0b95
Expand Down
Loading

0 comments on commit 35cbc1d

Please sign in to comment.