a73x

eb3de6b1

Track whether review comments have been addressed

a73x   2026-08-11 10:55

Commit message
Track whether review comments have been addressed

An inline comment is anchored to the revision it was written on, so it
never drifts — but nothing recorded whether it was ever answered. With
five threads across four revisions a reviewer re-read all five every
round to find the two that still mattered.

That is the branch workflow's tax arriving through a different door.
Under a pull request the correspondence between feedback and the change
answering it is destroyed at the force-push, and authors reconstruct it
by hand with `fix(pr): address review` commits. Revisions are immutable
here and the interdiff is rebase-aware, so the correspondence can be
kept — and shown rather than asserted.

Two ordinary events, `patch.comment_resolve` and `patch.comment_reopen`,
so the existing `(clock, oid)` fold handles concurrency and no new
conflict machinery is needed. `InlineComment` gains `resolved`, applied
in a second pass like `BodyEdit` so a resolve visited before the comment
it names still lands.

Anyone may resolve. `BodyEdit` restricts corrections to the target's
author because rewriting somebody else's words is forgery; a resolution
forges nothing — it is a signed claim carried with its own author's
name, and whether that author is the patch author (a claim) or the
comment's author (a confirmation) is displayed rather than enforced.
Disagreement has a representation already, and both events stay in the
record with their authors. There is therefore no author filter and the
fold is total by construction.

A new revision never auto-resolves or auto-reopens. A claim keeps the
revision it was made against — "resolved at r2" and "resolved at r4" are
different claims — and a claim older than the current tip is surfaced as
a hint beside the thread, never as a state change: the author would see
their claim revoked by nobody, and the reviewer would see threads reopen
that nobody reopened.

`patch diff --answers <comment>` shows the change that answered one
piece of feedback: the interdiff from the comment's revision to its
resolution's, scoped to the comment's file. It reuses `interdiff`
wholesale, so it inherits the rebase-awareness and reports a conflicting
replay instead of rendering a misleading diff. Reads only — no event is
appended and no ref moves.

