cli: make jj git push default to -r 'remote_branches()..@'

The way `jj git push` without arguments chooses branches pointing to
either `@` or `@-` is unusual and difficult to explain. Now that we
have `-r`, we could instead default it to `-r '@-::@'`. However, I
think it seems likely that users will want to push all local branches
leading up to `@` from the closest remote branch. That's typically
what I want. This patch changes the default to do that.

CHANGELOG.md

@@ -44,6 +44,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   function parameter. For example, `author(foo@)` is now an error, and
   a revset alias `'revset-aliases.foo@' = '@'` will be ignored. 
 
+* `jj git push` will now push all branches in the range `remote_branches()..@`
+  instead of only branches pointing to `@` or `@-`.
+
 ### New features
 
 * Default template for `jj log` now does not show irrelevant information

cli/src/commands/git.rs

@@ -16,7 +16,7 @@ use jj_lib::op_store::{BranchTarget, RefTarget};
 use jj_lib::refs::{classify_branch_push_action, BranchPushAction, BranchPushUpdate};
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::revset::{self, RevsetIteratorExt as _};
+use jj_lib::revset::{self, RevsetExpression, RevsetIteratorExt as _, StringPattern};
 use jj_lib::settings::{ConfigResultExt as _, UserSettings};
 use jj_lib::store::Store;
 use jj_lib::view::View;
@@ -122,10 +122,10 @@ pub struct GitCloneArgs {
 
 /// Push to a Git remote
 ///
-/// By default, pushes any branches pointing to `@`, or `@-` if no branches
+/// By default, pushes any branches pointing to
-/// point to `@`. Use `--branch` to push specific branches. Use `--all` to push
+/// `remote_branches(remote=<remote>)..@`. Use `--branch` to push specific
-/// all branches. Use `--change` to generate branch names based on the change
+/// branches. Use `--all` to push all branches. Use `--change` to generate
-/// IDs of specific commits.
+/// branch names based on the change IDs of specific commits.
 #[derive(clap::Args, Clone, Debug)]
 #[command(group(ArgGroup::new("specific").args(&["branch", "change", "revisions"]).multiple(true)))]
 #[command(group(ArgGroup::new("what").args(&["all", "deleted"]).conflicts_with("specific")))]
@@ -703,7 +703,7 @@ fn cmd_git_push(
             }
         }
         tx_description = format!("push all deleted branches to git remote {remote}");
