Refuse argument combinations that used to resolve to something arbitrary

src/cli.rs

@@ -471,23 +471,23 @@ pub struct EditArgs {
     pub unarchive: bool,
 
     /// Set the description
-    #[arg(long, short = 'd')]
+    #[arg(long, short = 'd', conflicts_with = "clear_description")]
     pub description: Option<String>,
 
     /// Remove the description
     #[arg(long)]
     pub clear_description: bool,
 
     /// Correct the capture time, as ISO-8601 or YYYY-MM-DD
-    #[arg(long)]
+    #[arg(long, conflicts_with = "reset_captured_at")]
     pub captured_at: Option<String>,
 
     /// Discard a capture-time correction and go back to the imported date
     #[arg(long)]
     pub reset_captured_at: bool,
 
     /// Set where it was taken: lat,lon[,altitude]
-    #[arg(long)]
+    #[arg(long, conflicts_with = "clear_location")]
     pub location: Option<String>,
 
     /// Forget where it was taken
@@ -932,6 +932,68 @@ mod tests {
         );
     }
 
+    #[test]
+    fn setting_and_clearing_a_location_are_not_both_accepted() {
+        // Taking both used to let the clear win silently, which loses the value the
+        // person actually asked to set.
+        assert!(
+            Cli::try_parse_from([
+                "imogen",
+                "edit",
+                "asset-1",
+                "--location",
+                "50.1,-5.5",
+                "--clear-location"
+            ])
+            .is_err(),
+            "a location cannot both be set and forgotten"
+        );
+        assert!(
+            Cli::try_parse_from(["imogen", "edit", "asset-1", "--location", "50.1,-5.5"]).is_ok()
+        );
+        assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-location"]).is_ok());
+    }
+
+    #[test]
+    fn setting_and_clearing_the_other_editable_fields_are_not_both_accepted() {
+        // The same contradiction as --location, in the two sibling pairs of the same
+        // command. --captured-at with --reset-captured-at reached the server as both, and
+        // the server drops the date; -d with --clear-description sent a null.
+        assert!(
+            Cli::try_parse_from([
+                "imogen",
+                "edit",
+                "asset-1",
+                "--captured-at",
+                "2020-01-01",
+                "--reset-captured-at"
+            ])
+            .is_err(),
+            "a capture time cannot both be corrected and reset"
+        );
+        assert!(
+            Cli::try_parse_from([
+                "imogen",
+                "edit",
+                "asset-1",
+                "-d",
+                "a caption",
+                "--clear-description"
+            ])
+            .is_err(),
+            "a description cannot both be written and removed"
+        );
+        // Both sides of each pair alone, so a `conflicts_with` naming the wrong id cannot
+        // stop a legitimate single flag parsing without a test noticing.
+        assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--reset-captured-at"]).is_ok());
+        assert!(
+            Cli::try_parse_from(["imogen", "edit", "asset-1", "--captured-at", "2020-01-01"])
+                .is_ok()
+        );
+        assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "-d", "a caption"]).is_ok());
+        assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-description"]).is_ok());
+    }
+
     #[test]
     fn a_manifest_and_paths_are_not_both_accepted() {
         // Taking both would leave it unclear which metadata applied to what.

src/commands/assets.rs

@@ -544,6 +544,8 @@ fn build_patch(args: &EditArgs) -> Result<AssetUpdate> {
     if args.unarchive {
         patch.archived = Some(false);
     }
+    // Each setting flag below is declared to conflict with the clearing one that follows
+    // it, so clap has already refused the pair and neither can overwrite the other.
     if let Some(description) = &args.description {
         patch.description = Some(Some(description.clone()));
     }

src/commands/people.rs

@@ -286,7 +286,21 @@ mod tests {
 
     const PEOPLE: &str = r#"{"items":[
         {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false},
-        {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false}
+        {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false},
+        {"id":"person-3","name":"Al","coverFaceId":null,"photoCount":2,"hidden":false}
+    ]}"#;
+
+    /// A library with nobody to be ambiguous with, which is where an empty reference does
+    /// its damage quietly rather than colliding with a second match.
+    const ONE_PERSON: &str = r#"{"items":[
+        {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false}
+    ]}"#;
+
+    /// Two clusters carrying the same whole name, which is the state `people merge` exists
+    /// to resolve and therefore one an exact-name match has to cope with.
+    const TWO_ALS: &str = r#"{"items":[
+        {"id":"person-1","name":"Al","coverFaceId":null,"photoCount":3,"hidden":false},
+        {"id":"person-2","name":"Al","coverFaceId":null,"photoCount":1,"hidden":false}
     ]}"#;
 
     fn person(id: &str, name: Option<&str>) -> Person {
@@ -372,6 +386,76 @@ mod tests {
         assert!(sent["personId"].is_null());
     }
 
+    #[tokio::test]
+    async fn an_empty_destination_is_refused_before_anything_moves() {
+        // `--to "$PERSON"` with the variable unset. `contains("")` is true of every name,
+        // so this used to resolve to the only person in the library and move faces onto
+        // them, exit 0.
+        for reference in ["", "   "] {
+            let stub = stub_returning(ONE_PERSON).await;
+            let error = reassign(
+                &context(&stub.base_url),
+                &["face-1".into()],
+                Some(reference),
+            )
+            .await
+            .expect_err("an empty --to is not a person");
+            assert!(
+                error.to_string().contains("Name somebody"),
+                "said {error} instead of naming the problem"
+            );
+            assert!(
+                stub.calls().is_empty(),
+                "nothing should reach the wire for a reference that cannot name anybody"
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn an_exact_name_beats_the_longer_one_it_is_a_prefix_of() {
+        // "Al" is somebody's whole name as well as the start of "Alice". Refusing it as
+        // ambiguous left that person unreachable by the name they actually have.
+        let stub = stub().await;
+        reassign(&context(&stub.base_url), &["face-1".into()], Some("Al"))
+            .await
+            .unwrap();
+
+        let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap();
+        assert_eq!(sent["personId"], "person-3");
+    }
+
+    #[tokio::test]
+    async fn a_name_two_people_share_is_still_refused() {
+        // Person names are not unique. An exact match that took the first of them would be
+        // the same arbitrary resolution as the empty reference, just harder to notice.
+        let stub = stub_returning(TWO_ALS).await;
+        let error = reassign(&context(&stub.base_url), &["face-1".into()], Some("Al"))
+            .await
+            .expect_err("two people are called Al");
+        assert!(
+            error.to_string().contains("use an id instead"),
+            "said {error} instead of refusing"
+        );
+    }
+
+    #[tokio::test]
+    async fn a_name_with_a_stray_newline_still_finds_them() {
+        // The same shell interpolation the empty guard is about: `--to "$(cat name.txt)"`
+        // arrives as "Alice\n". Refusing an empty reference on its trimmed form and then
+        // searching on the untrimmed one would report nobody called "Alice\n".
+        let stub = stub().await;
+        reassign(
+            &context(&stub.base_url),
+            &["face-1".into()],
+            Some("Alice\n"),
+        )
+        .await
+        .unwrap();
+
+        let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap();
+        assert_eq!(sent["personId"], "person-1");
+    }
+
     fn context(server: &str) -> Context {
         let global = GlobalArgs {
             server: None,
@@ -412,6 +496,11 @@ mod tests {
     /// makes and to record what it was asked. The SDK's own conformance suite does the
     /// same rather than depending on a server framework nothing else here needs.
     async fn stub() -> Stub {
+        stub_returning(PEOPLE).await
+    }
+
+    /// The same stub over a library of the caller's choosing.
+    async fn stub_returning(people: &'static str) -> Stub {
         let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
         let base_url = format!("http://{}", listener.local_addr().unwrap());
         let calls: Arc<Mutex<Vec<(String, String)>>> = Arc::new(Mutex::new(Vec::new()));
@@ -431,8 +520,8 @@ mod tests {
                         "HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n".to_string()
                     } else {
                         format!(
-                            "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{PEOPLE}",
-                            PEOPLE.len()
+                            "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{people}",
+                            people.len()
                         )
                     };
                     recorded.lock().unwrap().push((path, body));

src/context.rs

@@ -230,12 +230,39 @@ impl Context {
         }
     }
 
+    /// A person by id, by their whole name when exactly one person carries it, or by
+    /// enough of it to be unambiguous. The whole-name tier is why somebody called "Al" is
+    /// reachable at all rather than lost to "Alice" starting with it. `find_album` has the
+    /// same three tiers but neither of the guards below: it takes the first exact name
+    /// without checking it is unique, and accepts an empty reference.
     pub async fn find_person(&self, reference: &str) -> Result<Person> {
+        // `contains("")` is true of every name, so an unset `$PERSON` in a script would
+        // otherwise resolve to whoever happened to be listed first — and `merge` and
+        // `reassign` move data. Refused before the lookup, so nothing reaches the wire.
+        // Trimmed once, so the guard and the search agree: the shell interpolation this
+        // is about is also where a trailing newline comes from.
+        let reference = reference.trim();
+        if reference.is_empty() {
+            bail!("Name somebody by id or name — an empty reference cannot pick anybody");
+        }
         let people = self.client.people.list(true).await?;
         if let Some(exact) = people.iter().find(|person| person.id == reference) {
             return Ok(exact.clone());
         }
         let lowered = reference.to_lowercase();
+        // Only when it is somebody's whole name and nobody else's. Person names are not
+        // unique — `people merge` exists because grouping produces two clusters for one
+        // person — so two called "Al" fall through to the ambiguity refusal below rather
+        // than resolving to whichever the server listed first.
+        let mut whole_name = people.iter().filter(|person| {
+            person
+                .name
+                .as_deref()
+                .is_some_and(|name| name.to_lowercase() == lowered)
+        });
+        if let (Some(named), None) = (whole_name.next(), whole_name.next()) {
+            return Ok(named.clone());
+        }
         let matches: Vec<&Person> = people
             .iter()
             .filter(|person| {