completion: teach operation commands about ids

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<String>,
     /// Enable debug logging
     #[arg(long, global = true)]

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,

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,
 }
 

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<String>,
     /// Show repository changes from this operation
-    #[arg(long, conflicts_with = "operation")]
+    #[arg(
+        long,
+        conflicts_with = "operation",
+        add = ArgValueCandidates::new(complete::operations),
+    )]
     from: Option<String>,
     /// Show repository changes to this operation
-    #[arg(long, conflicts_with = "operation")]
+    #[arg(
+        long,
+        conflicts_with = "operation",
+        add = ArgValueCandidates::new(complete::operations),
+    )]
     to: Option<String>,
     /// Don't show the graph, show a flat list of modified changes
     #[arg(long)]

cli/src/commands/operation/restore.rs

@@ -12,13 +12,15 @@
 // 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;
 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=<operation ID> 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)

cli/src/commands/operation/show.rs

@@ -12,13 +12,15 @@
 // 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;
 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)]

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)

cli/src/complete.rs

@@ -241,6 +241,36 @@ pub fn all_revisions() -> Vec<CompletionCandidate> {
     revisions("all()")
 }
 
+pub fn operations() -> Vec<CompletionCandidate> {
+    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::<String>::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(&current_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))
 }

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");
+}