f9785c84

Diagnose refname conflicts instead of advising a retry

a73x 2026-08-10 10:08

Commit message

Diagnose refname conflicts instead of advising a retry

A remote still holding the pre-migration bare `refs/collab/patches/<id>`
cannot accept `<id>/events`, and never will: git will not let one name be
both a ref and a directory. Sync reported this as a retryable failure and
pointed at a binary that does not exist, so the one thing that actually
fixes it -- a deletion on the remote -- appeared nowhere.

Classify the failure and print the remedy instead, scoped to the ids that
actually conflicted rather than a glob over every patch ref. Non-conflict
failures keep the retry line, now spelled `git-collab`. Both messages can
appear together; the retry count covers only the retryable refs.

Also makes `sync::init` idempotent (fixes 7824bd7b). It called
remote_add_fetch unconditionally, appending a duplicate collab refspec on
every run while claiming to have configured something new. It now reuses
the predicate `collab_remotes` already had, and says "already configured".

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

src/sync.rs

+// Summary printing (T012)
+// ---------------------------------------------------------------------------
+const PATCH_REF_PREFIX: &str = "refs/collab/patches/";
+/// The patch id whose stale remote ref is blocking this push, if that is what
+/// went wrong. `None` for every other kind of failure, which then keeps the
+/// ordinary retry advice.
+///
+/// The push path shells out to `git` (see `push_ref_single`), so what lands in
+/// `RefPushResult::error` is git's own stderr, not a libgit2 message. Both
+/// markers below were read off a real rejection rather than guessed:
+///
+///   remote: error: cannot lock ref 'refs/collab/patches/<id>/events':
+///           'refs/collab/patches/<id>' exists; cannot create '.../events'
+///    ! [remote rejected] .../events -> .../events (refname conflict)
+///
+/// `(refname conflict)` is receive-pack's own name for this rejection reason,
+/// and `exists; cannot create` is the underlying directory/file conflict
+/// message from the ref backend. Either alone is conclusive; both are matched
+/// because the first is a comparatively recent addition to git's per-ref
+/// reason codes and the second has been stable for far longer.
+///
+/// Matching has to be this narrow. A stale lock file on the remote also
+/// reports `cannot lock ref`, but reads `Unable to create '...events.lock':
+/// File exists.` and is rejected as `(reference already exists)` — a genuinely
+/// transient failure that *should* keep the retry advice. Keying off
+/// `cannot lock ref`, or off a bare `exists`, would swallow it.
+fn refname_conflict_patch_id(result: &RefPushResult) -> Option<String> {
+    let error = result.error.as_deref()?;
+    if !error.contains("(refname conflict)") && !error.contains("exists; cannot create") {
+        return None;
+    }
+    // Only patch refs changed layout, so only they can collide this way.
+    // Anything else matching the text is a situation we have not diagnosed and
+    // must not hand a destructive remedy for.
+    let id = result
+        .ref_name
+        .strip_prefix(PATCH_REF_PREFIX)?
+        .split('/')
+        .next()?;
+    validate_collab_ref_id(id).ok()?;
+    Some(id.to_string())
+}
+/// Explain a refname conflict and print the command that clears it.
+///
+/// Worth being emphatic about, because everything else about this failure
+/// looks benign: `git push --dry-run` reports `[new reference]` for the very
+/// push that is about to be rejected, and the fetch path already understands
+/// the legacy layout, so every read looks healthy while only writes are wedged.
+fn print_refname_conflict_advice(remote: &str, ids: &[String]) {
+    let stale: Vec<String> = ids
+        .iter()
+        .map(|id| format!("{}{}", PATCH_REF_PREFIX, id))
+        .collect();
+    eprintln!(
+        "\nRefname conflict: '{}' still has the pre-migration patch ref layout.",
+        remote
+    );
+    eprintln!(
+        "\nPatch refs moved from a single `{}<id>` to `<id>/events` plus\n\
+         `<id>/rev/<oid>`. Git cannot hold both a ref and a directory at the same\n\
+         path, so while the old ref is still on the remote every push of the new\n\
+         layout is rejected. Retrying cannot clear this — it will fail identically\n\
+         on every future sync until the stale ref is deleted on the remote.",
+        PATCH_REF_PREFIX
+    );
+    // One conflicting patch is by far the common case; reading "1 stale
+    // remote ref(s) ... delete them" in the middle of a destructive
+    // instruction just makes the reader wonder what else they were not told.
+    let (count, them, refs_above) = if stale.len() == 1 {
+        ("1 stale remote ref".to_string(), "it", "the ref")
+    } else {
+        (
+            format!("{} stale remote refs", stale.len()),
+            "them",
+            "the refs",
+        )
+    };
+    eprintln!("\nBlocked by {}:", count);
+    for name in &stale {
+        eprintln!("  {}", name);
+    }
+    eprintln!("\nTo fix, delete {} on the remote and sync again:\n", them);
+    eprintln!("  git push {} --delete {}", remote, stale.join(" "));
+    eprintln!(
+        "\nThat deletes only {} listed above, and only on '{}'. Nothing local is\n\
+         touched: the next sync republishes the same events under `<id>/events`,\n\
+         which is where every current version reads them from.",
+        refs_above, remote
+    );
+}
+/// Print a failure summary to stderr.
+fn print_sync_summary(result: &SyncResult) {
+    let succeeded = result.succeeded().len();
@@ @@ -272,11 +367,38 @@ fn print_sync_summary(result: &SyncResult) { @@
+        );
+    // A single stale bare ref blocks every ref in that patch's subtree —
+    // `<id>/events` and each `<id>/rev/<oid>` alike — so the same id arrives
+    // here several times over. Deduplicate, or the remedy would list the same
+    // deletion repeatedly and overstate how much is being removed. Order
+    // follows the push order so the output is stable.
+    let mut conflicted: Vec<String> = Vec::new();
+    let mut retryable = 0usize;
+    for f in &failed {
+        match refname_conflict_patch_id(f) {
+            Some(id) => {
+                if !conflicted.contains(&id) {
+                    conflicted.push(id);
+                }
+            }
+            None => retryable += 1,
+        }
+    }
+    if !conflicted.is_empty() {
+        print_refname_conflict_advice(&result.remote, &conflicted);
+    }
+    // Printed alongside the conflict advice, never instead of it: a sync can
+    // fail for both reasons at once, and each needs its own answer. Counting
+    // only the retryable refs keeps this line honest — retrying the conflicted
+    // ones is exactly what the advice above says not to do.
+    if retryable > 0 {
+        eprintln!(
+            "\nRun `git-collab sync --remote {}` again to retry {} failed ref(s).",
+            result.remote, retryable
+        );
+    }
+/// Clean up refs/collab/sync/* refs.
+    Ok(())
+/// Whether `remote_name` already carries a collab fetch refspec.
+///
+/// The same predicate `collab_remotes` selects on, factored out so the two
+/// cannot drift: if `init` decided a remote was unconfigured on a basis
+/// `collab_remotes` disagreed with, `init` would append a duplicate refspec on
+/// every run while `sync_all` kept reporting the remote as already configured.
+fn has_collab_refspec(repo: &Repository, remote_name: &str) -> Result<bool, Error> {
+    let remote = repo.find_remote(remote_name)?;
+    Ok(remote
+        .refspecs()
+        .any(|rs| rs.direction() == git2::Direction::Fetch && rs.src() == Some("refs/collab/*")))
+}
+/// Add collab refspecs to all remotes.
+///
+/// Idempotent: `git remote_add_fetch` appends unconditionally, so without this
+/// check a second `init` silently gives the remote a duplicate refspec — and
+/// says "Configured remote" as though it had done something new, which is how
+/// a repo ends up fetching every collab ref twice with nothing to indicate it.
+pub fn init(repo: &Repository) -> Result<(), Error> {
+    let remotes = repo.remotes()?;
+    if remotes.is_empty() {
@@ @@ -311,6 +451,10 @@ pub fn init(repo: &Repository) -> Result<(), Error> { @@
+        return Ok(());
+    for remote_name in remotes.iter().flatten() {
+        if has_collab_refspec(repo, remote_name)? {
+            println!("Remote '{}' already configured", remote_name);
+            continue;
+        }
+        let fetch_spec = format!("+refs/collab/*:refs/collab/sync/{}/*", remote_name);
+        repo.remote_add_fetch(remote_name, &fetch_spec)?;
+        println!("Configured remote '{}'", remote_name);
+pub fn collab_remotes(repo: &Repository) -> Result<Vec<String>, Error> {
+    let mut names = Vec::new();
+    for remote_name in repo.remotes()?.iter().flatten() {
+        if has_collab_refspec(repo, remote_name)? {
+            names.push(remote_name.to_string());

tests/sync_diagnostics_test.rs

@@ @@ -0,0 +1,280 @@ @@
+//! Sync diagnostics: telling the user something true about a failed push,
+//! and not lying to them about `init` having done something.
+//!
+//! Both tests here drive the real CLI against a real local bare remote. The
+//! refname-conflict case in particular is built end to end — an actual stale
+//! bare ref planted on the remote, an actual `git push` rejected by an actual
+//! `receive-pack` — because the whole point of the diagnostic is that it keys
+//! off git's own wording. A test that fed a hand-written error string into the
+//! classifier would only prove the classifier matches its own constant, and
+//! would keep passing on the day git rephrases the rejection.
+mod common;
+use std::process::Command;
+use tempfile::TempDir;
+use common::TestRepo;
+// ---------------------------------------------------------------------------
+// Harness
+// ---------------------------------------------------------------------------
+/// A `TestRepo` with a local bare `origin`, with collab refspecs configured.
+/// Never a real network remote. The returned `TempDir` owns the bare repo —
+/// keep it alive for the duration of the test.
+fn repo_with_origin() -> (TestRepo, TempDir) {
+    let bare = TempDir::new().unwrap();
+    let status = Command::new("git")
+        .args(["init", "--bare", "-b", "main"])
+        .arg(bare.path())
+        .status()
+        .unwrap();
+    assert!(status.success());
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["remote", "add", "origin", bare.path().to_str().unwrap()]);
+    repo.git(&["push", "-u", "origin", "main"]);
+    repo.run_ok(&["init"]);
+    (repo, bare)
+}
+/// The full 40-char ids of every patch in the repo, deduplicated. `patch
+/// create` only prints the 8-char short form, and the ref names a conflict has
+/// to talk about are full ids.
+fn patch_ids(repo: &TestRepo) -> Vec<String> {
+    let refs = repo.git(&[
+        "for-each-ref",
+        "--format=%(refname)",
+        "refs/collab/patches/",
+    ]);
+    let mut ids: Vec<String> = Vec::new();
+    for name in refs.lines() {
+        let Some(rest) = name.strip_prefix("refs/collab/patches/") else {
+            continue;
+        };
+        let Some(id) = rest.split('/').next() else {
+            continue;
+        };
+        if !ids.iter().any(|existing| existing == id) {
+            ids.push(id.to_string());
+        }
+    }
+    ids
+}
+/// The full id of the single patch in the repo, asserting there is exactly one.
+fn only_patch_id(repo: &TestRepo) -> String {
+    let ids = patch_ids(repo);
+    assert_eq!(ids.len(), 1, "expected exactly one patch, found: {:?}", ids);
+    ids.into_iter().next().unwrap()
+}
+/// Plant the pre-migration layout on the remote: a bare `refs/collab/patches/<id>`
+/// holding the event DAG, exactly as a remote that has never been migrated
+/// still carries it. This is what makes the new `<id>/events` unpushable.
+fn plant_legacy_bare_ref(repo: &TestRepo, id: &str) {
+    repo.git(&[
+        "push",
+        "origin",
+        &format!(
+            "refs/collab/patches/{}/events:refs/collab/patches/{}",
+            id, id
+        ),
+    ]);
+}
+// ---------------------------------------------------------------------------
+// Fix 1 — a refname conflict is not retryable (issue 866b28c5)
+// ---------------------------------------------------------------------------
+#[test]
+fn refname_conflict_is_diagnosed_not_reported_as_retryable() {
+    let (repo, _bare) = repo_with_origin();
+    repo.patch_create("Needs a migrated remote");
+    let id = only_patch_id(&repo);
+    plant_legacy_bare_ref(&repo, &id);
+    let stderr = repo.run_err(&["sync", "--remote", "origin"]);
+    // The retry advice must be gone: retrying can never clear this.
+    assert!(
+        !stderr.contains("again to retry"),
+        "refname conflict was reported as retryable:\n{}",
+        stderr
+    );
+    // And the old wrong binary name must not appear anywhere.
+    assert!(
+        !stderr.contains("`collab sync"),
+        "stderr uses the wrong binary name `collab`:\n{}",
+        stderr
+    );
+    // It must name the condition, the specific patch, and the exact remedy.
+    assert!(
+        stderr.contains("refname conflict"),
+        "diagnostic does not name the condition:\n{}",
+        stderr
+    );
+    assert!(
+        stderr.contains(&format!(
+            "git push origin --delete refs/collab/patches/{}",
+            id
+        )),
+        "diagnostic does not contain the pasteable fix for {}:\n{}",
+        id,
+        stderr
+    );
+    // The remedy must be scoped to the conflicting id, never a blanket glob
+    // over every patch ref on the remote.
+    assert!(
+        !stderr.contains("refs/collab/patches/*"),
+        "diagnostic suggests a blanket glob deletion:\n{}",
+        stderr
+    );
+}
+/// The remedy the diagnostic prints has to actually work. Running it verbatim
+/// must unblock the very sync that produced it.
+#[test]
+fn the_suggested_deletion_actually_unblocks_the_sync() {
+    let (repo, _bare) = repo_with_origin();
+    repo.patch_create("Needs a migrated remote");
+    let id = only_patch_id(&repo);
+    plant_legacy_bare_ref(&repo, &id);
+    repo.run_err(&["sync", "--remote", "origin"]);
+    // Exactly the command the diagnostic tells the user to paste.
+    repo.git(&[
+        "push",
+        "origin",
+        "--delete",
+        &format!("refs/collab/patches/{}", id),
+    ]);
+    repo.run_ok(&["sync", "--remote", "origin"]);
+}
+/// A push failure that is *not* a refname conflict keeps the ordinary retry
+/// advice — spelled with the real binary name. A stale lock on the remote
+/// produces "cannot lock ref" too, so this pins the boundary: the classifier
+/// must not fire on every locking failure.
+#[test]
+fn ordinary_push_failure_still_gets_retry_advice() {
+    let (repo, bare) = repo_with_origin();
+    repo.patch_create("Ordinary failure");
+    let id = only_patch_id(&repo);
+    // A stale lock file on the remote: rejected, but nothing to do with the
+    // ref layout, and clearing it does make a retry succeed.
+    let lock_dir = bare.path().join("refs/collab/patches").join(&id);
+    std::fs::create_dir_all(&lock_dir).unwrap();
+    std::fs::write(lock_dir.join("events.lock"), "0000000000000000000000000000000000000000\n")
+        .unwrap();
+    let stderr = repo.run_err(&["sync", "--remote", "origin"]);
+    assert!(
+        stderr.contains("git-collab sync --remote origin"),
+        "ordinary failure lost its retry advice:\n{}",
+        stderr
+    );
+    assert!(
+        !stderr.contains("--delete"),
+        "ordinary failure was misclassified as a refname conflict:\n{}",
+        stderr
+    );
+}
+/// A conflict and an ordinary failure in the same sync each need their own
+/// answer, and neither may suppress the other. The retry count must cover only
+/// the ordinary failure — telling the user to retry the conflicted ref is
+/// exactly what the conflict advice says not to do.
+#[test]
+fn a_conflict_and_an_ordinary_failure_are_both_reported() {
+    let (repo, bare) = repo_with_origin();
+    repo.patch_create("Conflicting");
+    let conflicted = only_patch_id(&repo);
+    plant_legacy_bare_ref(&repo, &conflicted);
+    repo.patch_create("Merely locked");
+    let locked = patch_ids(&repo)
+        .into_iter()
+        .find(|id| *id != conflicted)
+        .expect("second patch id");
+    let lock_dir = bare.path().join("refs/collab/patches").join(&locked);
+    std::fs::create_dir_all(&lock_dir).unwrap();
+    std::fs::write(
+        lock_dir.join("events.lock"),
+        "0000000000000000000000000000000000000000\n",
+    )
+    .unwrap();
+    let stderr = repo.run_err(&["sync", "--remote", "origin"]);
+    assert!(
+        stderr.contains(&format!(
+            "git push origin --delete refs/collab/patches/{}",
+            conflicted
+        )),
+        "conflict advice missing when an ordinary failure was also present:\n{}",
+        stderr
+    );
+    assert!(
+        stderr.contains("git-collab sync --remote origin"),
+        "retry advice missing when a conflict was also present:\n{}",
+        stderr
+    );
+    // The locked patch must not be offered up for deletion.
+    assert!(
+        !stderr.contains(&format!("--delete refs/collab/patches/{}", locked))
+            && !stderr.contains(&format!(" refs/collab/patches/{} ", locked)),
+        "the merely-locked patch {} was offered for deletion:\n{}",
+        locked,
+        stderr
+    );
+    // Only the locked ref is retryable, not the two conflicted ones.
+    assert!(
+        stderr.contains("retry 1 failed ref(s)"),
+        "retry count includes conflicted refs:\n{}",
+        stderr
+    );
+}
+// ---------------------------------------------------------------------------
+// Fix 2 — `init` is idempotent (issue 7824bd7b)
+// ---------------------------------------------------------------------------
+#[test]
+fn init_twice_leaves_exactly_one_collab_refspec() {
+    let (repo, _bare) = repo_with_origin();
+    // repo_with_origin already ran `init` once.
+    repo.run_ok(&["init"]);
+    repo.run_ok(&["init"]);
+    let fetch_specs = repo.git(&["config", "--get-all", "remote.origin.fetch"]);
+    let collab_specs: Vec<&str> = fetch_specs
+        .lines()
+        .filter(|l| l.contains("refs/collab/*"))
+        .collect();
+    assert_eq!(
+        collab_specs.len(),
+,
+        "expected exactly one collab refspec after repeated init, got {:?}",
+        collab_specs
+    );
+}
+#[test]
+fn init_says_already_configured_on_a_second_run() {
+    let (repo, _bare) = repo_with_origin();
+    let out = repo.run_ok(&["init"]);
+    assert!(
+        out.contains("already configured"),
+        "second init claimed to configure something new:\n{}",
+        out
+    );
+}