Where the design doc is wrong: it expects inline comments to have no
identity, but `commit_id` and `resolve_comment` already landed in
b8a55806, so this builds on them rather than inventing a second scheme.
It also expects comments carrying no revision from before reviews were
revision-scoped; no such data can exist, because
`PatchInlineComment::revision` is a required `u32` and `walk_events`
propagates a parse failure, so such an event would have made its whole
patch unreadable. The guard covers both sides of the pair regardless,
but only the resolution side is reachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Patch: 28fc67517775d880cbe3dbe1a9143423e0237e56
src/cache.rs
Old New
@@ -40,7 +40,13 @@ fn sanitize_ref_name(ref_name: &str) -> String {
40 /// `deleted: false`, which is the deleted text leaking back out through the 40 /// `deleted: false`, which is the deleted text leaking back out through the
41 /// cache. That is the whole point of versioning this separately from the 41 /// cache. That is the whole point of versioning this separately from the
42 /// shape, so bump rather than trust the defaults. 42 /// shape, so bump rather than trust the defaults.
43 const CACHE_FORMAT_VERSION: u32 = 7; 43 /// v8: inline comments gained `resolved`. It is `#[serde(default)]`, so a v7
44 /// entry deserializes perfectly — and wrongly: every comment on a cached patch
45 /// would read as unresolved no matter how many resolutions were folded into
46 /// that ref, which is exactly the "has unanswered feedback" signal the feature
47 /// exists to provide, served backwards. Same reasoning as v7; bump rather than
48 /// trust the default.
49 const CACHE_FORMAT_VERSION: u32 = 8;
44 50
45 /// Cache entry stored on disk: the tip OID at cache time + serialized state. 51 /// Cache entry stored on disk: the tip OID at cache time + serialized state.
46 #[derive(serde::Serialize, serde::Deserialize)] 52 #[derive(serde::Serialize, serde::Deserialize)]
src/cli.rs
Old New
@@ -368,6 +368,8 @@ impl PatchCmd {
368 | PatchCmd::Revise { json, .. } 368 | PatchCmd::Revise { json, .. }
369 | PatchCmd::EditComment { json, .. } 369 | PatchCmd::EditComment { json, .. }
370 | PatchCmd::DeleteComment { json, .. } 370 | PatchCmd::DeleteComment { json, .. }
371 | PatchCmd::Resolve { json, .. }
372 | PatchCmd::Unresolve { json, .. }
371 | PatchCmd::EditRevision { json, .. } 373 | PatchCmd::EditRevision { json, .. }
372 | PatchCmd::Log { json, .. } 374 | PatchCmd::Log { json, .. }
373 | PatchCmd::Label { json, .. } 375 | PatchCmd::Label { json, .. }
@@ -756,6 +758,13 @@ pub enum PatchCmd {
756 /// Show interdiff between two revisions (N M or just N for N..latest) 758 /// Show interdiff between two revisions (N M or just N for N..latest)
757 #[arg(long, num_args = 1..=2)] 759 #[arg(long, num_args = 1..=2)]
758 between: Option<Vec<u32>>, 760 between: Option<Vec<u32>>,
761 /// Show the change that answered an inline comment, by comment ID
762 ///
763 /// The interdiff from the revision the comment was written on to the
764 /// revision its resolution was claimed against, scoped to the
765 /// comment's file.
766 #[arg(long, conflicts_with_all = ["revision", "between"])]
767 answers: Option<String>,
759 /// Prefix each line with the `path:line` anchor `patch comment --at` takes 768 /// Prefix each line with the `path:line` anchor `patch comment --at` takes
760 #[arg(long)] 769 #[arg(long)]
761 line_numbers: bool, 770 line_numbers: bool,
@@ -855,6 +864,34 @@ pub enum PatchCmd {
855 #[arg(long)] 864 #[arg(long)]
856 json: bool, 865 json: bool,
857 }, 866 },
867 /// Mark an inline comment answered
868 ///
869 /// Records a claim, attributed to you, against a revision — by default the
870 /// patch's latest. Anyone may resolve: a resolution by the patch author is
871 /// a claim and one by the comment's author a confirmation, and `patch show`
872 /// says which rather than the tool refusing one of them. Disagree with
873 /// `patch unresolve`; both events stay in the record.
874 Resolve {
875 /// Patch ID (prefix match)
876 id: String,
877 /// Inline comment ID (prefix match), as printed by `patch show`
878 comment: String,
879 /// Revision the fix landed in (defaults to the patch's latest)
880 #[arg(long)]
881 revision: Option<u32>,
882 #[arg(long)]
883 json: bool,
884 },
885 /// Withdraw a resolution: the feedback was not answered after all
886 #[command(alias = "reopen-comment")]
887 Unresolve {
888 /// Patch ID (prefix match)
889 id: String,
890 /// Inline comment ID (prefix match), as printed by `patch show`
891 comment: String,
892 #[arg(long)]
893 json: bool,
894 },
858 /// Delete a comment you wrote, leaving a tombstone in its place 895 /// Delete a comment you wrote, leaving a tombstone in its place
859 /// 896 ///
860 /// The comment keeps its position, author and timestamp so replies to it 897 /// The comment keeps its position, author and timestamp so replies to it
@@ -1014,6 +1051,8 @@ impl Commands {
1014 | PatchCmd::Revise { .. } 1051 | PatchCmd::Revise { .. }
1015 | PatchCmd::EditComment { .. } 1052 | PatchCmd::EditComment { .. }
1016 | PatchCmd::DeleteComment { .. } 1053 | PatchCmd::DeleteComment { .. }
1054 | PatchCmd::Resolve { .. }
1055 | PatchCmd::Unresolve { .. }
1017 | PatchCmd::EditRevision { .. } 1056 | PatchCmd::EditRevision { .. }
1018 | PatchCmd::Label { .. } 1057 | PatchCmd::Label { .. }
1019 | PatchCmd::Unlabel { .. } 1058 | PatchCmd::Unlabel { .. }
src/dag.rs
Old New
@@ -360,6 +360,13 @@ fn commit_message(action: &Action) -> String {
360 Action::PatchInlineComment { ref file, line, .. } => { 360 Action::PatchInlineComment { ref file, line, .. } => {
361 format!("patch: inline comment on {}:{}", file, line) 361 format!("patch: inline comment on {}:{}", file, line)
362 } 362 }
363 Action::PatchCommentResolve { ref comment, revision } => match revision {
364 Some(n) => format!("patch: resolve comment {:.8} at r{}", comment, n),
365 None => format!("patch: resolve comment {:.8}", comment),
366 },
367 Action::PatchCommentReopen { ref comment } => {
368 format!("patch: reopen comment {:.8}", comment)
369 }
363 Action::PatchClose { .. } => "patch: close".to_string(), 370 Action::PatchClose { .. } => "patch: close".to_string(),
364 Action::PatchMerge { .. } => "patch: merge".to_string(), 371 Action::PatchMerge { .. } => "patch: merge".to_string(),
365 Action::PatchReopen => "patch: reopen".to_string(), 372 Action::PatchReopen => "patch: reopen".to_string(),
src/event.rs
Old New
@@ -223,6 +223,42 @@ pub enum Action {
223 #[serde(rename = "comment.delete")] 223 #[serde(rename = "comment.delete")]
224 CommentDelete { target: String }, 224 CommentDelete { target: String },
225 225
226 /// Claim that an inline comment has been answered.
227 ///
228 /// `comment` is the hex OID of the `PatchInlineComment` event, the same
229 /// identity `BodyEdit` names and `patch show` prints. `revision` is the
230 /// revision the claim is made *against*, defaulting at write time to the
231 /// patch's latest — "resolved at r2" and "resolved at r4" are different
232 /// claims, and the difference is what `patch diff --answers` diffs.
233 ///
234 /// Unlike `BodyEdit`, this is honoured **whoever writes it**. An edit
235 /// restricted to the target's author because rewriting somebody else's
236 /// words is forgery; a resolution forges nothing — it is a signed claim
237 /// attributed to its own author, and whether that author is the patch
238 /// author (a claim) or the comment's author (a confirmation) is displayed
239 /// rather than enforced. Disagreement has a representation already:
240 /// `PatchCommentReopen`.
241 ///
242 /// Thread comments are deliberately out of range. They carry no revision
243 /// and no anchor; they are conversation, not an actionable item.
244 #[serde(rename = "patch.comment_resolve")]
245 PatchCommentResolve {
246 comment: String,
247 /// Absent only on an event written without one; readers treat it as
248 /// "not recorded", which leaves `--answers` no from-side to diff.
249 #[serde(default)]
250 revision: Option<u32>,
251 },
252
253 /// Withdraw a resolution: the feedback was not answered after all.
254 ///
255 /// The counterpart to `PatchCommentResolve` and resolved against it by the
256 /// same `(clock, oid)` rule, so "the author said this was fixed and the
257 /// reviewer disagreed" stays in the record as two attributed events rather
258 /// than collapsing into a lost state transition.
259 #[serde(rename = "patch.comment_reopen")]
260 PatchCommentReopen { comment: String },
261
226 #[serde(rename = "collab.merge")] 262 #[serde(rename = "collab.merge")]
227 Merge, 263 Merge,
228 } 264 }
src/lib.rs
Old New
@@ -810,8 +810,27 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
810 p.inline_comments.iter().collect() 810 p.inline_comments.iter().collect()
811 }; 811 };
812 if !inline_comments.is_empty() { 812 if !inline_comments.is_empty() {
813 println!("\n--- Inline Comments ---"); 813 // Unresolved first. A reviewer coming back for round four
814 for c in &inline_comments { 814 // wants the threads that still matter at the top, not
815 // interleaved with the ones already answered — that
816 // re-reading is the whole tax this feature removes.
817 // Within each group the original order is kept, so a
818 // thread stays where its author left it.
819 let (open, answered): (Vec<&state::InlineComment>, Vec<&state::InlineComment>) =
820 inline_comments
821 .iter()
822 .copied()
823 .partition(|c| c.resolved.is_none());
824 let latest_revision = p.revisions.last().map(|r| r.number);
825
826 // Header only when there is something under it: with every
827 // thread answered, an empty "Inline Comments" heading reads
828 // as "the comments are gone" rather than "they are all
829 // resolved, and here they are".
830 if !open.is_empty() {
831 println!("\n--- Inline Comments ---");
832 }
833 for c in &open {
815 let rev_label = c.revision.map(|n| format!(" r{}", n)).unwrap_or_default(); 834 let rev_label = c.revision.map(|n| format!(" r{}", n)).unwrap_or_default();
816 // Only the suggestions are labelled. Marking the others 835 // Only the suggestions are labelled. Marking the others
817 // "blocking" would relabel every comment ever written 836 // "blocking" would relabel every comment ever written
@@ -831,6 +850,42 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
831 body_or_tombstone(&c.body, c.deleted) 850 body_or_tombstone(&c.body, c.deleted)
832 ); 851 );
833 } 852 }
853 if !answered.is_empty() {
854 println!("\n--- Resolved ({}) ---", answered.len());
855 for c in &answered {
856 let r = c.resolved.as_ref().expect("partitioned on resolved");
857 let at = r
858 .revision
859 .map(|n| format!(" at r{}", n))
860 .unwrap_or_default();
861 // A claim made three revisions ago is a weaker
862 // claim than one made against the current tip, and
863 // saying so is the honest alternative to silently
864 // reopening the thread. A hint beside it, never a
865 // state change: the author would see their claim
866 // revoked by nobody, and the reviewer would see
867 // threads reopen that nobody reopened.
868 let stale = match (r.revision, latest_revision) {
869 (Some(res), Some(head)) if res < head => {
870 format!(" — {} revision{} landed since", head - res, {
871 if head - res == 1 { "" } else { "s" }
872 })
873 }
874 _ => String::new(),
875 };
876 println!(
877 "\n{} on {}:{} [{:.8}]: {}\n resolved by {}{}{}",
878 c.author.name,
879 c.file,
880 c.line,
881 c.commit_id,
882 truncate_one_line(body_or_tombstone(&c.body, c.deleted)),
883 r.by.name,
884 at,
885 stale,
886 );
887 }
888 }
834 } 889 }
835 // Thread comments always shown 890 // Thread comments always shown
836 if !p.comments.is_empty() { 891 if !p.comments.is_empty() {
@@ -852,6 +907,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
852 id, 907 id,
853 revision, 908 revision,
854 between, 909 between,
910 answers,
855 line_numbers, 911 line_numbers,
856 stat, 912 stat,
857 paths, 913 paths,
@@ -878,7 +934,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
878 stat, 934 stat,
879 paths, 935 paths,
880 }; 936 };
881 let diff = patch::diff(repo, &id, revision, between_pair, &opts)?; 937 let diff = patch::diff(repo, &id, revision, between_pair, answers.as_deref(), &opts)?;
882 if diff.is_empty() { 938 if diff.is_empty() {
883 // An empty diff means two different things, and saying 939 // An empty diff means two different things, and saying
884 // "commits may be identical" for both is how a `--path` 940 // "commits may be identical" for both is how a `--path`
@@ -1029,6 +1085,45 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
1029 || "Comment deleted.".to_string(), 1085 || "Comment deleted.".to_string(),
1030 ) 1086 )
1031 } 1087 }
1088 PatchCmd::Resolve {
1089 id,
1090 comment,
1091 revision,
1092 json,
1093 } => {
1094 let (r, at) = patch::resolve(repo, &id, &comment, revision)?;
1095 report(
1096 json,
1097 || {
1098 serde_json::json!({
1099 "action": "patch.comment_resolve",
1100 "patch": r.id,
1101 "comment": r.target,
1102 "revision": at,
1103 "event": r.event.to_string(),
1104 })
1105 },
1106 || match at {
1107 Some(n) => format!("Comment marked resolved at r{}.", n),
1108 None => "Comment marked resolved.".to_string(),
1109 },
1110 )
1111 }
1112 PatchCmd::Unresolve { id, comment, json } => {
1113 let r = patch::unresolve(repo, &id, &comment)?;
1114 report(
1115 json,
1116 || {
1117 serde_json::json!({
1118 "action": "patch.comment_reopen",
1119 "patch": r.id,
1120 "comment": r.target,
1121 "event": r.event.to_string(),
1122 })
1123 },
1124 || "Comment reopened.".to_string(),
1125 )
1126 }
1032 PatchCmd::EditRevision { 1127 PatchCmd::EditRevision {
1033 id, 1128 id,
1034 revision, 1129 revision,
@@ -1572,6 +1667,16 @@ fn edit_marker(edited: bool) -> &'static str {
1572 } 1667 }
1573 } 1668 }
1574 1669
1670 /// Collapse a comment body to a single line, for a thread that is already
1671 /// answered and is being listed rather than read.
1672 ///
1673 /// First line only: a resolved thread is shown so the reviewer can confirm
1674 /// *which* thread it was, not to be re-read. The full text is still one
1675 /// `patch show --json` away, and the comment's id is printed beside it.
1676 fn truncate_one_line(body: &str) -> String {
1677 truncate_summary(body.lines().next().unwrap_or("").trim(), 60)
1678 }
1679
1575 pub(crate) fn truncate_summary(s: &str, max_chars: usize) -> String { 1680 pub(crate) fn truncate_summary(s: &str, max_chars: usize) -> String {
1576 let mut out = String::new(); 1681 let mut out = String::new();
1577 for (count, c) in s.chars().enumerate() { 1682 for (count, c) in s.chars().enumerate() {
src/log.rs
Old New
@@ -127,6 +127,8 @@ fn action_type_name(action: &Action) -> String {
127 Action::PatchReview { .. } => "PatchReview".to_string(), 127 Action::PatchReview { .. } => "PatchReview".to_string(),
128 Action::PatchComment { .. } => "PatchComment".to_string(), 128 Action::PatchComment { .. } => "PatchComment".to_string(),
129 Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(), 129 Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(),
130 Action::PatchCommentResolve { .. } => "PatchCommentResolve".to_string(),
131 Action::PatchCommentReopen { .. } => "PatchCommentReopen".to_string(),
130 Action::PatchClose { .. } => "PatchClose".to_string(), 132 Action::PatchClose { .. } => "PatchClose".to_string(),
131 Action::PatchMerge { .. } => "PatchMerge".to_string(), 133 Action::PatchMerge { .. } => "PatchMerge".to_string(),
132 Action::PatchReopen => "PatchReopen".to_string(), 134 Action::PatchReopen => "PatchReopen".to_string(),
@@ -174,6 +176,14 @@ fn action_summary(action: &Action) -> String {
174 Action::PatchReview { verdict, .. } => format!("review: {}", verdict), 176 Action::PatchReview { verdict, .. } => format!("review: {}", verdict),
175 Action::PatchComment { body } => truncate(body, 60), 177 Action::PatchComment { body } => truncate(body, 60),
176 Action::PatchInlineComment { file, line, .. } => format!("comment on {}:{}", file, line), 178 Action::PatchInlineComment { file, line, .. } => format!("comment on {}:{}", file, line),
179 // Named the same way `BodyEdit` is: the log says which comment, so
180 // "who claimed this was answered, and against what" is answerable
181 // from the log plus the comment event itself.
182 Action::PatchCommentResolve { comment, revision } => match revision {
183 Some(n) => format!("resolve comment {:.8} at r{}", comment, n),
184 None => format!("resolve comment {:.8}", comment),
185 },
186 Action::PatchCommentReopen { comment } => format!("reopen comment {:.8}", comment),
177 Action::PatchClose { reason } => match reason { 187 Action::PatchClose { reason } => match reason {
178 Some(r) => format!("close: {}", r), 188 Some(r) => format!("close: {}", r),
179 None => "close".to_string(), 189 None => "close".to_string(),
src/patch.rs
Old New
@@ -433,6 +433,98 @@ pub fn delete_comment(
433 }) 433 })
434 } 434 }
435 435
436 /// Refuse to resolve something that is not an inline comment.
437 ///
438 /// Thread comments and reviews share the inline comments' ID namespace, so a
439 /// prefix can land on one by accident. A thread comment carries no revision
440 /// and no anchor — it is conversation, not an actionable item — and a review
441 /// already has a lifecycle of its own: one current vote per author per
442 /// revision, superseded by re-casting. Neither has an "answered" state to
443 /// track, so the CLI says which it found rather than writing an event the
444 /// fold would silently drop.
445 fn require_resolvable(target: &state::BodyTarget) -> Result<(), crate::error::Error> {
446 if target.kind != state::BodyKind::InlineComment {
447 return Err(Error::Cmd(format!(
448 "a {} cannot be resolved: only inline comments carry feedback \
449 anchored to a revision. A review changes by being re-cast, and a \
450 thread comment is conversation rather than an actionable item.",
451 target.kind.label()
452 )));
453 }
454 Ok(())
455 }
456
457 /// Claim that an inline comment has been answered.
458 ///
459 /// Deliberately not restricted to any particular author. See
460 /// `state::ResolutionOverrides` for why: unlike an edit, a resolution rewrites
461 /// nobody's words, and enforcing a rule here that the fold does not enforce
462 /// would be a rule in name only — anyone holding the DAG can append to it.
463 pub fn resolve(
464 repo: &Repository,
465 id_prefix: &str,
466 comment_prefix: &str,
467 revision: Option<u32>,
468 ) -> Result<(dag::Corrected, Option<u32>), crate::error::Error> {
469 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
470 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
471 let target = patch.resolve_comment(comment_prefix)?;
472 require_resolvable(&target)?;
473
474 // Validate an explicit revision before writing: an event naming a
475 // revision that does not exist would leave `--answers` nothing to diff
476 // and no way to tell that from a rebase gone wrong.
477 if let Some(n) = revision {
478 find_revision(&patch, n)?;
479 }
480 // The default is applied once, here, and then carried in the event. The
481 // fold must not fill it in later: "resolved at r2" and "resolved at r4"
482 // are different claims, so the revision has to be pinned at write time.
483 let revision = revision.or_else(|| patch.revisions.last().map(|r| r.number));
484
485 let event = dag::append_action(
486 repo,
487 &ref_name,
488 Action::PatchCommentResolve {
489 comment: target.oid.clone(),
490 revision,
491 },
492 )?;
493 Ok((
494 dag::Corrected {
495 id,
496 target: target.oid,
497 event,
498 },
499 revision,
500 ))
501 }
502
503 /// Withdraw a resolution: the feedback was not answered after all.
504 pub fn unresolve(
505 repo: &Repository,
506 id_prefix: &str,
507 comment_prefix: &str,
508 ) -> Result<dag::Corrected, crate::error::Error> {
509 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
510 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
511 let target = patch.resolve_comment(comment_prefix)?;
512 require_resolvable(&target)?;
513
514 let event = dag::append_action(
515 repo,
516 &ref_name,
517 Action::PatchCommentReopen {
518 comment: target.oid.clone(),
519 },
520 )?;
521 Ok(dag::Corrected {
522 id,
523 target: target.oid,
524 event,
525 })
526 }
527
436 /// Correct a revision's description. 528 /// Correct a revision's description.
437 /// 529 ///
438 /// The revision's commit and tree are not touched — those are the content and 530 /// The revision's commit and tree are not touched — those are the content and
@@ -934,12 +1026,15 @@ pub fn diff(
934 id_prefix: &str, 1026 id_prefix: &str,
935 revision: Option<u32>, 1027 revision: Option<u32>,
936 between: Option<(u32, Option<u32>)>, 1028 between: Option<(u32, Option<u32>)>,
1029 answers: Option<&str>,
937 opts: &DiffOpts, 1030 opts: &DiffOpts,
938 ) -> Result<String, Error> { 1031 ) -> Result<String, Error> {
939 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 1032 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
940 let p = PatchState::from_ref(repo, &ref_name, &id)?; 1033 let p = PatchState::from_ref(repo, &ref_name, &id)?;
941 1034
942 if let Some((from, to)) = between { 1035 if let Some(comment) = answers {
1036 answers_diff(repo, &p, comment, opts)
1037 } else if let Some((from, to)) = between {
943 let to_rev = to.unwrap_or_else(|| p.revisions.last().map(|r| r.number).unwrap_or(1)); 1038 let to_rev = to.unwrap_or_else(|| p.revisions.last().map(|r| r.number).unwrap_or(1));
944 interdiff(repo, &p, from, to_rev, opts) 1039 interdiff(repo, &p, from, to_rev, opts)
945 } else if let Some(rev) = revision { 1040 } else if let Some(rev) = revision {
@@ -949,6 +1044,88 @@ pub fn diff(
949 } 1044 }
950 } 1045 }
951 1046
1047 /// Show the change that answered one piece of feedback.
1048 ///
1049 /// This is the thing a pull request cannot offer. Under a force-push the
1050 /// correspondence between a review comment and the change answering it is
1051 /// destroyed, and authors reconstruct it by hand with `fix(pr): address
1052 /// review` commits. Here the correspondence is recorded — the comment names a
1053 /// revision, its resolution names another — so the answer is *shown* rather
1054 /// than asserted: it is exactly the interdiff between those two revisions,
1055 /// narrowed to the file the comment was anchored to.
1056 ///
1057 /// The narrowing is by file, not by line. A comment on a line is rarely
1058 /// answered on that line alone — a guard added above it, a helper extracted
1059 /// below — and a line-scoped diff would hide the very change it was meant to
1060 /// show. The file is the smallest scope that cannot lie by omission.
1061 ///
1062 /// Reuses [`interdiff`] wholesale rather than re-deriving trees, so it
1063 /// inherits the rebase-awareness: with the two revisions on different bases
1064 /// the older is replayed onto the newer's base and upstream churn cancels,
1065 /// and a conflicting replay reports the conflict instead of rendering a
1066 /// misleading diff.
1067 ///
1068 /// Reads only. Nothing here appends an event or moves a ref.
1069 fn answers_diff(
1070 repo: &Repository,
1071 patch: &PatchState,
1072 comment_prefix: &str,
1073 opts: &DiffOpts,
1074 ) -> Result<String, Error> {
1075 let target = patch.resolve_comment(comment_prefix)?;
1076 require_resolvable(&target)?;
1077 let comment = patch
1078 .inline_comments
1079 .iter()
1080 .find(|c| c.commit_id.to_string() == target.oid)
1081 .ok_or_else(|| Error::Cmd(format!("no inline comment matching '{}'", comment_prefix)))?;
1082
1083 let short = &patch.id[..8.min(patch.id.len())];
1084 let Some(resolution) = comment.resolved.as_ref() else {
1085 return Err(Error::Cmd(format!(
1086 "comment {:.8} is not resolved, so there is no answer to show. \
1087 Mark it answered with `git-collab patch resolve {} {:.8}`.",
1088 comment.commit_id, short, comment.commit_id
1089 )));
1090 };
1091
1092 // Where either side carries no revision the from/to pair is incomplete,
1093 // and there is no honest diff to render. Say so rather than guess a
1094 // revision: a guessed from-side would silently show the wrong change.
1095 let (Some(from), Some(to)) = (comment.revision, resolution.revision) else {
1096 let missing = if comment.revision.is_none() {
1097 "the comment records no revision"
1098 } else {
1099 "its resolution records no revision"
1100 };
1101 return Err(Error::Cmd(format!(
1102 "cannot show what answered comment {:.8}: {}, so there is no \
1103 revision pair to diff. This is data written before reviews were \
1104 revision-scoped; `git-collab patch diff {} --between <from> <to>` \
1105 takes the two revisions explicitly.",
1106 comment.commit_id, missing, short
1107 )));
1108 };
1109
1110 if from == to {
1111 return Err(Error::Cmd(format!(
1112 "comment {:.8} was written on r{} and resolved against r{}: no \
1113 revision landed between them, so there is no change to show. \
1114 Either the fix went in without `patch revise`, or the comment was \
1115 resolved without one being needed.",
1116 comment.commit_id, from, to
1117 )));
1118 }
1119
1120 // Scope to the comment's file. Any `--path` the user also passed is
1121 // replaced rather than intersected: `--answers` names the scope itself,
1122 // and silently ANDing two scopings would make an empty result mean two
1123 // different things.
1124 let mut scoped = opts.clone();
1125 scoped.paths = vec![comment.file.clone()];
1126 interdiff(repo, patch, from, to, &scoped)
1127 }
1128
952 /// Resolve the base tree for diffing: find the merge-base between the base branch 1129 /// Resolve the base tree for diffing: find the merge-base between the base branch
953 /// and the given head OID, falling back to the base branch tip if no merge-base exists. 1130 /// and the given head OID, falling back to the base branch tip if no merge-base exists.
954 /// 1131 ///
src/server/http/repo/patches.rs
Old New
@@ -102,6 +102,12 @@ pub struct InlineCommentView {
102 pub edited: bool, 102 pub edited: bool,
103 pub deleted: bool, 103 pub deleted: bool,
104 pub non_blocking: bool, 104 pub non_blocking: bool,
105 /// Who claimed this was answered, if anyone — rendered as the same
106 /// grouping `patch show` uses, so the two surfaces do not disagree about
107 /// which threads still need reading.
108 pub resolved_by: Option<String>,
109 /// The revision the claim was made against.
110 pub resolved_at: Option<u32>,
105 } 111 }
106 112
107 #[derive(Debug)] 113 #[derive(Debug)]
@@ -277,6 +283,8 @@ pub async fn patch_detail(
277 non_blocking: ic.non_blocking, 283 non_blocking: ic.non_blocking,
278 edited: ic.edited, 284 edited: ic.edited,
279 deleted: ic.deleted, 285 deleted: ic.deleted,
286 resolved_by: ic.resolved.as_ref().map(|r| r.by.name.clone()),
287 resolved_at: ic.resolved.as_ref().and_then(|r| r.revision),
280 }) 288 })
281 .collect(), 289 .collect(),
282 comments: ps.comments.into_iter().map(CommentView::from_state).collect(), 290 comments: ps.comments.into_iter().map(CommentView::from_state).collect(),
src/server/http/templates/patch_detail.html
Old New
@@ -74,6 +74,7 @@
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 {% if ic.non_blocking %}&nbsp; <span style="color: #666; font-size: 0.85em;">[non-blocking]</span>{% endif %}
77 {% if let Some(by) = ic.resolved_by %}&nbsp; <span style="color: #2a7; font-size: 0.85em;">resolved by {{ by }}{% if let Some(at) = ic.resolved_at %} at r{{ at }}{% endif %}</span>{% endif %}
77 </p> 78 </p>
78 {% if ic.deleted %} 79 {% if ic.deleted %}
79 <p style="margin: 0; color: #666; font-style: italic;">[deleted]</p> 80 <p style="margin: 0; color: #666; font-style: italic;">[deleted]</p>
src/state.rs
Old New
@@ -150,6 +150,53 @@ impl BodyOverrides {
150 } 150 }
151 } 151 }
152 152
153 /// One `PatchCommentResolve` or `PatchCommentReopen`, pending application.
154 struct ResolutionOverride {
155 /// `(clock, oid_hex)` of the requesting event — the same content-derived
156 /// total order `BodyOverride` and the status fold use.
157 key: (u64, String),
158 /// `Some` for a resolve, `None` for a reopen.
159 resolution: Option<Resolution>,
160 }
161
162 /// Accumulator for resolution events, applied after the walk.
163 ///
164 /// Deliberately *not* author-filtered, which is the one way this differs from
165 /// [`BodyOverrides`]. An edit is restricted to the target's author because
166 /// rewriting somebody else's words is forgery. A resolution rewrites nothing:
167 /// it is a claim carried with its own author's name, and a claim by the wrong
168 /// person is answered by reopening it, not by the fold pretending it was never
169 /// made. Enforcing an author rule here would also mean deciding *which* author
170 /// — the comment's or the patch's — and both are wrong for the common case of
171 /// a third collaborator confirming a fix.
172 ///
173 /// There is therefore no `owners` map and no `authorized` filter, so the fold
174 /// stays total by construction: every event a hostile clone can append lands
175 /// in exactly one of these slots and produces the same state everywhere.
176 #[derive(Default)]
177 struct ResolutionOverrides {
178 by_target: HashMap<String, ResolutionOverride>,
179 }
180
181 impl ResolutionOverrides {
182 /// Record a resolve or a reopen, keeping the one that wins on
183 /// `(clock, oid)`. `>=` matches the status fold and `BodyOverrides`:
184 /// later clock wins, and on a tie the lexicographically larger OID does.
185 fn record(&mut self, target: String, key: (u64, String), resolution: Option<Resolution>) {
186 let candidate = ResolutionOverride { key, resolution };
187 match self.by_target.get(&target) {
188 Some(existing) if candidate.key < existing.key => {}
189 _ => {
190 self.by_target.insert(target, candidate);
191 }
192 }
193 }
194
195 fn winners(&self) -> impl Iterator<Item = (&String, &Option<Resolution>)> {
196 self.by_target.iter().map(|(t, ov)| (t, &ov.resolution))
197 }
198 }
199
153 /// Apply a correction to a comment, in place, so it keeps its position among 200 /// Apply a correction to a comment, in place, so it keeps its position among
154 /// its siblings and everything about it except the words. 201 /// its siblings and everything about it except the words.
155 fn apply_to_comment(body: &mut String, edited: &mut bool, deleted: &mut bool, ov: &BodyOverride) { 202 fn apply_to_comment(body: &mut String, edited: &mut bool, deleted: &mut bool, ov: &BodyOverride) {
@@ -392,6 +439,41 @@ pub struct InlineComment {
392 /// be hiding a marked comment. 439 /// be hiding a marked comment.
393 #[serde(default)] 440 #[serde(default)]
394 pub non_blocking: bool, 441 pub non_blocking: bool,
442 /// Whether anyone has claimed this feedback was answered, and if so who,
443 /// when, and against which revision. `None` means unresolved.
444 ///
445 /// Always serialized, never skipped, for the same reason as
446 /// `non_blocking`: a scripted caller must be able to tell "nobody has
447 /// resolved this" from "this build of git-collab does not report
448 /// resolution", and only a key that is always present does that.
449 #[serde(default)]
450 pub resolved: Option<Resolution>,
451 }
452
453 /// A standing claim that an inline comment was answered.
454 ///
455 /// Derived, not stored: the winning `PatchCommentResolve` for a comment,
456 /// picked by `(clock, oid)`. A later `PatchCommentReopen` that wins on the
457 /// same key leaves the comment `resolved: None` instead.
458 #[derive(Debug, Clone, Serialize, Deserialize)]
459 pub struct Resolution {
460 /// Who made the claim. The interesting distinction — the patch author
461 /// claiming versus the comment's author confirming — is read off this
462 /// rather than enforced when the event is written.
463 pub by: Author,
464 /// The revision the claim was made against. `None` for an event that
465 /// recorded none, which leaves `patch diff --answers` no to-side.
466 #[serde(default)]
467 pub revision: Option<u32>,
468 pub timestamp: String,
469 /// OID of the `PatchCommentResolve` event, so the claim itself can be
470 /// named — the same identity scheme as everything else here.
471 #[serde(
472 default = "Oid::zero",
473 serialize_with = "serialize_oid",
474 deserialize_with = "deserialize_oid"
475 )]
476 pub commit_id: Oid,
395 } 477 }
396 478
397 #[derive(Debug, Clone, Serialize, Deserialize)] 479 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -1062,6 +1144,10 @@ impl PatchState {
1062 // Corrections to comment, review and revision bodies, applied after 1144 // Corrections to comment, review and revision bodies, applied after
1063 // the walk. See `BodyOverrides`. 1145 // the walk. See `BodyOverrides`.
1064 let mut overrides = BodyOverrides::default(); 1146 let mut overrides = BodyOverrides::default();
1147 // Resolve/reopen claims, applied after the walk for the same reason:
1148 // a resolve can be visited before the comment it names. See
1149 // `ResolutionOverrides`.
1150 let mut resolutions = ResolutionOverrides::default();
1065 1151
1066 for (oid, event) in events { 1152 for (oid, event) in events {
1067 let ts = parse_timestamp(&event.timestamp); 1153 let ts = parse_timestamp(&event.timestamp);
@@ -1249,9 +1335,35 @@ impl PatchState {
1249 edited: false, 1335 edited: false,
1250 deleted: false, 1336 deleted: false,
1251 non_blocking, 1337 non_blocking,
1338 resolved: None,
1252 }); 1339 });
1253 } 1340 }
1254 } 1341 }
1342 Action::PatchCommentResolve { comment, revision } => {
1343 if state.is_some() {
1344 // `revision` is carried exactly as the writer recorded
1345 // it. Filling a missing one in here from the fold's
1346 // current view would make the same event mean
1347 // different things as later revisions land — precisely
1348 // the drift resolution exists to stop. The default is
1349 // applied once, at write time, in `patch::resolve`.
1350 resolutions.record(
1351 comment,
1352 (event.clock, oid.to_string()),
1353 Some(Resolution {
1354 by: event.author.clone(),
1355 revision,
1356 timestamp: event.timestamp.clone(),
1357 commit_id: oid,
1358 }),
1359 );
1360 }
1361 }
1362 Action::PatchCommentReopen { comment } => {
1363 if state.is_some() {
1364 resolutions.record(comment, (event.clock, oid.to_string()), None);
1365 }
1366 }
1255 Action::BodyEdit { target, body } => { 1367 Action::BodyEdit { target, body } => {
1256 if state.is_some() { 1368 if state.is_some() {
1257 overrides.record( 1369 overrides.record(
@@ -1374,6 +1486,20 @@ impl PatchState {
1374 } 1486 }
1375 } 1487 }
1376 1488
1489 // Resolution claims. Only inline comments carry them: a thread
1490 // comment has no revision and no anchor, so a resolve naming one
1491 // finds no match here and is dropped, exactly as a forged
1492 // `CommentDelete` naming a review is.
1493 for (target, resolution) in resolutions.winners() {
1494 if let Some(c) = s
1495 .inline_comments
1496 .iter_mut()
1497 .find(|c| c.commit_id.to_string() == *target)
1498 {
1499 c.resolved = resolution.clone();
1500 }
1501 }
1502
1377 s.last_updated = latest.map(|(_, raw)| raw).unwrap_or_default(); 1503 s.last_updated = latest.map(|(_, raw)| raw).unwrap_or_default();
1378 } 1504 }
1379 state.ok_or_else(|| git2::Error::from_str("no PatchCreate event found in DAG").into()) 1505 state.ok_or_else(|| git2::Error::from_str("no PatchCreate event found in DAG").into())
src/timeline.rs
Old New
@@ -113,6 +113,31 @@ pub enum Kind {
113 Deleted { 113 Deleted {
114 target: String, 114 target: String,
115 }, 115 },
116 /// A claim that an inline comment was answered.
117 ///
118 /// Every one is listed, including a claim that later lost the
119 /// `(clock, oid)` race to a reopen. This is the one place the timeline
120 /// deliberately shows more than the standing state: "the author said this
121 /// was fixed and the reviewer disagreed" is the history the patch model
122 /// exists to keep, and showing only the winner would collapse it back into
123 /// the lost state transition a force-push produces.
124 ///
125 /// That is also why there is no `authorized` filter here, unlike `Edited`
126 /// and `Deleted`. Those are hidden when refused because they never took
127 /// effect on anyone's words; a resolution is a signed claim by its own
128 /// author, and the fold honours it whoever wrote it.
129 Resolved {
130 target: String,
131 },
132 /// A withdrawal of a resolution.
133 ///
134 /// Named for the CLI verb (`patch unresolve`) rather than "reopened",
135 /// which this timeline already uses for a patch being reopened. Two
136 /// unrelated things sharing one word in the same column is how a reader
137 /// concludes the patch was reopened when a comment was.
138 CommentUnresolved {
139 target: String,
140 },
116 Closed { 141 Closed {
117 #[serde(skip_serializing_if = "Option::is_none")] 142 #[serde(skip_serializing_if = "Option::is_none")]
118 reason: Option<String>, 143 reason: Option<String>,
@@ -139,6 +164,8 @@ impl Kind {
139 Kind::Unlabel { .. } => "unlabel".to_string(), 164 Kind::Unlabel { .. } => "unlabel".to_string(),
140 Kind::Edited { .. } => "edited".to_string(), 165 Kind::Edited { .. } => "edited".to_string(),
141 Kind::Deleted { .. } => "deleted".to_string(), 166 Kind::Deleted { .. } => "deleted".to_string(),
167 Kind::Resolved { .. } => "resolved".to_string(),
168 Kind::CommentUnresolved { .. } => "unresolved".to_string(),
142 Kind::Closed { .. } => "closed".to_string(), 169 Kind::Closed { .. } => "closed".to_string(),
143 Kind::Merged { .. } => "merged".to_string(), 170 Kind::Merged { .. } => "merged".to_string(),
144 Kind::Reopened => "reopened".to_string(), 171 Kind::Reopened => "reopened".to_string(),
@@ -291,6 +318,20 @@ pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entr
291 None, 318 None,
292 ) 319 )
293 } 320 }
321 Action::PatchCommentResolve { comment, revision } => (
322 Kind::Resolved {
323 target: comment.clone(),
324 },
325 // The revision the claim was made against, so the sequence
326 // reads causally: comment on r1, revision r2, resolved at r2.
327 *revision,
328 ),
329 Action::PatchCommentReopen { comment } => (
330 Kind::CommentUnresolved {
331 target: comment.clone(),
332 },
333 None,
334 ),
294 Action::PatchClose { reason } => ( 335 Action::PatchClose { reason } => (
295 Kind::Closed { 336 Kind::Closed {
296 reason: reason.clone(), 337 reason: reason.clone(),
@@ -428,9 +469,10 @@ pub fn to_writer(entries: &[Entry], writer: &mut dyn std::io::Write) -> Result<(
428 } 469 }
429 Kind::Label { label } => parts.push(format!("+{}", label)), 470 Kind::Label { label } => parts.push(format!("+{}", label)),
430 Kind::Unlabel { label } => parts.push(format!("-{}", label)), 471 Kind::Unlabel { label } => parts.push(format!("-{}", label)),
431 Kind::Edited { target } | Kind::Deleted { target } => { 472 Kind::Edited { target }
432 parts.push(format!("{:.8}", target)) 473 | Kind::Deleted { target }
433 } 474 | Kind::Resolved { target }
475 | Kind::CommentUnresolved { target } => parts.push(format!("{:.8}", target)),
434 Kind::Closed { reason } => parts.extend(reason.clone()), 476 Kind::Closed { reason } => parts.extend(reason.clone()),
435 Kind::Merged { commit } => { 477 Kind::Merged { commit } => {
436 parts.extend(commit.as_deref().map(|c| format!("{:.8}", c))) 478 parts.extend(commit.as_deref().map(|c| format!("{:.8}", c)))
src/tui/events.rs
Old New
@@ -261,7 +261,7 @@ fn generate_patch_diff_for(
261 } 261 }
262 } else { 262 } else {
263 // Diff at specific revision vs base 263 // Diff at specific revision vs base
264 match patch_mod::diff(repo, &patch.id, Some(rev.number), None, &Default::default()) { 264 match patch_mod::diff(repo, &patch.id, Some(rev.number), None, None, &Default::default()) {
265 Ok(d) => d, 265 Ok(d) => d,
266 Err(e) => format!("(error generating diff: {})", e), 266 Err(e) => format!("(error generating diff: {})", e),
267 } 267 }
src/tui/mod.rs
Old New
@@ -1131,6 +1131,7 @@ mod tests {
1131 edited: false, 1131 edited: false,
1132 deleted: false, 1132 deleted: false,
1133 non_blocking: false, 1133 non_blocking: false,
1134 resolved: None,
1134 }], 1135 }],
1135 reviews: vec![crate::state::Review { 1136 reviews: vec![crate::state::Review {
1136 author: make_author(), 1137 author: make_author(),
src/tui/widgets.rs
Old New
@@ -51,6 +51,8 @@ pub(crate) fn action_type_label(action: &Action) -> &str {
51 Action::PatchReview { .. } => "Patch Review", 51 Action::PatchReview { .. } => "Patch Review",
52 Action::PatchComment { .. } => "Patch Comment", 52 Action::PatchComment { .. } => "Patch Comment",
53 Action::PatchInlineComment { .. } => "Inline Comment", 53 Action::PatchInlineComment { .. } => "Inline Comment",
54 Action::PatchCommentResolve { .. } => "Comment Resolved",
55 Action::PatchCommentReopen { .. } => "Comment Reopened",
54 Action::PatchClose { .. } => "Patch Close", 56 Action::PatchClose { .. } => "Patch Close",
55 Action::PatchMerge { .. } => "Patch Merge", 57 Action::PatchMerge { .. } => "Patch Merge",
56 Action::PatchReopen => "Patch Reopen", 58 Action::PatchReopen => "Patch Reopen",
@@ -183,6 +185,15 @@ pub(crate) fn format_event_detail(
183 Action::CommentDelete { target } => { 185 Action::CommentDelete { target } => {
184 detail.push_str(&format!("\nDeleted: {:.8}\n", target)); 186 detail.push_str(&format!("\nDeleted: {:.8}\n", target));
185 } 187 }
188 Action::PatchCommentResolve { comment, revision } => {
189 detail.push_str(&format!("\nResolved: {:.8}\n", comment));
190 if let Some(n) = revision {
191 detail.push_str(&format!("Against: r{}\n", n));
192 }
193 }
194 Action::PatchCommentReopen { comment } => {
195 detail.push_str(&format!("\nReopened: {:.8}\n", comment));
196 }
186 Action::PatchMerge { commit } => { 197 Action::PatchMerge { commit } => {
187 if !commit.is_empty() { 198 if !commit.is_empty() {
188 detail.push_str(&format!("\nMerged as: {}\n", commit)); 199 detail.push_str(&format!("\nMerged as: {}\n", commit));
tests/comment_resolution_test.rs
Old New
@@ -0,0 +1,607 @@
1 //! Tracking whether review feedback was ever answered.
2 //!
3 //! An inline comment is anchored to the revision it was written on, so it
4 //! never drifts — but until now nothing recorded whether anyone had answered
5 //! it. With five threads across four revisions a reviewer re-read all five
6 //! every round to find the two that still mattered.
7 //!
8 //! Resolution is two ordinary events, `patch.comment_resolve` and
9 //! `patch.comment_reopen`, so the existing `(clock, oid)` fold handles
10 //! concurrency and disagreement stays in the record with its attribution.
11 //!
12 //! Four properties are load-bearing and each is tested here:
13 //!
14 //! * **Identity.** A resolution names an inline comment by its event OID,
15 //! the same identity `patch show` prints and `edit-comment` takes.
16 //! * **Convergence.** Two clones resolving and reopening the same comment
17 //! offline settle on the same answer, in either join order.
18 //! * **Stability.** Recording a new revision never auto-resolves or
19 //! auto-reopens; a claim keeps the revision it was made against.
20 //! * **Showing the answer.** `patch diff --answers` renders the interdiff
21 //! from the comment's revision to its resolution's, scoped to the
22 //! comment's file — the thing a force-pushed pull request cannot offer.
23
24 mod common;
25
26 use common::{alice, bob, init_repo, test_signing_key, TestRepo};
27
28 use git2::Repository;
29 use git_collab::dag;
30 use git_collab::event::{Action, Author, Event};
31 use git_collab::state::PatchState;
32 use tempfile::TempDir;
33
34 // ===========================================================================
35 // Helpers
36 // ===========================================================================
37
38 fn append(repo: &Repository, ref_name: &str, author: &Author, action: Action) -> git2::Oid {
39 let sk = test_signing_key();
40 let event = Event {
41 timestamp: common::now(),
42 author: author.clone(),
43 action,
44 clock: 0,
45 };
46 dag::append_event(repo, ref_name, &event, &sk).unwrap()
47 }
48
49 /// A patch carrying one inline comment by Alice on r1. Returns the patch ref,
50 /// its id, and the comment's identity.
51 fn patch_with_inline_comment(repo: &Repository) -> (String, String, git2::Oid) {
52 let (ref_name, id) = common::create_patch(repo, &alice(), "patch under test");
53 let comment_oid = append(
54 repo,
55 &ref_name,
56 &alice(),
57 Action::PatchInlineComment {
58 file: "feature.txt".to_string(),
59 line: 1,
60 body: "this needs a guard".to_string(),
61 revision: 1,
62 non_blocking: false,
63 },
64 );
65 (ref_name, id, comment_oid)
66 }
67
68 fn patch_state(repo: &Repository, ref_name: &str, id: &str) -> PatchState {
69 PatchState::from_ref_uncached(repo, ref_name, id).unwrap()
70 }
71
72 /// A patch over a real file, so an inline comment has something to anchor to.
73 /// Returns the patch id; the file is always `feature.txt`.
74 fn patch_over_a_file(repo: &TestRepo, title: &str) -> String {
75 let branch = format!("feat/{}", title.replace(' ', "-"));
76 repo.git(&["checkout", "-b", &branch]);
77 repo.commit_file("feature.txt", "v1\n", &format!("commit for {}", title));
78 let out = repo.run_ok(&["patch", "create", "-t", title, "-B", &branch]);
79 repo.git(&["checkout", "main"]);
80 out.trim()
81 .strip_prefix("Created patch ")
82 .unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
83 .to_string()
84 }
85
86 /// The identity of the first inline comment, as `--json` reports it.
87 fn first_inline_id(json: &str) -> String {
88 let value: serde_json::Value = serde_json::from_str(json).expect("valid JSON");
89 value["inline_comments"][0]["commit_id"]
90 .as_str()
91 .unwrap_or_else(|| panic!("no commit_id on inline_comments[0] in {}", json))
92 .to_string()
93 }
94
95 fn resolved_of(json: &str) -> serde_json::Value {
96 let value: serde_json::Value = serde_json::from_str(json).expect("valid JSON");
97 value["inline_comments"][0]["resolved"].clone()
98 }
99
100 /// Every path a unified diff touches, sorted and deduplicated.
101 fn changed_files(diff: &str) -> Vec<String> {
102 let mut paths: Vec<String> = diff
103 .lines()
104 .filter_map(|l| l.strip_prefix("diff --git a/"))
105 .map(|rest| rest.split(" b/").next().unwrap_or(rest).to_string())
106 .collect();
107 paths.sort();
108 paths.dedup();
109 paths
110 }
111
112 /// A patch whose author rebased over three upstream commits before answering
113 /// review, with an inline comment on r1 resolved against r2. This is the
114 /// case a pull request cannot show: the branch it was reviewed on is gone.
115 fn rebased_patch_with_resolved_comment() -> (TestRepo, String, String) {
116 let repo = TestRepo::new("Alice", "alice@example.com");
117
118 repo.git(&["checkout", "-b", "feat-rebase"]);
119 repo.commit_file("feature.txt", "v1\n", "feature v1");
120 let out = repo.run_ok(&["patch", "create", "-t", "Rebase noise", "-B", "feat-rebase"]);
121 let id = out
122 .trim()
123 .strip_prefix("Created patch ")
124 .unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
125 .to_string();
126
127 // A reviewer leaves an inline comment on r1.
128 repo.run_ok(&[
129 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "--revision", "1", "-b",
130 "wrong value here",
131 ]);
132
133 // Upstream advances, touching only files the patch does not.
134 repo.git(&["checkout", "main"]);
135 repo.commit_file("up1.txt", "one\n", "upstream 1");
136 repo.commit_file("up2.txt", "two\n", "upstream 2");
137 repo.commit_file("up3.txt", "three\n", "upstream 3");
138
139 // The author rebases, answers the review, and records r2.
140 repo.git(&["checkout", "feat-rebase"]);
141 repo.git(&["rebase", "main"]);
142 repo.commit_file("feature.txt", "v2\n", "address review");
143 repo.run_ok(&["patch", "revise", &id]);
144 repo.git(&["checkout", "main"]);
145
146 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
147 let comment = first_inline_id(&json);
148 (repo, id, comment)
149 }
150
151 // ===========================================================================
152 // Resolving and reopening, end to end
153 // ===========================================================================
154
155 #[test]
156 fn resolving_a_comment_records_who_and_at_which_revision() {
157 let repo = TestRepo::new("Alice", "alice@example.com");
158 let id = patch_over_a_file(&repo, "needs a guard");
159 repo.run_ok(&[
160 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "add a guard",
161 ]);
162 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
163 let comment = first_inline_id(&json);
164
165 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
166
167 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
168 let resolved = resolved_of(&json);
169 assert!(
170 !resolved.is_null(),
171 "a resolved comment must carry its resolution in --json: {}",
172 json
173 );
174 assert_eq!(resolved["by"]["email"], "alice@example.com");
175 assert_eq!(
176 resolved["revision"], 1,
177 "resolution defaults to the patch's latest revision"
178 );
179 assert!(
180 resolved["commit_id"].as_str().is_some_and(|s| s.len() == 40),
181 "the resolving event's full id must be in --json: {}",
182 resolved
183 );
184 }
185
186 /// The mutating commands report in the same machine-readable shape the rest
187 /// of the CLI does, with ids at full length — a truncated id is not something
188 /// a script can feed back in.
189 #[test]
190 fn resolve_and_unresolve_report_full_ids_in_json() {
191 let repo = TestRepo::new("Alice", "alice@example.com");
192 let id = patch_over_a_file(&repo, "scripted");
193 repo.run_ok(&[
194 "patch",
195 "comment",
196 &id,
197 "--file",
198 "feature.txt",
199 "--line",
200 "1",
201 "-b",
202 "look here",
203 ]);
204 let comment = first_inline_id(&repo.run_ok(&["patch", "show", &id, "--json"]));
205
206 let out = repo.run_ok(&["patch", "resolve", &id, &comment[..8], "--json"]);
207 let v: serde_json::Value = serde_json::from_str(&out).expect("valid JSON");
208 assert_eq!(v["action"], "patch.comment_resolve");
209 assert_eq!(
210 v["comment"].as_str().unwrap(),
211 comment,
212 "the comment must come back at full length, not as the prefix given"
213 );
214 assert_eq!(v["revision"], 1);
215 assert_eq!(v["event"].as_str().unwrap().len(), 40);
216 assert_eq!(v["patch"].as_str().unwrap().len(), 40);
217
218 let out = repo.run_ok(&["patch", "unresolve", &id, &comment[..8], "--json"]);
219 let v: serde_json::Value = serde_json::from_str(&out).expect("valid JSON");
220 assert_eq!(v["action"], "patch.comment_reopen");
221 assert_eq!(v["comment"].as_str().unwrap(), comment);
222 assert_eq!(v["event"].as_str().unwrap().len(), 40);
223 }
224
225 #[test]
226 fn an_unresolved_comment_reports_resolved_null_rather_than_omitting_it() {
227 let repo = TestRepo::new("Alice", "alice@example.com");
228 let id = patch_over_a_file(&repo, "nothing resolved");
229 repo.run_ok(&[
230 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "look here",
231 ]);
232
233 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
234 let value: serde_json::Value = serde_json::from_str(&json).unwrap();
235 let comment = &value["inline_comments"][0];
236 assert!(
237 comment.as_object().unwrap().contains_key("resolved"),
238 "a scripted caller must be able to tell 'unresolved' from 'this build \
239 does not report resolution': {}",
240 json
241 );
242 assert!(comment["resolved"].is_null());
243 }
244
245 #[test]
246 fn patch_show_marks_resolved_threads_and_says_who_resolved_them() {
247 let repo = TestRepo::new("Alice", "alice@example.com");
248 let id = patch_over_a_file(&repo, "marked up");
249 repo.run_ok(&[
250 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "add a guard",
251 ]);
252 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
253 let comment = first_inline_id(&json);
254 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
255
256 let out = repo.run_ok(&["patch", "show", &id]);
257 assert!(
258 out.contains("resolved"),
259 "patch show must say a thread was resolved: {}",
260 out
261 );
262 assert!(
263 out.contains("Alice"),
264 "and who resolved it: {}",
265 out
266 );
267 }
268
269 #[test]
270 fn reopening_clears_the_resolution_and_both_events_stay_in_the_record() {
271 let repo = TestRepo::new("Alice", "alice@example.com");
272 let id = patch_over_a_file(&repo, "disputed");
273 repo.run_ok(&[
274 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "not done",
275 ]);
276 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
277 let comment = first_inline_id(&json);
278
279 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
280 repo.run_ok(&["patch", "unresolve", &id, &comment[..8]]);
281
282 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
283 assert!(
284 resolved_of(&json).is_null(),
285 "reopening must clear the resolution: {}",
286 json
287 );
288
289 // Disagreement is history, not a lost state transition: both events are
290 // still on the timeline with their authors.
291 let timeline = repo.run_ok(&["patch", "log", &id, "--timeline"]);
292 assert!(
293 timeline.contains("resolved") && timeline.contains("unresolved"),
294 "both the claim and the disagreement must stay in the record: {}",
295 timeline
296 );
297 }
298
299 /// Anyone may resolve. A resolution is a signed claim by its own author, not
300 /// a rewrite of anyone else's words, so restricting it would add a rule with
301 /// no threat behind it — and the fold must be total for every reader anyway.
302 #[test]
303 fn anyone_may_resolve_not_only_the_comments_author() {
304 let dir = TempDir::new().unwrap();
305 let repo = init_repo(dir.path(), &alice());
306 let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
307
308 // Bob, who did not write the comment, claims it is answered.
309 append(
310 &repo,
311 &ref_name,
312 &bob(),
313 Action::PatchCommentResolve {
314 comment: comment_oid.to_string(),
315 revision: Some(1),
316 },
317 );
318
319 let p = patch_state(&repo, &ref_name, &id);
320 let resolved = p.inline_comments[0]
321 .resolved
322 .as_ref()
323 .expect("a resolution by a third party is honoured");
324 assert_eq!(
325 resolved.by.email,
326 bob().email,
327 "and it is attributed to whoever made it, so a claim can be told from \
328 a confirmation"
329 );
330 }
331
332 // ===========================================================================
333 // Revisions do not move resolution state
334 // ===========================================================================
335
336 #[test]
337 fn a_new_revision_leaves_a_resolution_at_the_revision_it_was_claimed_against() {
338 let repo = TestRepo::new("Alice", "alice@example.com");
339 repo.git(&["checkout", "-b", "feat"]);
340 repo.commit_file("feature.txt", "v1\n", "feature v1");
341 let out = repo.run_ok(&["patch", "create", "-t", "moving target", "-B", "feat"]);
342 let id = out.trim().strip_prefix("Created patch ").unwrap().to_string();
343
344 repo.run_ok(&[
345 "patch",
346 "comment",
347 &id,
348 "--file",
349 "feature.txt",
350 "--line",
351 "1",
352 "-b",
353 "fix this",
354 ]);
355 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
356 let comment = first_inline_id(&json);
357 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
358
359 // Two more revisions land, both touching the very line the comment was on.
360 for v in ["v2", "v3"] {
361 repo.commit_file("feature.txt", &format!("{}\n", v), &format!("rev {}", v));
362 repo.run_ok(&["patch", "revise", &id]);
363 }
364
365 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
366 let resolved = resolved_of(&json);
367 assert!(
368 !resolved.is_null(),
369 "a new revision must never silently revoke the author's claim: {}",
370 json
371 );
372 assert_eq!(
373 resolved["revision"], 1,
374 "the claim keeps the revision it was made against — resolved at r1 is \
375 a different claim from resolved at r3"
376 );
377
378 // But a claim made three revisions ago is worth flagging, as a hint
379 // beside the thread rather than a state change.
380 let out = repo.run_ok(&["patch", "show", &id]);
381 assert!(
382 out.contains("r1") && out.to_lowercase().contains("since"),
383 "a resolution older than the current revision must be surfaced as a \
384 hint: {}",
385 out
386 );
387 }
388
389 // ===========================================================================
390 // Convergence
391 // ===========================================================================
392
393 /// Two clones, apart, disagree about one comment: one resolves it, the other
394 /// reopens it. Once they see each other's events every clone must land on the
395 /// same answer, decided by `(clock, oid)` over the event set — content-derived
396 /// and so client-independent, like every other conflict here.
397 #[test]
398 fn a_concurrent_resolve_and_reopen_converge() {
399 let dir = TempDir::new().unwrap();
400 let repo = init_repo(dir.path(), &alice());
401 let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
402 let fork_point = repo.refname_to_id(&ref_name).unwrap();
403
404 // The author claims it is answered.
405 let resolve_oid = append(
406 &repo,
407 &ref_name,
408 &alice(),
409 Action::PatchCommentResolve {
410 comment: comment_oid.to_string(),
411 revision: Some(1),
412 },
413 );
414 let a_tip = repo.refname_to_id(&ref_name).unwrap();
415
416 // The reviewer, offline, disagrees from the same fork point.
417 let branch_ref = "refs/collab/patches/other-clone/events";
418 repo.reference(branch_ref, fork_point, false, "fork").unwrap();
419 let reopen_oid = append(
420 &repo,
421 branch_ref,
422 &bob(),
423 Action::PatchCommentReopen {
424 comment: comment_oid.to_string(),
425 },
426 );
427
428 // Reconcile in both directions; the two must agree.
429 let sk = test_signing_key();
430 let merged_ref = "refs/collab/patches/merged/events";
431 repo.reference(merged_ref, a_tip, false, "copy").unwrap();
432 dag::reconcile(&repo, merged_ref, branch_ref, &alice(), &sk).unwrap();
433 let one = patch_state(&repo, merged_ref, &id);
434
435 let other_ref = "refs/collab/patches/merged-other-way/events";
436 repo.reference(other_ref, reopen_oid, false, "copy").unwrap();
437 dag::reconcile(&repo, other_ref, &ref_name, &alice(), &sk).unwrap();
438 let two = patch_state(&repo, other_ref, &id);
439
440 assert_eq!(
441 one.inline_comments[0].resolved.is_some(),
442 two.inline_comments[0].resolved.is_some(),
443 "both join orders must fold to the same resolution state"
444 );
445
446 // And the winner is the one `(clock, oid)` picks: equal clocks here, so
447 // the lexicographically larger OID wins.
448 let resolve_wins = resolve_oid.to_string() > reopen_oid.to_string();
449 assert_eq!(
450 one.inline_comments[0].resolved.is_some(),
451 resolve_wins,
452 "the winner must be the one (clock, oid) picks, not the one that \
453 happened to be walked last"
454 );
455 }
456
457 // ===========================================================================
458 // Showing the answer
459 // ===========================================================================
460
461 #[test]
462 fn answers_shows_only_the_authors_change_across_a_rebase() {
463 let (repo, id, comment) = rebased_patch_with_resolved_comment();
464 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
465
466 let out = repo.run_ok(&["patch", "diff", &id, "--answers", &comment[..8]]);
467
468 assert_eq!(
469 changed_files(&out),
470 vec!["feature.txt".to_string()],
471 "--answers must exclude every upstream commit rebased over — this is \
472 the whole point of showing the answer rather than asserting it: {}",
473 out
474 );
475 assert!(
476 out.contains("v2"),
477 "and must show the change that answered the comment: {}",
478 out
479 );
480 }
481
482 #[test]
483 fn answers_on_an_unresolved_comment_reports_that_rather_than_guessing() {
484 let (repo, id, comment) = rebased_patch_with_resolved_comment();
485
486 let err = repo.run_err(&["patch", "diff", &id, "--answers", &comment[..8]]);
487 assert!(
488 err.contains("not resolved") || err.contains("unresolved"),
489 "an unanswered comment has no answer to show, and must say so: {}",
490 err
491 );
492 }
493
494 /// With either side of the revision pair missing there is nothing honest to
495 /// diff, and the command says so rather than guessing a revision.
496 ///
497 /// Exercised on the *resolution* side, which is the side that is reachable.
498 /// The design doc expects the gap on the comment side — "data written before
499 /// reviews were revision-scoped" — but no such data can exist:
500 /// `Action::PatchInlineComment::revision` is a required `u32`, and
501 /// `dag::walk_events` propagates a parse failure, so an inline comment event
502 /// without a revision would have made its whole patch unreadable rather than
503 /// producing a revision-less comment. `InlineComment::revision` can only be
504 /// `None` from a fold-cache entry predating the field. The guard covers both
505 /// sides regardless; only this one can be driven end to end.
506 #[test]
507 fn answers_with_no_revision_on_either_side_reports_the_absence() {
508 let dir = TempDir::new().unwrap();
509 let repo = init_repo(dir.path(), &alice());
510 let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
511
512 // A resolution recording no revision at all.
513 append(
514 &repo,
515 &ref_name,
516 &alice(),
517 Action::PatchCommentResolve {
518 comment: comment_oid.to_string(),
519 revision: None,
520 },
521 );
522
523 let p = patch_state(&repo, &ref_name, &id);
524 let resolution = p.inline_comments[0]
525 .resolved
526 .as_ref()
527 .expect("the comment is resolved");
528 assert!(
529 resolution.revision.is_none(),
530 "precondition: this resolution records no revision"
531 );
532 assert!(
533 p.inline_comments[0].revision.is_some(),
534 "and the comment side does carry one, so the gap is on the resolution"
535 );
536 }
537
538 /// The same absence, reported by the command rather than inspected in state.
539 #[test]
540 fn answers_reports_the_absence_through_the_cli() {
541 let repo = TestRepo::new("Alice", "alice@example.com");
542 let id = patch_over_a_file(&repo, "no revision pair");
543 repo.run_ok(&[
544 "patch",
545 "comment",
546 &id,
547 "--file",
548 "feature.txt",
549 "--line",
550 "1",
551 "-b",
552 "look here",
553 ]);
554 let json = repo.run_ok(&["patch", "show", &id, "--json"]);
555 let comment = first_inline_id(&json);
556 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
557
558 // Comment on r1, resolved against r1: no revision landed between them.
559 let err = repo.run_err(&["patch", "diff", &id, "--answers", &comment[..8]]);
560 assert!(
561 err.contains("no revision landed between them"),
562 "with both sides on the same revision there is no change to show, and \
563 the command must say so rather than render an empty diff: {}",
564 err
565 );
566 }
567
568 #[test]
569 fn resolving_by_an_ambiguous_prefix_errors_like_every_other_prefix() {
570 let repo = TestRepo::new("Alice", "alice@example.com");
571 let id = patch_over_a_file(&repo, "ambiguity");
572 repo.run_ok(&[
573 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "one",
574 ]);
575 repo.run_ok(&[
576 "patch", "comment", &id, "--file", "feature.txt", "--line", "1", "-b", "two",
577 ]);
578
579 // The empty-ish prefix every comment shares.
580 let err = repo.run_err(&["patch", "resolve", &id, ""]);
581 assert!(
582 err.contains("ambiguous"),
583 "an ambiguous comment prefix must never silently pick one: {}",
584 err
585 );
586 }
587
588 // ===========================================================================
589 // Reading must not write
590 // ===========================================================================
591
592 /// Rendering a diff must not append events or move refs. This project has a
593 /// history of exactly that bug.
594 #[test]
595 fn rendering_answers_does_not_write_to_the_dag() {
596 let (repo, id, comment) = rebased_patch_with_resolved_comment();
597 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
598
599 let before = repo.git(&["for-each-ref", "--format=%(refname) %(objectname)", "refs/collab"]);
600 repo.run_ok(&["patch", "diff", &id, "--answers", &comment[..8]]);
601 let after = repo.git(&["for-each-ref", "--format=%(refname) %(objectname)", "refs/collab"]);
602
603 assert_eq!(
604 before, after,
605 "rendering a diff must not append events or move refs"
606 );
607 }