From 0f56684839bd92240bdec3e2f84bf2d4c8c8a09e Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Thu, 2 Nov 2023 20:36:18 -0500 Subject: [PATCH 1/2] cli: support multiple `--revision` arguments to `workspace add` Summary: A natural extension of the existing support, as suggested by Scott Olson. Closes #2496. Signed-off-by: Austin Seipp Change-Id: I91c9c8c377ad67ccde7945ed41af6c79 --- CHANGELOG.md | 4 +++ cli/src/commands/workspace.rs | 30 ++++++++++++---- cli/tests/test_workspaces.rs | 66 +++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5603e96f08..96f647b8db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* `jj workspace add` can now take _multiple_ `--revision` arguments, which will + create a new workspace with its working-copy commit on top of all the parents, + as if you had run `jj new r1 r2 r3 ...`. + ### Fixed bugs diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 429ed65271..088b11da48 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -61,10 +61,20 @@ pub(crate) struct WorkspaceAddArgs { /// directory. #[arg(long)] name: Option, - /// The revision that the workspace should be created at; a new working copy - /// commit will be created on top of it. + /// A list of parent revisions for the working-copy commit of the newly + /// created workspace. You may specify nothing, or any number of parents. + /// + /// If no revisions are specified, the new workspace will be created, and + /// its working-copy commit will exist on top of the parent(s) of the + /// working-copy commit in the current workspace, i.e. they will share the + /// same parent(s). + /// + /// If any revisions are specified, the new workspace will be created, and + /// the new working-copy commit will be created with all these revisions as + /// parents, i.e. the working-copy commit will exist as if you had run `jj + /// new r1 r2 r3 ...`. #[arg(long, short)] - revision: Option, + revision: Vec, } /// Stop tracking a workspace's working-copy commit in the repo @@ -167,9 +177,9 @@ fn cmd_workspace_add( &name )); - let parents = if let Some(specific_rev) = &args.revision { - vec![old_workspace_command.resolve_single_rev(specific_rev, ui)?] - } else { + // If no parent revisions are specified, create a working-copy commit based + // on the parent of the current working-copy commit. + let parents = if args.revision.is_empty() { // Check out parents of the current workspace's working-copy commit, or the // root if there is no working-copy commit in the current workspace. if let Some(old_wc_commit_id) = tx @@ -181,6 +191,14 @@ fn cmd_workspace_add( } else { vec![tx.repo().store().root_commit()] } + } else { + crate::commands::rebase::resolve_destination_revs( + &old_workspace_command, + ui, + &args.revision, + )? + .into_iter() + .collect_vec() }; let tree = merge_commit_trees(tx.repo(), &parents)?; diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index cf12b8e0fa..b2445bbce1 100644 --- a/cli/tests/test_workspaces.rs +++ b/cli/tests/test_workspaces.rs @@ -164,6 +164,72 @@ fn test_workspaces_add_workspace_at_revision() { "###); } +/// Test multiple `-r` flags to `workspace add` to create a workspace +/// working-copy commit with multiple parents. +#[test] +fn test_workspaces_add_workspace_multiple_revisions() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "--git", "main"]); + let main_path = test_env.env_root().join("main"); + + std::fs::write(main_path.join("file-1"), "contents").unwrap(); + test_env.jj_cmd_ok(&main_path, &["commit", "-m", "first"]); + test_env.jj_cmd_ok(&main_path, &["new", "-r", "root()"]); + + std::fs::write(main_path.join("file-2"), "contents").unwrap(); + test_env.jj_cmd_ok(&main_path, &["commit", "-m", "second"]); + test_env.jj_cmd_ok(&main_path, &["new", "-r", "root()"]); + + std::fs::write(main_path.join("file-3"), "contents").unwrap(); + test_env.jj_cmd_ok(&main_path, &["commit", "-m", "third"]); + test_env.jj_cmd_ok(&main_path, &["new", "-r", "root()"]); + + insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###" + @ 5b36783cd11c4607a329c5e8c2fd9097c9ce2add + │ ◉ 23881f07b53ce1ea936ca8842e344dea9c3356e5 + ├─╯ + │ ◉ 1f6a15f0af2a985703864347f5fdf27a82fc3d73 + ├─╯ + │ ◉ e7d7dbb91c5a543ea680711093e689916d5f31df + ├─╯ + ◉ 0000000000000000000000000000000000000000 + "###); + + let (_, stderr) = test_env.jj_cmd_ok( + &main_path, + &[ + "workspace", + "add", + "--name=merge", + "../merged", + "-r=238", + "-r=1f6", + "-r=e7d", + ], + ); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" + Created workspace in "../merged" + Working copy now at: wmwvqwsz fa8fdc28 (empty) (no description set) + Parent commit : mzvwutvl 23881f07 third + Parent commit : kkmpptxz 1f6a15f0 second + Parent commit : qpvuntsm e7d7dbb9 first + Added 3 files, modified 0 files, removed 0 files + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###" + ◉ fa8fdc28af12d3c96b1e0ed062f5a8f9a99818f0 merge@ + ├─┬─╮ + │ │ ◉ e7d7dbb91c5a543ea680711093e689916d5f31df + │ ◉ │ 1f6a15f0af2a985703864347f5fdf27a82fc3d73 + │ ├─╯ + ◉ │ 23881f07b53ce1ea936ca8842e344dea9c3356e5 + ├─╯ + │ @ 5b36783cd11c4607a329c5e8c2fd9097c9ce2add default@ + ├─╯ + ◉ 0000000000000000000000000000000000000000 + "###); +} + /// Test making changes to the working copy in a workspace as it gets rewritten /// from another workspace #[test] From 962507cfb545cae992963b556d713b0c318a190e Mon Sep 17 00:00:00 2001 From: Austin Seipp Date: Fri, 3 Nov 2023 13:10:01 -0500 Subject: [PATCH 2/2] cli: move `resolve_destination_revs` to `cli_utils` and rename Summary: This is currently used by `new.rs`, `workspace.rs`, and `rebase.rs`, and may be useful for other commands and custom CLIs. So just go ahead and move it into the parent module hierarchy. Also rename the function to `resolve_all_revs`, as it isn't actually specific to rebase at all. Signed-off-by: Austin Seipp Change-Id: I0ea12afd8107f95a37a91340820221a0 --- cli/src/cli_util.rs | 17 +++++++++++++++++ cli/src/commands/new.rs | 3 +-- cli/src/commands/rebase.rs | 23 +++-------------------- cli/src/commands/workspace.rs | 12 ++++-------- 4 files changed, 25 insertions(+), 30 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 48d5874756..6ddfd9720d 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1907,6 +1907,23 @@ fn resolve_single_op( Ok(operation) } +/// Resolves revsets into revisions for use; useful for rebases or operations +/// that take multiple parents. +pub fn resolve_all_revs( + workspace_command: &WorkspaceCommandHelper, + ui: &mut Ui, + revisions: &[RevisionArg], +) -> Result, CommandError> { + let commits = + resolve_multiple_nonempty_revsets_default_single(workspace_command, ui, revisions)?; + let root_commit_id = workspace_command.repo().store().root_commit_id(); + if commits.len() >= 2 && commits.iter().any(|c| c.id() == root_commit_id) { + Err(user_error("Cannot merge with root revision")) + } else { + Ok(commits) + } +} + fn find_all_operations( op_store: &Arc, op_heads_store: &Arc, diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index 91801dca20..49d187a879 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -25,7 +25,6 @@ use tracing::instrument; use crate::cli_util::{ self, short_commit_hash, user_error, CommandError, CommandHelper, RevisionArg, }; -use crate::commands::rebase::resolve_destination_revs; use crate::ui::Ui; /// Create a new, empty change and edit it in the working copy @@ -76,7 +75,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", !args.revisions.is_empty(), "expected a non-empty list from clap" ); - let target_commits = resolve_destination_revs(&workspace_command, ui, &args.revisions)? + let target_commits = cli_util::resolve_all_revs(&workspace_command, ui, &args.revisions)? .into_iter() .collect_vec(); let target_ids = target_commits.iter().map(|c| c.id().clone()).collect_vec(); diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 5e9e45454f..6cfbfe8beb 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -27,8 +27,8 @@ use jj_lib::settings::UserSettings; use tracing::instrument; use crate::cli_util::{ - resolve_multiple_nonempty_revsets_default_single, short_commit_hash, user_error, CommandError, - CommandHelper, RevisionArg, WorkspaceCommandHelper, + self, resolve_multiple_nonempty_revsets_default_single, short_commit_hash, user_error, + CommandError, CommandHelper, RevisionArg, WorkspaceCommandHelper, }; use crate::ui::Ui; @@ -161,7 +161,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets )); } let mut workspace_command = command.workspace_helper(ui)?; - let new_parents = resolve_destination_revs(&workspace_command, ui, &args.destination)? + let new_parents = cli_util::resolve_all_revs(&workspace_command, ui, &args.destination)? .into_iter() .collect_vec(); if let Some(rev_str) = &args.revision { @@ -390,20 +390,3 @@ fn check_rebase_destinations( } Ok(()) } - -/// Resolves revsets into revisions to rebase onto. These revisions don't have -/// to be rewriteable. -pub(crate) fn resolve_destination_revs( - workspace_command: &WorkspaceCommandHelper, - ui: &mut Ui, - revisions: &[RevisionArg], -) -> Result, CommandError> { - let commits = - resolve_multiple_nonempty_revsets_default_single(workspace_command, ui, revisions)?; - let root_commit_id = workspace_command.repo().store().root_commit_id(); - if commits.len() >= 2 && commits.iter().any(|c| c.id() == root_commit_id) { - Err(user_error("Cannot merge with root revision")) - } else { - Ok(commits) - } -} diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 088b11da48..9da722e177 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -27,7 +27,7 @@ use jj_lib::workspace::{default_working_copy_initializer, Workspace}; use tracing::instrument; use crate::cli_util::{ - check_stale_working_copy, print_checkout_stats, user_error, CommandError, CommandHelper, + self, check_stale_working_copy, print_checkout_stats, user_error, CommandError, CommandHelper, RevisionArg, WorkspaceCommandHelper, }; use crate::ui::Ui; @@ -192,13 +192,9 @@ fn cmd_workspace_add( vec![tx.repo().store().root_commit()] } } else { - crate::commands::rebase::resolve_destination_revs( - &old_workspace_command, - ui, - &args.revision, - )? - .into_iter() - .collect_vec() + cli_util::resolve_all_revs(&old_workspace_command, ui, &args.revision)? + .into_iter() + .collect_vec() }; let tree = merge_commit_trees(tx.repo(), &parents)?;