e921ed54

Address review: label parity in list_to_writer, cache version bump

a73x 2026-08-10 05:57

Commit message

Address review: label parity in list_to_writer, cache version bump

- patch::list_to_writer threaded the --label filter but never rendered
  the ` [a, b]` suffix, silently dropping labels on the one path that
  bypasses lib.rs's own rendering. Fixed by extracting the suffix logic
  into cli::label_suffix() and using it at all four call sites
  (issue.rs, lib.rs x2, patch.rs), removing the duplication rather than
  adding a fourth divergent copy.
- Bumped CACHE_FORMAT_VERSION to 4. serde(default) on PatchState.labels
  is what lets old JSON deserialize, not what keeps the fold cache
  correct -- a stale v3 entry missing the key deserializes identically
  whether or not a patch.label event was later folded into that ref, so
  without the bump a stale entry would be served as a silent-loss cache
  hit. Reworded the field comment to stop implying serde(default) alone
  settles this, and added a regression test that reproduces the exact
  silent-loss scenario (confirmed red before the bump, green after).
- Added a test isolating the --all list-filter axis from --archived,
  using an auto-detected merge (active, non-open, never archived) since
  patch close conflates the two by always archiving.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

src/cache.rs

Old		New
@@ -19,8 +19,11 @@ fn sanitize_ref_name(ref_name: &str) -> String {
19	/// entries computed with older logic are discarded even if the ref tip	19	/// entries computed with older logic are discarded even if the ref tip
20	/// hasn't moved. v2: review vote supersession per (author, revision).	20	/// hasn't moved. v2: review vote supersession per (author, revision).
21	/// v3: `IssueState::relates_to` became a multi-member set instead of a	21	/// v3: `IssueState::relates_to` became a multi-member set instead of a
22	/// single optional value.	22	/// single optional value. v4: `PatchState` gained a `labels` field; a
23	const CACHE_FORMAT_VERSION: u32 = 3;	23	/// stale v3 entry would deserialize fine (the field is `#[serde(default)]`)
		24	/// but silently omit any labels folded in since, so it must be rejected by
		25	/// version rather than relying on that default to catch it.
		26	const CACHE_FORMAT_VERSION: u32 = 4;
24		27
25	/// Cache entry stored on disk: the tip OID at cache time + serialized state.	28	/// Cache entry stored on disk: the tip OID at cache time + serialized state.
26	#[derive(serde::Serialize, serde::Deserialize)]	29	#[derive(serde::Serialize, serde::Deserialize)]

src/cli.rs

@@ @@ -53,6 +53,41 @@ pub fn filter_sort_paginate<T: Listable>( @@
+        .collect()
+/// The `" [a, b]"` suffix list output appends after an item's title when it
+/// carries labels, or `""` when it carries none. Shared so `issue list` and
+/// `patch list` (both their CLI rendering and their `list_to_writer` twins)
+/// render labels identically rather than each re-deriving the same suffix.
+pub fn label_suffix(labels: &[String]) -> String {
+    if labels.is_empty() {
+        String::new()
+    } else {
+        format!(" [{}]", labels.join(", "))
+    }
+}
+#[cfg(test)]
+mod tests {
+    use super::*;
+    #[test]
+    fn label_suffix_empty_is_blank() {
+        assert_eq!(label_suffix(&[]), "");
+    }
+    #[test]
+    fn label_suffix_one_label() {
+        assert_eq!(label_suffix(&["bug".to_string()]), " [bug]");
+    }
+    #[test]
+    fn label_suffix_multiple_labels_joined_with_comma_space() {
+        assert_eq!(
+            label_suffix(&["bug".to_string(), "priority".to_string()]),
+            " [bug, priority]"
+        );
+    }
+}
+#[derive(Parser)]
+#[command(
+    name = "git-collab",

src/issue.rs

Old		New
@@ -81,11 +81,7 @@ pub fn list_to_writer(
81	for e in &entries {	81	for e in &entries {
82	let i = &e.issue;	82	let i = &e.issue;
83	let status = i.status.as_str();	83	let status = i.status.as_str();
84	let labels = if i.labels.is_empty() {	84	let labels = cli::label_suffix(&i.labels);
85	String::new()
86	} else {
87	format!(" [{}]", i.labels.join(", "))
88	};
89	let unread = match e.unread {	85	let unread = match e.unread {
90	Some(n) if n > 0 => format!(" ({} new)", n),	86	Some(n) if n > 0 => format!(" ({} new)", n),
91	_ => String::new(),	87	_ => String::new(),

src/lib.rs

Old		New
@@ -143,11 +143,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
143	for e in &entries {	143	for e in &entries {
144	let i = &e.issue;	144	let i = &e.issue;
145	let status = i.status.as_str();	145	let status = i.status.as_str();
146	let labels = if i.labels.is_empty() {	146	let labels = cli::label_suffix(&i.labels);
147	String::new()
148	} else {
149	format!(" [{}]", i.labels.join(", "))
150	};
151	let unread = match e.unread {	147	let unread = match e.unread {
152	Some(n) if n > 0 => format!(" ({} new)", n),	148	Some(n) if n > 0 => format!(" ({} new)", n),
153	_ => String::new(),	149	_ => String::new(),
@@ -361,11 +357,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
361	} else {	357	} else {
362	for e in &entries {	358	for e in &entries {
363	let p = &e.patch;	359	let p = &e.patch;
364	let labels = if p.labels.is_empty() {	360	let labels = cli::label_suffix(&p.labels);
365	String::new()
366	} else {
367	format!(" [{}]", p.labels.join(", "))
368	};
369	let stale = match p.staleness(repo) {	361	let stale = match p.staleness(repo) {
370	Ok((_, behind)) if behind > 0 => format!(" [behind {}]", behind),	362	Ok((_, behind)) if behind > 0 => format!(" [behind {}]", behind),
371	Ok(_) => String::new(),	363	Ok(_) => String::new(),

src/patch.rs

Old		New
@@ -150,6 +150,7 @@ pub fn list_to_writer(
150	}	150	}
151	for e in &entries {	151	for e in &entries {
152	let p = &e.patch;	152	let p = &e.patch;
		153	let labels = cli::label_suffix(&p.labels);
153	let stale = match p.staleness(repo) {	154	let stale = match p.staleness(repo) {
154	Ok((_, behind)) if behind > 0 => format!(" [behind {}]", behind),	155	Ok((_, behind)) if behind > 0 => format!(" [behind {}]", behind),
155	Ok(_) => String::new(),	156	Ok(_) => String::new(),
@@ -161,8 +162,8 @@ pub fn list_to_writer(
161	};	162	};
162	writeln!(	163	writeln!(
163	writer,	164	writer,
164	"{:.8} {:6} {} (by {}){}{}",	165	"{:.8} {:6} {}{} (by {}){}{}",
165	p.id, p.status, p.title, p.author.name, stale, unread	166	p.id, p.status, p.title, labels, p.author.name, stale, unread
166	)	167	)
167	.ok();	168	.ok();
168	}	169	}

src/state.rs

Old		New
@@ -238,7 +238,14 @@ pub struct PatchState {
238	/// revision refs, so an ephemeral or rewritten branch costs it nothing.	238	/// revision refs, so an ephemeral or rewritten branch costs it nothing.
239	pub branch: String,	239	pub branch: String,
240	/// Postdates patch labelling: absent on any `PatchState` serialized	240	/// Postdates patch labelling: absent on any `PatchState` serialized
241	/// before this field existed, which must load as an empty vec.	241	/// before this field existed, which must load as an empty vec. This
		242	/// `#[serde(default)]` is what makes such JSON deserialize at all; it is
		243	/// not what keeps the on-disk fold cache correct. A stale cache entry
		244	/// missing this key would deserialize just as cleanly whether or not it
		245	/// predates a `patch.label` event actually folded into that ref -- so
		246	/// the cache still needs `cache::CACHE_FORMAT_VERSION` bumped alongside
		247	/// this field to force a refold instead of quietly serving a
		248	/// labels-empty hit.
242	#[serde(default)]	249	#[serde(default)]
243	pub labels: Vec<String>,	250	pub labels: Vec<String>,
244	pub comments: Vec<Comment>,	251	pub comments: Vec<Comment>,

tests/cache_test.rs

@@ @@ -217,3 +217,46 @@ fn cache_entry_from_older_format_is_a_miss() { @@
+    // Stale-format entries must not be served
+    assert!(cache::get_cached_state::<IssueState>(&repo, &ref_name).is_none());
+#[test]
+fn stale_v3_patch_cache_entry_missing_labels_is_a_miss() {
+    // A v3 `PatchState` cache entry predates patch labelling and has no
+    // `labels` key. Because `PatchState::labels` is `#[serde(default)]`,
+    // such an entry deserializes cleanly on its own -- so if the cache
+    // format were not bumped, a stale v3 entry left behind at a tip that
+    // *now* contains a `patch.label` event would be served as a hit with
+    // the label silently dropped, rather than forcing a refold. Bumping
+    // CACHE_FORMAT_VERSION to 4 closes that: any v3-tagged entry is
+    // rejected outright, regardless of which fields it happens to have.
+    let dir = TempDir::new().unwrap();
+    let repo = init_repo(dir.path(), &alice());
+    let (ref_name, id) = create_patch(&repo, &alice(), "Cache label test");
+    git_collab::patch::label(&repo, &id, "bug").unwrap();
+    // Populate the cache at the current (post-label) tip.
+    let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
+    assert_eq!(state.labels, vec!["bug".to_string()]);
+    // Simulate a v3 entry (predating labels) surviving at this same tip:
+    // same tip_oid, version rolled back to 3, and the `labels` key removed
+    // from the serialized state -- exactly what a v3 fold would have written.
+    let path = repo
+        .path()
+        .join("collab")
+        .join("cache")
+        .join(ref_name.replace('/', "_"));
+    let data = std::fs::read_to_string(&path).unwrap();
+    let mut entry: serde_json::Value = serde_json::from_str(&data).unwrap();
+    entry["version"] = serde_json::json!(3);
+    entry["state"].as_object_mut().unwrap().remove("labels");
+    std::fs::write(&path, serde_json::to_string(&entry).unwrap()).unwrap();
+    // A v3-tagged entry must be a miss -- not served with the label
+    // silently missing.
+    assert!(cache::get_cached_state::<PatchState>(&repo, &ref_name).is_none());
+    // A fresh from_ref() refolds from the DAG and recovers the label.
+    let refolded = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
+    assert_eq!(refolded.labels, vec!["bug".to_string()]);
+}

tests/cli_test.rs

@@ @@ -726,6 +726,44 @@ fn test_patch_list_label_composes_with_all_archived() { @@
+#[test]
+fn test_patch_list_label_all_flag_isolated_from_archived() {
+    // The test above only exercises "neither flag" vs "both flags" -- the
+    // one pair that cannot tell --all and --archived apart, since `patch
+    // close` always archives the ref. There is no way to reach an "open but
+    // archived" patch (archiving happens only inside `close`), so that half
+    // of the pair genuinely cannot be isolated under current semantics.
+    //
+    // But the other half can: a patch auto-detected as Merged (its branch
+    // fast-forwarded into base *outside* git-collab, which
+    // `PatchState::check_auto_merge` picks up on read) becomes non-open
+    // without ever being archived -- archiving is something only `close`
+    // does. That gives a labeled, non-open, still-*active* patch, which
+    // isolates --all: visible with --all alone, hidden without it, and
+    // --archived is irrelevant either way since it was never archived.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "merge-me"]);
+    repo.commit_file("merge-me.txt", "content", "commit for merge-me");
+    let out = repo.run_ok(&["patch", "create", "-t", "Merged labeled", "-B", "merge-me"]);
+    let id = out
+        .trim()
+        .strip_prefix("Created patch ")
+        .unwrap()
+        .to_string();
+    repo.run_ok(&["patch", "label", &id, "bug"]);
+    // Simulate `git merge merge-me` on main, outside of git-collab.
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "merge-me"]);
+    let out = repo.run_ok(&["patch", "list", "--label", "bug"]);
+    assert!(!out.contains("Merged labeled"));
+    let out = repo.run_ok(&["patch", "list", "--all", "--label", "bug"]);
+    assert!(out.contains("Merged labeled"));
+}
+#[test]
+fn test_patch_list_label_limit_offset_applies_after_filter() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let a = repo.patch_create("Alpha patch");

tests/collab_test.rs

@@ @@ -1456,6 +1456,26 @@ fn test_patch_list_offset_beyond_end() { @@
+    );
+#[test]
+fn test_patch_list_to_writer_renders_labels() {
+    // patch::list_to_writer threaded the `labels` filter parameter through
+    // but, unlike issue::list_to_writer, did not render the ` [a, b]` suffix
+    // -- the one place a patch's labels were silently dropped. Regression
+    // test for that parity gap.
+    let tmp = TempDir::new().unwrap();
+    let repo = init_repo(tmp.path(), &alice());
+    let (_ref_name, id) = create_patch(&repo, &alice(), "Labeled patch");
+    patch::label(&repo, &id, "bug").unwrap();
+    let output = capture_patch_list(&repo, false, None, None);
+    assert!(
+        output.contains("[bug]"),
+        "expected labels rendered in list_to_writer output, got: {}",
+        output
+    );
+}
+// ---------------------------------------------------------------------------
+// JSON serialization tests
+// ---------------------------------------------------------------------------