a73x

53f3bfbb

Show the sequence, and say what left the machine

a73x   2026-08-11 10:09

Commit message
Show the sequence, and say what left the machine

Two views that answer questions nobody asks, and one operation that
happened without saying so.

`patch log` listed revisions; `patch show` listed comments and reviews.
Between them they held everything that happened to a patch, and neither
answered the question a reviewer actually has: what answered what. A
comment on r1 followed by r2 is a causal chain, and reconstructing it
meant interleaving two commands' output by eye.

`patch log --timeline` renders that sequence, each entry anchored to the
revision it was made against. A flag rather than a new command because
`patch log` is already the chronological view — it opens with r1 and
already ends with the merge — and rather than a change to `patch show`,
which is the current-state view and, with `patch log`, is consumed by
scripts and by this suite. Default output of both is untouched, plain
and `--json`, and a test pins that.

Content comes from `PatchState`, not from the raw events: revision
numbering, the per-(author, revision) vote rule and every correction are
resolved there, and a second implementation of those rules here would be
free to drift from the one `patch show` prints. The walk supplies
ordering and the corrections; everything else is joined on by event OID.

Ordering is the DAG walk stable-sorted by Lamport clock. Not by
`(clock, oid)`: events written before clocks existed all carry clock 0,
so the oid tiebreak would sort a legacy patch's history into hash order.
A stable sort keyed on the clock alone leaves those in the only order
meaningful for them, and still interleaves a joined fork correctly.

An edit and a delete get their own entries, at the point they happened.
The DAG is append-only so the record stays an audit trail rather than a
summary, and folding a correction silently into the text it changed
would give back the summary. A tombstoned comment keeps its slot as
`[deleted]`. A correction the fold refused -- one author editing
another's words -- appears nowhere, held to the same author rule the
fold applies, because listing it would show a rewrite that never took
effect.

Rendering writes nothing: no event, no ref. A test sweeps every ref
before and after. (`patch show` still moves its `refs/collab/local/seen/`
read-marker; that is pre-existing, it is not a collab event, and the
timeline deliberately does not adopt it -- it marks nothing read.)

Second, 1a9f3573: mutating commands auto-synced silently. The banner
went to stderr while the fetch and push it introduced went to *stdout*,
interleaved with the command's own result, so `issue open` printed the
id you asked for and then four lines of network traffic you did not, and
a script capturing stdout got all of it.

Recording an event locally and publishing it are two facts with two
failure modes, so they are now reported separately. `sync` narrates
through `output` instead of printing directly: normally to stdout as
before, and under a write command to stderr with every line prefixed
`auto-sync:`. The remotes are named before anything is sent to them.
stdout is left carrying the command's result and nothing else --  which
is also what stops the `--json` gap (a6adfe39, untouched here) from
widening: when a write command does grow `--json`, its stdout is already
pure result and the sync status is already separable by stream.

A failed push cannot fail the command, and now says so:

    Opened issue 27eff033                                   # exit 0
    auto-sync: publishing to 'origin'...
    auto-sync: failed: git fetch exited with status 128
    auto-sync: your changes are recorded locally; run 'git-collab sync'
               to publish them.

Tested including the exit code, since the failure mode is a caller
retrying a write that already happened. Both config keys are documented
in the README alongside what the output now looks like.

