cli: make jj branch take subcommands, not flags

As per jj-vcs#330.

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+	"files.trimTrailingWhitespace": false
+}

src/commands.rs

@@ -57,6 +57,7 @@ use jujutsu_lib::settings::UserSettings;
 use jujutsu_lib::store::Store;
 use jujutsu_lib::transaction::Transaction;
 use jujutsu_lib::tree::{merge_trees, Tree, TreeDiffIterator, TreeMergeError};
+use jujutsu_lib::view::View;
 use jujutsu_lib::working_copy::{
     CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, WorkingCopy,
 };
@@ -1133,8 +1134,8 @@ enum Commands {
     Merge(MergeArgs),
     Rebase(RebaseArgs),
     Backout(BackoutArgs),
-    Branch(BranchArgs),
-    Branches(BranchesArgs),
+    #[clap(subcommand)]
+    Branch(BranchSubcommand),
     /// Undo an operation (shortcut for `jj op undo`)
     Undo(OperationUndoArgs),
     Operation(OperationArgs),
@@ -1615,44 +1616,64 @@ struct BackoutArgs {
     destination: Vec<String>,
 }
 
-/// Create, update, or delete a branch
+/// Manage branches.
 ///
 /// For information about branches, see
 /// https://github.com/martinvonz/jj/blob/main/docs/branches.md.
-#[derive(clap::Args, Clone, Debug)]
-struct BranchArgs {
-    /// The branch's target revision
-    #[clap(long, short, group = "action")]
-    revision: Option<String>,
-
-    /// Allow moving the branch backwards or sideways
-    #[clap(long, requires = "revision")]
-    allow_backwards: bool,
-
-    /// Delete the branch locally
+#[derive(clap::Subcommand, Clone, Debug)]
+enum BranchSubcommand {
+    /// Create a new branch.
+    #[clap(visible_alias("c"))]
+    Create {
+        /// The branch's target revision.
+        #[clap(long, short)]
+        revision: Option<String>,
+
+        /// The branches to create.
+        names: Vec<String>,
+    },
+
+    /// Delete an existing branch and propagate the deletion to remotes on the next push.
+    #[clap(visible_alias("d"))]
+    Delete {
+        /// The branches to delete.
+        names: Vec<String>,
+    },
+
+    /// Delete the local version of an existing branch, without propagating the
+    /// deletion to remotes.
+    #[clap(visible_alias("f"))]
+    Forget {
+        /// The branches to delete.
+        names: Vec<String>,
+    },
+
+    /// List branches and their targets
     ///
-    /// The deletion will be propagated to remotes on push.
-    #[clap(long, group = "action")]
-    delete: bool,
-
-    /// The name of the branch to move or delete
-    #[clap(long, group = "action")]
-    forget: bool,
-
-    /// The branches to update.
-    names: Vec<String>,
+    /// A remote branch will be included only if its target is different from
+    /// the local target. For a conflicted branch (both local and remote), old
+    /// target revisions are preceded by a "-" and new target revisions are
+    /// preceded by a "+". For information about branches, see
+    /// https://github.com/martinvonz/jj/blob/main/docs/branches.md.
+    #[clap(visible_alias("l"))]
+    List,
+
+    /// Update a given branch to point to a certain commit.
+    #[clap(visible_alias("s"))]
+    Set {
+        /// The branch's target revision.
+        #[clap(long, short)]
+        revision: Option<String>,
+
+        /// Allow moving the branch backwards or sideways.
+        #[clap(long)]
+        allow_backwards: bool,
+
+        /// The branches to update.
+        names: Vec<String>,
+    },
 }
 
-/// List branches and their targets
-///
-/// A remote branch will be included only if its target is different from the
-/// local target. For a conflicted branch (both local and remote), old target
-/// revisions are preceded by a "-" and new target revisions are preceded by a
-/// "+". For information about branches, see
-/// https://github.com/martinvonz/jj/blob/main/docs/branches.md.
-#[derive(clap::Args, Clone, Debug)]
-struct BranchesArgs {}
-
 /// Commands for working with the operation log
 ///
 /// Commands for working with the operation log. For information about the
@@ -4009,11 +4030,18 @@ fn is_fast_forward(repo: RepoRef, branch_name: &str, new_target_id: &CommitId) -
     }
 }
 
-fn cmd_branch(ui: &mut Ui, command: &CommandHelper, args: &BranchArgs) -> Result<(), CommandError> {
+fn cmd_branch(
+    ui: &mut Ui,
+    command: &CommandHelper,
+    subcommand: &BranchSubcommand,
+) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let branch_names: Vec<&str> = if args.delete || args.forget {
-        let view = workspace_command.repo().view();
-        args.names
+    let view = workspace_command.repo().view();
+    fn assert_branch_names_exist<'a>(
+        view: &'a View,
+        names: &'a [String],
+    ) -> Result<Vec<&'a str>, CommandError> {
+        let result: Vec<_> = names
             .iter()
             .map(|branch_name| match view.get_local_branch(branch_name) {
                 Some(_) => Ok(branch_name.as_str()),
@@ -4022,78 +4050,138 @@ fn cmd_branch(ui: &mut Ui, command: &CommandHelper, args: &BranchArgs) -> Result
                     branch_name
                 ))),
             })
