From fd8466564c10b9303d383aadc3811ce5f765fe55 Mon Sep 17 00:00:00 2001
From: Justus Winter <justus@sequoia-pgp.org>
Date: Tue, 22 Oct 2024 18:13:31 +0200
Subject: [PATCH] Make `sq key delete --file` require `--output`.

  - Previously, the certificate was imported.
---
 NEWS                               |  1 +
 src/cli/key/delete.rs              |  4 +++-
 src/cli/types/cert_designator.rs   | 29 ++++++++++++++++++++---------
 src/sq.rs                          |  5 ++---
 tests/integration/common.rs        |  1 +
 tests/integration/sq_key_delete.rs |  3 ++-
 6 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/NEWS b/NEWS
index 4c6ced93..68b0a29a 100644
--- a/NEWS
+++ b/NEWS
@@ -112,6 +112,7 @@
    - `--cert` now only looks up by primary key fingerprint.
    - The argument `sq key delete --cert-file` has been renamed to
      `--file`.
+   - The argument `sq key delete --file` now requires `--output`.
 
 * Changes in 0.38.0
 ** Notable changes
diff --git a/src/cli/key/delete.rs b/src/cli/key/delete.rs
index 9b24e98e..96f57db2 100644
--- a/src/cli/key/delete.rs
+++ b/src/cli/key/delete.rs
@@ -1,5 +1,7 @@
 //! Command-line parser for `sq key delete`.
 
+use std::ops::BitOr;
+
 use clap::Args;
 
 use crate::cli::types::*;
@@ -16,7 +18,7 @@ pub struct Command {
     #[command(flatten)]
     pub cert: CertDesignators<FileCertUserIDEmailDomainGrepArgs,
                               NoPrefix,
-                              OneValue,
+                              <OneValue as BitOr<FileRequiresOutput>>::Output,
                               DeleteKeyDoc>,
 
     #[clap(
diff --git a/src/cli/types/cert_designator.rs b/src/cli/types/cert_designator.rs
index 1bca379c..2475b8d0 100644
--- a/src/cli/types/cert_designator.rs
+++ b/src/cli/types/cert_designator.rs
@@ -147,6 +147,10 @@ pub type OneValue = typenum::U1;
 /// completely optional.
 pub type OptionalValue = typenum::U2;
 
+/// Normally it is possible to designate multiple certificates.  This
+/// errors out if there is more than one value.
+pub type FileRequiresOutput = typenum::U4;
+
 // Additional documentation.
 
 /// The prefix for the designators.
@@ -378,6 +382,8 @@ where
         let options = Options::to_usize();
         let one_value = (options & OneValue::to_usize()) > 0;
         let optional_value = (options & OptionalValue::to_usize()) > 0;
+        let file_requires_output =
+            (options & FileRequiresOutput::to_usize()) > 0;
 
         let group = format!("cert-designator-{}-{:X}-{:X}",
                             Prefix::name(),
@@ -534,15 +540,20 @@ where
         // Add all of the variants that are enabled.
         if file_arg {
             let full_name = full_name("file");
-            cmd = cmd.arg(
-                clap::Arg::new(&full_name)
-                    .long(&full_name)
-                    .value_name("PATH")
-                    .value_parser(clap::value_parser!(PathBuf))
-                    .action(action.clone())
-                    .help(Doc::help(
-                        "file",
-                        "Read certificates from PATH")));
+            let mut arg = clap::Arg::new(&full_name)
+                .long(&full_name)
+                .value_name("PATH")
+                .value_parser(clap::value_parser!(PathBuf))
+                .action(action.clone())
+                .help(Doc::help(
+                    "file",
+                    "Read certificates from PATH"));
+
+            if file_requires_output {
+                arg = arg.requires("output");
+            }
+
+            cmd = cmd.arg(arg);
             arg_group = arg_group.arg(full_name);
         }
 
diff --git a/src/sq.rs b/src/sq.rs
index 04505a59..11a3aded 100644
--- a/src/sq.rs
+++ b/src/sq.rs
@@ -47,7 +47,6 @@ use keystore::Protection;
 use crate::cli::types::CertDesignators;
 use crate::cli::types::cert_designator::ArgumentPrefix;
 use crate::cli::types::cert_designator::CertDesignator;
-use crate::cli::types::cert_designator::OneValue;
 use crate::cli::types::FileStdinOrKeyHandle;
 use crate::common::password;
 use crate::output::hint::Hint;
@@ -2127,9 +2126,9 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> {
     /// certificates.
     ///
     /// Returns whether the certificate was read from a file.
-    pub fn resolve_cert<Arguments, Prefix, Doc>(
+    pub fn resolve_cert<Arguments, Prefix, Options, Doc>(
         &self,
-        designators: &CertDesignators<Arguments, Prefix, OneValue, Doc>,
+        designators: &CertDesignators<Arguments, Prefix, Options, Doc>,
         trust_amount: usize,
     )
         -> Result<(Cert, FileStdinOrKeyHandle)>
diff --git a/tests/integration/common.rs b/tests/integration/common.rs
index 7c7c5226..e0623d5b 100644
--- a/tests/integration/common.rs
+++ b/tests/integration/common.rs
@@ -541,6 +541,7 @@ impl Sq {
         match &cert_handle {
             FileOrKeyHandle::FileOrStdin(path) => {
                 cmd.arg("--file").arg(path);
+                assert!(output_file.is_some());
             }
             FileOrKeyHandle::KeyHandle((_kh, s)) => {
                 cmd.arg("--cert").arg(&s);
diff --git a/tests/integration/sq_key_delete.rs b/tests/integration/sq_key_delete.rs
index c0854bf4..32963773 100644
--- a/tests/integration/sq_key_delete.rs
+++ b/tests/integration/sq_key_delete.rs
@@ -12,7 +12,8 @@ fn sq_key_delete() -> Result<()> {
 
     // Delete all the secret key material from a certificate stored in
     // a file.  Make sure the result contains no secret key material.
-    let updated = sq.key_delete(&cert_file, None);
+    let updated = sq.key_delete(&cert_file,
+                                std::path::PathBuf::from("-").as_path());
     assert!(! updated.is_tsk());
 
     // Do the same for a certificate whose secret key material is