-    } else if !args.branch.is_empty() || !args.change.is_empty() || !args.revisions.is_empty() {
+    } else {
         let mut seen_branches = hashset! {};
         for branch_name in &args.branch {
             if !seen_branches.insert(branch_name.clone()) {
@@ -771,13 +771,30 @@ fn cmd_git_push(
             }
         }
 
-        // TODO: Narrow search space to local target commits.
+        let use_default_revset =
-        // TODO: Remove redundant CommitId -> Commit -> CommitId round trip.
+            args.branch.is_empty() && args.change.is_empty() && args.revisions.is_empty();
-        let revision_commit_ids: HashSet<_> =
+        let revision_commit_ids: HashSet<_> = if use_default_revset {
+            let Some(wc_commit_id) = wc_commit_id else {
+                return Err(user_error("Nothing checked out in this workspace"));
+            };
+            let current_branches_expression = RevsetExpression::remote_branches(
+                StringPattern::everything(),
+                StringPattern::Exact(remote.to_owned()),
+            )
+            .range(&RevsetExpression::commit(wc_commit_id))
+            .intersection(&RevsetExpression::branches(StringPattern::everything()));
+            let current_branches_revset = tx
+                .base_workspace_helper()
+                .evaluate_revset(current_branches_expression)?;
+            current_branches_revset.iter().collect()
+        } else {
+            // TODO: Narrow search space to local target commits.
+            // TODO: Remove redundant CommitId -> Commit -> CommitId round trip.
             resolve_multiple_nonempty_revsets(&args.revisions, tx.base_workspace_helper(), ui)?
                 .iter()
                 .map(|commit| commit.id().clone())
-                .collect();
+                .collect()
+        };
         let branches_targeted =
             find_branches_targeting(repo.view(), |id| revision_commit_ids.contains(id));
         for &(branch_name, branch_target) in &branches_targeted {
@@ -790,7 +807,7 @@ fn cmd_git_push(
                 Err(message) => writeln!(ui.warning(), "{message}")?,
             }
         }
-        if !args.revisions.is_empty() && branches_targeted.is_empty() {
+        if (!args.revisions.is_empty() || use_default_revset) && branches_targeted.is_empty() {
             writeln!(
                 ui.warning(),
                 "No branches point to the specified revisions."
@@ -807,39 +824,7 @@ fn cmd_git_push(
             ),
             &remote
         );
-    } else {
-        match wc_commit_id {
-            None => {
-                return Err(user_error("Nothing checked out in this workspace"));
-            }
-            Some(wc_commit) => {
-                // Search for branches targeting @
-                let mut branches = find_branches_targeting(repo.view(), |id| id == &wc_commit);
-                if branches.is_empty() {
-                    // Try @- instead if @ is discardable
-                    let commit = repo.store().get_commit(&wc_commit)?;
-                    if commit.is_discardable() {
-                        if let [parent_commit_id] = commit.parent_ids() {
-                            branches =
-                                find_branches_targeting(repo.view(), |id| id == parent_commit_id);
-                        }
-                    }
-                }
-                if branches.is_empty() {
-                    return Err(user_error("No current branch."));
-                }
-                for (branch_name, branch_target) in branches {
-                    match classify_branch_update(branch_name, branch_target, &remote) {
-                        Ok(Some(update)) => branch_updates.push((branch_name.clone(), update)),
-                        Ok(None) => {}
-                        Err(message) => return Err(user_error(message)),
-                    }
-                }
-            }
-        }
-        tx_description = format!("push current branch(es) to git remote {}", &remote);
     }
-
     if branch_updates.is_empty() {
         writeln!(ui, "Nothing changed.")?;
         return Ok(());

cli/tests/test_git_push.rs

@@ -110,7 +110,8 @@ fn test_git_push_parent_branch() {
         &workspace_root,
         &["describe", "-m", "modified branch1 commit"],
     );
-    test_env.jj_cmd_success(&workspace_root, &["new"]);
+    test_env.jj_cmd_success(&workspace_root, &["new", "-m", "non-empty description"]);
+    std::fs::write(workspace_root.join("file"), "file").unwrap();
     let stdout = test_env.jj_cmd_success(&workspace_root, &["git", "push"]);
     insta::assert_snapshot!(stdout, @r###"
     Branch changes to push to origin:
@@ -119,54 +120,79 @@ fn test_git_push_parent_branch() {
 }
 
 #[test]
-fn test_git_no_push_parent_branch_non_empty_commit() {
+fn test_git_push_no_matching_branch() {
-    let (test_env, workspace_root) = set_up();
-    test_env.jj_cmd_success(&workspace_root, &["edit", "branch1"]);
-    test_env.jj_cmd_success(
-        &workspace_root,
-        &["describe", "-m", "modified branch1 commit"],
-    );
-    test_env.jj_cmd_success(&workspace_root, &["new"]);
-    std::fs::write(workspace_root.join("file"), "file").unwrap();
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Error: No current branch.
-    "###);
-}
-
-#[test]
-fn test_git_no_push_parent_branch_description() {
-    let (test_env, workspace_root) = set_up();
-    test_env.jj_cmd_success(&workspace_root, &["edit", "branch1"]);
-    test_env.jj_cmd_success(
-        &workspace_root,
-        &["describe", "-m", "modified branch1 commit"],
-    );
-    test_env.jj_cmd_success(&workspace_root, &["new", "-m", "non-empty description"]);
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Error: No current branch.
-    "###);
-}
-
-#[test]
-fn test_git_push_no_current_branch() {
     let (test_env, workspace_root) = set_up();
     test_env.jj_cmd_success(&workspace_root, &["new"]);
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
-    insta::assert_snapshot!(stderr, @r###"
-    Error: No current branch.
-    "###);
-}
-
-#[test]
-fn test_git_push_current_branch_unchanged() {
-    let (test_env, workspace_root) = set_up();
-    test_env.jj_cmd_success(&workspace_root, &["co", "branch1"]);
-    let stdout = test_env.jj_cmd_success(&workspace_root, &["git", "push"]);
     insta::assert_snapshot!(stdout, @r###"
     Nothing changed.
     "###);
+    insta::assert_snapshot!(stderr, @r###"
+    No branches point to the specified revisions.
+    "###);
+}
+
+#[test]
+fn test_git_push_matching_branch_unchanged() {
+    let (test_env, workspace_root) = set_up();
+    test_env.jj_cmd_success(&workspace_root, &["co", "branch1"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Nothing changed.
+    "###);
+    insta::assert_snapshot!(stderr, @r###"
+    No branches point to the specified revisions.
+    "###);
+}
+
+/// Test that `jj git push` without arguments pushes a branch to the specified
+/// remote even if it's already up to date on another remote
+/// (`remote_branches(remote=<remote>)..@` vs. `remote_branches()..@`).
+#[test]
+fn test_git_push_other_remote_has_branch() {
+    let (test_env, workspace_root) = set_up();
+    // Create another remote (but actually the same)
+    let other_remote_path = test_env
+        .env_root()
+        .join("origin")
+        .join(".jj")
+        .join("repo")
+        .join("store")
+        .join("git");
+    test_env.jj_cmd_success(
+        &workspace_root,
+        &[
+            "git",
+            "remote",
+            "add",
+            "other",
+            other_remote_path.to_str().unwrap(),
+        ],
+    );
+    // Modify branch1 and push it to `origin`
+    test_env.jj_cmd_success(&workspace_root, &["edit", "branch1"]);
+    test_env.jj_cmd_success(&workspace_root, &["describe", "-m=modified"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Branch changes to push to origin:
+      Force branch branch1 from 45a3aa29e907 to 50421a29358a
+    "###);
+    insta::assert_snapshot!(stderr, @"");
+    // 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"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Nothing changed.
+    "###);
+    insta::assert_snapshot!(stderr, @r###"
+    No branches point to the specified revisions.
+    "###);
+    // But it will still get pushed to another remote
+    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push", "--remote=other"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Branch changes to push to other:
+      Add branch branch1 to 50421a29358a
+    "###);
+    insta::assert_snapshot!(stderr, @"");
 }
 
 #[test]
@@ -582,9 +608,12 @@ fn test_git_push_conflicting_branches() {
     };
 
     // Conflicting branch at @
-    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Nothing changed.
+    "###);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Branch branch2 is conflicted
+    Branch branch2 is conflicted
     "###);
 
     // --branch should be blocked by conflicting branch