Two existing assertions on the old `Auto-syncing with 'x'...` banner are
updated to the new wording -- the only intentional output break.
README.md
Old New
@@ -41,6 +41,28 @@ $ git-collab patch diff a1b2c3d4 --between 1 2 # interdiff: what changed betwe
41 $ git-collab patch diff a1b2c3d4 --revision 2 # revision 2 against the base 41 $ git-collab patch diff a1b2c3d4 --revision 2 # revision 2 against the base
42 ``` 42 ```
43 43
44 `patch log --timeline` puts the whole story in one sequence — revisions,
45 comments, reviews, corrections and the merge — each entry anchored to the
46 revision it was made against, so the causal chain is legible without
47 interleaving two commands' output by eye:
48
49 ```console
50 $ git-collab patch log a1b2c3d4 --timeline
51 2026-08-11T10:01:03Z r1 Alice edd93bf7 (initial)
52 2026-08-11T10:01:08Z comment Bob @r1 needs a test for the empty case
53 2026-08-11T10:01:12Z review Bob @r1 request-changes holding off until that lands
54 2026-08-11T10:01:31Z r2 Alice 37a3675a added the empty-case test
55 2026-08-11T10:01:36Z review Bob @r2 approve that covers it
56 2026-08-11T10:01:44Z merged Alice 66f21e82
57 ```
58
59 Reading it top to bottom: the request for changes was made against r1, and r2 is
60 what answered it. Corrections appear as their own entries rather than being
61 folded silently into the text they changed — the DAG is append-only so that the
62 record stays an audit trail, and the timeline is where that is visible. Add
63 `--json` for the same sequence as an array. Without `--timeline`, `patch log`
64 prints the revision list exactly as before.
65
44 Each revision is pinned by a ref in the patch's own namespace, named after the 66 Each revision is pinned by a ref in the patch's own namespace, named after the
45 commit it points at (`refs/collab/patches/<id>/rev/<oid>`), so `git-collab sync` 67 commit it points at (`refs/collab/patches/<id>/rev/<oid>`), so `git-collab sync`
46 carries the patch and every revision it ever had. Contributing needs no `git push` of a branch and no 68 carries the patch and every revision it ever had. Contributing needs no `git push` of a branch and no
@@ -258,6 +280,35 @@ $ git config collab.autoSync false # disable auto-sync
258 $ git config collab.autoSyncRemote origin # auto-sync only 'origin' 280 $ git config collab.autoSyncRemote origin # auto-sync only 'origin'
259 ``` 281 ```
260 282
283 A write command does two separable things, and says so separately. Its own
284 result goes to stdout; the auto-sync that follows narrates itself on stderr,
285 every line under an `auto-sync:` prefix, naming the remotes *before* it reaches
286 them:
287
288 ```console
289 $ git-collab issue open -t "Something broke"
290 Opened issue 27eff033
291 auto-sync: publishing to 'origin'...
292 auto-sync: Pushing to 'origin'...
293 auto-sync: published to 'origin'.
294 ```
295
296 The split is the point. Recording the event locally and publishing it are
297 different operations with different failure modes, so a failed push never fails
298 the command — the event is already recorded, and reporting otherwise would send
299 a script off to retry a write that already happened:
300
301 ```console
302 $ git-collab issue open -t "Something broke" # exit status 0
303 Opened issue 27eff033
304 auto-sync: publishing to 'origin'...
305 auto-sync: failed: git fetch exited with status exit status: 128
306 auto-sync: your changes are recorded locally; run 'git-collab sync' to publish them.
307 ```
308
309 Because the narration stays on stderr, stdout carries the command's result and
310 nothing else, whether or not a sync happened.
311
261 ## Trust 312 ## Trust
262 313
263 Sync verifies every signature it fetches. Until you add a trusted key, valid 314 Sync verifies every signature it fetches. Until you add a trusted key, valid
src/cli.rs
Old New
@@ -708,9 +708,17 @@ pub enum PatchCmd {
708 body_file: Option<String>, 708 body_file: Option<String>,
709 }, 709 },
710 /// Show revision log for a patch 710 /// Show revision log for a patch
711 ///
712 /// With --timeline, shows every event on the patch instead — revisions,
713 /// comments, reviews, corrections and the merge — in the order they
714 /// happened, each anchored to the revision it was made against, so the
715 /// sequence reads as what answered what.
711 Log { 716 Log {
712 /// Patch ID (prefix match) 717 /// Patch ID (prefix match)
713 id: String, 718 id: String,
719 /// Show all events, not just revisions, in the order they happened
720 #[arg(long)]
721 timeline: bool,
714 /// Output as JSON 722 /// Output as JSON
715 #[arg(long)] 723 #[arg(long)]
716 json: bool, 724 json: bool,
src/lib.rs
Old New
@@ -12,6 +12,7 @@ pub mod identity;
12 pub mod issue; 12 pub mod issue;
13 pub mod log; 13 pub mod log;
14 pub mod merge_scan; 14 pub mod merge_scan;
15 pub mod output;
15 pub mod patch; 16 pub mod patch;
16 pub mod release; 17 pub mod release;
17 pub mod signing; 18 pub mod signing;
@@ -19,6 +20,7 @@ pub mod state;
19 pub mod status; 20 pub mod status;
20 pub mod sync; 21 pub mod sync;
21 pub mod sync_lock; 22 pub mod sync_lock;
23 pub mod timeline;
22 pub mod trailer; 24 pub mod trailer;
23 pub mod trust; 25 pub mod trust;
24 pub mod tui; 26 pub mod tui;
@@ -70,6 +72,23 @@ pub fn staleness_warning(repo: &Repository, patch: &state::PatchState) -> Option
70 )) 72 ))
71 } 73 }
72 74
75 /// Publish the event a write command just recorded, and say so out loud.
76 ///
77 /// Two facts, reported separately, because they fail separately: the command's
78 /// own result (on stdout, already printed by the time this runs) says the event
79 /// is recorded locally; everything below is about whether it also reached a
80 /// remote. All of it goes to stderr under an `auto-sync:` prefix — see
81 /// `output` for why the split matters and what used to happen instead.
82 ///
83 /// The remote is named *before* anything is sent to it. Something that reaches
84 /// a remote should never be indistinguishable from local work, and a reviewer
85 /// working under an explicit "do not push" instruction should not have to read
86 /// the source to discover that `patch comment` pushes.
87 ///
88 /// Nothing here can fail the command. The local write already succeeded, and
89 /// reporting it as a failure would send a caller checking the exit code off to
90 /// retry a write that already happened — so a failed push says the write is
91 /// safe locally and names the command that will publish it later.
73 fn maybe_auto_sync(repo: &Repository) { 92 fn maybe_auto_sync(repo: &Repository) {
74 let enabled = repo 93 let enabled = repo
75 .config() 94 .config()
@@ -90,48 +109,55 @@ fn maybe_auto_sync(repo: &Repository) {
90 .ok() 109 .ok()
91 .and_then(|c| c.get_string("collab.autoSyncRemote").ok()); 110 .and_then(|c| c.get_string("collab.autoSyncRemote").ok());
92 111
93 if let Some(remote) = pinned_remote { 112 let (targets, result) = if let Some(remote) = pinned_remote {
94 eprintln!("Auto-syncing with '{}'...", remote); 113 let targets = format!("'{}'", remote);
95 match sync::sync(repo, &remote) { 114 let result = output::during_auto_sync(|| {
96 Ok(()) => {} 115 outln!("publishing to {}...", targets);
97 Err(error::Error::PartialSync { succeeded, total }) => { 116 sync::sync(repo, &remote)
98 eprintln!( 117 });
99 "warning: auto-sync partially failed ({}/{} refs pushed)", 118 (targets, result)
100 succeeded, total 119 } else {
101 ); 120 let remotes = match sync::collab_remotes(repo) {
102 } 121 Ok(remotes) => remotes,
103 Err(e) => { 122 Err(e) => {
104 eprintln!("warning: auto-sync failed: {}", e); 123 errln!("warning: auto-sync failed to list remotes: {}", e);
124 return;
105 } 125 }
106 } 126 };
107 return;
108 }
109 127
110 let remotes = match sync::collab_remotes(repo) { 128 // No remote to publish to is not a failed publish. Staying silent keeps
111 Ok(remotes) => remotes, 129 // a purely local repo's write commands free of network vocabulary.
112 Err(e) => { 130 if remotes.is_empty() {
113 eprintln!("warning: auto-sync failed to list remotes: {}", e);
114 return; 131 return;
115 } 132 }
116 };
117 133
118 if remotes.is_empty() { 134 let targets = sync::format_remote_list(&remotes);
119 return; 135 let result = output::during_auto_sync(|| {
120 } 136 outln!("publishing to {}...", targets);
137 sync::sync_all(repo)
138 });
139 (targets, result)
140 };
121 141
122 eprintln!("Auto-syncing with {}...", sync::format_remote_list(&remotes)); 142 output::during_auto_sync(|| match result {
123 match sync::sync_all(repo) { 143 Ok(()) => outln!("published to {}.", targets),
124 Ok(()) => {}
125 Err(error::Error::MultiRemoteSync { failed, total }) => {
126 eprintln!(
127 "warning: auto-sync failed for {} of {} remote(s)",
128 failed, total
129 );
130 }
131 Err(e) => { 144 Err(e) => {
132 eprintln!("warning: auto-sync failed: {}", e); 145 // `sync` itself already itemizes a partial or multi-remote
146 // failure — which refs, which remotes — so restating the counts
147 // here would only say the same thing twice. Anything else reaches
148 // this point unreported.
149 if !matches!(
150 e,
151 error::Error::PartialSync { .. } | error::Error::MultiRemoteSync { .. }
152 ) {
153 outln!("failed: {}", e);
154 }
155 // The one thing the user actually needs from a failed push, and
156 // the fact the old interleaved output never stated: the write is
157 // not lost, and this is how to publish it.
158 outln!("your changes are recorded locally; run 'git-collab sync' to publish them.");
133 } 159 }
134 } 160 });
135 } 161 }
136 162
137 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { 163 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
@@ -776,7 +802,20 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
776 println!("Revision description updated."); 802 println!("Revision description updated.");
777 Ok(()) 803 Ok(())
778 } 804 }
779 PatchCmd::Log { id, json } => { 805 PatchCmd::Log {
806 id,
807 timeline,
808 json,
809 } => {
810 if timeline {
811 let (_, entries) = timeline::build(repo, &id)?;
812 if json {
813 println!("{}", timeline::to_json(&entries)?);
814 } else {
815 timeline::to_writer(&entries, &mut std::io::stdout())?;
816 }
817 return Ok(());
818 }
780 let p = patch::patch_log(repo, &id)?; 819 let p = patch::patch_log(repo, &id)?;
781 if json { 820 if json {
782 let output = patch::patch_log_json(&p)?; 821 let output = patch::patch_log_json(&p)?;
src/output.rs
Old New
@@ -0,0 +1,105 @@
1 //! Where a command's own result ends and a network operation's narration begins.
2 //!
3 //! A write command does two separable things: it records an event in the local
4 //! DAG, and — unless `collab.autoSync` says otherwise — it tries to publish that
5 //! event to every configured remote. They have different failure modes. The
6 //! local write either happened or it didn't; the push may fail for reasons that
7 //! say nothing about whether the event is safely recorded.
8 //!
9 //! Those two facts used to arrive as one stream, and worse, split across the
10 //! wrong pair of streams: the `Auto-syncing with...` banner went to stderr while
11 //! the fetch and push progress it introduced went to *stdout*, interleaved with
12 //! the command's own result. `issue open` printed the id you asked for and then
13 //! four lines of network traffic you did not, and a script capturing stdout got
14 //! all of it.
15 //!
16 //! So `sync` narrates through this module instead of printing directly. Run
17 //! normally, it prints to stdout as before. Run underneath a write command, the
18 //! whole narration moves to stderr and every line of it is prefixed, which
19 //! leaves stdout carrying the command's own result and nothing else, and makes
20 //! the network operation legible as a separate thing that happened afterwards.
21 //!
22 //! Keeping stdout clean is also what stops the `--json` gap (issue a6adfe39,
23 //! tracked separately) from getting worse: `--json` exists only on read
24 //! commands today, and read commands never auto-sync, so there is no overlap
25 //! yet. When a write command does grow `--json`, its stdout is already pure
26 //! result and the sync status is already separable — by stream, without any
27 //! caller having to learn a new envelope.
28
29 use std::io::Write;
30 use std::sync::atomic::{AtomicBool, Ordering};
31
32 /// Set while an auto-sync triggered by a write command is running.
33 ///
34 /// A plain atomic rather than anything scoped because the CLI is a single
35 /// process running one command to completion, and the alternative — threading
36 /// a writer through every function in `sync` — buys nothing here.
37 static IN_AUTO_SYNC: AtomicBool = AtomicBool::new(false);
38
39 /// Marks every line auto-sync emits, so a reader can tell at a glance which
40 /// output came from the command they ran and which came from the network
41 /// operation it triggered.
42 pub const AUTO_SYNC_PREFIX: &str = "auto-sync: ";
43
44 /// Run `f` with sync's narration redirected to stderr and prefixed.
45 ///
46 /// Restores the previous channel afterwards, including on the error paths
47 /// inside `f`, so a failed sync cannot leave the process redirecting.
48 pub fn during_auto_sync<T>(f: impl FnOnce() -> T) -> T {
49 IN_AUTO_SYNC.store(true, Ordering::Relaxed);
50 let result = f();
51 IN_AUTO_SYNC.store(false, Ordering::Relaxed);
52 result
53 }
54
55 pub fn in_auto_sync() -> bool {
56 IN_AUTO_SYNC.load(Ordering::Relaxed)
57 }
58
59 /// Emit a line of progress: stdout normally, prefixed stderr under auto-sync.
60 ///
61 /// Multi-line text is prefixed per line rather than as a block, so no line of
62 /// an auto-sync ever appears unlabelled. Blank lines stay blank — a prefix on
63 /// its own reads as a truncated message rather than as the separator it is.
64 pub fn emit(text: &str) {
65 if in_auto_sync() {
66 emit_prefixed(&mut std::io::stderr().lock(), text);
67 } else {
68 let _ = writeln!(std::io::stdout().lock(), "{}", text);
69 }
70 }
71
72 /// Emit a warning or error. Always stderr; prefixed under auto-sync so a
73 /// warning from the push cannot be mistaken for one from the command.
74 pub fn emit_err(text: &str) {
75 let mut err = std::io::stderr().lock();
76 if in_auto_sync() {
77 emit_prefixed(&mut err, text);
78 } else {
79 let _ = writeln!(err, "{}", text);
80 }
81 }
82
83 fn emit_prefixed(w: &mut impl Write, text: &str) {
84 for line in text.split('\n') {
85 if line.is_empty() {
86 let _ = writeln!(w);
87 } else {
88 let _ = writeln!(w, "{}{}", AUTO_SYNC_PREFIX, line);
89 }
90 }
91 }
92
93 /// `println!` for progress that auto-sync must be able to redirect.
94 #[macro_export]
95 macro_rules! outln {
96 () => { $crate::output::emit("") };
97 ($($arg:tt)*) => { $crate::output::emit(&format!($($arg)*)) };
98 }
99
100 /// `eprintln!` for warnings that auto-sync must be able to label.
101 #[macro_export]
102 macro_rules! errln {
103 () => { $crate::output::emit_err("") };
104 ($($arg:tt)*) => { $crate::output::emit_err(&format!($($arg)*)) };
105 }
src/patch.rs
Old New
@@ -1230,7 +1230,7 @@ pub fn patch_log(repo: &Repository, id_prefix: &str) -> Result<PatchState, Error
1230 /// them in full. 1230 /// them in full.
1231 /// 1231 ///
1232 /// Truncating by chars rather than bytes keeps this total for any input. 1232 /// Truncating by chars rather than bytes keeps this total for any input.
1233 fn summarize_body(body: &str) -> String { 1233 pub(crate) fn summarize_body(body: &str) -> String {
1234 const WIDTH: usize = 60; 1234 const WIDTH: usize = 60;
1235 let first_line = body.lines().next().unwrap_or_default().trim(); 1235 let first_line = body.lines().next().unwrap_or_default().trim();
1236 let more = body.lines().nth(1).is_some(); 1236 let more = body.lines().nth(1).is_some();
src/sync.rs
Old New
@@ -12,6 +12,7 @@ use crate::signing;
12 use crate::state; 12 use crate::state;
13 use crate::sync_lock::SyncLock; 13 use crate::sync_lock::SyncLock;
14 use crate::trust; 14 use crate::trust;
15 use crate::{errln, outln};
15 16
16 // --------------------------------------------------------------------------- 17 // ---------------------------------------------------------------------------
17 // Ref name validation 18 // Ref name validation
@@ -114,7 +115,7 @@ impl SyncState {
114 Ok(contents) => match serde_json::from_str(&contents) { 115 Ok(contents) => match serde_json::from_str(&contents) {
115 Ok(state) => Some(state), 116 Ok(state) => Some(state),
116 Err(e) => { 117 Err(e) => {
117 eprintln!( 118 errln!(
118 "warning: corrupted sync state file ({}); ignoring and proceeding with full sync", 119 "warning: corrupted sync state file ({}); ignoring and proceeding with full sync",
119 e 120 e
120 ); 121 );
@@ -302,11 +303,11 @@ fn print_refname_conflict_advice(remote: &str, ids: &[String]) {
302 .map(|id| format!("{}{}", PATCH_REF_PREFIX, id)) 303 .map(|id| format!("{}{}", PATCH_REF_PREFIX, id))
303 .collect(); 304 .collect();
304 305
305 eprintln!( 306 errln!(
306 "\nRefname conflict: '{}' still has the pre-migration patch ref layout.", 307 "\nRefname conflict: '{}' still has the pre-migration patch ref layout.",
307 remote 308 remote
308 ); 309 );
309 eprintln!( 310 errln!(
310 "\nPatch refs moved from a single `{}<id>` to `<id>/events` plus\n\ 311 "\nPatch refs moved from a single `{}<id>` to `<id>/events` plus\n\
311 `<id>/rev/<oid>`. Git cannot hold both a ref and a directory at the same\n\ 312 `<id>/rev/<oid>`. Git cannot hold both a ref and a directory at the same\n\
312 path, so while the old ref is still on the remote every push of the new\n\ 313 path, so while the old ref is still on the remote every push of the new\n\
@@ -328,14 +329,14 @@ fn print_refname_conflict_advice(remote: &str, ids: &[String]) {
328 ) 329 )
329 }; 330 };
330 331
331 eprintln!("\nBlocked by {}:", count); 332 errln!("\nBlocked by {}:", count);
332 for name in &stale { 333 for name in &stale {
333 eprintln!(" {}", name); 334 errln!(" {}", name);
334 } 335 }
335 336
336 eprintln!("\nTo fix, delete {} on the remote and sync again:\n", them); 337 errln!("\nTo fix, delete {} on the remote and sync again:\n", them);
337 eprintln!(" git push {} --delete {}", remote, stale.join(" ")); 338 errln!(" git push {} --delete {}", remote, stale.join(" "));
338 eprintln!( 339 errln!(
339 "\nThat deletes only {} listed above, and only on '{}'. Nothing local is\n\ 340 "\nThat deletes only {} listed above, and only on '{}'. Nothing local is\n\
340 touched: the next sync republishes the same events under `<id>/events`,\n\ 341 touched: the next sync republishes the same events under `<id>/events`,\n\
341 which is where every current version reads them from.", 342 which is where every current version reads them from.",
@@ -349,18 +350,18 @@ fn print_sync_summary(result: &SyncResult) {
349 let total = result.results.len(); 350 let total = result.results.len();
350 351
351 if succeeded == 0 { 352 if succeeded == 0 {
352 eprintln!("\nSync failed: {} of {} refs pushed.", succeeded, total); 353 errln!("\nSync failed: {} of {} refs pushed.", succeeded, total);
353 } else { 354 } else {
354 eprintln!( 355 errln!(
355 "\nSync partially failed: {} of {} refs pushed.", 356 "\nSync partially failed: {} of {} refs pushed.",
356 succeeded, total 357 succeeded, total
357 ); 358 );
358 } 359 }
359 360
360 let failed = result.failed(); 361 let failed = result.failed();
361 eprintln!("Failed refs:"); 362 errln!("Failed refs:");
362 for f in &failed { 363 for f in &failed {
363 eprintln!( 364 errln!(
364 " {}: {}", 365 " {}: {}",
365 f.ref_name, 366 f.ref_name,
366 f.error.as_deref().unwrap_or("unknown error") 367 f.error.as_deref().unwrap_or("unknown error")
@@ -394,7 +395,7 @@ fn print_sync_summary(result: &SyncResult) {
394 // only the retryable refs keeps this line honest — retrying the conflicted 395 // only the retryable refs keeps this line honest — retrying the conflicted
395 // ones is exactly what the advice above says not to do. 396 // ones is exactly what the advice above says not to do.
396 if retryable > 0 { 397 if retryable > 0 {
397 eprintln!( 398 errln!(
398 "\nRun `git-collab sync --remote {}` again to retry {} failed ref(s).", 399 "\nRun `git-collab sync --remote {}` again to retry {} failed ref(s).",
399 result.remote, retryable 400 result.remote, retryable
400 ); 401 );
@@ -458,19 +459,19 @@ pub fn init(repo: &Repository) -> Result<(), Error> {
458 fn init_refspecs(repo: &Repository) -> Result<(), Error> { 459 fn init_refspecs(repo: &Repository) -> Result<(), Error> {
459 let remotes = repo.remotes()?; 460 let remotes = repo.remotes()?;
460 if remotes.is_empty() { 461 if remotes.is_empty() {
461 println!("No remotes configured."); 462 outln!("No remotes configured.");
462 return Ok(()); 463 return Ok(());
463 } 464 }
464 for remote_name in remotes.iter().flatten() { 465 for remote_name in remotes.iter().flatten() {
465 if has_collab_refspec(repo, remote_name)? { 466 if has_collab_refspec(repo, remote_name)? {
466 println!("Remote '{}' already configured", remote_name); 467 outln!("Remote '{}' already configured", remote_name);
467 continue; 468 continue;
468 } 469 }
469 let fetch_spec = format!("+refs/collab/*:refs/collab/sync/{}/*", remote_name); 470 let fetch_spec = format!("+refs/collab/*:refs/collab/sync/{}/*", remote_name);
470 repo.remote_add_fetch(remote_name, &fetch_spec)?; 471 repo.remote_add_fetch(remote_name, &fetch_spec)?;
471 println!("Configured remote '{}'", remote_name); 472 outln!("Configured remote '{}'", remote_name);
472 } 473 }
473 println!("Collab refspecs initialized."); 474 outln!("Collab refspecs initialized.");
474 Ok(()) 475 Ok(())
475 } 476 }
476 477
@@ -480,7 +481,7 @@ fn init_refspecs(repo: &Repository) -> Result<(), Error> {
480 fn init_hook(repo: &Repository) { 481 fn init_hook(repo: &Repository) {
481 match crate::hooks::install(repo) { 482 match crate::hooks::install(repo) {
482 Ok(outcome) => crate::hooks::report_install(&outcome), 483 Ok(outcome) => crate::hooks::report_install(&outcome),
483 Err(e) => eprintln!("warning: could not install the commit-msg hook: {}", e), 484 Err(e) => errln!("warning: could not install the commit-msg hook: {}", e),
484 } 485 }
485 } 486 }
486 487
@@ -528,8 +529,10 @@ pub fn sync_all(repo: &Repository) -> Result<(), Error> {
528 } 529 }
529 530
530 let multiple = remotes.len() > 1; 531 let multiple = remotes.len() > 1;
531 if multiple { 532 // Auto-sync has already named the same remotes in its own opening line, so
532 println!( 533 // repeating them here would be the second banner in two lines.
534 if multiple && !crate::output::in_auto_sync() {
535 outln!(
533 "Syncing {} remotes: {}", 536 "Syncing {} remotes: {}",
534 remotes.len(), 537 remotes.len(),
535 format_remote_list(&remotes) 538 format_remote_list(&remotes)
@@ -540,12 +543,12 @@ pub fn sync_all(repo: &Repository) -> Result<(), Error> {
540 let mut failed = Vec::new(); 543 let mut failed = Vec::new();
541 for remote_name in &remotes { 544 for remote_name in &remotes {
542 if multiple { 545 if multiple {
543 println!(); 546 outln!();
544 } 547 }
545 match sync(repo, remote_name) { 548 match sync(repo, remote_name) {
546 Ok(()) => succeeded.push(remote_name.clone()), 549 Ok(()) => succeeded.push(remote_name.clone()),
547 Err(e) => { 550 Err(e) => {
548 eprintln!("error: sync with '{}' failed: {}", remote_name, e); 551 errln!("error: sync with '{}' failed: {}", remote_name, e);
549 failed.push(remote_name.clone()); 552 failed.push(remote_name.clone());
550 } 553 }
551 } 554 }
@@ -553,7 +556,7 @@ pub fn sync_all(repo: &Repository) -> Result<(), Error> {
553 556
554 if failed.is_empty() { 557 if failed.is_empty() {
555 if multiple { 558 if multiple {
556 println!( 559 outln!(
557 "\nAll {} remotes synced: {}", 560 "\nAll {} remotes synced: {}",
558 remotes.len(), 561 remotes.len(),
559 format_remote_list(&remotes) 562 format_remote_list(&remotes)
@@ -561,14 +564,14 @@ pub fn sync_all(repo: &Repository) -> Result<(), Error> {
561 } 564 }
562 Ok(()) 565 Ok(())
563 } else { 566 } else {
564 eprintln!( 567 errln!(
565 "\nSync incomplete: {} of {} remote(s) failed.", 568 "\nSync incomplete: {} of {} remote(s) failed.",
566 failed.len(), 569 failed.len(),
567 remotes.len() 570 remotes.len()
568 ); 571 );
569 eprintln!(" Failed: {}", format_remote_list(&failed)); 572 errln!(" Failed: {}", format_remote_list(&failed));
570 if !succeeded.is_empty() { 573 if !succeeded.is_empty() {
571 eprintln!(" Succeeded: {}", format_remote_list(&succeeded)); 574 errln!(" Succeeded: {}", format_remote_list(&succeeded));
572 } 575 }
573 Err(Error::MultiRemoteSync { 576 Err(Error::MultiRemoteSync {
574 failed: failed.len(), 577 failed: failed.len(),
@@ -598,17 +601,17 @@ fn run_local_scans(
598 ) { 601 ) {
599 // Scan local branches for Issue: trailers and emit link events. 602 // Scan local branches for Issue: trailers and emit link events.
600 match crate::commit_link::scan_and_link(repo, author, sk) { 603 match crate::commit_link::scan_and_link(repo, author, sk) {
601 Ok(n) if n > 0 => println!("Linked {} commit(s) to issues.", n), 604 Ok(n) if n > 0 => outln!("Linked {} commit(s) to issues.", n),
602 Ok(_) => {} 605 Ok(_) => {}
603 Err(e) => eprintln!("warning: commit link scan failed: {}", e), 606 Err(e) => errln!("warning: commit link scan failed: {}", e),
604 } 607 }
605 608
606 // Scan each open patch's own base branch for Patch: trailers and record 609 // Scan each open patch's own base branch for Patch: trailers and record
607 // the merges they name. 610 // the merges they name.
608 match crate::merge_scan::scan_and_record_merges(repo, author, sk) { 611 match crate::merge_scan::scan_and_record_merges(repo, author, sk) {
609 Ok(n) if n > 0 => println!("Recorded {} merged patch(es).", n), 612 Ok(n) if n > 0 => outln!("Recorded {} merged patch(es).", n),
610 Ok(_) => {} 613 Ok(_) => {}
611 Err(e) => eprintln!("warning: merge scan failed: {}", e), 614 Err(e) => errln!("warning: merge scan failed: {}", e),
612 } 615 }
613 } 616 }
614 617
@@ -625,7 +628,7 @@ fn sync_local_only(repo: &Repository) -> Result<(), Error> {
625 // the case the lock exists for, and having no remote does not change it. 628 // the case the lock exists for, and having no remote does not change it.
626 let _lock = SyncLock::acquire(repo)?; 629 let _lock = SyncLock::acquire(repo)?;
627 630
628 println!("No remotes with collab refspecs configured — scanning local history only."); 631 outln!("No remotes with collab refspecs configured — scanning local history only.");
629 632
630 let author = get_author(repo)?; 633 let author = get_author(repo)?;
631 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 634 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
@@ -637,7 +640,7 @@ fn sync_local_only(repo: &Repository) -> Result<(), Error> {
637 640
638 run_local_scans(repo, &author, &sk); 641 run_local_scans(repo, &author, &sk);
639 642
640 println!("Local scan complete. Add a remote and run `git-collab init` to share these events."); 643 outln!("Local scan complete. Add a remote and run `git-collab init` to share these events.");
641 644
642 // The same closing hint the remote path prints, and on the same terms: a 645 // The same closing hint the remote path prints, and on the same terms: a
643 // suggestion drawn from reachability, which cannot see a squash — so its 646 // suggestion drawn from reachability, which cannot see a squash — so its
@@ -664,7 +667,7 @@ pub fn sync(repo: &Repository, remote_name: &str) -> Result<(), Error> {
664 let author = get_author(repo)?; 667 let author = get_author(repo)?;
665 668
666 // Step 1: Fetch collab refs using system git (handles SSH agent, credentials, etc.) 669 // Step 1: Fetch collab refs using system git (handles SSH agent, credentials, etc.)
667 println!("Fetching from '{}'...", remote_name); 670 outln!("Fetching from '{}'...", remote_name);
668 let fetch_status = Command::new("git") 671 let fetch_status = Command::new("git")
669 .args([ 672 .args([
670 "fetch", 673 "fetch",
@@ -713,11 +716,11 @@ pub fn sync(repo: &Repository, remote_name: &str) -> Result<(), Error> {
713 run_local_scans(&repo, &author, &sk); 716 run_local_scans(&repo, &author, &sk);
714 717
715 // Step 3: Push collab refs individually 718 // Step 3: Push collab refs individually
716 println!("Pushing to '{}'...", remote_name); 719 outln!("Pushing to '{}'...", remote_name);
717 let refs_to_push = collect_push_refs(&repo)?; 720 let refs_to_push = collect_push_refs(&repo)?;
718 721
719 if refs_to_push.is_empty() { 722 if refs_to_push.is_empty() {
720 println!("Nothing to push."); 723 outln!("Nothing to push.");
721 } else { 724 } else {
722 let sync_result = push_refs(&workdir, remote_name, &refs_to_push); 725 let sync_result = push_refs(&workdir, remote_name, &refs_to_push);
723 726
@@ -757,7 +760,7 @@ pub fn sync(repo: &Repository, remote_name: &str) -> Result<(), Error> {
757 // Step 4: Clean up sync refs 760 // Step 4: Clean up sync refs
758 cleanup_sync_refs(&repo)?; 761 cleanup_sync_refs(&repo)?;
759 762
760 println!("Sync complete for '{}'.", remote_name); 763 outln!("Sync complete for '{}'.", remote_name);
761 764
762 // A closing hint, not a decision: patches whose head is reachable from 765 // A closing hint, not a decision: patches whose head is reachable from
763 // their base tip but which carry no `PatchMerge`. Reachability cannot see a 766 // their base tip but which carry no `PatchMerge`. Reachability cannot see a
@@ -773,7 +776,7 @@ fn sync_resume(
773 workdir: &Path, 776 workdir: &Path,
774 state: &SyncState, 777 state: &SyncState,
775 ) -> Result<(), Error> { 778 ) -> Result<(), Error> {
776 println!( 779 outln!(
777 "Resuming sync to '{}' ({} refs pending from previous failure)...", 780 "Resuming sync to '{}' ({} refs pending from previous failure)...",
778 remote_name, 781 remote_name,
779 state.pending_refs.len() 782 state.pending_refs.len()
@@ -781,7 +784,7 @@ fn sync_resume(
781 784
782 // Print pending refs with their last error 785 // Print pending refs with their last error
783 for (ref_name, last_error) in &state.pending_refs { 786 for (ref_name, last_error) in &state.pending_refs {
784 println!(" Pending: {} (last error: {})", ref_name, last_error); 787 outln!(" Pending: {} (last error: {})", ref_name, last_error);
785 } 788 }
786 789
787 // Clean up stale sync refs (T018b) 790 // Clean up stale sync refs (T018b)
@@ -795,14 +798,14 @@ fn sync_resume(
795 // All pending refs pushed successfully 798 // All pending refs pushed successfully
796 SyncState::clear(repo)?; 799 SyncState::clear(repo)?;
797 if sync_result.results.is_empty() { 800 if sync_result.results.is_empty() {
798 println!("All pending refs are already up to date. Clearing stale sync state."); 801 outln!("All pending refs are already up to date. Clearing stale sync state.");
799 } else { 802 } else {
800 println!( 803 outln!(
801 "\nSync complete for '{}'. All previously-failed refs pushed.", 804 "\nSync complete for '{}'. All previously-failed refs pushed.",
802 remote_name 805 remote_name
803 ); 806 );
804 } 807 }
805 println!("Sync complete for '{}'.", remote_name); 808 outln!("Sync complete for '{}'.", remote_name);
806 Ok(()) 809 Ok(())
807 } else { 810 } else {
808 // Some refs still failing - update state 811 // Some refs still failing - update state
@@ -827,7 +830,7 @@ fn sync_resume(
827 new_state.save(repo)?; 830 new_state.save(repo)?;
828 831
829 print_sync_summary(&sync_result); 832 print_sync_summary(&sync_result);
830 eprintln!("To force a full sync, delete .git/collab/sync-state.json"); 833 errln!("To force a full sync, delete .git/collab/sync-state.json");
831 834
832 let succeeded = sync_result.succeeded().len(); 835 let succeeded = sync_result.succeeded().len();
833 let total = sync_result.results.len(); 836 let total = sync_result.results.len();
@@ -840,9 +843,9 @@ fn push_refs(workdir: &Path, remote_name: &str, refs: &[String]) -> SyncResult {
840 let results = push_refs_batched(workdir, remote_name, refs); 843 let results = push_refs_batched(workdir, remote_name, refs);
841 for result in &results { 844 for result in &results {
842 match &result.status { 845 match &result.status {
843 PushStatus::Pushed => println!(" Pushed {}", result.ref_name), 846 PushStatus::Pushed => outln!(" Pushed {}", result.ref_name),
844 PushStatus::Failed => { 847 PushStatus::Failed => {
845 eprintln!( 848 errln!(
846 " FAILED {}: {}", 849 " FAILED {}: {}",
847 result.ref_name, 850 result.ref_name,
848 result.error.as_deref().unwrap_or("unknown error") 851 result.error.as_deref().unwrap_or("unknown error")
@@ -930,7 +933,7 @@ fn reconcile_refs(
930 for (remote_ref, id, classified) in &sync_refs { 933 for (remote_ref, id, classified) in &sync_refs {
931 // Validate the ref ID format before processing 934 // Validate the ref ID format before processing
932 if let Err(e) = validate_collab_ref_id(id) { 935 if let Err(e) = validate_collab_ref_id(id) {
933 eprintln!(" Skipping {} with invalid ref ID {:.8}: {}", kind, id, e); 936 errln!(" Skipping {} with invalid ref ID {:.8}: {}", kind, id, e);
934 continue; 937 continue;
935 } 938 }
936 939
@@ -948,7 +951,7 @@ fn reconcile_refs(
948 951
949 if matches!(trust_policy, trust::TrustPolicy::Unconfigured) && !warned_unconfigured 952 if matches!(trust_policy, trust::TrustPolicy::Unconfigured) && !warned_unconfigured
950 { 953 {
951 eprintln!("warning: no trusted keys configured — all valid signatures accepted. Run 'collab key add --self' to start."); 954 errln!("warning: no trusted keys configured — all valid signatures accepted. Run 'collab key add --self' to start.");
952 warned_unconfigured = true; 955 warned_unconfigured = true;
953 } 956 }
954 957
@@ -958,7 +961,7 @@ fn reconcile_refs(
958 .collect(); 961 .collect();
959 if !failures.is_empty() { 962 if !failures.is_empty() {
960 for f in &failures { 963 for f in &failures {
961 eprintln!( 964 errln!(
962 " Rejecting {} {:.8}: commit {} — {}", 965 " Rejecting {} {:.8}: commit {} — {}",
963 kind, 966 kind,
964 id, 967 id,
@@ -970,7 +973,7 @@ fn reconcile_refs(
970 } 973 }
971 } 974 }
972 Err(e) => { 975 Err(e) => {
973 eprintln!(" Failed to verify {} {:.8}: {}", kind, id, e); 976 errln!(" Failed to verify {} {:.8}: {}", kind, id, e);
974 continue; 977 continue;
975 } 978 }
976 } 979 }
@@ -987,14 +990,14 @@ fn reconcile_refs(
987 dag::ReconcileOutcome::FastForward => "fast-forwarded", 990 dag::ReconcileOutcome::FastForward => "fast-forwarded",
988 dag::ReconcileOutcome::Merge => "merged", 991 dag::ReconcileOutcome::Merge => "merged",
989 }; 992 };
990 println!(" Reconciled {} {:.8} ({})", kind, id, action); 993 outln!(" Reconciled {} {:.8} ({})", kind, id, action);
991 } 994 }
992 Err(e) => eprintln!(" Failed to reconcile {} {:.8}: {}", kind, id, e), 995 Err(e) => errln!(" Failed to reconcile {} {:.8}: {}", kind, id, e),
993 } 996 }
994 } else { 997 } else {
995 let oid = repo.refname_to_id(remote_ref)?; 998 let oid = repo.refname_to_id(remote_ref)?;
996 repo.reference(local_ref, oid, false, "sync: new from remote")?; 999 repo.reference(local_ref, oid, false, "sync: new from remote")?;
997 println!(" New {} {:.8} from remote", kind, id); 1000 outln!(" New {} {:.8} from remote", kind, id);
998 } 1001 }
999 1002
1000 // Pin every revision the DAG we just reconciled lists, while the 1003 // Pin every revision the DAG we just reconciled lists, while the
@@ -1003,7 +1006,7 @@ fn reconcile_refs(
1003 // without a ref of its own becomes gc-eligible at that point. 1006 // without a ref of its own becomes gc-eligible at that point.
1004 if kind == "patches" { 1007 if kind == "patches" {
1005 if let Err(e) = state::pin_dag_revisions(repo, local_ref, id) { 1008 if let Err(e) = state::pin_dag_revisions(repo, local_ref, id) {
1006 eprintln!(" Failed to pin revisions for patch {:.8}: {}", id, e); 1009 errln!(" Failed to pin revisions for patch {:.8}: {}", id, e);
1007 } 1010 }
1008 } 1011 }
1009 } 1012 }
src/timeline.rs
Old New
@@ -0,0 +1,468 @@
1 //! The unified event timeline for a patch: one sequence, every kind of event.
2 //!
3 //! `patch log` lists revisions. `patch show` lists comments and reviews. Between
4 //! them they hold everything that happened to a patch and answer, separately,
5 //! two questions nobody asks. The question a reviewer actually has is *what
6 //! answered what* — a comment on revision 1 followed by revision 2 is a causal
7 //! chain, and it is the chain this project's revision anchoring exists to
8 //! record. Reconstructing it meant reading two commands' output and interleaving
9 //! them by timestamp by eye.
10 //!
11 //! # Where the ordering comes from
12 //!
13 //! Events are ordered by the DAG walk, then stable-sorted by Lamport clock.
14 //! Both halves of that matter:
15 //!
16 //! - The walk is `Sort::TOPOLOGICAL | Sort::REVERSE`, the same order
17 //! `PatchState::from_ref_uncached` folds in, which is what guarantees this
18 //! view can never number a revision differently from `patch log`.
19 //! - Clocks are monotonic along parent links, so for the linear DAG every patch
20 //! in the wild actually has, sorting by clock changes nothing. Where it earns
21 //! its keep is a DAG joined from a fork: topological order emits one branch
22 //! then the other, and the clock interleaves them back into the order things
23 //! happened.
24 //!
25 //! Sorting by `(clock, oid)` — the total order the status fold resolves
26 //! conflicts with — was the obvious alternative and is wrong here. Events
27 //! written before clocks existed all carry clock 0, so the oid tiebreak would
28 //! sort a legacy patch's whole history into hash order. A *stable* sort keyed on
29 //! the clock alone leaves those events in the only order that is meaningful for
30 //! them, the one the DAG's parent links record.
31 //!
32 //! # Where the content comes from
33 //!
34 //! From `PatchState`, not from the raw events: revision numbering, the
35 //! per-`(author, revision)` vote rule, and every correction have already been
36 //! resolved there. Re-deriving any of it here would be a second implementation
37 //! of the same rules, free to drift from the first — and the first is the one
38 //! `patch show` prints. The walk supplies ordering and the correction events;
39 //! everything a reader sees is joined onto it by event OID.
40 //!
41 //! # Reads do not write
42 //!
43 //! Building a timeline appends no event and moves no ref. It resolves the
44 //! patch's ref, walks it, and folds it. (`resolve_patch_ref` runs the one-time
45 //! `migrate_patch_layout`, exactly as `patch show` and `patch log` already do;
46 //! that is a pre-existing ref-layout migration, not a collab event, and this
47 //! view adds nothing to it. Note `patch show` additionally moves a
48 //! `refs/collab/local/seen/` read-marker — the timeline deliberately does *not*
49 //! adopt that pattern and marks nothing read.)
50
51 use git2::Repository;
52 use serde::Serialize;
53
54 use crate::error::Error;
55 use crate::event::{Action, Author};
56 use crate::state::{self, PatchState};
57
58 /// One thing that happened to a patch.
59 ///
60 /// `revision` is the anchor that makes the sequence causal rather than merely
61 /// chronological: the revision a comment or review was made against, or the
62 /// revision a revision entry *is*. `None` only where the event is not about a
63 /// revision at all (a label, a correction, the merge).
64 #[derive(Debug, Clone, Serialize)]
65 pub struct Entry {
66 #[serde(flatten)]
67 pub kind: Kind,
68 #[serde(skip_serializing_if = "Option::is_none")]
69 pub revision: Option<u32>,
70 pub author: Author,
71 pub timestamp: String,
72 /// The event's own OID, so a caller can name it to `edit-comment`.
73 pub id: String,
74 }
75
76 #[derive(Debug, Clone, Serialize)]
77 #[serde(tag = "type", rename_all = "snake_case")]
78 pub enum Kind {
79 Revision {
80 commit: String,
81 #[serde(skip_serializing_if = "Option::is_none")]
82 body: Option<String>,
83 edited: bool,
84 },
85 Comment {
86 body: String,
87 edited: bool,
88 deleted: bool,
89 },
90 InlineComment {
91 file: String,
92 line: u32,
93 body: String,
94 edited: bool,
95 deleted: bool,
96 },
97 Review {
98 verdict: String,
99 body: String,
100 edited: bool,
101 },
102 Label {
103 label: String,
104 },
105 Unlabel {
106 label: String,
107 },
108 /// A `BodyEdit` that the fold honoured, naming what it corrected.
109 Edited {
110 target: String,
111 },
112 /// A `CommentDelete` that the fold honoured.
113 Deleted {
114 target: String,
115 },
116 Closed {
117 #[serde(skip_serializing_if = "Option::is_none")]
118 reason: Option<String>,
119 },
120 Merged {
121 #[serde(skip_serializing_if = "Option::is_none")]
122 commit: Option<String>,
123 },
124 }
125
126 impl Kind {
127 /// The fixed-width label opening a rendered line.
128 fn label(&self, revision: Option<u32>) -> String {
129 match self {
130 Kind::Revision { .. } => format!("r{}", revision.unwrap_or(0)),
131 Kind::Comment { .. } => "comment".to_string(),
132 Kind::InlineComment { .. } => "inline".to_string(),
133 Kind::Review { .. } => "review".to_string(),
134 Kind::Label { .. } => "label".to_string(),
135 Kind::Unlabel { .. } => "unlabel".to_string(),
136 Kind::Edited { .. } => "edited".to_string(),
137 Kind::Deleted { .. } => "deleted".to_string(),
138 Kind::Closed { .. } => "closed".to_string(),
139 Kind::Merged { .. } => "merged".to_string(),
140 }
141 }
142 }
143
144 /// Build the timeline for the patch `id_prefix` resolves to.
145 pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entry>), Error> {
146 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
147 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
148 let events = crate::dag::walk_events(repo, &ref_name)?;
149
150 // Who wrote each body-carrying event, so a correction naming one can be
151 // held to the same rule the fold applies: only the author of a body may
152 // correct it. Everything else is dropped in silence, exactly as
153 // `BodyOverrides::authorized` drops it — a timeline that listed a refused
154 // edit would be reporting a rewrite of someone else's words that never
155 // took effect.
156 let mut owners: std::collections::HashMap<String, String> = std::collections::HashMap::new();
157 for (oid, event) in &events {
158 if carries_a_body(&event.action) {
159 owners.insert(oid.to_string(), event.author.email.clone());
160 }
161 }
162
163 // Derived content, indexed by the event that produced it. Joining on the
164 // OID is what keeps this view's text identical to `patch show`'s.
165 let comments: std::collections::HashMap<String, &state::Comment> = patch
166 .comments
167 .iter()
168 .map(|c| (c.commit_id.to_string(), c))
169 .collect();
170 let inline: std::collections::HashMap<String, &state::InlineComment> = patch
171 .inline_comments
172 .iter()
173 .map(|c| (c.commit_id.to_string(), c))
174 .collect();
175 let reviews: std::collections::HashMap<String, &state::Review> = patch
176 .reviews
177 .iter()
178 .map(|r| (r.commit_id.to_string(), r))
179 .collect();
180 let revisions: std::collections::HashMap<String, &state::Revision> = patch
181 .revisions
182 .iter()
183 .filter(|r| !r.event_id.is_empty())
184 .map(|r| (r.event_id.clone(), r))
185 .collect();
186
187 let mut entries: Vec<(u64, Entry)> = Vec::new();
188
189 for (oid, event) in &events {
190 let oid_hex = oid.to_string();
191 let author = event.author.clone();
192 let timestamp = event.timestamp.clone();
193
194 let (kind, revision) = match &event.action {
195 Action::PatchCreate { .. } | Action::PatchRevision { .. } => {
196 // A revision the fold discarded as a duplicate commit has no
197 // entry here, which is right: it added nothing to the patch.
198 let Some(rev) = revisions.get(&oid_hex) else {
199 continue;
200 };
201 (
202 Kind::Revision {
203 commit: rev.commit.clone(),
204 body: rev.body.clone(),
205 edited: rev.edited,
206 },
207 Some(rev.number),
208 )
209 }
210 Action::PatchComment { .. } => {
211 let Some(c) = comments.get(&oid_hex) else {
212 continue;
213 };
214 (
215 Kind::Comment {
216 body: c.body.clone(),
217 edited: c.edited,
218 deleted: c.deleted,
219 },
220 // Anchored below, once the sequence is in order: a thread
221 // comment carries no revision of its own.
222 None,
223 )
224 }
225 Action::PatchInlineComment { .. } => {
226 let Some(c) = inline.get(&oid_hex) else {
227 continue;
228 };
229 (
230 Kind::InlineComment {
231 file: c.file.clone(),
232 line: c.line,
233 body: c.body.clone(),
234 edited: c.edited,
235 deleted: c.deleted,
236 },
237 c.revision,
238 )
239 }
240 Action::PatchReview { .. } => {
241 // A superseded vote is gone from `patch show`, so it is gone
242 // from here too; the two must not disagree about what stands.
243 let Some(r) = reviews.get(&oid_hex) else {
244 continue;
245 };
246 (
247 Kind::Review {
248 verdict: r.verdict.as_str().to_string(),
249 body: r.body.clone(),
250 edited: r.edited,
251 },
252 r.revision,
253 )
254 }
255 Action::PatchLabel { label } => (
256 Kind::Label {
257 label: label.clone(),
258 },
259 None,
260 ),
261 Action::PatchUnlabel { label } => (
262 Kind::Unlabel {
263 label: label.clone(),
264 },
265 None,
266 ),
267 Action::BodyEdit { target, .. } => {
268 if !authorized(&owners, target, &author) {
269 continue;
270 }
271 (
272 Kind::Edited {
273 target: target.clone(),
274 },
275 None,
276 )
277 }
278 Action::CommentDelete { target } => {
279 if !authorized(&owners, target, &author) {
280 continue;
281 }
282 (
283 Kind::Deleted {
284 target: target.clone(),
285 },
286 None,
287 )
288 }
289 Action::PatchClose { reason } => (
290 Kind::Closed {
291 reason: reason.clone(),
292 },
293 None,
294 ),
295 Action::PatchMerge { commit } => (
296 Kind::Merged {
297 commit: (!commit.is_empty()).then(|| commit.clone()),
298 },
299 None,
300 ),
301 _ => continue,
302 };
303
304 entries.push((
305 event.clock,
306 Entry {
307 kind,
308 revision,
309 author,
310 timestamp,
311 id: oid_hex,
312 },
313 ));
314 }
315
316 // Stable, so events sharing a clock — every event on a pre-clock patch —
317 // keep the DAG's own order rather than being shuffled by the sort.
318 entries.sort_by_key(|(clock, _)| *clock);
319 let mut entries: Vec<Entry> = entries.into_iter().map(|(_, e)| e).collect();
320
321 // Anchor thread comments to the revision that was current where they sit in
322 // the sequence — which is the revision their author was in fact looking at.
323 //
324 // By position, not by timestamp: timestamps come from whichever machine
325 // wrote the event, so comparing two authors' clocks decides nothing, and a
326 // skewed clock would file a comment under the wrong revision. Position in
327 // this sequence is derived from the DAG, which is the same basis the fold
328 // uses to attribute a revision-less review.
329 let mut current = 1;
330 for entry in &mut entries {
331 match (&entry.kind, entry.revision) {
332 (Kind::Revision { .. }, Some(n)) => current = n,
333 (Kind::Comment { .. }, None) => entry.revision = Some(current),
334 _ => {}
335 }
336 }
337
338 Ok((patch, entries))
339 }
340
341 /// Whether `action` carries a body a correction could name.
342 fn carries_a_body(action: &Action) -> bool {
343 matches!(
344 action,
345 Action::PatchCreate { .. }
346 | Action::PatchRevision { .. }
347 | Action::PatchComment { .. }
348 | Action::PatchInlineComment { .. }
349 | Action::PatchReview { .. }
350 )
351 }
352
353 fn authorized(
354 owners: &std::collections::HashMap<String, String>,
355 target: &str,
356 author: &Author,
357 ) -> bool {
358 owners.get(target).is_some_and(|owner| *owner == author.email)
359 }
360
361 /// Render the timeline as one line per event.
362 ///
363 /// One line each, because the value of the view is the shape of the sequence;
364 /// a full body per entry would bury it. `patch show` prints bodies in full.
365 pub fn to_writer(entries: &[Entry], writer: &mut dyn std::io::Write) -> Result<(), Error> {
366 if entries.is_empty() {
367 writeln!(writer, "No events recorded.")?;
368 return Ok(());
369 }
370
371 for entry in entries {
372 let label = entry.kind.label(entry.revision);
373 // The anchor. `patch log` already opens a revision line with `rN`, so
374 // repeating it as `@rN` there would be noise; everything else needs it
375 // to say which revision it is about.
376 let anchor = match (&entry.kind, entry.revision) {
377 (Kind::Revision { .. }, _) | (_, None) => String::new(),
378 (_, Some(n)) => format!("@r{} ", n),
379 };
380
381 // Built as parts and joined, so no arm has to reason about whether the
382 // one before it left a separator behind.
383 let mut parts: Vec<String> = Vec::new();
384 match &entry.kind {
385 Kind::Revision {
386 commit,
387 body,
388 edited,
389 } => {
390 parts.push(if commit.is_empty() {
391 state::UNKNOWN_COMMIT.to_string()
392 } else {
393 commit.chars().take(8).collect()
394 });
395 if entry.revision == Some(1) {
396 parts.push("(initial)".to_string());
397 }
398 parts.extend(body_part(body.as_deref(), *edited, false));
399 }
400 Kind::Comment {
401 body,
402 edited,
403 deleted,
404 } => parts.extend(body_part(Some(body), *edited, *deleted)),
405 Kind::InlineComment {
406 file,
407 line,
408 body,
409 edited,
410 deleted,
411 } => {
412 parts.push(format!("{}:{}", file, line));
413 parts.extend(body_part(Some(body), *edited, *deleted));
414 }
415 Kind::Review {
416 verdict,
417 body,
418 edited,
419 } => {
420 parts.push(verdict.clone());
421 parts.extend(body_part(Some(body), *edited, false));
422 }
423 Kind::Label { label } => parts.push(format!("+{}", label)),
424 Kind::Unlabel { label } => parts.push(format!("-{}", label)),
425 Kind::Edited { target } | Kind::Deleted { target } => {
426 parts.push(format!("{:.8}", target))
427 }
428 Kind::Closed { reason } => parts.extend(reason.clone()),
429 Kind::Merged { commit } => {
430 parts.extend(commit.as_deref().map(|c| format!("{:.8}", c)))
431 }
432 }
433
434 writeln!(
435 writer,
436 "{} {:<8} {} {}{}",
437 entry.timestamp,
438 label,
439 entry.author.name,
440 anchor,
441 parts.join(" ")
442 )?;
443 }
444 Ok(())
445 }
446
447 /// A body reduced to one line, with `(edited)` following the words it changed,
448 /// or the tombstone in place of a deleted one. `None` when there is nothing to
449 /// show — an empty body contributes no trailing whitespace.
450 fn body_part(body: Option<&str>, edited: bool, deleted: bool) -> Option<String> {
451 if deleted {
452 return Some(crate::TOMBSTONE.to_string());
453 }
454 let marker = if edited { " (edited)" } else { "" };
455 match body {
456 Some(b) if !b.trim().is_empty() => {
457 Some(format!("{}{}", crate::patch::summarize_body(b), marker))
458 }
459 // A body edited down to nothing still happened; say so rather than
460 // rendering an entry that looks like it never had one.
461 _ if edited => Some(marker.trim().to_string()),
462 _ => None,
463 }
464 }
465
466 pub fn to_json(entries: &[Entry]) -> Result<String, Error> {
467 Ok(serde_json::to_string_pretty(entries)?)
468 }
tests/sync_test.rs
Old New
@@ -422,7 +422,7 @@ fn test_auto_sync_after_write_covers_every_configured_remote() {
422 assert!(output.status.success()); 422 assert!(output.status.success());
423 let stderr = String::from_utf8(output.stderr).unwrap(); 423 let stderr = String::from_utf8(output.stderr).unwrap();
424 assert!( 424 assert!(
425 stderr.contains("Auto-syncing with 'origin', 'second'..."), 425 stderr.contains("auto-sync: publishing to 'origin', 'second'..."),
426 "stderr: {}", 426 "stderr: {}",
427 stderr 427 stderr
428 ); 428 );
@@ -463,7 +463,7 @@ fn test_auto_sync_remote_config_pins_to_single_remote() {
463 assert!(output.status.success()); 463 assert!(output.status.success());
464 let stderr = String::from_utf8(output.stderr).unwrap(); 464 let stderr = String::from_utf8(output.stderr).unwrap();
465 assert!( 465 assert!(
466 stderr.contains("Auto-syncing with 'second'..."), 466 stderr.contains("auto-sync: publishing to 'second'..."),
467 "stderr: {}", 467 "stderr: {}",
468 stderr 468 stderr
469 ); 469 );
@@ -486,6 +486,148 @@ fn test_auto_sync_remote_config_pins_to_single_remote() {
486 assert!(second_bare_repo.refname_to_id(&issue_ref).is_ok()); 486 assert!(second_bare_repo.refname_to_id(&issue_ref).is_ok());
487 } 487 }
488 488
489 // ---------------------------------------------------------------------------
490 // Auto-sync is a separate operation from the command that triggers it
491 // ---------------------------------------------------------------------------
492 //
493 // A write command records an event locally and then, by default, tries to
494 // publish it. Those are two facts with two different failure modes, and they
495 // used to arrive as one interleaved stream on two streams at once: the banner
496 // on stderr, the fetch/push progress on stdout, mixed into the command's own
497 // result. A reader could not tell which part had failed, or that a network
498 // operation had happened at all.
499
500 /// Register `remote_name` pointing at a path that does not exist, so any
501 /// attempt to reach it fails. Never a real network remote.
502 fn add_unreachable_remote(repo_dir: &std::path::Path, remote_name: &str) {
503 let missing = repo_dir.join("no-such-remote.git");
504 Command::new("git")
505 .args([
506 "remote",
507 "add",
508 remote_name,
509 missing.to_str().unwrap(),
510 ])
511 .current_dir(repo_dir)
512 .status()
513 .unwrap();
514 }
515
516 /// The load-bearing one. The event *was* recorded; only publishing it failed.
517 /// Reporting that as a failed command would be a lie, and a caller checking the
518 /// exit code would retry a write that already happened.
519 #[test]
520 fn test_failed_auto_sync_does_not_fail_the_local_write() {
521 let cluster = TestCluster::new_without_collab_init();
522 add_unreachable_remote(cluster.alice_dir.path(), "broken");
523 cluster.run_collab_ok(cluster.alice_dir.path(), &["init"]);
524
525 // Pin auto-sync to the unreachable remote so the sync can only fail.
526 Command::new("git")
527 .args(["config", "collab.autoSyncRemote", "broken"])
528 .current_dir(cluster.alice_dir.path())
529 .status()
530 .unwrap();
531
532 let output = cluster.run_collab(
533 cluster.alice_dir.path(),
534 &["issue", "open", "-t", "Recorded but not published"],
535 );
536 let stdout = String::from_utf8(output.stdout).unwrap();
537 let stderr = String::from_utf8(output.stderr).unwrap();
538
539 assert_eq!(
540 output.status.code(),
541 Some(0),
542 "a local write that succeeded must exit 0 even when publishing it failed\
543 \nstdout: {}\nstderr: {}",
544 stdout,
545 stderr
546 );
547
548 // The write really did land locally.
549 let issue = state::list_issues(&cluster.alice_repo())
550 .unwrap()
551 .into_iter()
552 .find(|issue| issue.title == "Recorded but not published");
553 assert!(issue.is_some(), "the event must be recorded locally");
554
555 // And the user is told, in as many words, both facts: it is recorded here,
556 // and it did not reach the remote.
557 assert!(
558 stderr.contains("recorded locally"),
559 "a failed auto-sync must say the write is safe locally:\n{}",
560 stderr
561 );
562 assert!(
563 stderr.contains("git-collab sync"),
564 "a failed auto-sync must say how to publish the write:\n{}",
565 stderr
566 );
567 }
568
569 /// Auto-sync narrates itself on stderr, so stdout carries the command's own
570 /// result and nothing else. `--json` exists only on read commands today, which
571 /// never auto-sync — keeping the network chatter off stdout is what stops that
572 /// gap from widening the day a write command grows `--json`.
573 #[test]
574 fn test_auto_sync_output_stays_off_stdout() {
575 let cluster = TestCluster::new_without_collab_init();
576 cluster.run_collab_ok(cluster.alice_dir.path(), &["init"]);
577
578 let output = cluster.run_collab(
579 cluster.alice_dir.path(),
580 &["issue", "open", "-t", "Clean stdout"],
581 );
582 assert!(output.status.success());
583 let stdout = String::from_utf8(output.stdout).unwrap();
584 let stderr = String::from_utf8(output.stderr).unwrap();
585
586 // stdout is the command's own result, whole and alone.
587 assert!(
588 stdout.starts_with("Opened issue "),
589 "stdout: {:?}",
590 stdout
591 );
592 assert_eq!(
593 stdout.lines().count(),
594 1,
595 "no network output belongs on stdout:\nstdout: {}\nstderr: {}",
596 stdout,
597 stderr
598 );
599 for noise in ["Fetching from", "Pushing to", "Sync complete", "Pushed refs/"] {
600 assert!(
601 !stdout.contains(noise),
602 "{:?} is auto-sync's output, not the command's:\nstdout: {}",
603 noise,
604 stdout
605 );
606 }
607
608 // ...and the sync is announced as its own operation, naming the remote it
609 // reaches, before it reaches it.
610 assert!(
611 stderr.contains("auto-sync:"),
612 "auto-sync must label itself as a separate operation:\n{}",
613 stderr
614 );
615 assert!(
616 stderr.contains("'origin'"),
617 "auto-sync must name the remote it pushes to:\n{}",
618 stderr
619 );
620 let announce = stderr
621 .find("auto-sync:")
622 .expect("auto-sync section must be present");
623 let push = stderr.find("Pushing to").unwrap_or(usize::MAX);
624 assert!(
625 announce < push,
626 "the remote must be named before anything is sent to it:\n{}",
627 stderr
628 );
629 }
630
489 #[test] 631 #[test]
490 fn test_bob_comments_on_alice_issue_then_sync() { 632 fn test_bob_comments_on_alice_issue_then_sync() {
491 let cluster = TestCluster::new(); 633 let cluster = TestCluster::new();
tests/timeline_test.rs
Old New
@@ -0,0 +1,444 @@
1 //! `patch log --timeline`: the unified event timeline for a patch.
2 //!
3 //! `patch log` lists revisions; `patch show` lists comments and reviews. Neither
4 //! answers the question a reviewer actually asks — *what answered what*. A
5 //! comment on revision 1 followed by revision 2 is the causal chain this
6 //! project's revision anchoring exists to record, and reconstructing it by
7 //! interleaving two commands' output by eye is exactly the work the tool should
8 //! be doing.
9 //!
10 //! These tests pin three things: that the sequence is rendered in order with
11 //! every event kind in it, that each entry names the revision it is anchored to,
12 //! and that rendering it writes nothing.
13
14 mod common;
15
16 use common::TestRepo;
17
18 /// Build a patch with a full review round-trip on it:
19 ///
20 /// r1 -> comment -> request-changes -> r2 -> approve -> merged
21 ///
22 /// Returns the patch's short ID.
23 fn patch_with_a_review_round(repo: &TestRepo) -> String {
24 repo.git(&["checkout", "-b", "feat-timeline"]);
25 repo.commit_file("a.txt", "first", "v1");
26 let out = repo.run_ok(&["patch", "create", "-t", "Timeline patch", "-B", "feat-timeline"]);
27 let id = out
28 .trim()
29 .strip_prefix("Created patch ")
30 .unwrap()
31 .to_string();
32
33 repo.run_ok(&["patch", "comment", &id, "-b", "needs a test"]);
34 repo.run_ok(&[
35 "patch",
36 "review",
37 &id,
38 "-v",
39 "request-changes",
40 "-b",
41 "not yet",
42 ]);
43
44 repo.git(&["checkout", "feat-timeline"]);
45 repo.commit_file("b.txt", "second", "v2");
46 repo.run_ok(&["patch", "revise", &id, "-b", "added the test"]);
47
48 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "looks good"]);
49 repo.git(&["checkout", "main"]);
50
51 id
52 }
53
54 /// The whole point: one command, every event kind, in the order they happened.
55 #[test]
56 fn timeline_interleaves_revisions_comments_and_reviews_in_order() {
57 let repo = TestRepo::new("Alice", "alice@example.com");
58 let id = patch_with_a_review_round(&repo);
59
60 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
61
62 // Every kind of event is present. `patch log` alone shows only the
63 // revisions; `patch show` alone shows only the comment and the reviews.
64 assert!(out.contains("r1"), "timeline must show revision 1: {}", out);
65 assert!(out.contains("needs a test"), "timeline must show the comment: {}", out);
66 assert!(
67 out.contains("request-changes"),
68 "timeline must show the review verdict: {}",
69 out
70 );
71 assert!(out.contains("r2"), "timeline must show revision 2: {}", out);
72 assert!(out.contains("approve"), "timeline must show the approval: {}", out);
73
74 // ...and in the order they happened. This is the assertion that makes it a
75 // timeline rather than two lists printed one after the other.
76 let at = |needle: &str| {
77 out.find(needle)
78 .unwrap_or_else(|| panic!("{:?} missing from timeline:\n{}", needle, out))
79 };
80 assert!(
81 at("needs a test") < at("request-changes"),
82 "the comment came before the review:\n{}",
83 out
84 );
85 assert!(
86 at("request-changes") < at("added the test"),
87 "revision 2 answered the request for changes and must follow it:\n{}",
88 out
89 );
90 assert!(
91 at("added the test") < at("looks good"),
92 "the approval came after revision 2:\n{}",
93 out
94 );
95 }
96
97 /// A timeline that does not say which revision a comment was made against
98 /// cannot answer "what answered what" — it is just a sorted list.
99 #[test]
100 fn timeline_anchors_each_event_to_its_revision() {
101 let repo = TestRepo::new("Alice", "alice@example.com");
102 let id = patch_with_a_review_round(&repo);
103
104 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
105
106 let line = |needle: &str| {
107 out.lines()
108 .find(|l| l.contains(needle))
109 .unwrap_or_else(|| panic!("{:?} missing from timeline:\n{}", needle, out))
110 };
111
112 assert!(
113 line("not yet").contains("@r1"),
114 "the request-changes review was made against r1: {:?}",
115 line("not yet")
116 );
117 assert!(
118 line("looks good").contains("@r2"),
119 "the approval was made against r2: {:?}",
120 line("looks good")
121 );
122 }
123
124 /// A thread comment carries no revision of its own, so it is anchored to the
125 /// revision that was current where it sits in the sequence. Without that, the
126 /// view answers "what answered what" for reviews only, and a thread comment —
127 /// the most common kind — floats free of the round it belongs to.
128 #[test]
129 fn timeline_anchors_a_thread_comment_to_the_revision_it_was_written_on() {
130 let repo = TestRepo::new("Alice", "alice@example.com");
131 let id = patch_with_a_review_round(&repo);
132
133 // Written after r2 exists, so it belongs to r2, not to r1.
134 repo.run_ok(&["patch", "comment", &id, "-b", "one more thought"]);
135
136 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
137 let line = out
138 .lines()
139 .find(|l| l.contains("one more thought"))
140 .unwrap_or_else(|| panic!("comment missing:\n{}", out));
141 assert!(
142 line.contains("@r2"),
143 "a comment written after r2 is anchored to r2: {:?}",
144 line
145 );
146
147 // And the earlier one is still anchored to r1 — the anchor tracks position
148 // in the sequence, not simply the latest revision.
149 let earlier = out
150 .lines()
151 .find(|l| l.contains("needs a test"))
152 .unwrap_or_else(|| panic!("earlier comment missing:\n{}", out));
153 assert!(
154 earlier.contains("@r1"),
155 "a comment written before r2 stays anchored to r1: {:?}",
156 earlier
157 );
158 }
159
160 /// The merge is the last beat of the story, and for a squash or a rebase-merge
161 /// the recorded commit is the only route from the patch back to the code.
162 #[test]
163 fn timeline_ends_with_the_recorded_merge() {
164 let repo = TestRepo::new("Alice", "alice@example.com");
165 let id = patch_with_a_review_round(&repo);
166
167 repo.git(&["merge", "--no-ff", "-m", "land it", "feat-timeline"]);
168 let landed = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
169 repo.run_ok(&["patch", "merge", &id]);
170
171 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
172 let last = out.lines().rfind(|l| !l.trim().is_empty()).unwrap();
173 assert!(
174 last.contains("merged"),
175 "the timeline ends with the merge: {:?}",
176 last
177 );
178 assert!(
179 last.contains(&landed[..8]),
180 "the merge entry names the commit that landed the patch: {:?}",
181 last
182 );
183 }
184
185 /// Scripted callers need the same sequence without parsing columns.
186 #[test]
187 fn timeline_json_carries_the_same_sequence() {
188 let repo = TestRepo::new("Alice", "alice@example.com");
189 let id = patch_with_a_review_round(&repo);
190
191 let out = repo.run_ok(&["patch", "log", &id, "--timeline", "--json"]);
192 let entries: Vec<serde_json::Value> = serde_json::from_str(&out).unwrap();
193
194 let kinds: Vec<&str> = entries
195 .iter()
196 .map(|e| e["type"].as_str().unwrap())
197 .collect();
198 assert_eq!(
199 kinds,
200 vec!["revision", "comment", "review", "revision", "review"],
201 "timeline JSON: {}",
202 out
203 );
204
205 assert_eq!(entries[0]["revision"], 1);
206 assert_eq!(entries[1]["revision"], 1, "the comment was made against r1");
207 assert_eq!(entries[2]["revision"], 1, "the review was made against r1");
208 assert_eq!(entries[3]["revision"], 2);
209 assert_eq!(entries[4]["revision"], 2, "the approval was made against r2");
210
211 assert_eq!(entries[2]["verdict"], "request-changes");
212 assert_eq!(entries[4]["verdict"], "approve");
213 assert_eq!(entries[1]["author"]["email"], "alice@example.com");
214 }
215
216 /// The governing constraint of this project: surfacing recorded facts is a pure
217 /// read. A view that appends an event or moves a ref corrupts the record it
218 /// claims to be showing.
219 ///
220 /// Note `patch show` deliberately moves `refs/collab/local/seen/patches/<id>`,
221 /// a purely local read-marker that is not a collab event. The timeline does
222 /// *not* extend that pattern: it moves nothing at all, which is what the
223 /// `refs/collab/` sweep below pins.
224 #[test]
225 fn rendering_a_timeline_writes_nothing() {
226 let repo = TestRepo::new("Alice", "alice@example.com");
227 let id = patch_with_a_review_round(&repo);
228 repo.git(&["merge", "--no-ff", "-m", "land it", "feat-timeline"]);
229 repo.run_ok(&["patch", "merge", &id]);
230
231 let refs_before = repo.git(&["for-each-ref", "--format=%(refname) %(objectname)"]);
232
233 repo.run_ok(&["patch", "log", &id, "--timeline"]);
234 repo.run_ok(&["patch", "log", &id, "--timeline", "--json"]);
235
236 let refs_after = repo.git(&["for-each-ref", "--format=%(refname) %(objectname)"]);
237 assert_eq!(
238 refs_before, refs_after,
239 "rendering a timeline must not create, move or delete any ref"
240 );
241 }
242
243 // ---------------------------------------------------------------------------
244 // Corrections
245 // ---------------------------------------------------------------------------
246 //
247 // A `BodyEdit` supersedes an earlier body and a `CommentDelete` tombstones one.
248 // Both are events in the DAG, appended rather than applied in place, precisely
249 // so the record stays an audit trail rather than a summary. A timeline that
250 // quietly showed the corrected text at the original position and nothing else
251 // would turn it back into a summary — so a correction gets its own slot, at the
252 // point in the sequence where it actually happened.
253
254 /// The 8-char event ID `patch show` prints for the first thread comment.
255 fn first_comment_id(repo: &TestRepo, patch: &str) -> String {
256 let show = repo.run_ok(&["patch", "show", patch]);
257 let comments = show
258 .split_once("--- Comments ---")
259 .unwrap_or_else(|| panic!("no comments section in:\n{}", show))
260 .1;
261 let start = comments.find('[').unwrap() + 1;
262 let end = comments[start..].find(']').unwrap() + start;
263 comments[start..end].to_string()
264 }
265
266 #[test]
267 fn timeline_records_an_edit_as_its_own_event() {
268 let repo = TestRepo::new("Alice", "alice@example.com");
269 let id = patch_with_a_review_round(&repo);
270
271 let comment_id = first_comment_id(&repo, &id);
272 repo.run_ok(&[
273 "patch",
274 "edit-comment",
275 &id,
276 &comment_id,
277 "-b",
278 "needs a test, and a changelog entry",
279 ]);
280
281 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
282
283 // The comment keeps its original slot, carrying the corrected text and
284 // saying that it was corrected.
285 let comment_line = out
286 .lines()
287 .find(|l| l.contains("needs a test, and a changelog entry"))
288 .unwrap_or_else(|| panic!("corrected comment missing:\n{}", out));
289 assert!(
290 comment_line.contains("(edited)"),
291 "a corrected comment must say so: {:?}",
292 comment_line
293 );
294
295 // ...and the correction is its own entry, later in the sequence, naming
296 // the comment it corrected.
297 let edit_line = out
298 .lines()
299 .find(|l| l.contains("edited") && l.contains(&comment_id))
300 .unwrap_or_else(|| panic!("the edit itself is missing from the timeline:\n{}", out));
301 assert!(
302 out.find(comment_line).unwrap() < out.find(edit_line).unwrap(),
303 "the edit happened after the comment it corrected:\n{}",
304 out
305 );
306 }
307
308 #[test]
309 fn timeline_keeps_a_deleted_comments_slot_and_records_the_deletion() {
310 let repo = TestRepo::new("Alice", "alice@example.com");
311 let id = patch_with_a_review_round(&repo);
312
313 let comment_id = first_comment_id(&repo, &id);
314 repo.run_ok(&["patch", "delete-comment", &id, &comment_id]);
315
316 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
317
318 assert!(
319 !out.contains("needs a test"),
320 "a tombstone drops the text, it does not merely hide it:\n{}",
321 out
322 );
323 assert!(
324 out.contains("[deleted]"),
325 "the deleted comment keeps its slot, marked as a tombstone:\n{}",
326 out
327 );
328 let delete_line = out
329 .lines()
330 .find(|l| l.contains("deleted") && l.contains(&comment_id))
331 .unwrap_or_else(|| panic!("the deletion itself is missing:\n{}", out));
332 assert!(
333 out.find("[deleted]").unwrap() < out.find(delete_line).unwrap(),
334 "the deletion happened after the comment it removed:\n{}",
335 out
336 );
337 }
338
339 /// The fold ignores a correction whose author is not the author of the event it
340 /// names — anyone holding the DAG can append anything to it, so the rule that
341 /// nobody rewrites somebody else's words holds where state is derived.
342 ///
343 /// The timeline has to honour the same rule. Listing every `BodyEdit` event it
344 /// finds would show a forged edit to someone else's words as though it had
345 /// happened, which is worse than not having a timeline at all.
346 #[test]
347 fn timeline_ignores_a_correction_the_fold_refused() {
348 use git_collab::dag;
349 use git_collab::event::Action;
350
351 // The patch's comments are Bob's, so an edit authored by Alice — which is
352 // what `write_raw_event` signs — is one author correcting another.
353 let repo = TestRepo::new("Bob", "bob@example.com");
354 let id = patch_with_a_review_round(&repo);
355
356 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
357 let events_ref = format!(
358 "refs/collab/patches/{}/events",
359 git_repo
360 .references_glob("refs/collab/patches/*/events")
361 .unwrap()
362 .filter_map(|r| r.ok())
363 .find_map(|r| {
364 let name = r.name()?.to_string();
365 let full = name.strip_prefix("refs/collab/patches/")?.strip_suffix("/events")?;
366 full.starts_with(&id).then(|| full.to_string())
367 })
368 .expect("patch events ref")
369 );
370
371 // The OID of Bob's thread comment: what a forged edit would name.
372 let target = dag::walk_events(&git_repo, &events_ref)
373 .unwrap()
374 .into_iter()
375 .find(|(_, e)| matches!(e.action, Action::PatchComment { .. }))
376 .map(|(oid, _)| oid.to_string())
377 .expect("thread comment event");
378
379 let tip = git_repo.refname_to_id(&events_ref).unwrap();
380 let forged = common::write_raw_event(
381 &git_repo,
382 Some(tip),
383 serde_json::json!({
384 "type": "body.edit",
385 "target": target,
386 "body": "words Bob never wrote",
387 }),
388 99,
389 );
390 git_repo
391 .reference(&events_ref, forged, true, "forged edit")
392 .unwrap();
393
394 let out = repo.run_ok(&["patch", "log", &id, "--timeline"]);
395 assert!(
396 !out.contains("words Bob never wrote"),
397 "a refused correction must not appear in the timeline:\n{}",
398 out
399 );
400 assert!(
401 out.contains("needs a test"),
402 "the original comment stands unchanged:\n{}",
403 out
404 );
405 assert!(
406 !out.contains("(edited)"),
407 "nothing was in fact edited:\n{}",
408 out
409 );
410 }
411
412 /// `patch log` and `patch log --json` are consumed by scripts and by the
413 /// existing suite. The timeline is strictly additive: without the flag, nothing
414 /// about either changes.
415 #[test]
416 fn timeline_flag_does_not_change_default_patch_log_output() {
417 let repo = TestRepo::new("Alice", "alice@example.com");
418 let id = patch_with_a_review_round(&repo);
419
420 let plain = repo.run_ok(&["patch", "log", &id]);
421 assert!(plain.contains("r1"), "{}", plain);
422 assert!(plain.contains("r2"), "{}", plain);
423 assert!(
424 !plain.contains("needs a test"),
425 "default patch log stays a revision log, with no comments in it: {}",
426 plain
427 );
428 assert!(
429 !plain.contains("request-changes"),
430 "default patch log stays a revision log, with no reviews in it: {}",
431 plain
432 );
433
434 // Default --json is still the bare revision array.
435 let json = repo.run_ok(&["patch", "log", &id, "--json"]);
436 let revisions: Vec<serde_json::Value> = serde_json::from_str(&json).unwrap();
437 assert_eq!(revisions.len(), 2);
438 assert_eq!(revisions[0]["number"], 1);
439 assert!(
440 revisions[0].get("type").is_none(),
441 "default --json keeps the revision shape, not the timeline shape: {}",
442 json
443 );
444 }