a73x

16259abc

Reject meaningless arguments where they are given

a73x   2026-08-10 16:54

Commit message
Reject meaningless arguments where they are given

Four commands accepted something that could not mean anything, and each
one deferred the consequence to somewhere the user was no longer looking.

`patch create --fixes` never resolved its argument. The first resolution
happened at merge, where a failure can only warn — so a patch could carry
a `fixes` pointing at nothing for its whole life and silently never close
the issue it promised to. It now resolves at create time, with the same
message merge would have printed, and rejects an ambiguous prefix there
too. What is stored is the resolved id rather than the prefix given: a
prefix that is unique today can go ambiguous tomorrow, and validating an
id that can still fail later is not validating it.

`patch create` accepted a branch with no commits ahead of its base and
recorded an empty revision 1. Revisions are immutable refs, so that empty
r1 cannot be tidied away afterwards; the recovery was to commit and
revise, leaving the mistake in the patch's history forever. This is a
hard failure rather than a warning, because a patch with no change is not
a proposal — claiming work before writing it is what `issue open` is for
— and because `create` already refuses the sibling mistake of being on
the base branch. An unknown `--base` is now named as such as well.

`patch show --revision N` rendered a revision that does not exist as an
empty result, indistinguishable from a revision with no comments. It now
uses the check and the message interdiff already had, and validates
before marking the patch seen, so a rejected read writes nothing. In
`--json` the flag was accepted and ignored outright; it is now honoured.

`patch checkout` left you on a branch it made without saying where you
had come from or that the branch was staying. It now names both, and
reuses a `collab/*` branch already standing at the wanted commit instead
of piling up numbered copies over a review session.

Failures on a `--json` command now also print `{"error": ...}` on stdout.
A caller that asked for JSON parses stdout and never sees stderr, so an
error delivered only there reads as success.