-            .try_collect()?
-    } else {
-        args.names.iter().map(|name| name.as_str()).collect()
-    };
+            .try_collect()?;
+        Ok(result)
+    }
 
-    if branch_names.is_empty() {
-        ui.write_warn("warning: No branches provided.\n")?;
+    fn make_branch_term(ui: &mut Ui, branch_names: &[impl AsRef<str>]) -> io::Result<String> {
+        if branch_names.is_empty() {
+            ui.write_warn("warning: No branches provided.\n")?;
+        }
+        let branch_term = if branch_names.len() == 1 {
+            "branch"
+        } else {
+            "branches"
+        };
+        Ok(format!(
+            "{branch_term} {}",
+            branch_names
+                .iter()
+                .map(|branch_name| branch_name.as_ref())
+                .join(", ")
+        ))
     }
-    let branch_term = if branch_names.len() == 1 {
-        "branch"
-    } else {
-        "branches"
-    };
-    let branch_term = format!("{branch_term} {}", branch_names.join(", "));
 
-    if args.delete {
-        let mut tx = workspace_command.start_transaction(&format!("delete {branch_term}"));
-        for branch_name in branch_names {
-            tx.mut_repo().remove_local_branch(branch_name);
+    match subcommand {
+        BranchSubcommand::Create { revision, names } => {
+            let branch_names: Vec<&str> = names
+                .iter()
+                .map(|branch_name| match view.get_local_branch(branch_name) {
+                    Some(_) => Err(CommandError::UserError(format!(
+                        "Branch already exists: {} (use `jj branch set` to update it)",
+                        branch_name
+                    ))),
+                    None => Ok(branch_name.as_str()),
+                })
+                .try_collect()?;
+
+            if branch_names.len() > 1 {
+                ui.write_warn(format!(
+                    "warning: Creating multiple branches ({}).\n",
+                    branch_names.len()
+                ))?;
+            }
+
+            let target_commit =
+                workspace_command.resolve_single_rev(ui, revision.as_deref().unwrap_or("@"))?;
+            let mut tx = workspace_command.start_transaction(&format!(
+                "create {} pointing to commit {}",
+                make_branch_term(ui, &branch_names)?,
+                target_commit.id().hex()
+            ));
+            for branch_name in branch_names {
+                tx.mut_repo().set_local_branch(
+                    branch_name.to_string(),
+                    RefTarget::Normal(target_commit.id().clone()),
+                );
+            }
+            workspace_command.finish_transaction(ui, tx)?;
         }
-        workspace_command.finish_transaction(ui, tx)?;
-    } else if args.forget {
-        let mut tx = workspace_command.start_transaction(&format!("forget {branch_term}"));
-        for branch_name in branch_names {
-            tx.mut_repo().remove_branch(branch_name);
+
+        BranchSubcommand::Set {
+            revision,
+            allow_backwards,
+            names: branch_names,
+        } => {
+            if branch_names.len() > 1 {
+                ui.write_warn(format!(
+                    "warning: Updating multiple branches ({}).\n",
+                    branch_names.len()
+                ))?;
+            }
+
+            let target_commit =
+                workspace_command.resolve_single_rev(ui, revision.as_deref().unwrap_or("@"))?;
+            if !allow_backwards
+                && !branch_names.iter().all(|branch_name| {
+                    is_fast_forward(
+                        workspace_command.repo().as_repo_ref(),
+                        branch_name,
+                        target_commit.id(),
+                    )
+                })
+            {
+                return Err(CommandError::UserError(
+                    "Use --allow-backwards to allow moving a branch backwards or sideways"
+                        .to_string(),
+                ));
+            }
+            let mut tx = workspace_command.start_transaction(&format!(
+                "point {} to commit {}",
+                make_branch_term(ui, branch_names)?,
+                target_commit.id().hex()
+            ));
+            for branch_name in branch_names {
+                tx.mut_repo().set_local_branch(
+                    branch_name.to_string(),
+                    RefTarget::Normal(target_commit.id().clone()),
+                );
+            }
+            workspace_command.finish_transaction(ui, tx)?;
         }
-        workspace_command.finish_transaction(ui, tx)?;
-    } else {
-        if branch_names.len() > 1 {
-            ui.write_warn(format!(
-                "warning: Updating multiple branches ({}).\n",
-                branch_names.len()
-            ))?;
+
+        BranchSubcommand::Delete { names } => {
+            let branch_names = assert_branch_names_exist(view, names)?;
+            let mut tx = workspace_command
+                .start_transaction(&format!("delete {}", make_branch_term(ui, &branch_names)?,));
+            for branch_name in branch_names {
+                tx.mut_repo().remove_local_branch(branch_name);
+            }
+            workspace_command.finish_transaction(ui, tx)?;
         }
 
-        let target_commit =
-            workspace_command.resolve_single_rev(ui, args.revision.as_deref().unwrap_or("@"))?;
-        if !args.allow_backwards
-            && !branch_names.iter().all(|branch_name| {
-                is_fast_forward(
-                    workspace_command.repo().as_repo_ref(),
-                    branch_name,
-                    target_commit.id(),
-                )
-            })
-        {
-            return Err(CommandError::UserError(
-                "Use --allow-backwards to allow moving a branch backwards or sideways".to_string(),
-            ));
+        BranchSubcommand::Forget { names } => {
+            let branch_names = assert_branch_names_exist(view, names)?;
+            let mut tx = workspace_command
+                .start_transaction(&format!("forget {}", make_branch_term(ui, &branch_names)?));
+            for branch_name in branch_names {
+                tx.mut_repo().remove_branch(branch_name);
+            }
+            workspace_command.finish_transaction(ui, tx)?;
         }
-        let mut tx = workspace_command.start_transaction(&format!(
-            "point {branch_term} to commit {}",
-            target_commit.id().hex()
-        ));
-        for branch_name in branch_names {
-            tx.mut_repo().set_local_branch(
-                branch_name.to_string(),
-                RefTarget::Normal(target_commit.id().clone()),
-            );
+
+        BranchSubcommand::List => {
+            list_branches(ui, &workspace_command)?;
         }
-        workspace_command.finish_transaction(ui, tx)?;
     }
 
     Ok(())
 }
 
-fn cmd_branches(
+fn list_branches(
     ui: &mut Ui,
-    command: &CommandHelper,
-    _args: &BranchesArgs,
+    workspace_command: &WorkspaceCommandHelper,
 ) -> Result<(), CommandError> {
-    let workspace_command = command.workspace_helper(ui)?;
     let repo = workspace_command.repo();
 
     let workspace_id = workspace_command.workspace_id();
@@ -5147,7 +5235,6 @@ where
         Commands::Rebase(sub_args) => cmd_rebase(ui, &command_helper, sub_args),
         Commands::Backout(sub_args) => cmd_backout(ui, &command_helper, sub_args),
         Commands::Branch(sub_args) => cmd_branch(ui, &command_helper, sub_args),
-        Commands::Branches(sub_args) => cmd_branches(ui, &command_helper, sub_args),
         Commands::Undo(sub_args) => cmd_op_undo(ui, &command_helper, sub_args),
         Commands::Operation(sub_args) => cmd_operation(ui, &command_helper, sub_args),
         Commands::Workspace(sub_args) => cmd_workspace(ui, &command_helper, sub_args),

tests/test_alias.rs

@@ -27,11 +27,11 @@ fn test_alias_basic() {
     b = ["log", "-r", "@", "-T", "branches"]
     "#,
     );
-    test_env.jj_cmd_success(&repo_path, &["branch", "my-branch"]);
+    test_env.jj_cmd_success(&repo_path, &["branch", "create", "my-branch"]);
     let stdout = test_env.jj_cmd_success(&repo_path, &["b"]);
     insta::assert_snapshot!(stdout, @r###"
     @ my-branch
-    ~ 
+    ~
     "###);
 }
 

tests/test_branch_command.rs

@@ -35,7 +35,7 @@ fn test_branch_multiple_names() {
     let repo_path = test_env.env_root().join("repo");
 
     let assert = test_env
-        .jj_cmd(&repo_path, &["branch", "foo", "bar"])
+        .jj_cmd(&repo_path, &["branch", "set", "foo", "bar"])
         .assert()
         .success();
     insta::assert_snapshot!(get_stdout_string(&assert), @"");
@@ -47,7 +47,7 @@ fn test_branch_multiple_names() {
     o  000000000000
     "###);
 
-    let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "--delete", "foo", "bar"]);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "delete", "foo", "bar"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  230dd059e1b0
@@ -62,7 +62,7 @@ fn test_branch_hint_no_branches() {
     let repo_path = test_env.env_root().join("repo");
 
     let assert = test_env
-        .jj_cmd(&repo_path, &["branch", "--delete"])
+        .jj_cmd(&repo_path, &["branch", "delete"])
         .assert()
         .success();
     let stderr = get_stderr_string(&assert);

tests/test_edit_command.rs

@@ -97,7 +97,7 @@ fn test_edit_merge() {
     std::fs::write(repo_path.join("file1"), "a\n").unwrap();
     std::fs::write(repo_path.join("file2"), "a\n").unwrap();
     test_env.jj_cmd_success(&repo_path, &["new"]);
-    test_env.jj_cmd_success(&repo_path, &["branch", "b"]);
+    test_env.jj_cmd_success(&repo_path, &["branch", "create", "b"]);
     std::fs::write(repo_path.join("file1"), "b\n").unwrap();
     std::fs::write(repo_path.join("file2"), "b\n").unwrap();
     test_env.jj_cmd_success(&repo_path, &["co", "@-"]);