From 0ceb4338f95d42729243b44152331f74ca170c1a Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Thu, 14 Nov 2024 20:46:01 +0100 Subject: [PATCH] completion: teach operation commands about ids --- cli/src/cli_util.rs | 9 +++- cli/src/commands/debug/operation.rs | 4 +- cli/src/commands/operation/abandon.rs | 3 ++ cli/src/commands/operation/diff.rs | 20 +++++-- cli/src/commands/operation/restore.rs | 3 ++ cli/src/commands/operation/show.rs | 4 +- cli/src/commands/operation/undo.rs | 4 +- cli/src/complete.rs | 78 +++++++++++++++++++++++---- cli/tests/test_completion.rs | 55 +++++++++++++++++++ 9 files changed, 162 insertions(+), 18 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index fbe7b26920..aa3d3e8a01 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -46,6 +46,7 @@ use clap::ArgAction; use clap::ArgMatches; use clap::Command; use clap::FromArgMatches; +use clap_complete::ArgValueCandidates; use indexmap::IndexMap; use indexmap::IndexSet; use itertools::Itertools; @@ -140,6 +141,7 @@ use crate::command_error::user_error_with_message; use crate::command_error::CommandError; use crate::commit_templater::CommitTemplateLanguage; use crate::commit_templater::CommitTemplateLanguageExtension; +use crate::complete; use crate::config::new_config_path; use crate::config::AnnotatedValue; use crate::config::CommandNameAndArgs; @@ -2984,7 +2986,12 @@ pub struct GlobalArgs { /// earlier operation. Doing that is equivalent to having run concurrent /// commands starting at the earlier operation. There's rarely a reason to /// do that, but it is possible. - #[arg(long, visible_alias = "at-op", global = true)] + #[arg( + long, + visible_alias = "at-op", + global = true, + add = ArgValueCandidates::new(complete::operations), + )] pub at_operation: Option, /// Enable debug logging #[arg(long, global = true)] diff --git a/cli/src/commands/debug/operation.rs b/cli/src/commands/debug/operation.rs index ccf3fdec53..53c4a62582 100644 --- a/cli/src/commands/debug/operation.rs +++ b/cli/src/commands/debug/operation.rs @@ -15,17 +15,19 @@ use std::fmt::Debug; use std::io::Write as _; +use clap_complete::ArgValueCandidates; use jj_lib::object_id::ObjectId; use jj_lib::op_walk; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; +use crate::complete; use crate::ui::Ui; /// Show information about an operation and its view #[derive(clap::Args, Clone, Debug)] pub struct DebugOperationArgs { - #[arg(default_value = "@")] + #[arg(default_value = "@", add = ArgValueCandidates::new(complete::operations))] operation: String, #[arg(long, value_enum, default_value = "all")] display: OperationDisplay, diff --git a/cli/src/commands/operation/abandon.rs b/cli/src/commands/operation/abandon.rs index 66c9e8105f..680032c5ba 100644 --- a/cli/src/commands/operation/abandon.rs +++ b/cli/src/commands/operation/abandon.rs @@ -16,6 +16,7 @@ use std::io::Write as _; use std::iter; use std::slice; +use clap_complete::ArgValueCandidates; use itertools::Itertools as _; use jj_lib::op_walk; @@ -24,6 +25,7 @@ use crate::cli_util::CommandHelper; use crate::command_error::cli_error; use crate::command_error::user_error; use crate::command_error::CommandError; +use crate::complete; use crate::ui::Ui; /// Abandon operation history @@ -40,6 +42,7 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] pub struct OperationAbandonArgs { /// The operation or operation range to abandon + #[arg(add = ArgValueCandidates::new(complete::operations))] operation: String, } diff --git a/cli/src/commands/operation/diff.rs b/cli/src/commands/operation/diff.rs index f06a70d957..941cf4db53 100644 --- a/cli/src/commands/operation/diff.rs +++ b/cli/src/commands/operation/diff.rs @@ -16,6 +16,7 @@ use std::collections::HashMap; use std::convert::Infallible; use std::sync::Arc; +use clap_complete::ArgValueCandidates; use indexmap::IndexMap; use itertools::Itertools; use jj_lib::backend::ChangeId; @@ -41,6 +42,7 @@ use crate::cli_util::CommandHelper; use crate::cli_util::LogContentFormat; use crate::command_error::CommandError; use crate::commit_templater::CommitTemplateLanguage; +use crate::complete; use crate::diff_util::diff_formats_for_log; use crate::diff_util::DiffFormatArgs; use crate::diff_util::DiffRenderer; @@ -55,13 +57,25 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] pub struct OperationDiffArgs { /// Show repository changes in this operation, compared to its parent - #[arg(long, visible_alias = "op")] + #[arg( + long, + visible_alias = "op", + add = ArgValueCandidates::new(complete::operations), + )] operation: Option, /// Show repository changes from this operation - #[arg(long, conflicts_with = "operation")] + #[arg( + long, + conflicts_with = "operation", + add = ArgValueCandidates::new(complete::operations), + )] from: Option, /// Show repository changes to this operation - #[arg(long, conflicts_with = "operation")] + #[arg( + long, + conflicts_with = "operation", + add = ArgValueCandidates::new(complete::operations), + )] to: Option, /// Don't show the graph, show a flat list of modified changes #[arg(long)] diff --git a/cli/src/commands/operation/restore.rs b/cli/src/commands/operation/restore.rs index 75f2ac8356..4412ab384e 100644 --- a/cli/src/commands/operation/restore.rs +++ b/cli/src/commands/operation/restore.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use jj_lib::object_id::ObjectId; use super::view_with_desired_portions_restored; @@ -19,6 +20,7 @@ use super::UndoWhatToRestore; use super::DEFAULT_UNDO_WHAT; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; +use crate::complete; use crate::ui::Ui; /// Create a new operation that restores the repo to an earlier state @@ -32,6 +34,7 @@ pub struct OperationRestoreArgs { /// Use `jj op log` to find an operation to restore to. Use e.g. `jj /// --at-op= log` before restoring to an operation to see the /// state of the repo at that operation. + #[arg(add = ArgValueCandidates::new(complete::operations))] operation: String, /// What portions of the local state to restore (can be repeated) diff --git a/cli/src/commands/operation/show.rs b/cli/src/commands/operation/show.rs index 8be3aed534..bd5ef606f0 100644 --- a/cli/src/commands/operation/show.rs +++ b/cli/src/commands/operation/show.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use itertools::Itertools; use super::diff::show_op_diff; @@ -19,6 +20,7 @@ use crate::cli_util::CommandHelper; use crate::cli_util::LogContentFormat; use crate::command_error::CommandError; use crate::commit_templater::CommitTemplateLanguage; +use crate::complete; use crate::diff_util::diff_formats_for_log; use crate::diff_util::DiffFormatArgs; use crate::diff_util::DiffRenderer; @@ -29,7 +31,7 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] pub struct OperationShowArgs { /// Show repository changes in this operation, compared to its parent(s) - #[arg(default_value = "@")] + #[arg(default_value = "@", add = ArgValueCandidates::new(complete::operations))] operation: String, /// Don't show the graph, show a flat list of modified changes #[arg(long)] diff --git a/cli/src/commands/operation/undo.rs b/cli/src/commands/operation/undo.rs index e3c0e39c93..bb9f3a80fe 100644 --- a/cli/src/commands/operation/undo.rs +++ b/cli/src/commands/operation/undo.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use jj_lib::object_id::ObjectId; use jj_lib::repo::Repo; @@ -21,6 +22,7 @@ use super::DEFAULT_UNDO_WHAT; use crate::cli_util::CommandHelper; use crate::command_error::user_error; use crate::command_error::CommandError; +use crate::complete; use crate::ui::Ui; /// Create a new operation that undoes an earlier operation @@ -32,7 +34,7 @@ pub struct OperationUndoArgs { /// The operation to undo /// /// Use `jj op log` to find an operation to undo. - #[arg(default_value = "@")] + #[arg(default_value = "@", add = ArgValueCandidates::new(complete::operations))] operation: String, /// What portions of the local state to restore (can be repeated) diff --git a/cli/src/complete.rs b/cli/src/complete.rs index 1f7dd94e98..997c890858 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -241,6 +241,36 @@ pub fn all_revisions() -> Vec { revisions("all()") } +pub fn operations() -> Vec { + with_jj(|mut jj, _| { + let output = jj + .arg("operation") + .arg("log") + .arg("--no-graph") + .arg("--limit") + .arg("100") + .arg("--template") + .arg( + r#" + separate(" ", + id.short(), + "(" ++ format_timestamp(time.end()) ++ ")", + description.first_line(), + ) ++ "\n""#, + ) + .output() + .map_err(user_error)?; + + Ok(String::from_utf8_lossy(&output.stdout) + .lines() + .map(|line| { + let (id, help) = split_help_text(line); + CompletionCandidate::new(id).help(help) + }) + .collect()) + }) +} + /// Shell out to jj during dynamic completion generation /// /// In case of errors, print them and early return an empty vector. @@ -267,13 +297,13 @@ where /// requirements of completions aren't very high. fn get_jj_command() -> Result<(std::process::Command, Config), CommandError> { let current_exe = std::env::current_exe().map_err(user_error)?; - let mut command = std::process::Command::new(current_exe); + let mut cmd_args = Vec::::new(); // Snapshotting could make completions much slower in some situations // and be undesired by the user. - command.arg("--ignore-working-copy"); - command.arg("--color=never"); - command.arg("--no-pager"); + cmd_args.push("--ignore-working-copy".into()); + cmd_args.push("--color=never".into()); + cmd_args.push("--no-pager".into()); // Parse some of the global args we care about for passing along to the // child process. This shouldn't fail, since none of the global args are @@ -311,17 +341,43 @@ fn get_jj_command() -> Result<(std::process::Command, Config), CommandError> { let _ = layered_configs.read_repo_config(loader.repo_path()); config = layered_configs.merge(); } - command.arg("--repository"); - command.arg(repository); + cmd_args.push("--repository".into()); + cmd_args.push(repository); } if let Some(at_operation) = args.at_operation { - command.arg("--at-operation"); - command.arg(at_operation); + // We cannot assume that the value of at_operation is valid, because + // the user may be requesting completions precisely for this invalid + // operation ID. Additionally, the user may have mistyped the ID, + // in which case adding the argument blindly would break all other + // completions, even unrelated ones. + // + // To avoid this, we shell out to ourselves once with the argument + // and check the exit code. There is some performance overhead to this, + // but this code path is probably only executed in exceptional + // situations. + let mut canary_cmd = std::process::Command::new(¤t_exe); + canary_cmd.args(&cmd_args); + canary_cmd.arg("--at-operation"); + canary_cmd.arg(&at_operation); + canary_cmd.arg("debug"); + canary_cmd.arg("snapshot"); + + match canary_cmd.output() { + Ok(output) if output.status.success() => { + // Operation ID is valid, add it to the completion command. + cmd_args.push("--at-operation".into()); + cmd_args.push(at_operation); + } + _ => {} // Invalid operation ID, ignore. + } } for config_toml in args.early_args.config_toml { - command.arg("--config-toml"); - command.arg(config_toml); + cmd_args.push("--config-toml".into()); + cmd_args.push(config_toml); } - Ok((command, config)) + let mut cmd = std::process::Command::new(current_exe); + cmd.args(&cmd_args); + + Ok((cmd, config)) } diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index 59f7e40ec7..c3bea0ac68 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -345,3 +345,58 @@ fn test_revisions() { r mutable "); } + +#[test] +fn test_operations() { + let test_env = TestEnvironment::default(); + + // suppress warnings on stderr of completions for invalid args + test_env.add_config("ui.default-command = 'log'"); + + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 0"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 1"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 2"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 3"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "description 4"]); + + let mut test_env = test_env; + test_env.add_env_var("COMPLETE", "fish"); + let test_env = test_env; + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "show", ""]); + let add_workspace_id = stdout.lines().nth(5).unwrap().split('\t').next().unwrap(); + insta::assert_snapshot!(add_workspace_id, @"eac759b9ab75"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "show", "5"]); + insta::assert_snapshot!(stdout, @r" + 5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710 + 518b588abbc6 (2001-02-03 08:05:09) describe commit 19611c995a342c01f525583e5fcafdd211f6d009 + "); + // make sure global --at-op flag is respected + let stdout = test_env.jj_cmd_success( + &repo_path, + &["--", "jj", "--at-op", "518b588abbc6", "op", "show", "5"], + ); + insta::assert_snapshot!(stdout, @"518b588abbc6 (2001-02-03 08:05:09) describe commit 19611c995a342c01f525583e5fcafdd211f6d009"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "--at-op", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "abandon", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "diff", "--op", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "diff", "--from", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "diff", "--to", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "restore", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["--", "jj", "op", "undo", "5b"]); + insta::assert_snapshot!(stdout, @"5bbb4ca536a8 (2001-02-03 08:05:12) describe commit 968261075dddabf4b0e333c1cc9a49ce26a3f710"); +}