go-git-hooks-index-env-leak

Answer

Part 1 — GIT_INDEX_FILE env leak into temp-repo git commands

Root cause. git commit runs pre-commit hooks with GIT_INDEX_FILE set to the host's in-flight index (.git/next-index-*.lock), plus GIT_DIR, GIT_OBJECT_DIRECTORY, etc. When the hook runs go test ./..., any test that execs git in a t.TempDir() repo inherits that environment. Because GIT_DIR/GIT_INDEX_FILE override repo discovery, a temp-repo git add silently writes into the host's in-flight index lock (verified: the file grows and gains temp-repo entries). When the commit finishes it renames that polluted lock over .git/index — the host index now references blobs that only exist in the temp repo's object store → git status → fatal: unable to read <sha>.

Fix. One shared helper strips every GIT_* variable from the environment of all temp-repo git commands:

// testgit.go (shared by all test packages)
package gitutil

import (
    "os"
    "os/exec"
    "strings"
    "testing"
)

// testGitEnv returns os.Environ() with every GIT_* variable removed
// (GIT_INDEX_FILE, GIT_DIR, GIT_OBJECT_DIRECTORY, GIT_WORK_TREE,
// GIT_CONFIG_COUNT, ...). Tests run under pre-commit hooks otherwise
// inherit the host repo's in-flight index and corrupt it.
func testGitEnv() []string {
    env := os.Environ()
    out := make([]string, 0, len(env))
    for _, kv := range env {
        key, _, ok := strings.Cut(kv, "=")
        if ok && strings.HasPrefix(key, "GIT_") {
            continue
        }
        out = append(out, kv)
    }
    return out
}

// gitCmd runs git in dir with the sanitized environment. Every test that
// execs git against a temp repo goes through this (or sets cmd.Env).
func gitCmd(t *testing.T, dir string, args ...string) string {
    t.Helper()
    cmd := exec.Command("git", args...)
    cmd.Dir = dir
    cmd.Env = testGitEnv()
    out, err := cmd.CombinedOutput()
    if err != nil {
        t.Fatalf("git %v (dir=%s) failed: %v\n%s", args, dir, err, out)
    }
    return string(out)
}

Applied to the 4 test files that exec git (repo_test.go, branch_test.go, commit_test.go, pr_test.go), replacing every raw exec:

  // before — leaks host GIT_* into the temp repo:
- cmd := exec.Command("git", "add", ".")
- cmd.Dir = tmp
+ cmd := exec.Command("git", "add", ".")
+ cmd.Dir = tmp
+ cmd.Env = testGitEnv()        // <- strip GIT_DIR / GIT_INDEX_FILE / ...

  // or, for the common run-and-assert pattern:
- out, err := exec.Command("git", "-C", tmp, "commit", "-m", "x").CombinedOutput()
+ out := gitCmd(t, tmp, "commit", "-m", "x")   // <- t.Helper() handles the failure

All 4 files use the identical swap; the helper lives in one package so the sanitization is enforced in exactly one place.

Part 2 — Forgejo MergePR payload casing + bounded retry

Forgejo v1.21 requires the merge payload key Do (capital-D); lowercase do is rejected with HTTP 405. Independently, even with correct casing the server can transiently 405 while mergeability is still being computed asynchronously — so the client must retry, but bound the attempts:

const (
    mergePRMaxAttempts = 5
    mergePRBaseDelay   = 250 * time.Millisecond
)

type mergePullRequestPayload struct {
    Do                string `json:"Do"` // capital-D required by Forgejo v1.21
    MergeMessageField string `json:"MergeMessageField,omitempty"`
    HeadSHA           string `json:"head_sha,omitempty"`
}

func (c *Client) MergePR(ctx context.Context, owner, repo string, num int, opts MergeOpts) error {
    payload := mergePullRequestPayload{
        Do:                "merge",
        MergeMessageField: opts.Message,
        HeadSHA:           opts.HeadSHA,
    }
    url := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, num)

    var lastErr error
    for attempt := 1; attempt <= mergePRMaxAttempts; attempt++ {
        err := c.post(ctx, url, payload, nil)
        if err == nil {
            return nil
        }
        lastErr = err
        if !isRetryableMergeErr(err) {
            return err
        }
        delay := mergePRBaseDelay * time.Duration(1<<(attempt-1)) // 250ms, 500ms, 1s, 2s
        select {
        case <-time.After(delay):
        case <-ctx.Done():
            return ctx.Err()
        }
    }
    return fmt.Errorf("merge PR %s/%s#%d: giving up after %d attempts: %w",
        owner, repo, num, mergePRMaxAttempts, lastErr)
}

// isRetryableMergeErr: 405 is transient while Forgejo computes mergeability
// asynchronously (it is also what the old lowercase-"do" payload produced).
func isRetryableMergeErr(err error) bool {
    var apiErr *APIError
    return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusMethodNotAllowed
}

The E2E test asserts the merge succeeds across a fake server that returns 405 (mergeability still computing) on the first three calls, and that the client stops at the bound when the server never becomes ready.