a73x

fae00c83

Make an inline comment land somewhere, and mean something

a73x   2026-08-11 10:06

Commit message
Make an inline comment land somewhere, and mean something

An inline comment is an anchor into a revision. Three things were wrong
with that: nothing checked the anchor resolved, nothing in `patch diff`
told a reviewer what to anchor to, and every comment that did land read
as a demand.

Validate the anchor against the revision's tree (fe555587). The tree is
the right oracle, not the working tree: comments are revision-anchored
precisely so they do not drift, so a line that exists in revision 1 is
a valid anchor for revision 1 however far the branch has moved on. A
file the revision contains but the patch never touched is accepted on
purpose — "you changed the caller here and not the parallel one over
there" is a real review move, and the anchor resolves against real
content either way. Both refusals name the fix, and the confirmation
now echoes what it recorded rather than a bare "Comment added."

Make `patch diff` output pasteable (9a0a143b). `--line-numbers` puts
the `path:line` token in the gutter and `patch comment --at` takes it
verbatim, so nothing has to be retyped or recombined; only new-side
lines get one, because a deleted line is not in the tree and could not
be anchored to. Pin the header prefixes to a/ and b/ — libgit2 honours
`diff.mnemonicPrefix`, so the reported `c/x c/x` was both unparseable
and dependent on whose config rendered it. Add `--stat` and `--path`,
and refuse a `--path` that matches nothing instead of reporting it as
"commits may be identical".

Add `--non-blocking` (0655b32f). A new field on the existing event,
skipped when false so existing signatures still verify. It deliberately
feeds nothing that decides patch state: a request-changes verdict is a
vote its author cast, and letting comment metadata rewrite it would
leave two events disagreeing about the patch.

The body is now resolved after the anchor is checked, so a bad anchor
is reported before $EDITOR opens rather than after the prose is written.