Fixes 6b1db172, 03163871, aa0d482e, 62abe4b9

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/cli.rs
Old New
@@ -165,6 +165,26 @@ pub struct Cli {
165 pub command: Commands, 165 pub command: Commands,
166 } 166 }
167 167
168 impl Cli {
169 /// Whether this invocation asked for machine-readable output.
170 ///
171 /// A caller that passed `--json` parses stdout and, in the normal case,
172 /// never looks at stderr — so an error delivered only to stderr is a
173 /// success as far as they can tell. Knowing the answer here lets the failure
174 /// path speak the same language the caller asked for.
175 pub fn wants_json(&self) -> bool {
176 match &self.command {
177 Commands::Issue(IssueCmd::List { json, .. })
178 | Commands::Issue(IssueCmd::Show { json, .. })
179 | Commands::Patch(PatchCmd::List { json, .. })
180 | Commands::Patch(PatchCmd::Show { json, .. })
181 | Commands::Patch(PatchCmd::Log { json, .. })
182 | Commands::Release(ReleaseCmd::List { json, .. }) => *json,
183 _ => false,
184 }
185 }
186 }
187
168 #[derive(Subcommand, Debug)] 188 #[derive(Subcommand, Debug)]
169 pub enum Commands { 189 pub enum Commands {
170 /// Initialize collab refspecs on all remotes 190 /// Initialize collab refspecs on all remotes
@@ -476,7 +496,7 @@ pub enum PatchCmd {
476 /// Source branch (defaults to current branch) 496 /// Source branch (defaults to current branch)
477 #[arg(short = 'B', long)] 497 #[arg(short = 'B', long)]
478 branch: Option<String>, 498 branch: Option<String>,
479 /// Issue ID this patch fixes (auto-closes on merge) 499 /// Issue ID this patch fixes (auto-closes on merge; must resolve to one existing issue)
480 #[arg(long)] 500 #[arg(long)]
481 fixes: Option<String>, 501 fixes: Option<String>,
482 }, 502 },
src/lib.rs
Old New
@@ -337,6 +337,12 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
337 branch, 337 branch,
338 fixes, 338 fixes,
339 } => { 339 } => {
340 // A branch invented here, rather than named by the user, is ours
341 // to undo: `patch create` validates its arguments and can still
342 // refuse, and a refusal that left a stray `collab/patch/*`
343 // branch behind would be the exact bad-state-persists failure
344 // the validation exists to prevent.
345 let mut auto_created: Option<String> = None;
340 // Resolve branch name: --branch takes priority, then current branch 346 // Resolve branch name: --branch takes priority, then current branch
341 let branch_name = if let Some(b) = branch { 347 let branch_name = if let Some(b) = branch {
342 b 348 b
@@ -366,10 +372,20 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
366 let short_oid = &oid.to_string()[..8]; 372 let short_oid = &oid.to_string()[..8];
367 let auto_branch = format!("collab/patch/{}", short_oid); 373 let auto_branch = format!("collab/patch/{}", short_oid);
368 repo.branch(&auto_branch, &commit, false)?; 374 repo.branch(&auto_branch, &commit, false)?;
375 auto_created = Some(auto_branch.clone());
369 auto_branch 376 auto_branch
370 } 377 }
371 }; 378 };
372 let id = patch::create(repo, &title, &body, &base, &branch_name, fixes.as_deref())?; 379 let created =
380 patch::create(repo, &title, &body, &base, &branch_name, fixes.as_deref());
381 if created.is_err() {
382 if let Some(name) = auto_created {
383 if let Ok(mut b) = repo.find_branch(&name, git2::BranchType::Local) {
384 let _ = b.delete();
385 }
386 }
387 }
388 let id = created?;
373 println!("Created patch {}", abbrev::for_patches(repo).of(&id)); 389 println!("Created patch {}", abbrev::for_patches(repo).of(&id));
374 Ok(()) 390 Ok(())
375 } 391 }
@@ -404,11 +420,11 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
404 } 420 }
405 PatchCmd::Show { id, json, revision } => { 421 PatchCmd::Show { id, json, revision } => {
406 if json { 422 if json {
407 let output = patch::show_json(repo, &id)?; 423 let output = patch::show_json(repo, &id, revision)?;
408 println!("{}", output); 424 println!("{}", output);
409 return Ok(()); 425 return Ok(());
410 } 426 }
411 let p = patch::show(repo, &id)?; 427 let p = patch::show(repo, &id, revision)?;
412 let rev_count = p.revisions.len(); 428 let rev_count = p.revisions.len();
413 let status_detail = match p.staleness(repo) { 429 let status_detail = match p.staleness(repo) {
414 Ok((_, behind)) if behind > 0 => { 430 Ok((_, behind)) if behind > 0 => {
src/main.rs
Old New
@@ -17,12 +17,11 @@ fn main() {
17 return; 17 return;
18 } 18 }
19 19
20 let json = cli.wants_json();
21
20 let repo = match Repository::open_from_env() { 22 let repo = match Repository::open_from_env() {
21 Ok(r) => r, 23 Ok(r) => r,
22 Err(e) => { 24 Err(e) => fail(&e.to_string(), json),
23 eprintln!("error: {}", e);
24 std::process::exit(1);
25 }
26 }; 25 };
27 26
28 if let Err(e) = git_collab::run(cli, &repo) { 27 if let Err(e) = git_collab::run(cli, &repo) {
@@ -32,10 +31,21 @@ fn main() {
32 // Summary already printed by sync; just exit with code 1 31 // Summary already printed by sync; just exit with code 1
33 std::process::exit(1); 32 std::process::exit(1);
34 } 33 }
35 _ => { 34 _ => fail(&e.to_string(), json),
36 eprintln!("error: {}", e);
37 std::process::exit(1);
38 }
39 } 35 }
40 } 36 }
41 } 37 }
38
39 /// Report a fatal error and exit.
40 ///
41 /// Under `--json` the error is *also* emitted on stdout as an object, because
42 /// that is the only stream the caller reads. It is emitted in addition to the
43 /// stderr line rather than instead of it, so a human running the same command
44 /// by hand still sees it where they expect.
45 fn fail(message: &str, json: bool) -> ! {
46 eprintln!("error: {}", message);
47 if json {
48 println!("{}", serde_json::json!({ "error": message }));
49 }
50 std::process::exit(1);
51 }
src/patch.rs
Old New
@@ -16,10 +16,42 @@ use crate::state::{self, PatchState, PatchStatus};
16 /// base branch advances and the author does not rebase, the merge-base does not 16 /// base branch advances and the author does not rebase, the merge-base does not
17 /// move, so a change in this value means the author actually rebased. 17 /// move, so a change in this value means the author actually rebased.
18 fn revision_base(repo: &Repository, base_ref: &str, commit: Oid) -> Result<Oid, Error> { 18 fn revision_base(repo: &Repository, base_ref: &str, commit: Oid) -> Result<Oid, Error> {
19 let base_oid = repo.refname_to_id(&format!("refs/heads/{}", base_ref))?; 19 let base_oid = base_tip(repo, base_ref)?;
20 Ok(repo.merge_base(base_oid, commit).unwrap_or(base_oid)) 20 Ok(repo.merge_base(base_oid, commit).unwrap_or(base_oid))
21 } 21 }
22 22
23 /// The tip of the base branch, named in the error when it is not there.
24 ///
25 /// The bare `refname_to_id` failure reports a ref path the user never typed,
26 /// which reads as an internal fault rather than as "you passed `--base` a
27 /// branch that does not exist".
28 fn base_tip(repo: &Repository, base_ref: &str) -> Result<Oid, Error> {
29 repo.refname_to_id(&format!("refs/heads/{}", base_ref))
30 .map_err(|e| Error::Cmd(format!("base branch '{}' not found: {}", base_ref, e)))
31 }
32
33 /// Resolve `--fixes` before the patch exists.
34 ///
35 /// `--fixes` is documented as auto-closing the issue on merge, and that promise
36 /// is only kept if the reference resolves. Left unchecked it was resolved for
37 /// the first time at merge, months later, where a failure could do nothing but
38 /// print a warning — so a patch could carry a `fixes` pointing at nothing for
39 /// its whole life and silently never close anything.
40 ///
41 /// What gets stored is the *resolved* id, not the prefix given. A prefix that is
42 /// unique today can go ambiguous tomorrow, and an id validated at create time
43 /// that can still fail to resolve at merge time would not be validated at all.
44 fn resolve_fixes(repo: &Repository, fixes: Option<&str>) -> Result<Option<String>, Error> {
45 let Some(prefix) = fixes else {
46 return Ok(None);
47 };
48 // The same resolution `patch merge` performs, so the message the user gets
49 // here is the message they would otherwise have got at merge.
50 let (_ref_name, id) = state::resolve_issue_ref(repo, prefix)
51 .map_err(|e| Error::Cmd(format!("--fixes: {}", e)))?;
52 Ok(Some(id))
53 }
54
23 pub fn create( 55 pub fn create(
24 repo: &Repository, 56 repo: &Repository,
25 title: &str, 57 title: &str,
@@ -41,6 +73,27 @@ pub fn create(
41 .refname_to_id(&branch_ref) 73 .refname_to_id(&branch_ref)
42 .map_err(|e| crate::error::Error::Cmd(format!("branch '{}' not found: {}", branch, e)))?; 74 .map_err(|e| crate::error::Error::Cmd(format!("branch '{}' not found: {}", branch, e)))?;
43 75
76 // Every argument is validated before the first write below, so a rejected
77 // create leaves the repo exactly as it found it.
78 let fixes = resolve_fixes(repo, fixes)?;
79
80 // A patch with no commits is not a proposal. Revision 1 is recorded from the
81 // branch tip as it stands now, so a branch level with (or behind) its base
82 // yields an empty r1 — and revisions are immutable refs, so that empty r1
83 // cannot be tidied away afterwards. `patch create` already refuses the
84 // sibling mistake of being *on* the base branch; this is the same mistake
85 // reached from a branch that merely has nothing on it.
86 let base_tip_oid = base_tip(repo, base_ref)?;
87 let (ahead, _behind) = repo.graph_ahead_behind(tip_oid, base_tip_oid)?;
88 if ahead == 0 {
89 return Err(crate::error::Error::Cmd(format!(
90 "branch '{}' has no commits ahead of base '{}'; \
91 commit your work before creating a patch, \
92 or pass --base to compare against a different branch",
93 branch, base_ref
94 )));
95 }
96
44 // Duplicate detection is by commit, not by branch name. Patch identity is 97 // Duplicate detection is by commit, not by branch name. Patch identity is
45 // declared in the event DAG, so a branch name means nothing: two worktrees 98 // declared in the event DAG, so a branch name means nothing: two worktrees
46 // on the same generated name are not the same patch, and one worktree that 99 // on the same generated name are not the same patch, and one worktree that
@@ -70,7 +123,7 @@ pub fn create(
70 body: body.to_string(), 123 body: body.to_string(),
71 base_ref: base_ref.to_string(), 124 base_ref: base_ref.to_string(),
72 branch: branch.to_string(), 125 branch: branch.to_string(),
73 fixes: fixes.map(|s| s.to_string()), 126 fixes,
74 commit: tip_oid.to_string(), 127 commit: tip_oid.to_string(),
75 tree: tree_oid.to_string(), 128 tree: tree_oid.to_string(),
76 base_commit: Some(base_oid.to_string()), 129 base_commit: Some(base_oid.to_string()),
@@ -224,15 +277,50 @@ pub fn list_json(
224 Ok(serde_json::to_string_pretty(&patches)?) 277 Ok(serde_json::to_string_pretty(&patches)?)
225 } 278 }
226 279
227 pub fn show_json(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> { 280 /// Drop every review and inline comment not anchored to `revision`.
281 ///
282 /// Thread comments are deliberately left alone: they are not anchored to a
283 /// revision at all, so filtering them by one would silently hide the whole
284 /// discussion. The plain renderer makes the same choice.
285 fn only_revision(patch: &mut PatchState, revision: u32) {
286 patch.reviews.retain(|r| r.revision == Some(revision));
287 patch
288 .inline_comments
289 .retain(|c| c.revision == Some(revision));
290 }
291
292 pub fn show_json(
293 repo: &Repository,
294 id_prefix: &str,
295 revision: Option<u32>,
296 ) -> Result<String, crate::error::Error> {
228 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 297 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
229 let p = PatchState::from_ref(repo, &ref_name, &id)?; 298 let mut p = PatchState::from_ref(repo, &ref_name, &id)?;
299 if let Some(n) = revision {
300 // `--json` used to accept `--revision` and ignore it, handing back
301 // everything to a caller that asked to be shown one revision. Same
302 // failure as the missing existence check: an argument accepted and
303 // quietly made to mean nothing.
304 find_revision(&p, n)?;
305 only_revision(&mut p, n);
306 }
230 Ok(serde_json::to_string_pretty(&patch_json_value(repo, &p)?)?) 307 Ok(serde_json::to_string_pretty(&patch_json_value(repo, &p)?)?)
231 } 308 }
232 309
233 pub fn show(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::error::Error> { 310 pub fn show(
311 repo: &Repository,
312 id_prefix: &str,
313 revision: Option<u32>,
314 ) -> Result<PatchState, crate::error::Error> {
234 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 315 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
235 let patch = PatchState::from_ref(repo, &ref_name, &id)?; 316 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
317 // Validate before marking seen. A `--revision` that does not exist rendered
318 // as an empty result, indistinguishable from "that revision has no
319 // comments" — and marking the patch read on the way out would have credited
320 // the user with reading output they never got.
321 if let Some(n) = revision {
322 find_revision(&patch, n)?;
323 }
236 // Mark as read: store current tip as seen 324 // Mark as read: store current tip as seen
237 let tip = repo.refname_to_id(&ref_name)?; 325 let tip = repo.refname_to_id(&ref_name)?;
238 let seen_ref = format!("refs/collab/local/seen/patches/{}", id); 326 let seen_ref = format!("refs/collab/local/seen/patches/{}", id);
@@ -858,9 +946,39 @@ pub fn patch_log_json(patch: &PatchState) -> Result<String, Error> {
858 Ok(serde_json::to_string_pretty(&patch.revisions)?) 946 Ok(serde_json::to_string_pretty(&patch.revisions)?)
859 } 947 }
860 948
949 /// Where HEAD was before a checkout moved it: what to call it, and what to hand
950 /// `git checkout` to get back there.
951 ///
952 /// A detached HEAD gets its commit rather than a branch name, because telling
953 /// someone to `git checkout` a branch they were never on would put them
954 /// somewhere they have not been.
955 struct PreviousHead {
956 display: String,
957 restore: String,
958 }
959
960 fn previous_head(repo: &Repository) -> Option<PreviousHead> {
961 let head = repo.head().ok()?;
962 if head.is_branch() {
963 let name = head.shorthand()?.to_string();
964 return Some(PreviousHead {
965 display: name.clone(),
966 restore: name,
967 });
968 }
969 let oid = head.target()?;
970 let short = short_oid(oid);
971 Some(PreviousHead {
972 display: format!("detached HEAD {}", short),
973 restore: short,
974 })
975 }
976
861 pub fn checkout(repo: &Repository, id_prefix: &str) -> Result<(), crate::error::Error> { 977 pub fn checkout(repo: &Repository, id_prefix: &str) -> Result<(), crate::error::Error> {
862 let (_ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 978 let (_ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
863 let patch = show(repo, id_prefix)?; 979 // Read where we are before anything moves it.
980 let previous = previous_head(repo);
981 let patch = show(repo, id_prefix, None)?;
864 982
865 let latest_rev = patch 983 let latest_rev = patch
866 .revisions 984 .revisions
@@ -873,32 +991,67 @@ pub fn checkout(repo: &Repository, id_prefix: &str) -> Result<(), crate::error::
873 .map_err(|e| Error::Cmd(format!("commit {} not found: {}", &latest_rev.commit, e)))?; 991 .map_err(|e| Error::Cmd(format!("commit {} not found: {}", &latest_rev.commit, e)))?;
874 992
875 let short_id = &id[..std::cmp::min(8, id.len())]; 993 let short_id = &id[..std::cmp::min(8, id.len())];
994 // Reusing a branch that already stands at the wanted commit is what keeps a
995 // review session from piling up `collab/<id>-1`, `-2`, `-3`. A branch of the
996 // same name pointing somewhere else is someone's work, so that one is left
997 // alone and a suffixed name is used instead.
998 let mut created = true;
876 let branch_name = { 999 let branch_name = {
877 let candidate = format!("collab/{}", short_id); 1000 let candidate = format!("collab/{}", short_id);
878 if repo.find_branch(&candidate, git2::BranchType::Local).is_err() { 1001 match repo.find_branch(&candidate, git2::BranchType::Local) {
879 candidate 1002 Err(_) => candidate,
880 } else { 1003 Ok(b) if b.get().target() == Some(commit_oid) => {
881 let mut n = 1u32; 1004 created = false;
882 loop { 1005 candidate
883 let suffixed = format!("collab/{}-{}", short_id, n); 1006 }
884 if repo.find_branch(&suffixed, git2::BranchType::Local).is_err() { 1007 Ok(_) => {
885 break suffixed; 1008 let mut n = 1u32;
1009 loop {
1010 let suffixed = format!("collab/{}-{}", short_id, n);
1011 match repo.find_branch(&suffixed, git2::BranchType::Local) {
1012 Err(_) => break suffixed,
1013 Ok(b) if b.get().target() == Some(commit_oid) => {
1014 created = false;
1015 break suffixed;
1016 }
1017 Ok(_) => n += 1,
1018 }
886 } 1019 }
887 n += 1;
888 } 1020 }
889 } 1021 }
890 }; 1022 };
891 1023
892 repo.branch(&branch_name, &commit, false)?; 1024 if created {
1025 repo.branch(&branch_name, &commit, false)?;
1026 }
893 1027
894 let refname = format!("refs/heads/{}", branch_name); 1028 let refname = format!("refs/heads/{}", branch_name);
895 let obj = repo.revparse_single(&refname)?; 1029 let obj = repo.revparse_single(&refname)?;
896 repo.checkout_tree(&obj, None)?; 1030 repo.checkout_tree(&obj, None)?;
897 repo.set_head(&refname)?; 1031 repo.set_head(&refname)?;
898 1032
1033 // Checking a patch out is a detour, so say how the detour ends. Without
1034 // this the reviewer has to have remembered where they were, and is left
1035 // with a branch nobody told them about.
1036 // Landing where you already were is not a move, so there is nothing to
1037 // report and nowhere to send you back to.
1038 let moved = previous.as_ref().is_some_and(|p| p.restore != branch_name);
1039 match previous.filter(|_| moved) {
1040 Some(prev) => {
1041 println!(
1042 "Checked out patch {} (revision {}) on branch {}; you were on {}.",
1043 short_id, latest_rev.number, branch_name, prev.display
1044 );
1045 println!("Return with `git checkout {}`.", prev.restore);
1046 }
1047 None => println!(
1048 "Checked out patch {} (revision {}) on branch {}.",
1049 short_id, latest_rev.number, branch_name
1050 ),
1051 }
899 println!( 1052 println!(
900 "Checked out patch {} (revision {}) on branch {}", 1053 "Branch {} is left behind; remove it with `git branch -D {}`.",
901 short_id, latest_rev.number, branch_name 1054 branch_name, branch_name
902 ); 1055 );
903 Ok(()) 1056 Ok(())
904 } 1057 }
tests/collab_test.rs
Old New
@@ -855,9 +855,17 @@ fn test_create_duplicate_patch_for_same_commit_returns_error() {
855 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); 855 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x");
856 repo.branch("feature/dup", &repo.find_commit(tip).unwrap(), false) 856 repo.branch("feature/dup", &repo.find_commit(tip).unwrap(), false)
857 .unwrap(); 857 .unwrap();
858 // The shared commit has to be one `main` does not have, or neither branch is
859 // ahead of base and `create` rejects both before it ever looks for a
860 // duplicate.
861 let shared = add_commit_on_branch(&repo, "feature/dup", "work.rs", b"work");
858 // A second branch name for the very same commit, as two worktrees produce. 862 // A second branch name for the very same commit, as two worktrees produce.
859 repo.branch("worktree-agent-dup", &repo.find_commit(tip).unwrap(), false) 863 repo.branch(
860 .unwrap(); 864 "worktree-agent-dup",
865 &repo.find_commit(shared).unwrap(),
866 false,
867 )
868 .unwrap();
861 869
862 // First creation should succeed 870 // First creation should succeed
863 let id = patch::create(&repo, "First", "", "main", "feature/dup", None).unwrap(); 871 let id = patch::create(&repo, "First", "", "main", "feature/dup", None).unwrap();
@@ -1658,7 +1666,7 @@ fn test_patch_show_json_output() {
1658 let (ref_name, id) = create_patch(&repo, &alice(), "Show patch JSON"); 1666 let (ref_name, id) = create_patch(&repo, &alice(), "Show patch JSON");
1659 add_review(&repo, &ref_name, &bob(), ReviewVerdict::Approve); 1667 add_review(&repo, &ref_name, &bob(), ReviewVerdict::Approve);
1660 1668
1661 let json_str = git_collab::patch::show_json(&repo, &id[..8]).unwrap(); 1669 let json_str = git_collab::patch::show_json(&repo, &id[..8], None).unwrap();
1662 let value: serde_json::Value = serde_json::from_str(&json_str).unwrap(); 1670 let value: serde_json::Value = serde_json::from_str(&json_str).unwrap();
1663 assert_eq!(value["title"], "Show patch JSON"); 1671 assert_eq!(value["title"], "Show patch JSON");
1664 assert_eq!(value["reviews"].as_array().unwrap().len(), 1); 1672 assert_eq!(value["reviews"].as_array().unwrap().len(), 1);
tests/early_validation_test.rs
Old New
@@ -0,0 +1,489 @@
1 //! Commands must reject a meaningless argument at the point it is given, with a
2 //! message naming the fix — not accept it and fail somewhere else later, or
3 //! never fail at all.
4 //!
5 //! Each test here pins the *message*, because the message is the feature. An
6 //! exit code alone tells the user nothing, and these bugs were all cases where
7 //! the user learned nothing until much later, if ever.
8
9 mod common;
10
11 use common::TestRepo;
12
13 /// A repo with `main` plus a `feat` branch carrying one commit, left checked
14 /// out on `main`. The common starting point: a patch that *should* be creatable.
15 fn repo_with_feature_branch() -> TestRepo {
16 let repo = TestRepo::new("Alice", "alice@example.com");
17 repo.git(&["checkout", "-b", "feat"]);
18 repo.commit_file("a.txt", "a", "do the work");
19 repo.git(&["checkout", "main"]);
20 repo
21 }
22
23 fn full_issue_id(repo: &TestRepo, short: &str) -> String {
24 repo.git(&["for-each-ref", "--format=%(refname)", "refs/collab/issues/"])
25 .lines()
26 .filter_map(|l| l.trim().strip_prefix("refs/collab/issues/"))
27 .find(|id| id.starts_with(short))
28 .unwrap_or_else(|| panic!("no issue ref matching {}", short))
29 .to_string()
30 }
31
32 /// Give an issue a second ref under a near-identical id, so its 8-character
33 /// short id matches two issues. Random ids practically never collide, so
34 /// ambiguity has to be constructed.
35 fn duplicate_issue_ref_with_colliding_id(repo: &TestRepo, short: &str) {
36 let full = full_issue_id(repo, short);
37 let last = full.chars().last().unwrap();
38 let replacement = if last == '0' { '1' } else { '0' };
39 let twin: String = full[..full.len() - 1]
40 .chars()
41 .chain([replacement])
42 .collect();
43 let tip = repo
44 .git(&["rev-parse", &format!("refs/collab/issues/{}", full)])
45 .trim()
46 .to_string();
47 repo.git(&["update-ref", &format!("refs/collab/issues/{}", twin), &tip]);
48 }
49
50 fn assert_no_patches(repo: &TestRepo) {
51 let out = repo.run_ok(&["patch", "list"]);
52 assert!(
53 out.contains("No patches found"),
54 "a rejected create must leave nothing behind, got: {}",
55 out
56 );
57 }
58
59 // ===========================================================================
60 // 6b1db172 — `patch create --fixes` accepts an unresolvable issue reference
61 // ===========================================================================
62
63 #[test]
64 fn patch_create_rejects_a_fixes_reference_that_resolves_to_nothing() {
65 let repo = repo_with_feature_branch();
66
67 let stderr = repo.run_err(&[
68 "patch", "create", "-t", "T", "-B", "feat", "--fixes", "deadbeef",
69 ]);
70
71 assert!(
72 stderr.contains("no issue found matching 'deadbeef'"),
73 "must fail with the same message `patch merge` reports, got: {}",
74 stderr
75 );
76 assert!(
77 stderr.contains("--fixes"),
78 "the message must name the flag at fault, got: {}",
79 stderr
80 );
81 assert_no_patches(&repo);
82 }
83
84 /// The failure that motivated the issue: a script guessed a verb wrong, captured
85 /// `issue open`'s usage text into a variable, and passed the whole thing on as a
86 /// `--fixes` value. It was accepted without complaint.
87 #[test]
88 fn patch_create_rejects_usage_text_passed_as_a_fixes_value() {
89 let repo = repo_with_feature_branch();
90 let usage = "Usage: git-collab issue open --title <TITLE>";
91
92 let stderr = repo.run_err(&["patch", "create", "-t", "T", "-B", "feat", "--fixes", usage]);
93
94 assert!(
95 stderr.contains(&format!("no issue found matching '{}'", usage)),
96 "the message must quote the value verbatim so the caller can see what it sent, got: {}",
97 stderr
98 );
99 assert_no_patches(&repo);
100 }
101
102 #[test]
103 fn patch_create_rejects_an_ambiguous_fixes_prefix() {
104 let repo = repo_with_feature_branch();
105 let issue = repo.issue_open("Login bug");
106 duplicate_issue_ref_with_colliding_id(&repo, &issue);
107
108 let stderr = repo.run_err(&[
109 "patch", "create", "-t", "T", "-B", "feat", "--fixes", &issue,
110 ]);
111
112 assert!(
113 stderr.contains("ambiguous issue prefix"),
114 "an ambiguous prefix must fail at create time too, got: {}",
115 stderr
116 );
117 assert!(
118 stderr.contains(&issue),
119 "the message must name the prefix given, got: {}",
120 stderr
121 );
122 assert_no_patches(&repo);
123 }
124
125 /// Resolving at create time is only durable if what gets *stored* is the
126 /// resolved id. A stored prefix that is unique today can go ambiguous tomorrow,
127 /// which is the very failure the create-time check exists to prevent.
128 #[test]
129 fn patch_create_stores_the_resolved_issue_id_not_the_prefix_given() {
130 let repo = repo_with_feature_branch();
131 let issue = repo.issue_open("Login bug");
132 let full = full_issue_id(&repo, &issue);
133
134 let out = repo.run_ok(&[
135 "patch", "create", "-t", "T", "-B", "feat", "--fixes", &issue,
136 ]);
137 let patch_id = out.trim().strip_prefix("Created patch ").unwrap();
138
139 let json = repo.run_ok(&["patch", "show", patch_id, "--json"]);
140 let v: serde_json::Value = serde_json::from_str(&json).unwrap();
141 assert_eq!(
142 v["fixes"], full,
143 "stored `fixes` must be the fully resolved issue id"
144 );
145 }
146
147 #[test]
148 fn patch_create_still_accepts_a_fixes_reference_that_resolves() {
149 let repo = repo_with_feature_branch();
150 let issue = repo.issue_open("Login bug");
151
152 let out = repo.run_ok(&[
153 "patch", "create", "-t", "T", "-B", "feat", "--fixes", &issue,
154 ]);
155 assert!(out.starts_with("Created patch "));
156 }
157
158 // ===========================================================================
159 // 03163871 — `patch create` succeeds with nothing in it
160 // ===========================================================================
161
162 #[test]
163 fn patch_create_rejects_a_branch_with_no_commits_ahead_of_base() {
164 let repo = TestRepo::new("Alice", "alice@example.com");
165 repo.git(&["checkout", "-b", "empty"]);
166 repo.git(&["checkout", "main"]);
167
168 let stderr = repo.run_err(&["patch", "create", "-t", "T", "-B", "empty"]);
169
170 assert!(
171 stderr.contains("branch 'empty' has no commits ahead of base 'main'"),
172 "the message must name both the branch and the base it compared against, got: {}",
173 stderr
174 );
175 assert!(
176 stderr.contains("commit"),
177 "the message must name the fix, got: {}",
178 stderr
179 );
180 assert_no_patches(&repo);
181 }
182
183 /// A branch that is merely *behind* base has no commits of its own either. It
184 /// must be rejected for the same reason and with the same message, not slip
185 /// through because it is not literally at the base tip.
186 #[test]
187 fn patch_create_rejects_a_branch_that_is_only_behind_base() {
188 let repo = TestRepo::new("Alice", "alice@example.com");
189 repo.git(&["checkout", "-b", "stale"]);
190 repo.git(&["checkout", "main"]);
191 repo.commit_file("moved.txt", "m", "main moves on");
192
193 let stderr = repo.run_err(&["patch", "create", "-t", "T", "-B", "stale"]);
194
195 assert!(
196 stderr.contains("branch 'stale' has no commits ahead of base 'main'"),
197 "got: {}",
198 stderr
199 );
200 assert_no_patches(&repo);
201 }
202
203 #[test]
204 fn patch_create_names_a_base_branch_that_does_not_exist() {
205 let repo = repo_with_feature_branch();
206
207 let stderr = repo.run_err(&["patch", "create", "-t", "T", "-B", "feat", "--base", "nope"]);
208
209 assert!(
210 stderr.contains("base branch 'nope' not found"),
211 "an unknown base must be reported as such, got: {}",
212 stderr
213 );
214 assert_no_patches(&repo);
215 }
216
217 /// From a detached HEAD, `patch create` invents a branch to hang the patch off
218 /// before it validates anything. A rejected create must not leave that branch
219 /// behind — the whole point of failing early is that nothing is left over.
220 #[test]
221 fn a_rejected_create_from_a_detached_head_leaves_no_branch_behind() {
222 let repo = repo_with_feature_branch();
223 repo.git(&["checkout", "feat"]);
224 repo.git(&["checkout", "--detach"]);
225
226 let before = repo.git(&["branch", "--list", "collab/*"]);
227 assert!(before.trim().is_empty(), "precondition");
228
229 repo.run_err(&["patch", "create", "-t", "T", "--fixes", "deadbeef"]);
230
231 let after = repo.git(&["branch", "--list", "collab/*"]);
232 assert!(
233 after.trim().is_empty(),
234 "a rejected create must not leave its auto-created branch behind, got: {}",
235 after
236 );
237 }
238
239 // ===========================================================================
240 // aa0d482e — `patch show --revision N` for a revision that does not exist
241 // ===========================================================================
242
243 fn repo_with_one_revision_patch() -> (TestRepo, String) {
244 let repo = repo_with_feature_branch();
245 let out = repo.run_ok(&["patch", "create", "-t", "T", "-B", "feat"]);
246 let id = out
247 .trim()
248 .strip_prefix("Created patch ")
249 .unwrap()
250 .to_string();
251 (repo, id)
252 }
253
254 #[test]
255 fn patch_show_rejects_a_revision_that_does_not_exist() {
256 let (repo, id) = repo_with_one_revision_patch();
257
258 let stderr = repo.run_err(&["patch", "show", &id, "--revision", "5"]);
259
260 assert!(
261 stderr.contains("revision 5 not found"),
262 "must use the same message interdiff already gives, got: {}",
263 stderr
264 );
265 }
266
267 /// A failed read must not leave a mark. `patch show` records a seen-ref to drive
268 /// unread counts; a rejected invocation showed the user nothing, so it must not
269 /// claim they have seen anything.
270 #[test]
271 fn a_rejected_patch_show_does_not_mark_the_patch_as_seen() {
272 let (repo, id) = repo_with_one_revision_patch();
273
274 repo.run_err(&["patch", "show", &id, "--revision", "5"]);
275
276 let seen = repo.git(&[
277 "for-each-ref",
278 "--format=%(refname)",
279 "refs/collab/local/seen/",
280 ]);
281 assert!(
282 seen.trim().is_empty(),
283 "a failed show must write nothing, but left: {}",
284 seen
285 );
286 }
287
288 #[test]
289 fn patch_show_still_accepts_a_revision_that_exists() {
290 let (repo, id) = repo_with_one_revision_patch();
291
292 let out = repo.run_ok(&["patch", "show", &id, "--revision", "1"]);
293 assert!(out.contains("Patch "));
294 }
295
296 // ---------------------------------------------------------------------------
297 // --json callers must be able to see the failure. An error that only reaches
298 // stderr is invisible to a script parsing stdout.
299 // ---------------------------------------------------------------------------
300
301 #[test]
302 fn a_json_command_reports_its_error_as_json_on_stdout() {
303 let (repo, id) = repo_with_one_revision_patch();
304
305 let out = repo.run(&["patch", "show", &id, "--json", "--revision", "5"]);
306 assert!(!out.status.success(), "must still exit non-zero");
307
308 let stdout = String::from_utf8(out.stdout).unwrap();
309 let v: serde_json::Value = serde_json::from_str(&stdout)
310 .unwrap_or_else(|e| panic!("--json failure must still be JSON ({}): {}", e, stdout));
311 assert_eq!(v["error"], "revision 5 not found");
312
313 let stderr = String::from_utf8(out.stderr).unwrap();
314 assert!(
315 stderr.contains("revision 5 not found"),
316 "the human-readable error must still reach stderr, got: {}",
317 stderr
318 );
319 }
320
321 #[test]
322 fn a_non_json_command_does_not_print_json_on_failure() {
323 let (repo, id) = repo_with_one_revision_patch();
324
325 let out = repo.run(&["patch", "show", &id, "--revision", "5"]);
326 let stdout = String::from_utf8(out.stdout).unwrap();
327 assert!(
328 !stdout.contains("\"error\""),
329 "plain output must stay plain, got: {}",
330 stdout
331 );
332 }
333
334 /// `--revision` was accepted and silently ignored in `--json` mode: the caller
335 /// asked to narrow the output and got everything. Same bug class as the one
336 /// above, so it is fixed with it.
337 #[test]
338 fn patch_show_json_filters_reviews_and_comments_by_revision() {
339 let (repo, id) = repo_with_one_revision_patch();
340 repo.run_ok(&[
341 "patch",
342 "review",
343 &id,
344 "-v",
345 "comment",
346 "-b",
347 "on r1",
348 "--revision",
349 "1",
350 ]);
351
352 // Add a second revision, and a review anchored to it.
353 repo.git(&["checkout", "feat"]);
354 repo.commit_file("b.txt", "b", "more work");
355 repo.run_ok(&["patch", "revise", &id, "-B", "feat"]);
356 repo.git(&["checkout", "main"]);
357 repo.run_ok(&[
358 "patch",
359 "review",
360 &id,
361 "-v",
362 "comment",
363 "-b",
364 "on r2",
365 "--revision",
366 "2",
367 ]);
368
369 let json = repo.run_ok(&["patch", "show", &id, "--json", "--revision", "1"]);
370 let v: serde_json::Value = serde_json::from_str(&json).unwrap();
371 let bodies: Vec<&str> = v["reviews"]
372 .as_array()
373 .unwrap()
374 .iter()
375 .map(|r| r["body"].as_str().unwrap())
376 .collect();
377 assert_eq!(
378 bodies,
379 vec!["on r1"],
380 "--json must honour --revision, not ignore it"
381 );
382 }
383
384 // ===========================================================================
385 // 62abe4b9 — `patch checkout` strands you on the created branch
386 // ===========================================================================
387
388 #[test]
389 fn patch_checkout_says_where_you_came_from_and_how_to_get_back() {
390 let (repo, id) = repo_with_one_revision_patch();
391
392 let out = repo.run_ok(&["patch", "checkout", &id]);
393
394 assert!(
395 out.contains("you were on main"),
396 "must name the branch you came from, got: {}",
397 out
398 );
399 assert!(
400 out.contains("git checkout main"),
401 "must name the command that gets you back, got: {}",
402 out
403 );
404 }
405
406 /// Re-checking-out the patch you are already on has nowhere to send you back
407 /// to. "you were on collab/x; return with `git checkout collab/x`" is noise, and
408 /// noise is what stops the useful lines from being read.
409 #[test]
410 fn patch_checkout_from_the_patchs_own_branch_offers_no_pointless_return() {
411 let (repo, id) = repo_with_one_revision_patch();
412 repo.run_ok(&["patch", "checkout", &id]);
413
414 let out = repo.run_ok(&["patch", "checkout", &id]);
415
416 assert!(
417 !out.contains("you were on"),
418 "must not report a move that did not happen, got: {}",
419 out
420 );
421 assert!(
422 !out.contains("Return with"),
423 "must not offer to return you where you already are, got: {}",
424 out
425 );
426 assert!(
427 out.contains(&format!("git branch -D collab/{}", id)),
428 "the cleanup hint is still worth having, got: {}",
429 out
430 );
431 }
432
433 #[test]
434 fn patch_checkout_says_the_branch_is_left_behind_and_how_to_remove_it() {
435 let (repo, id) = repo_with_one_revision_patch();
436
437 let out = repo.run_ok(&["patch", "checkout", &id]);
438
439 assert!(
440 out.contains(&format!("git branch -D collab/{}", id)),
441 "must name the cleanup command for the branch it left behind, got: {}",
442 out
443 );
444 }
445
446 #[test]
447 fn patch_checkout_from_a_detached_head_names_the_commit_to_return_to() {
448 let (repo, id) = repo_with_one_revision_patch();
449 let head = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
450 repo.git(&["checkout", "--detach"]);
451
452 let out = repo.run_ok(&["patch", "checkout", &id]);
453
454 assert!(
455 out.contains("detached HEAD"),
456 "must say you were detached rather than invent a branch name, got: {}",
457 out
458 );
459 assert!(
460 out.contains(&head[..8]),
461 "must name the commit to return to, got: {}",
462 out
463 );
464 }
465
466 /// Checking the same patch out twice used to accumulate `collab/<id>-1`,
467 /// `collab/<id>-2`, ... The reporter's complaint was about the branches a review
468 /// session piles up, so a repeat checkout of an unchanged patch must reuse.
469 #[test]
470 fn patch_checkout_reuses_an_existing_branch_that_already_matches() {
471 let (repo, id) = repo_with_one_revision_patch();
472
473 repo.run_ok(&["patch", "checkout", &id]);
474 repo.git(&["checkout", "main"]);
475 let out = repo.run_ok(&["patch", "checkout", &id]);
476
477 assert!(
478 out.contains(&format!("on branch collab/{}", id)),
479 "must reuse the existing branch, got: {}",
480 out
481 );
482 let branches = repo.git(&["branch", "--list", "collab/*"]);
483 assert_eq!(
484 branches.lines().count(),
485 1,
486 "a repeat checkout must not pile up branches, got: {}",
487 branches
488 );
489 }