From abcfebf1e257a71cc0ca1760072bc5bc0baa4bbb Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Fri, 2 Jun 2023 23:24:28 -0700
Subject: [PATCH] cli: allow `jj git push --change xyz --branch foo`

This was pretty simple. I simplified a bit by making the transaction
description mention only branches, not changes. It still mentions the
branches created for the changes, however. Also, since the operation
"tags" contain the full command line, I think it'll still be
relatively easy for the user to understand what the operation was
about.
---
 CHANGELOG.md           |  5 ++++-
 src/commands/git.rs    | 36 ++++++++++++++----------------------
 tests/test_git_push.rs | 21 +++++++++++++++++++++
 3 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c11e45c5f..bc1904276 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -82,7 +82,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * `jj obslog` and `jj log` now show abandoned commits as hidden.
 
-* `jj git fetch` and `jj git push` will now use the single defined remote even if it is not named "origin".
+* `jj git fetch` and `jj git push` will now use the single defined remote even
+  if it is not named "origin".
+
+* `jj git push` now accepts `--branch` and `--change` arguments together. 
 
 * `jj` with no subcommand now defaults to `jj log` instead of showing help. This
   command can be overridden by setting `ui.default-command`.
diff --git a/src/commands/git.rs b/src/commands/git.rs
index c597f2c3f..7203cd599 100644
--- a/src/commands/git.rs
+++ b/src/commands/git.rs
@@ -119,7 +119,8 @@ pub struct GitCloneArgs {
 /// all branches. Use `--change` to generate branch names based on the change
 /// IDs of specific commits.
 #[derive(clap::Args, Clone, Debug)]
-#[command(group(ArgGroup::new("what").args(&["branch", "all", "change", "deleted"])))]
+#[command(group(ArgGroup::new("specific").args(&["branch", "change"]).multiple(true)))]
+#[command(group(ArgGroup::new("what").args(&["all", "deleted"]).conflicts_with("specific")))]
 pub struct GitPushArgs {
     /// The remote to push to (only named remotes are supported)
     #[arg(long)]
@@ -658,7 +659,7 @@ fn cmd_git_push(
             if args.deleted { "deleted " } else { "" },
             &remote
         );
-    } else if !args.branch.is_empty() {
+    } else if !args.branch.is_empty() || !args.change.is_empty() {
         for branch_name in &args.branch {
             if !seen_branches.insert(branch_name.clone()) {
                 continue;
@@ -673,26 +674,7 @@ fn cmd_git_push(
                 )?;
             }
         }
-        tx_description = format!(
-            "push {} to git remote {}",
-            make_branch_term(&args.branch),
-            &remote
-        );
-    } else if !args.change.is_empty() {
-        // TODO: Allow specifying --branch and --change at the same time
-        tx_description = format!(
-            "push {} {} to git remote {}",
-            if change_commits.len() > 1 {
-                "changes"
-            } else {
-                "change"
-            },
-            change_commits
-                .iter()
-                .map(|c| c.change_id().hex())
-                .join(", "),
-            &remote
-        );
+
         for (change_str, commit) in std::iter::zip(args.change.iter(), change_commits) {
             let mut branch_name = format!(
                 "{}{}",
@@ -740,6 +722,16 @@ fn cmd_git_push(
                 )?;
             }
         }
+        tx_description = format!(
+            "push {} to git remote {}",
+            make_branch_term(
+                &branch_updates
+                    .iter()
+                    .map(|(branch, _)| branch.as_str())
+                    .collect_vec()
+            ),
+            &remote
+        );
     } else {
         match wc_commit_id {
             None => {
diff --git a/tests/test_git_push.rs b/tests/test_git_push.rs
index 22f8225be..f8f856830 100644
--- a/tests/test_git_push.rs
+++ b/tests/test_git_push.rs
@@ -279,6 +279,27 @@ fn test_git_push_changes() {
     "###);
 }
 
+#[test]
+fn test_git_push_mixed() {
+    let (test_env, workspace_root) = set_up();
+    test_env.jj_cmd_success(&workspace_root, &["describe", "-m", "foo"]);
+    std::fs::write(workspace_root.join("file"), "contents").unwrap();
+    test_env.jj_cmd_success(&workspace_root, &["new", "-m", "bar"]);
+    test_env.jj_cmd_success(&workspace_root, &["branch", "create", "my-branch"]);
+    std::fs::write(workspace_root.join("file"), "modified").unwrap();
+
+    let stdout = test_env.jj_cmd_success(
+        &workspace_root,
+        &["git", "push", "--change=@-", "--branch=my-branch"],
+    );
+    insta::assert_snapshot!(stdout, @r###"
+    Creating branch push-yqosqzytrlsw for revision @-
+    Branch changes to push to origin:
+      Add branch my-branch to 7decc7932d9c
+      Add branch push-yqosqzytrlsw to fa16a14170fb
+    "###);
+}
+
 #[test]
 fn test_git_push_existing_long_branch() {
     let (test_env, workspace_root) = set_up();