Fixes: fe555587, 9a0a143b, 0655b32f

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/cli.rs
Old New
@@ -592,8 +592,17 @@ pub enum PatchCmd {
592 /// Show interdiff between two revisions (N M or just N for N..latest) 592 /// Show interdiff between two revisions (N M or just N for N..latest)
593 #[arg(long, num_args = 1..=2)] 593 #[arg(long, num_args = 1..=2)]
594 between: Option<Vec<u32>>, 594 between: Option<Vec<u32>>,
595 /// Prefix each line with the `path:line` anchor `patch comment --at` takes
596 #[arg(long)]
597 line_numbers: bool,
598 /// Show a per-file summary instead of the diff body
599 #[arg(long)]
600 stat: bool,
601 /// Restrict the diff to these paths (repeatable)
602 #[arg(long = "path")]
603 paths: Vec<String>,
595 }, 604 },
596 /// Comment on a patch (use --file and --line for inline comments) 605 /// Comment on a patch (use --at, or --file and --line, for inline comments)
597 /// 606 ///
598 /// With no --body and no --body-file, opens $EDITOR. 607 /// With no --body and no --body-file, opens $EDITOR.
599 Comment { 608 Comment {
@@ -604,6 +613,9 @@ pub enum PatchCmd {
604 body: Option<String>, 613 body: Option<String>,
605 #[arg(short = 'F', long, help = BODY_FILE_HELP)] 614 #[arg(short = 'F', long, help = BODY_FILE_HELP)]
606 body_file: Option<String>, 615 body_file: Option<String>,
616 /// Inline anchor as `path:line`, exactly as `patch diff --line-numbers` prints it
617 #[arg(long)]
618 at: Option<String>,
607 /// File path for inline comment 619 /// File path for inline comment
608 #[arg(short, long)] 620 #[arg(short, long)]
609 file: Option<String>, 621 file: Option<String>,
@@ -613,6 +625,9 @@ pub enum PatchCmd {
613 /// Target revision for inline comment 625 /// Target revision for inline comment
614 #[arg(long)] 626 #[arg(long)]
615 revision: Option<u32>, 627 revision: Option<u32>,
628 /// Mark the comment a suggestion rather than something that must change
629 #[arg(long)]
630 non_blocking: bool,
616 }, 631 },
617 /// Review a patch 632 /// Review a patch
618 /// 633 ///
src/event.rs
Old New
@@ -133,6 +133,21 @@ pub enum Action {
133 line: u32, 133 line: u32,
134 body: String, 134 body: String,
135 revision: u32, 135 revision: u32,
136 /// "This is a suggestion, land it anyway." Absent means blocking, which
137 /// is what every comment written before this field existed meant.
138 ///
139 /// Skipped when false for the same reason as `PatchMerge::commit`:
140 /// signatures are verified by re-serializing the event, so writing back
141 /// a key the signer never wrote would invalidate every existing inline
142 /// comment.
143 ///
144 /// Deliberately *not* consulted by anything that decides a patch's
145 /// state. A `request-changes` verdict is a vote its author cast; a
146 /// reviewer marking all their comments non-blocking has not withdrawn
147 /// it, and letting comment metadata rewrite a vote would leave two
148 /// events disagreeing about the patch.
149 #[serde(default, skip_serializing_if = "is_false")]
150 non_blocking: bool,
136 }, 151 },
137 #[serde(rename = "patch.close")] 152 #[serde(rename = "patch.close")]
138 PatchClose { reason: Option<String> }, 153 PatchClose { reason: Option<String> },
@@ -198,6 +213,11 @@ pub enum Action {
198 Merge, 213 Merge,
199 } 214 }
200 215
216 /// `skip_serializing_if` predicate for a `bool` whose absence means `false`.
217 fn is_false(b: &bool) -> bool {
218 !*b
219 }
220
201 #[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)] 221 #[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
202 pub enum ReviewVerdict { 222 pub enum ReviewVerdict {
203 Approve, 223 Approve,
src/lib.rs
Old New
@@ -597,14 +597,20 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
597 println!("\n--- Inline Comments ---"); 597 println!("\n--- Inline Comments ---");
598 for c in &inline_comments { 598 for c in &inline_comments {
599 let rev_label = c.revision.map(|n| format!(" r{}", n)).unwrap_or_default(); 599 let rev_label = c.revision.map(|n| format!(" r{}", n)).unwrap_or_default();
600 // Only the suggestions are labelled. Marking the others
601 // "blocking" would relabel every comment ever written
602 // as a demand, which is the reading the flag exists to
603 // stop being the only one available.
604 let blocking = if c.non_blocking { " [non-blocking]" } else { "" };
600 println!( 605 println!(
601 "\n{} on {}:{} ({}{}){} [{:.8}]:\n {}", 606 "\n{} on {}:{} ({}{}){}{} [{:.8}]:\n {}",
602 c.author.name, 607 c.author.name,
603 c.file, 608 c.file,
604 c.line, 609 c.line,
605 c.timestamp, 610 c.timestamp,
606 rev_label, 611 rev_label,
607 edit_marker(c.edited), 612 edit_marker(c.edited),
613 blocking,
608 c.commit_id, 614 c.commit_id,
609 body_or_tombstone(&c.body, c.deleted) 615 body_or_tombstone(&c.body, c.deleted)
610 ); 616 );
@@ -630,19 +636,45 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
630 id, 636 id,
631 revision, 637 revision,
632 between, 638 between,
639 line_numbers,
640 stat,
641 paths,
633 } => { 642 } => {
634 if revision.is_some() && between.is_some() { 643 if revision.is_some() && between.is_some() {
635 return Err(error::Error::Cmd( 644 return Err(error::Error::Cmd(
636 "--revision and --between are mutually exclusive".to_string(), 645 "--revision and --between are mutually exclusive".to_string(),
637 )); 646 ));
638 } 647 }
648 if stat && line_numbers {
649 return Err(error::Error::Cmd(
650 "--stat is a per-file summary and has no lines for --line-numbers to \
651 number; drop one of them"
652 .to_string(),
653 ));
654 }
639 let between_pair = between.map(|v| { 655 let between_pair = between.map(|v| {
640 let from = v[0]; 656 let from = v[0];
641 let to = v.get(1).copied(); 657 let to = v.get(1).copied();
642 (from, to) 658 (from, to)
643 }); 659 });
644 let diff = patch::diff(repo, &id, revision, between_pair)?; 660 let opts = patch::DiffOpts {
661 line_numbers,
662 stat,
663 paths,
664 };
665 let diff = patch::diff(repo, &id, revision, between_pair, &opts)?;
645 if diff.is_empty() { 666 if diff.is_empty() {
667 // An empty diff means two different things, and saying
668 // "commits may be identical" for both is how a `--path`
669 // typo reads as "this patch changed nothing".
670 if !opts.paths.is_empty() {
671 return Err(error::Error::Cmd(format!(
672 "no changes under {} in this patch; run `git-collab patch diff {} \
673 --stat` for the paths it does touch",
674 opts.paths.join(", "),
675 id
676 )));
677 }
646 println!("No diff available (commits may be identical)."); 678 println!("No diff available (commits may be identical).");
647 } else { 679 } else {
648 print!("{}", diff); 680 print!("{}", diff);
@@ -653,14 +685,38 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
653 id, 685 id,
654 body, 686 body,
655 body_file, 687 body_file,
688 at,
656 file, 689 file,
657 line, 690 line,
658 revision, 691 revision,
692 non_blocking,
659 } => { 693 } => {
694 // `--at` and `--file`/`--line` say the same thing; saying it
695 // twice is a mistake, not a merge.
696 let (file, line) = match at {
697 Some(at) => {
698 if file.is_some() || line.is_some() {
699 return Err(error::Error::Cmd(
700 "--at already gives the file and the line; drop --file and --line"
701 .to_string(),
702 ));
703 }
704 let (f, l) = patch::parse_anchor(&at)?;
705 (Some(f), Some(l))
706 }
707 None => (file, line),
708 };
660 let args = body::BodyArgs::new(body.as_deref(), body_file.as_deref()); 709 let args = body::BodyArgs::new(body.as_deref(), body_file.as_deref());
661 let body = body::resolve_required(&args, "", "comment")?; 710 let placement = patch::comment(
662 patch::comment(repo, &id, &body, file.as_deref(), line, revision)?; 711 repo,
663 println!("Comment added."); 712 &id,
713 &args,
714 file.as_deref(),
715 line,
716 revision,
717 non_blocking,
718 )?;
719 println!("{}", placement);
664 Ok(()) 720 Ok(())
665 } 721 }
666 PatchCmd::Review { 722 PatchCmd::Review {
src/patch.rs
Old New
@@ -454,21 +454,157 @@ pub fn edit_revision(
454 Ok(()) 454 Ok(())
455 } 455 }
456 456
457 /// Where a comment landed, so the confirmation can say so. A bare
458 /// "Comment added." is the same output whether the anchor was right or
459 /// nonsense, which is exactly how a typo'd path used to go unnoticed.
460 pub enum CommentPlacement {
461 Thread,
462 Inline {
463 file: String,
464 line: u32,
465 revision: u32,
466 non_blocking: bool,
467 },
468 }
469
470 impl std::fmt::Display for CommentPlacement {
471 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
472 match self {
473 CommentPlacement::Thread => f.write_str("Comment added."),
474 CommentPlacement::Inline {
475 file,
476 line,
477 revision,
478 non_blocking,
479 } => write!(
480 f,
481 "Comment added on {}:{} (r{}){}",
482 file,
483 line,
484 revision,
485 if *non_blocking { " [non-blocking]" } else { "" }
486 ),
487 }
488 }
489 }
490
491 /// Split a pasteable `path:line` anchor — the token `patch diff --line-numbers`
492 /// prints in its gutter — into its parts.
493 ///
494 /// Rightmost colon wins: a path may contain one, a line number may not.
495 pub fn parse_anchor(at: &str) -> Result<(String, u32), Error> {
496 let malformed = || {
497 Error::Cmd(format!(
498 "--at expects <path>:<line>, as printed by `patch diff --line-numbers`; got '{}'",
499 at
500 ))
501 };
502 let (path, line) = at.rsplit_once(':').ok_or_else(malformed)?;
503 let line: u32 = line.parse().map_err(|_| malformed())?;
504 if path.is_empty() {
505 return Err(malformed());
506 }
507 Ok((path.to_string(), line))
508 }
509
510 /// Reject an inline comment whose anchor does not resolve in the revision it is
511 /// anchored to.
512 ///
513 /// The revision's tree is the right oracle, not the working tree: comments are
514 /// revision-anchored precisely so they do not drift, so a line that exists in
515 /// revision 1 stays a valid anchor for revision 1 however far the branch has
516 /// moved on. Checking the checkout instead would reject comments on old
517 /// revisions and accept comments on code no revision ever contained.
518 ///
519 /// A file the revision contains but the patch never touched is accepted on
520 /// purpose. "You changed the caller here and not the parallel one over there"
521 /// is a real and common review move, and the anchor resolves against real
522 /// content in the revision, so it renders and cannot drift. The failures this
523 /// guards against — a typo'd path, a line read off the wrong side of a diff —
524 /// are both caught by the tree, and neither is a judgement about which files a
525 /// review may discuss.
526 ///
527 /// Purely a read: it resolves objects and writes nothing.
528 fn validate_anchor(
529 repo: &Repository,
530 patch: &PatchState,
531 revision: u32,
532 file: &str,
533 line: u32,
534 ) -> Result<(), Error> {
535 let rev = patch
536 .revisions
537 .iter()
538 .find(|r| r.number == revision)
539 .ok_or_else(|| Error::Cmd(format!("revision {} not found", revision)))?;
540
541 // Patches created before revisions recorded a tree have nothing to check
542 // against. Refusing would make them uncommentable; the anchor is simply
543 // unverifiable, which is the state every anchor used to be in.
544 if rev.tree.is_empty() {
545 return Ok(());
546 }
547 let tree = match Oid::from_str(&rev.tree).and_then(|oid| repo.find_tree(oid)) {
548 Ok(tree) => tree,
549 // The revision names a tree this clone does not have. Same reasoning.
550 Err(_) => return Ok(()),
551 };
552
553 let short = crate::abbrev::for_patches(repo).of(&patch.id).to_string();
554 let entry = tree.get_path(std::path::Path::new(file)).map_err(|_| {
555 Error::Cmd(format!(
556 "no file '{}' in revision {} of patch {}; run `git-collab patch diff {} --revision {} --line-numbers` and paste an anchor from the gutter into --at",
557 file, revision, short, short, revision
558 ))
559 })?;
560 if entry.kind() != Some(git2::ObjectType::Blob) {
561 return Err(Error::Cmd(format!(
562 "'{}' is a directory in revision {} of patch {}, not a file; --file takes the path of a file",
563 file, revision, short
564 )));
565 }
566
567 let blob = entry.to_object(repo)?.peel_to_blob()?;
568 // A binary file has no lines to bound the anchor with. The file check has
569 // already done the work that matters.
570 if blob.is_binary() {
571 return Ok(());
572 }
573 let Ok(text) = std::str::from_utf8(blob.content()) else {
574 return Ok(());
575 };
576 let count = text.lines().count().max(1) as u32;
577 if line == 0 || line > count {
578 return Err(Error::Cmd(format!(
579 "line {} is out of range for '{}' in revision {} of patch {}; pass a line in 1-{}",
580 line, file, revision, short, count
581 )));
582 }
583 Ok(())
584 }
585
586 /// Add a thread comment, or an inline one anchored to a revision.
587 ///
588 /// The body arrives unresolved on purpose. Resolving it can open `$EDITOR`, and
589 /// asking someone to write a review comment and only then telling them the file
590 /// they named is not in the patch is the same insult as accepting it silently.
591 /// Everything that can be checked without the prose is checked first.
457 pub fn comment( 592 pub fn comment(
458 repo: &Repository, 593 repo: &Repository,
459 id_prefix: &str, 594 id_prefix: &str,
460 body: &str, 595 body_args: &crate::body::BodyArgs,
461 file: Option<&str>, 596 file: Option<&str>,
462 line: Option<u32>, 597 line: Option<u32>,
463 target_revision: Option<u32>, 598 target_revision: Option<u32>,
464 ) -> Result<(), crate::error::Error> { 599 non_blocking: bool,
600 ) -> Result<CommentPlacement, crate::error::Error> {
465 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 601 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
466 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 602 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
467 let patch = PatchState::from_ref(repo, &ref_name, &id)?; 603 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
468 604
469 let author = get_author(repo)?; 605 let author = get_author(repo)?;
470 606
471 let action = match (file, line) { 607 let placement = match (file, line) {
472 (Some(f), Some(l)) => { 608 (Some(f), Some(l)) => {
473 // Determine revision for the inline comment 609 // Determine revision for the inline comment
474 let rev = if let Some(target) = target_revision { 610 let rev = if let Some(target) = target_revision {
@@ -480,11 +616,12 @@ pub fn comment(
480 } else { 616 } else {
481 patch.revisions.last().map(|r| r.number).unwrap_or(1) 617 patch.revisions.last().map(|r| r.number).unwrap_or(1)
482 }; 618 };
483 Action::PatchInlineComment { 619 validate_anchor(repo, &patch, rev, f, l)?;
620 CommentPlacement::Inline {
484 file: f.to_string(), 621 file: f.to_string(),
485 line: l, 622 line: l,
486 body: body.to_string(),
487 revision: rev, 623 revision: rev,
624 non_blocking,
488 } 625 }
489 } 626 }
490 (Some(_), None) | (None, Some(_)) => { 627 (Some(_), None) | (None, Some(_)) => {
@@ -500,12 +637,32 @@ pub fn comment(
500 "thread comments are not revision-scoped; use --file and --line for inline comments".to_string(), 637 "thread comments are not revision-scoped; use --file and --line for inline comments".to_string(),
501 )); 638 ));
502 } 639 }
503 Action::PatchComment { 640 if non_blocking {
504 body: body.to_string(), 641 return Err(Error::Cmd(
642 "--non-blocking marks one inline comment; anchor it with --at <path>:<line>, or with --file and --line".to_string(),
643 ));
505 } 644 }
645 CommentPlacement::Thread
506 } 646 }
507 }; 647 };
508 648
649 let body = crate::body::resolve_required(body_args, "", "comment")?;
650 let action = match &placement {
651 CommentPlacement::Thread => Action::PatchComment { body },
652 CommentPlacement::Inline {
653 file,
654 line,
655 revision,
656 non_blocking,
657 } => Action::PatchInlineComment {
658 file: file.clone(),
659 line: *line,
660 body,
661 revision: *revision,
662 non_blocking: *non_blocking,
663 },
664 };
665
509 let event = Event { 666 let event = Event {
510 timestamp: chrono::Utc::now().to_rfc3339(), 667 timestamp: chrono::Utc::now().to_rfc3339(),
511 author, 668 author,
@@ -513,7 +670,7 @@ pub fn comment(
513 clock: 0, 670 clock: 0,
514 }; 671 };
515 dag::append_event(repo, &ref_name, &event, &sk)?; 672 dag::append_event(repo, &ref_name, &event, &sk)?;
516 Ok(()) 673 Ok(placement)
517 } 674 }
518 675
519 pub fn review( 676 pub fn review(
@@ -652,23 +809,56 @@ pub fn revise(
652 Ok(()) 809 Ok(())
653 } 810 }
654 811
812 /// How to render a diff. All three settings are presentation only — none of
813 /// them changes which trees are compared, and none of them writes anything.
814 #[derive(Debug, Default, Clone)]
815 pub struct DiffOpts {
816 /// Prefix every anchorable line with the `path:line` token that
817 /// `patch comment --at` accepts. Off by default: the plain diff stays a
818 /// valid patch that `git apply` will take.
819 pub line_numbers: bool,
820 /// Render the per-file summary instead of the body.
821 pub stat: bool,
822 /// Restrict the diff to these pathspecs. Empty means the whole diff.
823 pub paths: Vec<String>,
824 }
825
826 /// Build the libgit2 diff options for a render.
827 ///
828 /// The prefixes are pinned rather than left to libgit2, which honours the
829 /// user's `diff.mnemonicPrefix`. Under that setting a tree-to-tree diff comes
830 /// out as `diff --git c/x c/x` — the same path twice, with no way for anything
831 /// parsing the header to tell old from new — and, worse, the prefixes then
832 /// depend on whose config rendered the diff. `git format-patch` pins `a/` and
833 /// `b/` for the same reason: this output is meant to be read by other programs.
834 fn diff_options(opts: &DiffOpts) -> git2::DiffOptions {
835 let mut o = git2::DiffOptions::new();
836 o.old_prefix("a");
837 o.new_prefix("b");
838 for p in &opts.paths {
839 o.pathspec(p);
840 }
841 o
842 }
843
655 /// Generate a unified diff between a patch's base branch and head commit. 844 /// Generate a unified diff between a patch's base branch and head commit.
656 pub fn diff( 845 pub fn diff(
657 repo: &Repository, 846 repo: &Repository,
658 id_prefix: &str, 847 id_prefix: &str,
659 revision: Option<u32>, 848 revision: Option<u32>,
660 between: Option<(u32, Option<u32>)>, 849 between: Option<(u32, Option<u32>)>,
850 opts: &DiffOpts,
661 ) -> Result<String, Error> { 851 ) -> Result<String, Error> {
662 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 852 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
663 let p = PatchState::from_ref(repo, &ref_name, &id)?; 853 let p = PatchState::from_ref(repo, &ref_name, &id)?;
664 854
665 if let Some((from, to)) = between { 855 if let Some((from, to)) = between {
666 let to_rev = to.unwrap_or_else(|| p.revisions.last().map(|r| r.number).unwrap_or(1)); 856 let to_rev = to.unwrap_or_else(|| p.revisions.last().map(|r| r.number).unwrap_or(1));
667 interdiff(repo, &p, from, to_rev) 857 interdiff(repo, &p, from, to_rev, opts)
668 } else if let Some(rev) = revision { 858 } else if let Some(rev) = revision {
669 generate_diff_at_revision(repo, &p, rev) 859 generate_diff_at_revision(repo, &p, rev, opts)
670 } else { 860 } else {
671 generate_diff(repo, &p) 861 generate_diff(repo, &p, opts)
672 } 862 }
673 } 863 }
674 864
@@ -710,7 +900,11 @@ fn recorded_base_tree<'a>(repo: &'a Repository, base: Option<Oid>) -> Option<git
710 } 900 }
711 901
712 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff. 902 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff.
713 pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<String, Error> { 903 pub fn generate_diff(
904 repo: &Repository,
905 patch: &state::PatchState,
906 opts: &DiffOpts,
907 ) -> Result<String, Error> {
714 let head_oid = patch.resolve_head(repo)?; 908 let head_oid = patch.resolve_head(repo)?;
715 let head_commit = repo 909 let head_commit = repo
716 .find_commit(head_oid) 910 .find_commit(head_oid)
@@ -721,8 +915,12 @@ pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<Str
721 None => resolve_base_tree(repo, &patch.base_ref, head_oid)?, 915 None => resolve_base_tree(repo, &patch.base_ref, head_oid)?,
722 }; 916 };
723 917
724 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; 918 let git_diff = repo.diff_tree_to_tree(
725 format_diff(&git_diff) 919 base_tree.as_ref(),
920 Some(&head_tree),
921 Some(&mut diff_options(opts)),
922 )?;
923 format_diff(&git_diff, opts)
726 } 924 }
727 925
728 /// Generate a diff for a specific revision against the base branch (historical full diff). 926 /// Generate a diff for a specific revision against the base branch (historical full diff).
@@ -730,6 +928,7 @@ fn generate_diff_at_revision(
730 repo: &Repository, 928 repo: &Repository,
731 patch: &PatchState, 929 patch: &PatchState,
732 rev_number: u32, 930 rev_number: u32,
931 opts: &DiffOpts,
733 ) -> Result<String, Error> { 932 ) -> Result<String, Error> {
734 let revision = patch 933 let revision = patch
735 .revisions 934 .revisions
@@ -744,8 +943,12 @@ fn generate_diff_at_revision(
744 None => resolve_base_tree(repo, &patch.base_ref, commit_oid)?, 943 None => resolve_base_tree(repo, &patch.base_ref, commit_oid)?,
745 }; 944 };
746 945
747 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; 946 let git_diff = repo.diff_tree_to_tree(
748 format_diff(&git_diff) 947 base_tree.as_ref(),
948 Some(&head_tree),
949 Some(&mut diff_options(opts)),
950 )?;
951 format_diff(&git_diff, opts)
749 } 952 }
750 953
751 /// Compute the interdiff between two revisions: what the author changed in 954 /// Compute the interdiff between two revisions: what the author changed in
@@ -777,6 +980,7 @@ pub fn interdiff(
777 patch: &PatchState, 980 patch: &PatchState,
778 from_rev: u32, 981 from_rev: u32,
779 to_rev: u32, 982 to_rev: u32,
983 opts: &DiffOpts,
780 ) -> Result<String, Error> { 984 ) -> Result<String, Error> {
781 let from = find_revision(patch, from_rev)?; 985 let from = find_revision(patch, from_rev)?;
782 let to = find_revision(patch, to_rev)?; 986 let to = find_revision(patch, to_rev)?;
@@ -794,8 +998,12 @@ pub fn interdiff(
794 let from_tree = repo.find_tree(Oid::from_str(&from.tree)?)?; 998 let from_tree = repo.find_tree(Oid::from_str(&from.tree)?)?;
795 let to_tree = repo.find_tree(Oid::from_str(&to.tree)?)?; 999 let to_tree = repo.find_tree(Oid::from_str(&to.tree)?)?;
796 let plain = |note: String| -> Result<String, Error> { 1000 let plain = |note: String| -> Result<String, Error> {
797 let git_diff = repo.diff_tree_to_tree(Some(&from_tree), Some(&to_tree), None)?; 1001 let git_diff = repo.diff_tree_to_tree(
798 Ok(note + &format_diff(&git_diff)?) 1002 Some(&from_tree),
1003 Some(&to_tree),
1004 Some(&mut diff_options(opts)),
1005 )?;
1006 Ok(note + &format_diff(&git_diff, opts)?)
799 }; 1007 };
800 1008
801 let (older, newer) = if from_rev < to_rev { 1009 let (older, newer) = if from_rev < to_rev {
@@ -862,18 +1070,18 @@ pub fn interdiff(
862 1070
863 // The replayed side lives in an in-memory index rather than a written tree: 1071 // The replayed side lives in an in-memory index rather than a written tree:
864 // rendering a diff is a read and has no business adding objects to the odb. 1072 // rendering a diff is a read and has no business adding objects to the odb.
865 let mut opts = git2::DiffOptions::new(); 1073 let mut git_opts = diff_options(opts);
866 let git_diff = if from_rev < to_rev { 1074 let git_diff = if from_rev < to_rev {
867 // Older (replayed) -> newer. `diff_tree_to_index` only runs tree-first, 1075 // Older (replayed) -> newer. `diff_tree_to_index` only runs tree-first,
868 // so ask for it reversed. libgit2 swaps the path prefixes along with the 1076 // so ask for it reversed. libgit2 swaps the path prefixes along with the
869 // sides, so they go in pre-swapped to come out as the `a/`, `b/` every 1077 // sides, so they go in pre-swapped to come out as the `a/`, `b/` every
870 // consumer of a unified diff expects. 1078 // consumer of a unified diff expects.
871 opts.reverse(true).old_prefix("b").new_prefix("a"); 1079 git_opts.reverse(true).old_prefix("b").new_prefix("a");
872 repo.diff_tree_to_index(Some(&to_tree), Some(&replayed), Some(&mut opts))? 1080 repo.diff_tree_to_index(Some(&to_tree), Some(&replayed), Some(&mut git_opts))?
873 } else { 1081 } else {
874 repo.diff_tree_to_index(Some(&from_tree), Some(&replayed), Some(&mut opts))? 1082 repo.diff_tree_to_index(Some(&from_tree), Some(&replayed), Some(&mut git_opts))?
875 }; 1083 };
876 Ok(note + &format_diff(&git_diff)?) 1084 Ok(note + &format_diff(&git_diff, opts)?)
877 } 1085 }
878 1086
879 fn find_revision(patch: &PatchState, number: u32) -> Result<&state::Revision, Error> { 1087 fn find_revision(patch: &PatchState, number: u32) -> Result<&state::Revision, Error> {
@@ -932,28 +1140,78 @@ fn replay_onto(repo: &Repository, revision: Oid, base: Oid) -> Result<Replay, Er
932 } 1140 }
933 1141
934 /// Format a git2::Diff as a unified diff string. 1142 /// Format a git2::Diff as a unified diff string.
935 fn format_diff(git_diff: &git2::Diff) -> Result<String, Error> { 1143 ///
936 let mut output = String::new(); 1144 /// With `opts.line_numbers`, every line that a comment can be anchored to is
937 let mut lines = 0usize; 1145 /// prefixed with the `path:line` token `patch comment --at` accepts, so a
938 git_diff.print(DiffFormat::Patch, |_delta, _hunk, line| { 1146 /// reviewer copies an anchor out of the diff rather than reconstructing one by
939 if lines >= 5000 { 1147 /// hand. Only new-side lines get one: a deleted line is not in the revision's
1148 /// tree, so nothing could anchor to it and offering a token there would offer
1149 /// one that `patch comment` is about to reject.
1150 fn format_diff(git_diff: &git2::Diff, opts: &DiffOpts) -> Result<String, Error> {
1151 if opts.stat {
1152 let stats = git_diff.stats()?;
1153 let buf = stats.to_buf(git2::DiffStatsFormat::FULL, 80)?;
1154 return Ok(String::from_utf8_lossy(&buf).into_owned());
1155 }
1156
1157 // `(gutter, text)`. `None` marks a line with no anchor; a header line
1158 // carries no gutter at all, so it is held separately from "anchorable but
1159 // not on the new side".
1160 let mut rows: Vec<(Option<Option<String>>, String)> = Vec::new();
1161 git_diff.print(DiffFormat::Patch, |delta, _hunk, line| {
1162 if rows.len() >= 5000 {
940 return true; 1163 return true;
941 } 1164 }
942 let prefix = match line.origin() { 1165 let prefix = match line.origin() {
943 '+' => "+", 1166 '+' => Some("+"),
944 '-' => "-", 1167 '-' => Some("-"),
945 ' ' => " ", 1168 ' ' => Some(" "),
946 _ => "", 1169 _ => None,
947 }; 1170 };
948 output.push_str(prefix); 1171 let mut text = String::from(prefix.unwrap_or(""));
949 if let Ok(content) = std::str::from_utf8(line.content()) { 1172 if let Ok(content) = std::str::from_utf8(line.content()) {
950 output.push_str(content); 1173 text.push_str(content);
951 } 1174 }
952 lines += 1; 1175 let gutter = if opts.line_numbers && prefix.is_some() {
1176 Some(line.new_lineno().and_then(|n| {
1177 delta
1178 .new_file()
1179 .path()
1180 .map(|p| format!("{}:{}", p.display(), n))
1181 }))
1182 } else {
1183 None
1184 };
1185 rows.push((gutter, text));
953 true 1186 true
954 })?; 1187 })?;
955 1188
956 if lines >= 5000 { 1189 let truncated = rows.len() >= 5000;
1190 let width = rows
1191 .iter()
1192 .filter_map(|(g, _)| g.as_ref().and_then(|a| a.as_ref()).map(|s| s.chars().count()))
1193 .max()
1194 .unwrap_or(0);
1195
1196 let mut output = String::new();
1197 for (gutter, text) in &rows {
1198 match gutter {
1199 Some(Some(anchor)) => {
1200 output.push_str(anchor);
1201 for _ in anchor.chars().count()..width {
1202 output.push(' ');
1203 }
1204 output.push(' ');
1205 }
1206 // Anchorable position, no new-side line: keep the body aligned.
1207 Some(None) => output.push_str(&" ".repeat(width + 1)),
1208 // Headers and hunk markers stay flush left.
1209 None => {}
1210 }
1211 output.push_str(text);
1212 }
1213
1214 if truncated {
957 output.push_str("\n[truncated at 5000 lines]"); 1215 output.push_str("\n[truncated at 5000 lines]");
958 } 1216 }
959 1217
src/server/http/repo/patches.rs
Old New
@@ -101,6 +101,7 @@ pub struct InlineCommentView {
101 pub revision: Option<u32>, 101 pub revision: Option<u32>,
102 pub edited: bool, 102 pub edited: bool,
103 pub deleted: bool, 103 pub deleted: bool,
104 pub non_blocking: bool,
104 } 105 }
105 106
106 #[derive(Debug)] 107 #[derive(Debug)]
@@ -269,6 +270,7 @@ pub async fn patch_detail(
269 body: ic.body, 270 body: ic.body,
270 timestamp: ic.timestamp, 271 timestamp: ic.timestamp,
271 revision: ic.revision, 272 revision: ic.revision,
273 non_blocking: ic.non_blocking,
272 edited: ic.edited, 274 edited: ic.edited,
273 deleted: ic.deleted, 275 deleted: ic.deleted,
274 }) 276 })
src/server/http/templates/patch_detail.html
Old New
@@ -73,6 +73,7 @@
73 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ ic.timestamp }}</span> 73 &nbsp; <span class="mono" style="color: #666; font-size: 0.85em;">{{ ic.timestamp }}</span>
74 {% if let Some(rev) = ic.revision %}&nbsp; rev {{ rev }}{% endif %} 74 {% if let Some(rev) = ic.revision %}&nbsp; rev {{ rev }}{% endif %}
75 {% if ic.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %} 75 {% if ic.edited %}&nbsp; <span style="color: #666; font-size: 0.85em;">(edited)</span>{% endif %}
76 {% if ic.non_blocking %}&nbsp; <span style="color: #666; font-size: 0.85em;">[non-blocking]</span>{% endif %}
76 </p> 77 </p>
77 {% if ic.deleted %} 78 {% if ic.deleted %}
78 <p style="margin: 0; color: #666; font-style: italic;">[deleted]</p> 79 <p style="margin: 0; color: #666; font-style: italic;">[deleted]</p>
src/state.rs
Old New
@@ -381,6 +381,17 @@ pub struct InlineComment {
381 pub edited: bool, 381 pub edited: bool,
382 #[serde(default)] 382 #[serde(default)]
383 pub deleted: bool, 383 pub deleted: bool,
384 /// A suggestion rather than a demand. Always serialized, unlike the event
385 /// field it comes from: a scripted caller reading `--json` must be able to
386 /// tell "blocking" from "this build of git-collab does not report it", and
387 /// only a key that is always present does that.
388 ///
389 /// `#[serde(default)]` covers a fold-cache entry written before the field
390 /// existed. `false` is the right value for one: no event predating the
391 /// field could have carried `non_blocking: true`, so a stale entry cannot
392 /// be hiding a marked comment.
393 #[serde(default)]
394 pub non_blocking: bool,
384 } 395 }
385 396
386 #[derive(Debug, Clone, Serialize, Deserialize)] 397 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -1223,6 +1234,7 @@ impl PatchState {
1223 line, 1234 line,
1224 body, 1235 body,
1225 revision, 1236 revision,
1237 non_blocking,
1226 } => { 1238 } => {
1227 if let Some(ref mut s) = state { 1239 if let Some(ref mut s) = state {
1228 overrides.note_owner(oid, &event.author); 1240 overrides.note_owner(oid, &event.author);
@@ -1236,6 +1248,7 @@ impl PatchState {
1236 commit_id: oid, 1248 commit_id: oid,
1237 edited: false, 1249 edited: false,
1238 deleted: false, 1250 deleted: false,
1251 non_blocking,
1239 }); 1252 });
1240 } 1253 }
1241 } 1254 }
src/tui/events.rs
Old New
@@ -249,13 +249,13 @@ fn generate_patch_diff_for(
249 if interdiff_mode && rev_idx > 0 { 249 if interdiff_mode && rev_idx > 0 {
250 let from_rev = patch.revisions[rev_idx - 1].number; 250 let from_rev = patch.revisions[rev_idx - 1].number;
251 let to_rev = rev.number; 251 let to_rev = rev.number;
252 match patch_mod::interdiff(repo, patch, from_rev, to_rev) { 252 match patch_mod::interdiff(repo, patch, from_rev, to_rev, &Default::default()) {
253 Ok(d) => d, 253 Ok(d) => d,
254 Err(e) => format!("(error generating interdiff: {})", e), 254 Err(e) => format!("(error generating interdiff: {})", e),
255 } 255 }
256 } else { 256 } else {
257 // Diff at specific revision vs base 257 // Diff at specific revision vs base
258 match patch_mod::diff(repo, &patch.id, Some(rev.number), None) { 258 match patch_mod::diff(repo, &patch.id, Some(rev.number), None, &Default::default()) {
259 Ok(d) => d, 259 Ok(d) => d,
260 Err(e) => format!("(error generating diff: {})", e), 260 Err(e) => format!("(error generating diff: {})", e),
261 } 261 }
src/tui/mod.rs
Old New
@@ -436,6 +436,7 @@ mod tests {
436 line: 42, 436 line: 42,
437 body: "nit".to_string(), 437 body: "nit".to_string(),
438 revision: 1, 438 revision: 1,
439 non_blocking: false,
439 }; 440 };
440 assert_eq!(action_type_label(&action), "Inline Comment"); 441 assert_eq!(action_type_label(&action), "Inline Comment");
441 } 442 }
@@ -1056,6 +1057,7 @@ mod tests {
1056 commit_id: Oid::from_str("cccccccccccccccccccccccccccccccccccccccc").unwrap(), 1057 commit_id: Oid::from_str("cccccccccccccccccccccccccccccccccccccccc").unwrap(),
1057 edited: false, 1058 edited: false,
1058 deleted: false, 1059 deleted: false,
1060 non_blocking: false,
1059 }], 1061 }],
1060 reviews: vec![crate::state::Review { 1062 reviews: vec![crate::state::Review {
1061 author: make_author(), 1063 author: make_author(),
src/tui/widgets.rs
Old New
@@ -28,6 +28,15 @@ fn edited_span(edited: bool) -> Span<'static> {
28 ) 28 )
29 } 29 }
30 30
31 /// Only suggestions are labelled; an unmarked comment is blocking, which is
32 /// what every comment written before the flag existed was.
33 fn non_blocking_span(non_blocking: bool) -> Span<'static> {
34 Span::styled(
35 if non_blocking { " [non-blocking]" } else { "" },
36 Style::default().fg(Color::DarkGray),
37 )
38 }
39
31 pub(crate) fn action_type_label(action: &Action) -> &str { 40 pub(crate) fn action_type_label(action: &Action) -> &str {
32 match action { 41 match action {
33 Action::IssueOpen { .. } => "Issue Open", 42 Action::IssueOpen { .. } => "Issue Open",
@@ -779,6 +788,7 @@ fn build_patch_detail_text(app: &App) -> Text<'static> {
779 Span::raw(format!(" {}:{}", ic.file, ic.line)), 788 Span::raw(format!(" {}:{}", ic.file, ic.line)),
780 Span::raw(rev_label), 789 Span::raw(rev_label),
781 edited_span(ic.edited), 790 edited_span(ic.edited),
791 non_blocking_span(ic.non_blocking),
782 ])); 792 ]));
783 for l in body_lines(&ic.body, ic.deleted) { 793 for l in body_lines(&ic.body, ic.deleted) {
784 lines.push(Line::raw(format!(" {}", l))); 794 lines.push(Line::raw(format!(" {}", l)));
tests/body_edit_test.rs
Old New
@@ -128,8 +128,18 @@ fn a_patch_thread_comment_can_be_corrected() {
128 fn an_inline_comment_can_be_corrected_without_moving() { 128 fn an_inline_comment_can_be_corrected_without_moving() {
129 let repo = TestRepo::new("Alice", "alice@example.com"); 129 let repo = TestRepo::new("Alice", "alice@example.com");
130 let id = repo.patch_create("inline edit"); 130 let id = repo.patch_create("inline edit");
131 // The anchor has to resolve in the revision: `patch_create` commits
132 // `<title>.txt`, so that is the file under review.
131 repo.run_ok(&[ 133 repo.run_ok(&[
132 "patch", "comment", &id, "--file", "a.rs", "--line", "7", "-b", "wrong", 134 "patch",
135 "comment",
136 &id,
137 "--file",
138 "inline-edit.txt",
139 "--line",
140 "1",
141 "-b",
142 "wrong",
133 ]); 143 ]);
134 144
135 let json = repo.run_ok(&["patch", "show", &id, "--json"]); 145 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
@@ -141,11 +151,11 @@ fn an_inline_comment_can_be_corrected_without_moving() {
141 assert_eq!(body_at(&json, "/inline_comments/0/body"), "right"); 151 assert_eq!(body_at(&json, "/inline_comments/0/body"), "right");
142 assert_eq!( 152 assert_eq!(
143 body_at(&json, "/inline_comments/0/file"), 153 body_at(&json, "/inline_comments/0/file"),
144 "a.rs", 154 "inline-edit.txt",
145 "an edit touches the body and nothing else" 155 "an edit touches the body and nothing else"
146 ); 156 );
147 let value: serde_json::Value = serde_json::from_str(&json).unwrap(); 157 let value: serde_json::Value = serde_json::from_str(&json).unwrap();
148 assert_eq!(value["inline_comments"][0]["line"], 7); 158 assert_eq!(value["inline_comments"][0]["line"], 1);
149 } 159 }
150 160
151 /// The case the issue was actually filed about: a typo in a review body. 161 /// The case the issue was actually filed about: a typo in a review body.
@@ -809,7 +819,7 @@ fn show_prints_addressable_comment_ids() {
809 let id = repo.patch_create("ids"); 819 let id = repo.patch_create("ids");
810 repo.run_ok(&["patch", "comment", &id, "-b", "thread"]); 820 repo.run_ok(&["patch", "comment", &id, "-b", "thread"]);
811 repo.run_ok(&[ 821 repo.run_ok(&[
812 "patch", "comment", &id, "--file", "a.rs", "--line", "1", "-b", "inline", 822 "patch", "comment", &id, "--file", "ids.txt", "--line", "1", "-b", "inline",
813 ]); 823 ]);
814 repo.run_ok(&["patch", "review", &id, "-v", "comment", "-b", "review"]); 824 repo.run_ok(&["patch", "review", &id, "-v", "comment", "-b", "review"]);
815 825
tests/body_input_test.rs
Old New
@@ -129,7 +129,17 @@ fn patch_inline_comment_reads_body_from_stdin() {
129 129
130 repo.run_stdin_ok( 130 repo.run_stdin_ok(
131 &[ 131 &[
132 "patch", "comment", &id, "--file", "src/x.rs", "--line", "3", "-F", "-", 132 "patch",
133 "comment",
134 &id,
135 // The anchor must resolve in the revision; `patch_create` commits
136 // `<title>.txt`.
137 "--file",
138 "inline-from-stdin.txt",
139 "--line",
140 "1",
141 "-F",
142 "-",
133 ], 143 ],
134 NASTY.as_bytes(), 144 NASTY.as_bytes(),
135 ); 145 );
tests/cli_test.rs
Old New
@@ -815,6 +815,8 @@ fn test_patch_inline_comment() {
815 let repo = TestRepo::new("Alice", "alice@example.com"); 815 let repo = TestRepo::new("Alice", "alice@example.com");
816 let id = repo.patch_create("Review me"); 816 let id = repo.patch_create("Review me");
817 817
818 // The anchor is checked against the revision's tree, so it has to name a
819 // file the revision actually has: `patch_create` commits `<title>.txt`.
818 repo.run_ok(&[ 820 repo.run_ok(&[
819 "patch", 821 "patch",
820 "comment", 822 "comment",
@@ -822,15 +824,14 @@ fn test_patch_inline_comment() {
822 "-b", 824 "-b",
823 "Use const here", 825 "Use const here",
824 "-f", 826 "-f",
825 "src/main.rs", 827 "review-me.txt",
826 "-l", 828 "-l",
827 "42", 829 "1",
828 ]); 830 ]);
829 831
830 let out = repo.run_ok(&["patch", "show", &id]); 832 let out = repo.run_ok(&["patch", "show", &id]);
831 assert!(out.contains("Use const here")); 833 assert!(out.contains("Use const here"));
832 assert!(out.contains("src/main.rs")); 834 assert!(out.contains("review-me.txt"));
833 assert!(out.contains("42"));
834 assert!(out.contains("Inline Comments")); 835 assert!(out.contains("Inline Comments"));
835 } 836 }
836 837
tests/inline_anchor_test.rs
Old New
@@ -0,0 +1,608 @@
1 //! Where an inline review comment lands, and what it means.
2 //!
3 //! Three defects, one subject: an inline comment is an *anchor* into a
4 //! revision, and until now nothing checked that the anchor resolved
5 //! (fe555587), nothing in `patch diff` told a reviewer what to anchor to
6 //! (9a0a143b), and every comment that did land read as a demand (0655b32f).
7
8 mod common;
9
10 use common::TestRepo;
11
12 /// A patch on `branch` whose head adds `path` with `content`.
13 /// Returns the abbreviated patch id.
14 fn patch_with_file(repo: &TestRepo, branch: &str, path: &str, content: &str) -> String {
15 repo.git(&["checkout", "-b", branch]);
16 repo.commit_file(path, content, &format!("add {}", path));
17 let out = repo.run_ok(&["patch", "create", "-t", branch, "-B", branch]);
18 repo.git(&["checkout", "main"]);
19 out.trim()
20 .strip_prefix("Created patch ")
21 .unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
22 .to_string()
23 }
24
25 /// Every collab ref and the object it points at, as a single sortable blob.
26 /// A read must leave this identical.
27 fn collab_refs(repo: &TestRepo) -> String {
28 let mut lines: Vec<String> = repo
29 .git(&["for-each-ref", "--format=%(refname) %(objectname)", "refs/"])
30 .lines()
31 .map(|l| l.to_string())
32 .collect();
33 lines.sort();
34 lines.join("\n")
35 }
36
37 // ===========================================================================
38 // fe555587: the anchor must resolve in the revision it is anchored to
39 // ===========================================================================
40
41 #[test]
42 fn a_file_absent_from_the_revision_is_rejected_by_name() {
43 let repo = TestRepo::new("Alice", "alice@example.com");
44 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
45
46 let err = repo.run_err(&[
47 "patch",
48 "comment",
49 &id,
50 "-b",
51 "probe",
52 "-f",
53 "does/not/exist.rs",
54 "-l",
55 "1",
56 ]);
57
58 assert!(
59 err.contains("does/not/exist.rs"),
60 "the error must name the path it could not find: {}",
61 err
62 );
63 assert!(
64 err.contains("revision 1"),
65 "the error must name the revision it checked against: {}",
66 err
67 );
68 assert!(
69 err.contains("--line-numbers"),
70 "the error must name the fix — the command that prints pasteable anchors: {}",
71 err
72 );
73 }
74
75 #[test]
76 fn a_line_past_the_end_of_the_file_is_rejected_with_the_range() {
77 let repo = TestRepo::new("Alice", "alice@example.com");
78 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
79
80 let err = repo.run_err(&[
81 "patch",
82 "comment",
83 &id,
84 "-b",
85 "probe",
86 "-f",
87 "src/lib.rs",
88 "-l",
89 "9000",
90 ]);
91
92 assert!(
93 err.contains("9000"),
94 "the error must name the line that was rejected: {}",
95 err
96 );
97 assert!(
98 err.contains("1-2"),
99 "the error must name the range that would be accepted: {}",
100 err
101 );
102 }
103
104 #[test]
105 fn line_zero_is_rejected() {
106 let repo = TestRepo::new("Alice", "alice@example.com");
107 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
108
109 let err = repo.run_err(&[
110 "patch",
111 "comment",
112 &id,
113 "-b",
114 "probe",
115 "-f",
116 "src/lib.rs",
117 "-l",
118 "0",
119 ]);
120 assert!(
121 err.contains("1-1"),
122 "line 0 is out of range like any other: {}",
123 err
124 );
125 }
126
127 #[test]
128 fn a_directory_is_not_a_comment_anchor() {
129 let repo = TestRepo::new("Alice", "alice@example.com");
130 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
131
132 let err = repo.run_err(&[
133 "patch", "comment", &id, "-b", "probe", "-f", "src", "-l", "1",
134 ]);
135 assert!(
136 err.contains("directory"),
137 "a tree entry that is not a blob has no lines to anchor to: {}",
138 err
139 );
140 }
141
142 /// The deliberate decision: a file the revision contains but the patch never
143 /// touched is a legitimate anchor. "You changed the caller here and not the
144 /// parallel one over there" is a real review move, and the anchor still
145 /// resolves against real content, so it cannot drift.
146 #[test]
147 fn a_file_the_patch_did_not_touch_is_a_legitimate_anchor() {
148 let repo = TestRepo::new("Alice", "alice@example.com");
149 repo.commit_file("untouched.rs", "fn already_here() {}\n", "pre-existing");
150 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
151
152 let out = repo.run_ok(&[
153 "patch",
154 "comment",
155 &id,
156 "-b",
157 "this one needs the same change",
158 "-f",
159 "untouched.rs",
160 "-l",
161 "1",
162 ]);
163 assert!(out.contains("untouched.rs:1"), "unexpected output: {}", out);
164 }
165
166 /// Comments are revision-anchored so they do not drift. Validation has to
167 /// follow: a line that exists in revision 1 stays a valid anchor for revision 1
168 /// however far the working tree has moved on.
169 #[test]
170 fn validation_is_against_the_anchored_revision_not_the_working_tree() {
171 let repo = TestRepo::new("Alice", "alice@example.com");
172 let id = patch_with_file(&repo, "feat", "gone.rs", "one\ntwo\nthree\n");
173
174 // Revision 2 deletes the file entirely.
175 repo.git(&["checkout", "feat"]);
176 repo.git(&["rm", "-q", "gone.rs"]);
177 repo.git(&["commit", "-q", "-m", "drop it"]);
178 repo.git(&["checkout", "main"]);
179 repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
180
181 // r1 still has it: the anchor resolves.
182 let out = repo.run_ok(&[
183 "patch",
184 "comment",
185 &id,
186 "-b",
187 "on the old text",
188 "-f",
189 "gone.rs",
190 "-l",
191 "3",
192 "--revision",
193 "1",
194 ]);
195 assert!(out.contains("gone.rs:3"), "unexpected output: {}", out);
196
197 // r2 does not.
198 let err = repo.run_err(&[
199 "patch",
200 "comment",
201 &id,
202 "-b",
203 "on nothing",
204 "-f",
205 "gone.rs",
206 "-l",
207 "3",
208 "--revision",
209 "2",
210 ]);
211 assert!(err.contains("revision 2"), "unexpected error: {}", err);
212 }
213
214 #[test]
215 fn the_confirmation_echoes_the_anchor_it_recorded() {
216 let repo = TestRepo::new("Alice", "alice@example.com");
217 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
218
219 let out = repo.run_ok(&[
220 "patch",
221 "comment",
222 &id,
223 "-b",
224 "nit",
225 "-f",
226 "src/lib.rs",
227 "-l",
228 "2",
229 ]);
230 assert!(
231 out.contains("src/lib.rs:2") && out.contains("r1"),
232 "a bare 'Comment added.' hides a bad anchor; echo it: {}",
233 out
234 );
235 }
236
237 #[test]
238 fn a_rejected_comment_records_no_event() {
239 let repo = TestRepo::new("Alice", "alice@example.com");
240 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
241
242 let before = collab_refs(&repo);
243 repo.run_err(&[
244 "patch", "comment", &id, "-b", "probe", "-f", "nope.rs", "-l", "1",
245 ]);
246 assert_eq!(before, collab_refs(&repo), "a refusal must not move a ref");
247 }
248
249 // ===========================================================================
250 // 0655b32f: a comment that is a suggestion, not a demand
251 // ===========================================================================
252
253 #[test]
254 fn a_non_blocking_comment_is_marked_in_patch_show() {
255 let repo = TestRepo::new("Alice", "alice@example.com");
256 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
257
258 repo.run_ok(&[
259 "patch",
260 "comment",
261 &id,
262 "-b",
263 "could use a doc comment, land it anyway",
264 "-f",
265 "src/lib.rs",
266 "-l",
267 "1",
268 "--non-blocking",
269 ]);
270 repo.run_ok(&[
271 "patch",
272 "comment",
273 &id,
274 "-b",
275 "this is a bug",
276 "-f",
277 "src/lib.rs",
278 "-l",
279 "2",
280 ]);
281
282 let out = repo.run_ok(&["patch", "show", &id]);
283 let suggestion = out
284 .lines()
285 .find(|l| l.contains("src/lib.rs:1"))
286 .unwrap_or_else(|| panic!("no line for the suggestion: {}", out));
287 let demand = out
288 .lines()
289 .find(|l| l.contains("src/lib.rs:2"))
290 .unwrap_or_else(|| panic!("no line for the demand: {}", out));
291
292 assert!(
293 suggestion.contains("non-blocking"),
294 "the suggestion must say so: {}",
295 suggestion
296 );
297 assert!(
298 !demand.contains("non-blocking"),
299 "an unmarked comment is blocking, and must not be labelled: {}",
300 demand
301 );
302 }
303
304 #[test]
305 fn non_blocking_is_a_json_field_that_defaults_to_false() {
306 let repo = TestRepo::new("Alice", "alice@example.com");
307 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
308
309 repo.run_ok(&[
310 "patch",
311 "comment",
312 &id,
313 "-b",
314 "suggestion",
315 "-f",
316 "src/lib.rs",
317 "-l",
318 "1",
319 "--non-blocking",
320 ]);
321 repo.run_ok(&[
322 "patch",
323 "comment",
324 &id,
325 "-b",
326 "demand",
327 "-f",
328 "src/lib.rs",
329 "-l",
330 "2",
331 ]);
332
333 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
334 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
335 let inline = json["inline_comments"].as_array().unwrap();
336 assert_eq!(inline.len(), 2);
337 assert_eq!(
338 inline[0]["non_blocking"], true,
339 "scripted callers read JSON only: {}",
340 out
341 );
342 assert_eq!(
343 inline[1]["non_blocking"], false,
344 "the field must be present and false, not absent: {}",
345 out
346 );
347 }
348
349 #[test]
350 fn non_blocking_needs_an_anchor() {
351 let repo = TestRepo::new("Alice", "alice@example.com");
352 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
353
354 let err = repo.run_err(&["patch", "comment", &id, "-b", "nit", "--non-blocking"]);
355 assert!(
356 err.contains("--file") && err.contains("--at"),
357 "the error must name the fix: {}",
358 err
359 );
360 }
361
362 /// A verdict is a vote and a comment is a comment. Marking every inline comment
363 /// non-blocking says nothing about the vote the reviewer cast: `request-changes`
364 /// stays `request-changes` until its author changes it.
365 #[test]
366 fn request_changes_stays_blocking_when_every_comment_is_non_blocking() {
367 let repo = TestRepo::new("Alice", "alice@example.com");
368 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
369
370 repo.run_ok(&[
371 "patch",
372 "review",
373 &id,
374 "-v",
375 "request-changes",
376 "-b",
377 "see comments",
378 ]);
379 repo.run_ok(&[
380 "patch",
381 "comment",
382 &id,
383 "-b",
384 "only a suggestion",
385 "-f",
386 "src/lib.rs",
387 "-l",
388 "1",
389 "--non-blocking",
390 ]);
391
392 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
393 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
394 assert_eq!(
395 json["reviews"][0]["verdict"], "request-changes",
396 "comment metadata must not rewrite a vote: {}",
397 out
398 );
399 }
400
401 // ===========================================================================
402 // 9a0a143b: a diff a reviewer can comment from
403 // ===========================================================================
404
405 /// The reported `c/src/cli.rs c/src/cli.rs` is what libgit2 emits under
406 /// `diff.mnemonicPrefix`, which the reporter had set globally. So the prefixes
407 /// were not merely wrong, they varied by whose config rendered the diff. This
408 /// pins them against the config that produced the bug.
409 #[test]
410 fn diff_headers_use_a_and_b_prefixes() {
411 let repo = TestRepo::new("Alice", "alice@example.com");
412 repo.git(&["config", "diff.mnemonicPrefix", "true"]);
413 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
414
415 // Modify a tracked file rather than add one, so both sides have a path.
416 repo.git(&["checkout", "feat"]);
417 repo.commit_file("src/lib.rs", "fn a() {}\nfn b() {}\n", "and another");
418 repo.git(&["checkout", "main"]);
419 repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
420
421 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
422 assert!(
423 out.contains("diff --git a/src/lib.rs b/src/lib.rs"),
424 "old and new must be distinguishable in the header: {}",
425 out
426 );
427 assert!(
428 out.contains("--- a/src/lib.rs"),
429 "old side must be a/: {}",
430 out
431 );
432 assert!(
433 out.contains("+++ b/src/lib.rs"),
434 "new side must be b/: {}",
435 out
436 );
437 }
438
439 /// The whole point: what the reviewer copies out of the diff goes straight into
440 /// `patch comment` with no editing.
441 #[test]
442 fn a_line_numbered_diff_yields_an_anchor_that_pastes_verbatim() {
443 let repo = TestRepo::new("Alice", "alice@example.com");
444 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\nfn b() {}\n");
445
446 let out = repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
447 let anchor = out
448 .lines()
449 .find_map(|l| {
450 let token = l.split_whitespace().next()?;
451 token.starts_with("src/lib.rs:").then(|| token.to_string())
452 })
453 .unwrap_or_else(|| panic!("no pasteable anchor in the gutter:\n{}", out));
454 assert_eq!(anchor, "src/lib.rs:1");
455
456 let confirm = repo.run_ok(&["patch", "comment", &id, "-b", "here", "--at", &anchor]);
457 assert!(confirm.contains("src/lib.rs:1"), "unexpected: {}", confirm);
458 }
459
460 #[test]
461 fn a_removed_line_gets_no_new_side_anchor() {
462 let repo = TestRepo::new("Alice", "alice@example.com");
463 repo.commit_file("f.txt", "keep\ndrop\n", "base");
464 let id = patch_with_file(&repo, "feat", "other.txt", "x\n");
465 repo.git(&["checkout", "feat"]);
466 repo.commit_file("f.txt", "keep\n", "remove a line");
467 repo.git(&["checkout", "main"]);
468 repo.run_ok(&["patch", "revise", &id, "-b", "r2", "-B", "feat"]);
469
470 let out = repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
471 let removed = out
472 .lines()
473 .find(|l| l.trim_end().ends_with("-drop"))
474 .unwrap_or_else(|| panic!("no removed line in:\n{}", out));
475 assert!(
476 !removed.contains("f.txt:"),
477 "a deleted line is not in the new side and cannot be anchored to: {}",
478 removed
479 );
480 }
481
482 #[test]
483 fn diff_stat_gives_a_per_file_breakdown() {
484 let repo = TestRepo::new("Alice", "alice@example.com");
485 repo.git(&["checkout", "-b", "feat"]);
486 repo.commit_file("one.txt", "a\nb\n", "one");
487 repo.commit_file("two.txt", "c\n", "two");
488 let out = repo.run_ok(&["patch", "create", "-t", "two files", "-B", "feat"]);
489 repo.git(&["checkout", "main"]);
490 let id = out.trim().strip_prefix("Created patch ").unwrap();
491
492 let stat = repo.run_ok(&["patch", "diff", id, "--stat"]);
493 assert!(stat.contains("one.txt"), "unexpected stat: {}", stat);
494 assert!(stat.contains("two.txt"), "unexpected stat: {}", stat);
495 assert!(
496 !stat.contains("+a"),
497 "--stat is a summary, not the diff body: {}",
498 stat
499 );
500 }
501
502 #[test]
503 fn diff_can_be_scoped_to_one_path() {
504 let repo = TestRepo::new("Alice", "alice@example.com");
505 repo.git(&["checkout", "-b", "feat"]);
506 repo.commit_file("one.txt", "a\n", "one");
507 repo.commit_file("two.txt", "c\n", "two");
508 let out = repo.run_ok(&["patch", "create", "-t", "two files", "-B", "feat"]);
509 repo.git(&["checkout", "main"]);
510 let id = out.trim().strip_prefix("Created patch ").unwrap();
511
512 let scoped = repo.run_ok(&["patch", "diff", id, "--path", "one.txt"]);
513 assert!(scoped.contains("one.txt"), "unexpected: {}", scoped);
514 assert!(
515 !scoped.contains("two.txt"),
516 "--path must exclude everything else: {}",
517 scoped
518 );
519 }
520
521 /// "No diff available (commits may be identical)" is true of a patch with no
522 /// changes and false of a `--path` nobody in the patch touched. One message for
523 /// two situations is the same defect as accepting an unresolvable anchor.
524 #[test]
525 fn a_path_filter_that_matches_nothing_says_so() {
526 let repo = TestRepo::new("Alice", "alice@example.com");
527 let id = patch_with_file(&repo, "feat", "one.txt", "a\n");
528
529 let err = repo.run_err(&["patch", "diff", &id, "--path", "nosuch.txt"]);
530 assert!(
531 err.contains("nosuch.txt"),
532 "the error must name the path that matched nothing: {}",
533 err
534 );
535 assert!(
536 err.contains("--stat"),
537 "the error must name the fix — how to find the paths the patch has: {}",
538 err
539 );
540 }
541
542 #[test]
543 fn stat_and_line_numbers_are_refused_together() {
544 let repo = TestRepo::new("Alice", "alice@example.com");
545 let id = patch_with_file(&repo, "feat", "one.txt", "a\n");
546
547 let err = repo.run_err(&["patch", "diff", &id, "--stat", "--line-numbers"]);
548 assert!(
549 err.contains("--stat") && err.contains("--line-numbers"),
550 "the error must name both flags and which to drop: {}",
551 err
552 );
553 }
554
555 #[test]
556 fn rendering_a_diff_moves_no_ref() {
557 let repo = TestRepo::new("Alice", "alice@example.com");
558 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
559
560 let before = collab_refs(&repo);
561 repo.run_ok(&["patch", "diff", &id]);
562 repo.run_ok(&["patch", "diff", &id, "--line-numbers"]);
563 repo.run_ok(&["patch", "diff", &id, "--stat"]);
564 repo.run_ok(&["patch", "diff", &id, "--path", "src/lib.rs"]);
565 assert_eq!(
566 before,
567 collab_refs(&repo),
568 "rendering a diff is a read and must write nothing"
569 );
570 }
571
572 #[test]
573 fn at_and_file_line_are_two_ways_to_say_one_thing() {
574 let repo = TestRepo::new("Alice", "alice@example.com");
575 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
576
577 let err = repo.run_err(&[
578 "patch",
579 "comment",
580 &id,
581 "-b",
582 "x",
583 "--at",
584 "src/lib.rs:1",
585 "-f",
586 "src/lib.rs",
587 "-l",
588 "1",
589 ]);
590 assert!(
591 err.contains("--at"),
592 "naming the anchor twice is a mistake worth reporting: {}",
593 err
594 );
595 }
596
597 #[test]
598 fn a_malformed_at_names_the_form_it_wanted() {
599 let repo = TestRepo::new("Alice", "alice@example.com");
600 let id = patch_with_file(&repo, "feat", "src/lib.rs", "fn a() {}\n");
601
602 let err = repo.run_err(&["patch", "comment", &id, "-b", "x", "--at", "src/lib.rs"]);
603 assert!(
604 err.contains("<path>:<line>"),
605 "the error must show the form: {}",
606 err
607 );
608 }