From 4e0b2174bf866f281d56a81780ee4bfb85625b2d Mon Sep 17 00:00:00 2001 From: Doll Date: Fri, 15 May 2026 08:12:51 -0500 Subject: [PATCH] fix(shared-writer): make blocker/label/relation mutations idempotent (#600) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SharedWriter::{add,remove}_{blocker,label,relation} now short-circuit before write_commit_push when no change is needed, avoiding the empty git commit that previously failed with a confusing "git commit […] in cache failed:" error. (Git emits the "nothing to commit" hint on stdout, which git_commit_in_cache_with_args drops — see #601.) Method signatures change Result<()> -> Result (true = mutated, false = no-op). commands::{deps,label,relate} writer branches use the new bool to print differentiated "Dependency already exists" / "Label '' already exists" / "Issues X and Y are already related" messages, matching what their non-writer counterparts already did. Tightened test_add_label_idempotent (was swallowing the bug with `let _ =`) and added regression tests for the five sibling mutations. Also fixes 20 pre-existing clippy::pedantic warnings in lock_check.rs (11x map_unwrap_or on identical pattern), container.rs, compaction.rs, sentinel/collect.rs, and sync/tests.rs so cargo clippy -D warnings passes again. Co-Authored-By: Claude Opus 4.7 (1M context) --- crosslink/src/commands/container.rs | 5 +- crosslink/src/commands/deps.rs | 30 ++-- crosslink/src/commands/label.rs | 38 +++-- crosslink/src/commands/relate.rs | 38 +++-- crosslink/src/commands/sentinel/collect.rs | 42 ++--- crosslink/src/compaction.rs | 3 +- crosslink/src/lock_check.rs | 22 +-- crosslink/src/shared_writer/mutations.rs | 92 +++++++++-- crosslink/src/shared_writer/tests.rs | 174 ++++++++++++++++++++- crosslink/src/sync/tests.rs | 12 +- 10 files changed, 362 insertions(+), 94 deletions(-) diff --git a/crosslink/src/commands/container.rs b/crosslink/src/commands/container.rs index 9555ced06..01260f643 100644 --- a/crosslink/src/commands/container.rs +++ b/crosslink/src/commands/container.rs @@ -645,7 +645,7 @@ mod tests { /// The container subcommands MUST address the same registry-qualified /// image name as `crosslink kickoff run --container docker|podman`. - /// Regressing IMAGE_NAME back to the bare `crosslink-agent` form + /// Regressing `IMAGE_NAME` back to the bare `crosslink-agent` form /// silently un-composes the two code paths and re-opens GH#576. #[test] fn image_name_is_ghcr_namespaced() { @@ -654,8 +654,7 @@ mod tests { IMAGE_NAME, crate::commands::kickoff::DEFAULT_AGENT_IMAGE .rsplit_once(':') - .map(|(name, _)| name) - .unwrap_or(IMAGE_NAME), + .map_or(IMAGE_NAME, |(name, _)| name), "container.rs IMAGE_NAME diverged from kickoff DEFAULT_AGENT_IMAGE — \ re-opens the GH#576 compose-failure between `crosslink container build` \ and `crosslink kickoff run --container …`" diff --git a/crosslink/src/commands/deps.rs b/crosslink/src/commands/deps.rs index 602a7b30c..0e0a02d32 100644 --- a/crosslink/src/commands/deps.rs +++ b/crosslink/src/commands/deps.rs @@ -20,12 +20,15 @@ pub fn block( } if let Some(w) = writer { - w.add_blocker(db, issue_id, blocker_id)?; - println!( - "Issue {} is now blocked by {}", - format_issue_id(issue_id), - format_issue_id(blocker_id) - ); + if w.add_blocker(db, issue_id, blocker_id)? { + println!( + "Issue {} is now blocked by {}", + format_issue_id(issue_id), + format_issue_id(blocker_id) + ); + } else { + println!("Dependency already exists"); + } } else if db.add_dependency(issue_id, blocker_id)? { println!( "Issue {} is now blocked by {}", @@ -45,12 +48,15 @@ pub fn unblock( blocker_id: i64, ) -> Result<()> { if let Some(w) = writer { - w.remove_blocker(db, issue_id, blocker_id)?; - println!( - "Removed: {} no longer blocked by {}", - format_issue_id(issue_id), - format_issue_id(blocker_id) - ); + if w.remove_blocker(db, issue_id, blocker_id)? { + println!( + "Removed: {} no longer blocked by {}", + format_issue_id(issue_id), + format_issue_id(blocker_id) + ); + } else { + println!("No such dependency found"); + } } else if db.remove_dependency(issue_id, blocker_id)? { println!( "Removed: {} no longer blocked by {}", diff --git a/crosslink/src/commands/label.rs b/crosslink/src/commands/label.rs index 90e8abb4e..243418a41 100644 --- a/crosslink/src/commands/label.rs +++ b/crosslink/src/commands/label.rs @@ -8,12 +8,19 @@ pub fn add(db: &Database, writer: Option<&SharedWriter>, issue_id: i64, label: & db.require_issue(issue_id)?; if let Some(w) = writer { - w.add_label(db, issue_id, label)?; - println!( - "Added label '{}' to issue {}", - label, - format_issue_id(issue_id) - ); + if w.add_label(db, issue_id, label)? { + println!( + "Added label '{}' to issue {}", + label, + format_issue_id(issue_id) + ); + } else { + println!( + "Label '{}' already exists on issue {}", + label, + format_issue_id(issue_id) + ); + } } else if db.add_label(issue_id, label)? { println!( "Added label '{}' to issue {}", @@ -39,12 +46,19 @@ pub fn remove( db.require_issue(issue_id)?; if let Some(w) = writer { - w.remove_label(db, issue_id, label)?; - println!( - "Removed label '{}' from issue {}", - label, - format_issue_id(issue_id) - ); + if w.remove_label(db, issue_id, label)? { + println!( + "Removed label '{}' from issue {}", + label, + format_issue_id(issue_id) + ); + } else { + println!( + "Label '{}' not found on issue {}", + label, + format_issue_id(issue_id) + ); + } } else if db.remove_label(issue_id, label)? { println!( "Removed label '{}' from issue {}", diff --git a/crosslink/src/commands/relate.rs b/crosslink/src/commands/relate.rs index 7b96e5873..9425137fc 100644 --- a/crosslink/src/commands/relate.rs +++ b/crosslink/src/commands/relate.rs @@ -14,12 +14,19 @@ pub fn add( db.require_issue(related_id)?; if let Some(w) = writer { - w.add_relation(db, issue_id, related_id)?; - println!( - "Linked {} ↔ {}", - format_issue_id(issue_id), - format_issue_id(related_id) - ); + if w.add_relation(db, issue_id, related_id)? { + println!( + "Linked {} ↔ {}", + format_issue_id(issue_id), + format_issue_id(related_id) + ); + } else { + println!( + "Issues {} and {} are already related", + format_issue_id(issue_id), + format_issue_id(related_id) + ); + } } else if db.add_relation(issue_id, related_id)? { println!( "Linked {} ↔ {}", @@ -44,12 +51,19 @@ pub fn remove( related_id: i64, ) -> Result<()> { if let Some(w) = writer { - w.remove_relation(db, issue_id, related_id)?; - println!( - "Unlinked {} ↔ {}", - format_issue_id(issue_id), - format_issue_id(related_id) - ); + if w.remove_relation(db, issue_id, related_id)? { + println!( + "Unlinked {} ↔ {}", + format_issue_id(issue_id), + format_issue_id(related_id) + ); + } else { + println!( + "No relation found between {} and {}", + format_issue_id(issue_id), + format_issue_id(related_id) + ); + } } else if db.remove_relation(issue_id, related_id)? { println!( "Unlinked {} ↔ {}", diff --git a/crosslink/src/commands/sentinel/collect.rs b/crosslink/src/commands/sentinel/collect.rs index cf9458d41..b5c101e3b 100644 --- a/crosslink/src/commands/sentinel/collect.rs +++ b/crosslink/src/commands/sentinel/collect.rs @@ -507,6 +507,27 @@ fn build_fix_template(ctx: &TemplateContext<'_>) -> String { ) } +/// Post a comment to a GitHub issue via `gh`. +fn post_gh_comment(gh_issue_number: i64, body: &str) -> Result<()> { + let output = Command::new("gh") + .args([ + "issue", + "comment", + &gh_issue_number.to_string(), + "--body", + body, + ]) + .output() + .context("Failed to run `gh issue comment`")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("gh issue comment failed: {}", stderr.trim()); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -554,24 +575,3 @@ mod tests { assert_eq!(classify_status(" ", 1), None); } } - -/// Post a comment to a GitHub issue via `gh`. -fn post_gh_comment(gh_issue_number: i64, body: &str) -> Result<()> { - let output = Command::new("gh") - .args([ - "issue", - "comment", - &gh_issue_number.to_string(), - "--body", - body, - ]) - .output() - .context("Failed to run `gh issue comment`")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("gh issue comment failed: {}", stderr.trim()); - } - - Ok(()) -} diff --git a/crosslink/src/compaction.rs b/crosslink/src/compaction.rs index f17d9899c..4601dcf36 100644 --- a/crosslink/src/compaction.rs +++ b/crosslink/src/compaction.rs @@ -792,8 +792,7 @@ mod tests { .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status() - .map(|s| s.success()) - .unwrap_or(false) + .is_ok_and(|s| s.success()) } /// Release the compaction lease. diff --git a/crosslink/src/lock_check.rs b/crosslink/src/lock_check.rs index f7e272f41..f6fb9d8d1 100644 --- a/crosslink/src/lock_check.rs +++ b/crosslink/src/lock_check.rs @@ -697,7 +697,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { // git not available in this environment; skip gracefully return; } @@ -772,7 +772,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -844,7 +844,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -892,7 +892,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -933,7 +933,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -977,7 +977,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -1015,7 +1015,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -1081,7 +1081,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -1237,7 +1237,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -1273,7 +1273,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } @@ -1440,7 +1440,7 @@ mod tests { .args(["init", "-q"]) .current_dir(repo_root) .status(); - if init_status.map(|s| !s.success()).unwrap_or(true) { + if init_status.map_or(true, |s| !s.success()) { return; } diff --git a/crosslink/src/shared_writer/mutations.rs b/crosslink/src/shared_writer/mutations.rs index 2aeb49d11..c85c3950d 100644 --- a/crosslink/src/shared_writer/mutations.rs +++ b/crosslink/src/shared_writer/mutations.rs @@ -574,12 +574,24 @@ impl SharedWriter { /// Add a label to an issue. /// + /// Returns `Ok(true)` if the label was newly added, `Ok(false)` if the + /// issue already carried the label (no-op short-circuit). + /// /// # Errors /// /// Returns an error if the issue cannot be loaded or git operations fail. - pub fn add_label(&self, db: &Database, display_id: i64, label: &str) -> Result<()> { + pub fn add_label(&self, db: &Database, display_id: i64, label: &str) -> Result { let label_owned = label.to_string(); + // Idempotency short-circuit (#600): if the label is already present, + // serializing the unchanged issue would hand `write_commit_push` an + // identical file, which git rejects with "nothing to commit". Skip + // git entirely and report no-op via the boolean return. + let current = self.load_issue_by_id(display_id, db)?; + if current.labels.contains(&label_owned) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(display_id, db)?; @@ -599,17 +611,27 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Remove a label from an issue. /// + /// Returns `Ok(true)` if the label was removed, `Ok(false)` if the issue + /// did not carry the label (no-op short-circuit). + /// /// # Errors /// /// Returns an error if the issue cannot be loaded or git operations fail. - pub fn remove_label(&self, db: &Database, display_id: i64, label: &str) -> Result<()> { + pub fn remove_label(&self, db: &Database, display_id: i64, label: &str) -> Result { let label_owned = label.to_string(); + // Idempotency short-circuit (#600): if the label is absent, skip the + // write entirely to avoid an empty git commit. + let current = self.load_issue_by_id(display_id, db)?; + if !current.labels.contains(&label_owned) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(display_id, db)?; @@ -629,19 +651,35 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Add a blocker dependency: `issue_id` is blocked by `blocking_issue_id`. /// /// Only modifies the blocked issue's file (single-direction storage). /// + /// Returns `Ok(true)` if the blocker was newly added, `Ok(false)` if it + /// was already recorded (no-op short-circuit). + /// /// # Errors /// /// Returns an error if either issue cannot be resolved or git operations fail. - pub fn add_blocker(&self, db: &Database, issue_id: i64, blocking_issue_id: i64) -> Result<()> { + pub fn add_blocker( + &self, + db: &Database, + issue_id: i64, + blocking_issue_id: i64, + ) -> Result { let blocker_uuid = self.resolve_uuid(blocking_issue_id, db)?; + // Idempotency short-circuit (#600): if the blocker is already + // recorded, the closure would serialize an identical issue file and + // `git commit` would fail with "nothing to commit". + let current = self.load_issue_by_id(issue_id, db)?; + if current.blockers.contains(&blocker_uuid) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(issue_id, db)?; @@ -661,11 +699,14 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Remove a blocker dependency. /// + /// Returns `Ok(true)` if the blocker was removed, `Ok(false)` if the + /// blocker was not present (no-op short-circuit). + /// /// # Errors /// /// Returns an error if either issue cannot be resolved or git operations fail. @@ -674,9 +715,16 @@ impl SharedWriter { db: &Database, issue_id: i64, blocking_issue_id: i64, - ) -> Result<()> { + ) -> Result { let blocker_uuid = self.resolve_uuid(blocking_issue_id, db)?; + // Idempotency short-circuit (#600): if the blocker is absent, skip + // the write entirely to avoid an empty git commit. + let current = self.load_issue_by_id(issue_id, db)?; + if !current.blockers.contains(&blocker_uuid) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(issue_id, db)?; @@ -696,17 +744,27 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Add a relation between two issues (single-direction storage). /// + /// Returns `Ok(true)` if the relation was newly added, `Ok(false)` if + /// it was already recorded (no-op short-circuit). + /// /// # Errors /// /// Returns an error if either issue cannot be resolved or git operations fail. - pub fn add_relation(&self, db: &Database, issue_id: i64, related_id: i64) -> Result<()> { + pub fn add_relation(&self, db: &Database, issue_id: i64, related_id: i64) -> Result { let related_uuid = self.resolve_uuid(related_id, db)?; + // Idempotency short-circuit (#600): if the relation is already + // recorded, skip the write entirely to avoid an empty git commit. + let current = self.load_issue_by_id(issue_id, db)?; + if current.related.contains(&related_uuid) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(issue_id, db)?; @@ -726,17 +784,27 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Remove a relation between two issues. /// + /// Returns `Ok(true)` if the relation was removed, `Ok(false)` if no + /// such relation existed (no-op short-circuit). + /// /// # Errors /// /// Returns an error if either issue cannot be resolved or git operations fail. - pub fn remove_relation(&self, db: &Database, issue_id: i64, related_id: i64) -> Result<()> { + pub fn remove_relation(&self, db: &Database, issue_id: i64, related_id: i64) -> Result { let related_uuid = self.resolve_uuid(related_id, db)?; + // Idempotency short-circuit (#600): if the relation is absent, skip + // the write entirely to avoid an empty git commit. + let current = self.load_issue_by_id(issue_id, db)?; + if !current.related.contains(&related_uuid) { + return Ok(false); + } + let _ = self.write_commit_push( |writer| { let mut issue = writer.load_issue_by_id(issue_id, db)?; @@ -756,7 +824,7 @@ impl SharedWriter { )?; self.hydrate_with_retry(db); - Ok(()) + Ok(true) } /// Rewrite a just-committed issue to set `display_id: null` and revert the diff --git a/crosslink/src/shared_writer/tests.rs b/crosslink/src/shared_writer/tests.rs index a93b03766..15aaf2712 100644 --- a/crosslink/src/shared_writer/tests.rs +++ b/crosslink/src/shared_writer/tests.rs @@ -1531,6 +1531,8 @@ mod integration { #[test] fn test_add_label_idempotent() { + // Regression test for #600: re-adding an existing label must exit + // cleanly (Ok(false)) rather than failing on the empty git commit. let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); let db = make_db(work_dir.path()); @@ -1538,8 +1540,14 @@ mod integration { let id = writer .create_issue(&db, "Idempotent label", None, "medium", None, None) .unwrap(); - writer.add_label(&db, id, "tag").unwrap(); - let _ = writer.add_label(&db, id, "tag"); // duplicate -- may error on empty commit + assert!( + writer.add_label(&db, id, "tag").unwrap(), + "First add should report newly-added (Ok(true))" + ); + assert!( + !writer.add_label(&db, id, "tag").unwrap(), + "Duplicate add should report no-op (Ok(false)), not fail" + ); let labels = db.get_labels(id).unwrap(); let tag_count = labels.iter().filter(|l| l.as_str() == "tag").count(); @@ -1547,6 +1555,35 @@ mod integration { drop(work_dir); } + #[test] + fn test_remove_label_idempotent() { + // Removing a label that isn't present must return Ok(false), not + // fail on the empty git commit (#600). + let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); + let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); + let db = make_db(work_dir.path()); + + let id = writer + .create_issue(&db, "Remove absent label", None, "medium", None, None) + .unwrap(); + + assert!( + !writer.remove_label(&db, id, "never-added").unwrap(), + "Removing an absent label should report no-op (Ok(false))" + ); + + writer.add_label(&db, id, "tag").unwrap(); + assert!( + writer.remove_label(&db, id, "tag").unwrap(), + "Removing a present label should report removed (Ok(true))" + ); + assert!( + !writer.remove_label(&db, id, "tag").unwrap(), + "Removing again should report no-op (Ok(false))" + ); + drop(work_dir); + } + // --- add_blocker() / remove_blocker() --- #[test] @@ -1594,6 +1631,73 @@ mod integration { drop(work_dir); } + #[test] + fn test_add_blocker_idempotent() { + // Regression test for #600: `crosslink issue block ` must + // exit zero when is already blocked by , instead of failing + // with a confusing "git commit […] failed:" error from the empty + // commit path. + let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); + let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); + let db = make_db(work_dir.path()); + + let blocked = writer + .create_issue(&db, "Blocked twice", None, "medium", None, None) + .unwrap(); + let blocker = writer + .create_issue(&db, "Blocker", None, "high", None, None) + .unwrap(); + + assert!( + writer.add_blocker(&db, blocked, blocker).unwrap(), + "First add should report newly-added (Ok(true))" + ); + assert!( + !writer.add_blocker(&db, blocked, blocker).unwrap(), + "Re-adding the same blocker should report no-op (Ok(false)), not fail" + ); + + let issue_file = writer.load_issue_by_id(blocked, &db).unwrap(); + assert_eq!( + issue_file.blockers.len(), + 1, + "Idempotent add must not duplicate the blocker entry" + ); + drop(work_dir); + } + + #[test] + fn test_remove_blocker_idempotent() { + // Removing a blocker that isn't present must return Ok(false), not + // fail on the empty git commit (#600). + let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); + let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); + let db = make_db(work_dir.path()); + + let issue = writer + .create_issue(&db, "Never blocked", None, "medium", None, None) + .unwrap(); + let other = writer + .create_issue(&db, "Not a blocker", None, "medium", None, None) + .unwrap(); + + assert!( + !writer.remove_blocker(&db, issue, other).unwrap(), + "Removing an absent blocker should report no-op (Ok(false))" + ); + + writer.add_blocker(&db, issue, other).unwrap(); + assert!( + writer.remove_blocker(&db, issue, other).unwrap(), + "Removing a present blocker should report removed (Ok(true))" + ); + assert!( + !writer.remove_blocker(&db, issue, other).unwrap(), + "Removing again should report no-op (Ok(false))" + ); + drop(work_dir); + } + // --- add_relation() / remove_relation() --- #[test] @@ -1637,6 +1741,72 @@ mod integration { drop(work_dir); } + #[test] + fn test_add_relation_idempotent() { + // Regression test for #600: re-adding the same relation must exit + // cleanly via the no-op short-circuit rather than failing on the + // empty git commit. + let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); + let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); + let db = make_db(work_dir.path()); + + let id1 = writer + .create_issue(&db, "Related E", None, "medium", None, None) + .unwrap(); + let id2 = writer + .create_issue(&db, "Related F", None, "medium", None, None) + .unwrap(); + + assert!( + writer.add_relation(&db, id1, id2).unwrap(), + "First add should report newly-added (Ok(true))" + ); + assert!( + !writer.add_relation(&db, id1, id2).unwrap(), + "Re-adding the same relation should report no-op (Ok(false))" + ); + + let issue = writer.load_issue_by_id(id1, &db).unwrap(); + assert_eq!( + issue.related.len(), + 1, + "Idempotent add must not duplicate the relation entry" + ); + drop(work_dir); + } + + #[test] + fn test_remove_relation_idempotent() { + // Removing a relation that isn't present must return Ok(false), not + // fail on the empty git commit (#600). + let (work_dir, _remote, crosslink_dir) = setup_shared_writer_env(); + let writer = SharedWriter::new(&crosslink_dir).unwrap().unwrap(); + let db = make_db(work_dir.path()); + + let id1 = writer + .create_issue(&db, "Lonely G", None, "medium", None, None) + .unwrap(); + let id2 = writer + .create_issue(&db, "Lonely H", None, "medium", None, None) + .unwrap(); + + assert!( + !writer.remove_relation(&db, id1, id2).unwrap(), + "Removing an absent relation should report no-op (Ok(false))" + ); + + writer.add_relation(&db, id1, id2).unwrap(); + assert!( + writer.remove_relation(&db, id1, id2).unwrap(), + "Removing a present relation should report removed (Ok(true))" + ); + assert!( + !writer.remove_relation(&db, id1, id2).unwrap(), + "Removing again should report no-op (Ok(false))" + ); + drop(work_dir); + } + // --- create_milestone() / close_milestone() / delete_milestone() --- #[test] diff --git a/crosslink/src/sync/tests.rs b/crosslink/src/sync/tests.rs index fb727ca65..58a7cd650 100644 --- a/crosslink/src/sync/tests.rs +++ b/crosslink/src/sync/tests.rs @@ -1107,8 +1107,8 @@ fn test_clean_dirty_state_with_dirty_file() { /// branch's tree with hub-cache artifacts. /// /// The fix adds a `verify_cache_worktree` preflight that: -/// 1. Confirms `git rev-parse --show-toplevel` from cache_dir resolves to -/// cache_dir (not a parent — catches the walk-up case). +/// 1. Confirms `git rev-parse --show-toplevel` from `cache_dir` resolves to +/// `cache_dir` (not a parent — catches the walk-up case). /// 2. Confirms HEAD is on the configured `HUB_BRANCH`. /// /// This test simulates the broken-worktree state and asserts that @@ -2561,8 +2561,7 @@ fn test_migrate_from_locks_branch_with_old_remote_branch() { // After migration, crosslink/hub should exist on remote let has_hub = manager .git_in_repo(&["ls-remote", "--heads", "origin", HUB_BRANCH]) - .map(|o| !String::from_utf8_lossy(&o.stdout).trim().is_empty()) - .unwrap_or(false); + .is_ok_and(|o| !String::from_utf8_lossy(&o.stdout).trim().is_empty()); assert!( has_hub, "crosslink/hub should exist on remote after migration" @@ -2571,8 +2570,7 @@ fn test_migrate_from_locks_branch_with_old_remote_branch() { // Old branch should be gone from remote let has_old = manager .git_in_repo(&["ls-remote", "--heads", "origin", OLD_BRANCH]) - .map(|o| !String::from_utf8_lossy(&o.stdout).trim().is_empty()) - .unwrap_or(false); + .is_ok_and(|o| !String::from_utf8_lossy(&o.stdout).trim().is_empty()); assert!(!has_old, "crosslink/locks should be deleted from remote"); } @@ -2863,7 +2861,7 @@ fn test_configure_signing_does_not_revisit_completed_bootstrap() { let after_first = crate::sync::bootstrap::read_bootstrap_state(manager.cache_path()) .expect("first call should complete bootstrap"); assert_eq!(after_first.status, "complete"); - let first_ts = after_first.completed_at.clone(); + let first_ts = after_first.completed_at; manager.configure_signing(&crosslink_dir).unwrap(); manager.configure_signing(&crosslink_dir).unwrap();