Skip to content

Commit

Permalink
cli: push new non-tracking bookmarks by default, rename --allow-new flag
Browse files Browse the repository at this point in the history 
This partially reverts 296961c "cli: git push: do not push new bookmarks by
default." Apparently, the restriction introduced by that patch was too strict
for some use cases. The new rule is that any non-tracking (or local-only)
bookmarks will be pushed to new remote by default, but --allow-new-tracking is
required if a bookmark is already tracking other (real) remotes.

Closes jj-vcs#5094
  • Loading branch information
@yuja
yuja committed Dec 22, 2024
1 parent 2aaf6cc commit e5ab0d0
Show file tree
Hide file tree
Showing 10 changed files with 213 additions and 169 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
* `jj absorb` now abandons the source commit if it becomes empty and has no
description.

* `jj git push` now pushes new non-tracking bookmarks by default. The
`--allow-new` flag (introduced by 0.24.0) and relates changes are reverted.
New `--allow-new-tracking` flag is required in order to push tracking
bookmarks to other remotes. [#5094](https://github.com/jj-vcs/jj/issues/5094)

### Deprecations

* `--config-toml=TOML` is deprecated in favor of `--config=NAME=VALUE` and
Expand Down
40 changes: 26 additions & 14 deletions cli/src/commands/git/push.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ use crate::ui::Ui;

/// Push to a Git remote
///
/// By default, pushes tracking bookmarks pointing to
/// `remote_bookmarks(remote=<remote>)..@`. Use `--bookmark` to push specific
/// By default, pushes new non-tracking and existing 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 @@ -117,11 +117,11 @@ pub struct GitPushArgs {
/// correspond to missing local bookmarks.
#[arg(long)]
deleted: bool,
/// Allow pushing new bookmarks
/// Allow pushing tracking bookmarks to new remote
///
/// Newly-created remote bookmarks will be tracked automatically.
#[arg(long, short = 'N', conflicts_with = "what")]
allow_new: bool,
allow_new_tracking: bool,
/// Allow pushing commits with empty descriptions
#[arg(long)]
allow_empty_description: bool,
Expand Down Expand Up @@ -181,7 +181,7 @@ pub fn cmd_git_push(
if args.all {
for (bookmark_name, targets) in view.local_remote_bookmarks(&remote) {
let allow_new = true; // implied by --all
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
match classify_bookmark_update(view, 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 @@ -194,7 +194,7 @@ pub fn cmd_git_push(
continue;
}
let allow_new = false; // doesn't matter
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
match classify_bookmark_update(view, 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 @@ -207,7 +207,7 @@ pub fn cmd_git_push(
continue;
}
let allow_new = false; // doesn't matter
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
match classify_bookmark_update(view, 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 @@ -234,7 +234,7 @@ pub fn cmd_git_push(
continue;
}
let allow_new = true; // --change implies creation of remote bookmark
match classify_bookmark_update(bookmark_name, &remote, targets, allow_new) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => writeln!(
ui.status(),
Expand All @@ -244,12 +244,13 @@ pub fn cmd_git_push(
}
}

let allow_new = args.allow_new_tracking;
let bookmarks_by_name = find_bookmarks_to_push(view, &args.bookmark, &remote)?;
for &(bookmark_name, targets) in &bookmarks_by_name {
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets, args.allow_new) {
match classify_bookmark_update(view, bookmark_name, &remote, targets, allow_new) {
Ok(Some(update)) => bookmark_updates.push((bookmark_name.to_owned(), update)),
Ok(None) => writeln!(
ui.status(),
Expand All @@ -272,7 +273,7 @@ pub fn cmd_git_push(
if !seen_bookmarks.insert(bookmark_name) {
continue;
}
match classify_bookmark_update(bookmark_name, &remote, targets, args.allow_new) {
match classify_bookmark_update(view, 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 @@ -549,11 +550,20 @@ impl From<RejectedBookmarkUpdateReason> for CommandError {
}

fn classify_bookmark_update(
view: &View,
bookmark_name: &str,
remote_name: &str,
targets: LocalAndRemoteRef,
allow_new: bool,
) -> Result<Option<BookmarkPushUpdate>, RejectedBookmarkUpdateReason> {
let any_tracking = || {
view.remote_bookmarks_matching(
&StringPattern::exact(bookmark_name),
&StringPattern::everything(),
)
.filter(|&((_, remote), _)| remote != git::REMOTE_NAME_FOR_LOCAL_GIT_REPO)
.any(|(_, remote_ref)| remote_ref.is_tracking())
};
let push_action = classify_bookmark_push_action(targets);
match push_action {
BookmarkPushAction::AlreadyMatches => Ok(None),
Expand All @@ -575,14 +585,16 @@ fn classify_bookmark_update(
bookmark."
)),
}),
BookmarkPushAction::Update(update) if update.old_target.is_none() && !allow_new => {
BookmarkPushAction::Update(update)
if update.old_target.is_none() && !allow_new && any_tracking() =>
{
Err(RejectedBookmarkUpdateReason {
message: format!(
"Refusing to create new remote bookmark {bookmark_name}@{remote_name}"
"Refusing to track new remote {remote_name} by bookmark {bookmark_name}"
),
hint: Some(
"Use --allow-new to push new bookmark. Use --remote to specify the remote to \
push to."
"Use --allow-new-tracking to push bookmark to new remote. Use --remote to \
specify the remote to push to."
.to_owned(),
),
})
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ Create a new Git backed repo
Push to a Git remote
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.
By default, pushes new non-tracking and existing 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.
Unlike in Git, the remote to push to is not derived from the tracked remote bookmarks. Use `--remote` to select the remote Git repository by name. There is no option to push to multiple remotes.
Expand All @@ -1181,7 +1181,7 @@ Before the command actually moves, creates, or deletes a remote bookmark, it mak
* `--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.
* `-N`, `--allow-new` — Allow pushing new bookmarks
* `-N`, `--allow-new-tracking` — Allow pushing tracking bookmarks to new remote
Newly-created remote bookmarks will be tracked automatically.
* `--allow-empty-description` — Allow pushing commits with empty descriptions
Expand Down
14 changes: 6 additions & 8 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", "--allow-new", "-r@-"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-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", "--allow-new", "-b=bremote"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-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", "--allow-new", "-b", "main"]);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "-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,11 +1757,9 @@ fn test_bookmark_list_tracked() {
&[
"git",
"push",
"--allow-new",
"--remote",
"upstream",
"--bookmark",
"remote-unsync",
"--allow-new-tracking",
"--remote=upstream",
"--bookmark=remote-unsync",
],
);
test_env.jj_cmd_ok(
Expand Down
5 changes: 1 addition & 4 deletions cli/tests/test_completion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,7 @@ 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", "--allow-new", "--bookmark", "glob:*-tracked"],
);
test_env.jj_cmd_ok(&repo_path, &["git", "push", "--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
18 changes: 5 additions & 13 deletions cli/tests/test_git_private_commits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn set_up_remote_at_main(test_env: &TestEnvironment, workspace_root: &Path, remo
&[
"git",
"push",
"--allow-new",
"--allow-new-tracking",
"--remote",
remote_name,
"-b=main",
Expand Down Expand Up @@ -166,10 +166,7 @@ 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", "--allow-new", "-b=bookmark1"],
);
let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "-b=bookmark1"]);
insta::assert_snapshot!(stderr, @r"
Error: Won't push commit f1253a9b1ea9 since it is private
Hint: Rejected commit: yqosqzyt f1253a9b (empty) private 1
Expand Down Expand Up @@ -208,10 +205,8 @@ 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", "--allow-new", "-b=main", "-b=bookmark1"],
);
let (_, stderr) =
test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-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 @@ -241,10 +236,7 @@ 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", "--allow-new", "-b=bookmark2"],
);
let (_, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "-b=bookmark2"]);
insta::assert_snapshot!(stderr, @r#"
Changes to push to origin:
Add bookmark bookmark2 to ee5b808b0b95
Expand Down
Loading

0 comments on commit e5ab0d0

Please sign in to comment.