[PR #240] [MERGED] test(check-pr-size): pin initTestRepo's initial branch so go test passes under init.defaultBranch=main #246

New issue

Closed

opened 2026-10-26 22:37:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/240
Author: @mattmillerai
Created: 8/27/2026
Status: ✅ Merged
Merged: 8/27/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9614-pin-inittestrepo-branch

📝 Commits (2)

165efcc test(check-pr-size): pin initTestRepo's initial branch so go test passes under init.defaultBranch=main
934415d test(check-pr-size): isolate initTestRepo from global git config, drop init -b (BE-9614)

📊 Changes

1 file changed (+21 additions, -1 deletions)

View changed files

📝 scripts/check-pr-size/main_test.go (+21 -1)

📄 Description

ELI-5

The Go tests for the PR-size checker build a throwaway git repo to test against. They never told git what to call that repo's first branch, so git used whatever the developer's init.defaultBranch setting says. One test then tries to create its own branch called main — and on any machine where init.defaultBranch = main (a very common setting), that blows up with fatal: a branch named 'main' already exists. This PR gives the throwaway repo an explicit, deliberately-boring starting branch name so nothing the tests create can ever collide with it — and, after review, seals off the rest of that same class by ignoring the developer's global git config for the duration of these tests.

What changed

initTestRepo in scripts/check-pr-size/main_test.go, in two ways. No production code, no CI config, and no other test was touched.

1. The initial branch is pinned — git -c init.defaultBranch=pr-size-test-base init -q instead of a bare git init -q.

The first revision of this PR used git init -q -b groom-test-base; review (thread) caught that -b needs git >= 2.28 while gitRun escalates any non-zero exit to t.Fatalf — so on older git every repo-backed test in the file would die inside initTestRepo rather than reach its t.Skip("git too old for check-attr --source") guard. Those guards exist, so pre-2.28 git is explicitly anticipated here. The -c form (already used by .github/bump-callers/tests/test_preflight.sh) pins the branch identically and is ignored harmlessly by pre-2.28 git, which does not honor init.defaultBranch and therefore cannot collide in the first place. The name also moved groom-test-base -> pr-size-test-base, since groom is an unrelated subsystem under .github/groom/.

2. The developer's global/system git config is neutralized for these tests — t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + the same for GIT_CONFIG_SYSTEM, the idiom already used by .github/groom/tests/test_scope.py and groom.yml.

Pinning the branch fixed one setting; review (thread) pointed out it left the whole class open, and that turned out to be live rather than theoretical — see Verification. It is t.Setenv rather than gitRun's cmd.Env because the failing assertions come from diffFiles, production code the test runs in-process after t.Chdir, spawning its own git from the test process; a cmd.Env on the fixture helper would not have covered them. Safe because every initTestRepo call site is serial — the same constraint t.Chdir already imposes.

Why CI never caught it

test-pr-size.yml runs on ubuntu-latest, whose git still defaults to master, so checkout -b main finds no existing branch and the suite is green. The bug only surfaces locally, and only for developers whose git is configured to main — which is precisely the audience for the local Go gate command documented in AGENTS.md (line 33: (cd scripts/check-pr-size && [ -z "$(gofmt -l .)" ] && go vet ./... && go test ./...)), added by the docs-only #227.

Verification

Reproduced the failure on origin/main (75685ed) before changing anything:

--- FAIL: TestResolveMergeBaseWithAdvancedBaseBranch (0.14s)
    main_test.go:70: git [checkout -q -b main <sha>] failed: exit status 128
        fatal: a branch named 'main' already exists

Then ran the full matrix on the fix (-count=1 throughout — Go's test cache does not key on env that only the git subprocess reads, so a plain rerun can return a stale cached result). Local git 2.54.0, Go 1.26.5:

`init.defaultBranch`	result
`main` (`GIT_CONFIG_COUNT=1 …`)	`ok` — 258 tests/subtests passed, 0 failed
`master` (`GIT_CONFIG_COUNT=1 …`)	`ok`
unset (`GIT_CONFIG_COUNT=0`)	`ok`

gofmt -l . clean, go vet ./... clean. All three env variants used GIT_CONFIG_* rather than writing global git config.

Review round 2 — the global-config class was not theoretical. Under a global diff.renames=false, the pre-fix branch (165efcc) fails:

--- FAIL: TestClassifyRenameIntoAttrGeneratedTreeStillCounts (0.14s)
    main_test.go:404: gen/x.json OldPath = "", want handwritten/x.json (rename detection did not fire)
--- FAIL: TestAnnotateDiscountsPairsRenamesSoTheEditStillCounts (0.16s)
    main_test.go:485: moved/big.go OldPath = "", want handwritten/big.go (rename detection did not fire)

Both pass after the GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM change, and the suite is ok with no hostile config as well. Separately confirmed git -c init.defaultBranch=pr-size-test-base init -q still yields pr-size-test-base on git 2.54.0, and re-checked that no initTestRepo caller sits under a t.Parallel() (which would make t.Setenv panic) — all 15 are serial.

The repo gate itself is green on this PR: test-pr-size.yml (go test) passed in 25s — run 33083662393. As expected it proves nothing about the bug (its runner's git still defaults to master); it confirms only that the rename broke nothing. See Residual.

Blast-radius check on the rest of the suite

initTestRepo has 16 call sites in main_test.go and 0 in size_test.go. Grepping the whole check-pr-size package for branch-name literals (master, "main", refs/heads, symbolic-ref, branch --show-current, rev-parse --abbrev-ref) returns exactly one hit — line 70, the checkout -b main the tests create explicitly. Nothing reads the initial branch by name, so renaming it is behaviour-neutral for the other 15 call sites, which is what the green full-suite runs above confirm.

The half this PR does not fix

Swept the whole repo for the same unpinned pattern: 13 git init sites across 9 files. This PR pins 1. The other 12 — .github/workflows/{pr-risk,pr-derisk,pr-area-label,linear-ticket}.yml (7, all the ancestry-check scratch repos), .github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py (2), scripts/pr-risk/tests/test_pin_contract.sh, .github/groom/tests/test_scope.py, .github/groom/tests/sandbox-tests.sh — were each checked for a follow-on checkout -b / switch -c / branch -m and for hardcoded main/master literals near the init. None of them creates a named branch after git init, so none can collide the way this one does, and none is fixed here.

Judgment calls

Branch renamed to pr-size-test-base. The first revision kept the work item's literal groom-test-base on the reasoning that deviating for cosmetics would be unrequested drift; a reviewer did prefer the rename, so it moved. Either string is an arbitrary sentinel whose only job is to be nothing a developer would set as init.defaultBranch.
The global-config fix widened the original one-line scope. The work item asked only to pin the branch. Reviewers showed that pinning alone leaves a demonstrably reachable failure (above), and the wider fix stays inside the same helper in the same file, so it was taken here rather than deferred.
No regression test added and no CI change, both explicitly out of scope for this work item. See Residual.

Residual

Not fixed here, and worth its own follow-up:

CI still cannot catch a re-regression of this exact bug. test-pr-size.yml runs one go test ./... on ubuntu-latest, where git defaults to master; if a future edit drops the -b again, the job stays green and the breakage reappears only on developers' machines. The durable fix is a second go test step (or matrix leg) run under GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=init.defaultBranch GIT_CONFIG_VALUE_0=main, which costs ~2s. It is deliberately not in this PR because the work item scoped this to one file and said not to touch test-pr-size.yml.
The other 12 git init sites remain unpinned. They are safe today only because none of them happens to create a named branch afterwards (swept and enumerated above) — that is an incidental property, not an enforced one, and the next test added to any of those suites can reintroduce the same collision. A blanket -b pin across all 13 sites, or a lint that requires it, would make the property enforced.
Upstream investigation not re-verified. The read-only investigation that produced this work item reports having reproduced the bug on current main and verified this fix under all three init.defaultBranch settings. That tracker is not reachable from where this PR was produced, so its transcript was never read; the reproduction and the three-way verification above were re-done from scratch here rather than taken on trust, but the investigation's own artifacts remain unexercised.

Provenance

Authored by: agent-work loop
Verified: test-pr-size.yml (go test) green on this PR (21s, run 33087171862); all 4 PR checks pass. Reproduced the original failure on origin/main@75685ed; on this branch gofmt -l . clean, go vet ./... clean, go test -count=1 ./... ok under init.defaultBranch = main / master / unset (258 tests+subtests each). For the review-round change, reproduced the diff.renames=false failure on 165efcc and confirmed it green on 934415d.
Deviations: the review round widened the change beyond the work item's one-line scope — the branch pin now uses git -c init.defaultBranch= instead of init -b (old-git compatibility), the sentinel branch was renamed groom-test-base -> pr-size-test-base, and GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM isolation was added. All three are inside the same initTestRepo helper in the same file; still no CI step added and no other test touched.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/240 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9614-pin-inittestrepo-branch` --- ### 📝 Commits (2) - [`165efcc`](https://github.com/Comfy-Org/github-workflows/commit/165efcc7479461df35c832aa904a22e37ef6fde2) test(check-pr-size): pin initTestRepo's initial branch so go test passes under init.defaultBranch=main - [`934415d`](https://github.com/Comfy-Org/github-workflows/commit/934415d293d4cdcd82b1e5a983655b95e4999885) test(check-pr-size): isolate initTestRepo from global git config, drop `init -b` (BE-9614) ### 📊 Changes **1 file changed** (+21 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `scripts/check-pr-size/main_test.go` (+21 -1) </details> ### 📄 Description ## ELI-5 The Go tests for the PR-size checker build a throwaway git repo to test against. They never told git what to call that repo's first branch, so git used whatever the developer's `init.defaultBranch` setting says. One test then tries to create its own branch called `main` — and on any machine where `init.defaultBranch = main` (a very common setting), that blows up with `fatal: a branch named 'main' already exists`. This PR gives the throwaway repo an explicit, deliberately-boring starting branch name so nothing the tests create can ever collide with it — and, after review, seals off the rest of that same class by ignoring the developer's global git config for the duration of these tests. ## What changed `initTestRepo` in `scripts/check-pr-size/main_test.go`, in two ways. No production code, no CI config, and no other test was touched. **1. The initial branch is pinned** — `git -c init.defaultBranch=pr-size-test-base init -q` instead of a bare `git init -q`. The first revision of this PR used `git init -q -b groom-test-base`; review ([thread](https://github.com/Comfy-Org/github-workflows/pull/240#discussion_r3872861147)) caught that `-b` needs git >= 2.28 while `gitRun` escalates any non-zero exit to `t.Fatalf` — so on older git *every* repo-backed test in the file would die inside `initTestRepo` rather than reach its `t.Skip("git too old for check-attr --source")` guard. Those guards exist, so pre-2.28 git is explicitly anticipated here. The `-c` form (already used by `.github/bump-callers/tests/test_preflight.sh`) pins the branch identically and is ignored harmlessly by pre-2.28 git, which does not honor `init.defaultBranch` and therefore cannot collide in the first place. The name also moved `groom-test-base` -> `pr-size-test-base`, since `groom` is an unrelated subsystem under `.github/groom/`. **2. The developer's global/system git config is neutralized** for these tests — `t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull)` + the same for `GIT_CONFIG_SYSTEM`, the idiom already used by `.github/groom/tests/test_scope.py` and `groom.yml`. Pinning the branch fixed one setting; review ([thread](https://github.com/Comfy-Org/github-workflows/pull/240#discussion_r3872861155)) pointed out it left the whole class open, and that turned out to be live rather than theoretical — see Verification. It is `t.Setenv` rather than `gitRun`'s `cmd.Env` because the failing assertions come from `diffFiles`, production code the test runs in-process after `t.Chdir`, spawning its own git from the test process; a `cmd.Env` on the fixture helper would not have covered them. Safe because every `initTestRepo` call site is serial — the same constraint `t.Chdir` already imposes. ## Why CI never caught it `test-pr-size.yml` runs on `ubuntu-latest`, whose git still defaults to `master`, so `checkout -b main` finds no existing branch and the suite is green. The bug only surfaces locally, and only for developers whose git is configured to `main` — which is precisely the audience for the local Go gate command documented in `AGENTS.md` (line 33: `(cd scripts/check-pr-size && [ -z "$(gofmt -l .)" ] && go vet ./... && go test ./...)`), added by the docs-only #227. ## Verification Reproduced the failure on `origin/main` (75685ed) before changing anything: ``` --- FAIL: TestResolveMergeBaseWithAdvancedBaseBranch (0.14s) main_test.go:70: git [checkout -q -b main <sha>] failed: exit status 128 fatal: a branch named 'main' already exists ``` Then ran the full matrix on the fix (`-count=1` throughout — Go's test cache does not key on env that only the git subprocess reads, so a plain rerun can return a stale cached result). Local git 2.54.0, Go 1.26.5: | `init.defaultBranch` | result | | --- | --- | | `main` (`GIT_CONFIG_COUNT=1 …`) | `ok` — 258 tests/subtests passed, 0 failed | | `master` (`GIT_CONFIG_COUNT=1 …`) | `ok` | | unset (`GIT_CONFIG_COUNT=0`) | `ok` | `gofmt -l .` clean, `go vet ./...` clean. All three env variants used `GIT_CONFIG_*` rather than writing global git config. **Review round 2 — the global-config class was not theoretical.** Under a global `diff.renames=false`, the pre-fix branch (165efcc) fails: ``` --- FAIL: TestClassifyRenameIntoAttrGeneratedTreeStillCounts (0.14s) main_test.go:404: gen/x.json OldPath = "", want handwritten/x.json (rename detection did not fire) --- FAIL: TestAnnotateDiscountsPairsRenamesSoTheEditStillCounts (0.16s) main_test.go:485: moved/big.go OldPath = "", want handwritten/big.go (rename detection did not fire) ``` Both pass after the `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM` change, and the suite is `ok` with no hostile config as well. Separately confirmed `git -c init.defaultBranch=pr-size-test-base init -q` still yields `pr-size-test-base` on git 2.54.0, and re-checked that no `initTestRepo` caller sits under a `t.Parallel()` (which would make `t.Setenv` panic) — all 15 are serial. The repo gate itself is green on this PR: `test-pr-size.yml` (`go test`) passed in 25s — run 33083662393. As expected it proves nothing about the bug (its runner's git still defaults to `master`); it confirms only that the rename broke nothing. See Residual. ## Blast-radius check on the rest of the suite `initTestRepo` has 16 call sites in `main_test.go` and 0 in `size_test.go`. Grepping the whole `check-pr-size` package for branch-name literals (`master`, `"main"`, `refs/heads`, `symbolic-ref`, `branch --show-current`, `rev-parse --abbrev-ref`) returns exactly one hit — line 70, the `checkout -b main` the tests create explicitly. Nothing reads the initial branch by name, so renaming it is behaviour-neutral for the other 15 call sites, which is what the green full-suite runs above confirm. ## The half this PR does not fix Swept the whole repo for the same unpinned pattern: **13** `git init` sites across 9 files. This PR pins **1**. The other **12** — `.github/workflows/{pr-risk,pr-derisk,pr-area-label,linear-ticket}.yml` (7, all the ancestry-check scratch repos), `.github/public-repo-hygiene/tests/test_check_public_repo_hygiene.py` (2), `scripts/pr-risk/tests/test_pin_contract.sh`, `.github/groom/tests/test_scope.py`, `.github/groom/tests/sandbox-tests.sh` — were each checked for a follow-on `checkout -b` / `switch -c` / `branch -m` and for hardcoded `main`/`master` literals near the init. **None of them creates a named branch after `git init`**, so none can collide the way this one does, and none is fixed here. ## Judgment calls - **Branch renamed to `pr-size-test-base`.** The first revision kept the work item's literal `groom-test-base` on the reasoning that deviating for cosmetics would be unrequested drift; a reviewer did prefer the rename, so it moved. Either string is an arbitrary sentinel whose only job is to be nothing a developer would set as `init.defaultBranch`. - **The global-config fix widened the original one-line scope.** The work item asked only to pin the branch. Reviewers showed that pinning alone leaves a demonstrably reachable failure (above), and the wider fix stays inside the same helper in the same file, so it was taken here rather than deferred. - **No regression test added and no CI change**, both explicitly out of scope for this work item. See Residual. ## Residual Not fixed here, and worth its own follow-up: - **CI still cannot catch a re-regression of this exact bug.** `test-pr-size.yml` runs one `go test ./...` on `ubuntu-latest`, where git defaults to `master`; if a future edit drops the `-b` again, the job stays green and the breakage reappears only on developers' machines. The durable fix is a second `go test` step (or matrix leg) run under `GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=init.defaultBranch GIT_CONFIG_VALUE_0=main`, which costs ~2s. It is deliberately not in this PR because the work item scoped this to one file and said not to touch `test-pr-size.yml`. - **The other 12 `git init` sites remain unpinned.** They are safe today only because none of them happens to create a named branch afterwards (swept and enumerated above) — that is an incidental property, not an enforced one, and the next test added to any of those suites can reintroduce the same collision. A blanket `-b` pin across all 13 sites, or a lint that requires it, would make the property enforced. - **Upstream investigation not re-verified.** The read-only investigation that produced this work item reports having reproduced the bug on current `main` and verified this fix under all three `init.defaultBranch` settings. That tracker is not reachable from where this PR was produced, so its transcript was never read; the reproduction and the three-way verification above were re-done from scratch here rather than taken on trust, but the investigation's own artifacts remain unexercised. ## Provenance - **Authored by:** agent-work loop - **Verified:** `test-pr-size.yml` (`go test`) green on this PR (21s, run 33087171862); all 4 PR checks pass. Reproduced the original failure on origin/main@75685ed; on this branch `gofmt -l .` clean, `go vet ./...` clean, `go test -count=1 ./...` `ok` under `init.defaultBranch` = main / master / unset (258 tests+subtests each). For the review-round change, reproduced the `diff.renames=false` failure on 165efcc and confirmed it green on 934415d. - **Deviations:** the review round widened the change beyond the work item's one-line scope — the branch pin now uses `git -c init.defaultBranch=` instead of `init -b` (old-git compatibility), the sentinel branch was renamed `groom-test-base` -> `pr-size-test-base`, and `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM` isolation was added. All three are inside the same `initTestRepo` helper in the same file; still no CI step added and no other test touched. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:44 +00:00