| Retry teardown cleanup instead of reporting phantom test failures (#19) Every test builds real git repositories under a mktemp -d, and teardown was a bare `rm -rf`. If anything writes into those repositories while rm is emptying them, rm fails with "Directory not empty", teardown returns nonzero, and bats reports the test as failed with every assertion having passed. The failure message points at the teardown line, so it reads as a defect in the test rather than in the cleanup. This is not theoretical. A ~/.gitconfig carrying trace2.eventtarget = af_unix:stream:.../trace2.sock hands every git invocation to a background daemon, which then writes .git/ai/working_logs/..., .git/fast_import_crash_<pid> and .git/objects/pack/tmp_idx_* into the repo asynchronously — after git has exited and while teardown is running. Observed cost was one to three failures per full-suite run, on a different set of tests each time, which is the signature of a race rather than a bug. trace2 is only one member of the class. core.fsmonitor daemons, background `git maintenance` jobs, editor and IDE git integrations, file indexers such as Spotlight's mds, and antivirus scanners all write into a repository out of band. Any contributor running one of them sees the same phantom failures. Retry the removal up to ten times, a tenth of a second apart. These writers arrive in a short burst once the last git command exits, so the second attempt almost always wins; in the common case the loop body never runs at all and cleanup costs exactly what it did before. Exhausting the retries warns on bats' terminal descriptor and returns 0 rather than failing. A leaked directory under TMPDIR is cheap, visible, and swept up by the system; a suite that cries wolf is neither. Teardown governs cleanup, not whether a test passed — a failing assertion still fails the test regardless of what cleanup does. The rewrite also drops a latent bug in the old one-liner: when TEST_DIR was already gone, `[ -d "$TEST_DIR" ] && rm -rf` returned 1 and failed the test on its own. | 1 month ago |
| Fall back to @{upstream} when @{push} does not resolve (#16) * Fall back to @{upstream} when @{push} does not resolve (#15) Under push.default=simple, @{push} only resolves when the local branch name matches its upstream's. A CI gate worktree on a differently-named tracking branch — upstream configured, HEAD pushed — dead-ended with the misleading "current branch is not tracking a remote branch". is_clean() now falls back to @{upstream} when @{push} does not resolve: HEAD contained in the upstream is a sound proxy for "this SHA is on the remote". When both resolve, @{push} remains authoritative. is_clean() also no longer exits mid-predicate: it reports the failure reason via UNCLEAN_REASON and returns nonzero, so the caller's error names the real cause (uncommitted changes, unpushed changes, or no push destination/upstream) instead of a generic catch-all. * Parse leading -f before command dispatch The catch-all dispatcher case special-cased a bare leading -f by calling cmd_create -f and dropping every remaining argument, so "gh signoff -f linux" silently signed off the default context instead of linux. It also left "gh signoff -f status" and friends ambiguous. Parse a leading -f before dispatch, matching help's "gh signoff [flags] [command] [options]" grammar: it applies to create (explicit, implicit, or contextual) and is rejected for every other command. Trailing -f is unchanged via cmd_create's own parsing. * Complete commands after a leading -f The completion function only offered contexts after -f, so the newly supported "gh signoff -f create" form was not discoverable. A leading -f now completes to create plus contexts; a trailing -f still completes to contexts only. Splitting create out of the install/uninstall/check group also stops offering --branch, which cmd_create never accepted. The gh mock now proxies "gh signoff ..." to the gh-signoff under test so completion's dynamic context lookup works in tests. * Refuse upstream fallback when pushes are rerouted The @{upstream} fallback assumed triangular workflows were unaffected because @{push} always resolves for them. Not so: with remote.pushDefault=fork and push.default=simple, a renamed branch's @{push} fails to resolve when the fork's tracking ref was never fetched — while git push still targets the fork. Falling back to the upstream could then approve a SHA absent from the real push destination. Gate the fallback on the absence of push-rerouting config — branch.<name>.pushRemote, remote.pushDefault, or push refspecs on the upstream remote — rather than reimplementing git's push-routing rules. When @{push} does not resolve and the fallback is unsound or unavailable, say so: "cannot verify the current branch is pushed". * Allowlist the safe case for upstream fallback The rerouting denylist missed push.default modes that determine the push destination independently of the three explicit rerouting keys: current can target a branch-named ref that does not exist yet, nothing and matching have no single destination, and a '.' upstream is purely local. In each, @{push} fails to resolve and the fallback approved a SHA the real push destination lacks — or that has no push destination at all. Invert the check: fall back to @{upstream} only when HEAD is on a branch, effective push.default is simple (git's default), the upstream remote is real (not '.'), and no pushRemote/pushDefault/push-refspec rerouting is configured. Everything else keeps the honest "cannot verify the current branch is pushed" failure. * Require matching fetch and push URLs for upstream fallback remote.<name>.pushurl and url.*.pushInsteadOf send pushes to a different repository than fetches come from, and a remote with multiple push URLs has no single destination — in each case the upstream ref proves nothing about where a push would land, yet the allowlist accepted it. Compare resolved URLs via git remote get-url, which reflects pushurl, pushInsteadOf rewrites, and multi-URL remotes: the fallback now also requires exactly one fetch URL and one identical push URL. | 1 month ago |
| Don't leak an internal error when check reports no signoff requirement (#18) `gh signoff check` on an unprotected branch printed its correct result and then a spurious diagnostic: ✗ GitHub main branch does not require signoff Error: Unexpected error on line 674 (1) cmd_check ended that path with `return 1`. The nonzero status propagated to the top-level case dispatch, a command in the script's own body, which tripped the script-wide ERR trap. cmd_status had the same shape on its "Could not get status for commit" path. Exit nonzero from the command instead, as cmd_create already does for its failed-signoff path. The user-facing message and the nonzero exit status (meaningful for `gh signoff check && ...`) are unchanged, and the ERR trap keeps guarding genuinely unexpected failures. Alternatives considered and rejected: `esac || exit $?` on the dispatch, or `|| exit $?` per arm, would fix the whole class in one place but put the commands inside an AND-OR list, which disables errexit for the entire dynamic extent of the call — an unhandled failure inside any cmd_* would then continue instead of aborting. Verified on bash 3.2 and 5.3. Cover both paths with exact-output assertions. The existing tests used substring matches, and bats folds stderr into $output, so the extra error line passed unnoticed. | 1 month ago |