fix(hooks): pass --allow-unknown-hook-name on git 2.44+#727
fix(hooks): pass --allow-unknown-hook-name on git 2.44+#727aviator-app[bot] merged 2 commits intomasterfrom
Conversation
Git 2.54 tightened `git hook run` to reject non-native hook names unless `--allow-unknown-hook-name` is passed, so `av pr` and `av sync` fail on 2.54+ when invoking the `pre-av-pr` and `pre-av-sync` hooks (even when no such hook is configured). The flag was introduced in git 2.44, so add it conditionally based on a parsed `git --version` to avoid regressing older gits that don't recognize the flag.
✅ FlexReview StatusCommon Owner:
Review SLO: |
Current Aviator status
This PR was merged using Aviator.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements dynamic git hook argument generation based on the installed git version, specifically adding support for the --allow-unknown-hook-name flag required by newer git versions. It introduces a version parsing utility and updates hook execution calls in the pr and sync commands. A review comment suggests caching the git version to improve performance by avoiding redundant shell executions.
| func (r *Repo) Version(ctx context.Context) (major, minor int, err error) { | ||
| out, err := r.Git(ctx, "--version") | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
| return parseGitVersion(out) | ||
| } |
There was a problem hiding this comment.
The Version method executes an external git --version command every time it is called. While this is relatively fast, it is inefficient to perform this I/O and process creation repeatedly during a single CLI invocation. Consider caching the detected version in the Repo struct or using a package-level sync.OnceValues to ensure the version is only parsed once.
## Summary Follow-up to #727. That PR gated `--allow-unknown-hook-name` at git 2.44 based on incorrect version information. The flag was actually introduced in git 2.54 alongside the stricter hook-name validation, so passing it on 2.44–2.53 would be rejected as an unknown flag. ## Change - `internal/git/version.go`: bump the threshold in `supportsAllowUnknownHookName` from `>= 2.44` to `>= 2.54`, and update the surrounding comments. Behavior now: - `< 2.54` — flag omitted (no unknown-flag errors on older git). - `>= 2.54` — flag included; required to allow non-native hook names like `pre-av-pr`/`pre-av-sync`. If version detection fails, we still include the flag so modern git keeps working. ## Test plan - [x] `go build ./...` - [x] `go tool golangci-lint run ./internal/git/... ./cmd/av/...` - [x] `go test --vet=all ./internal/git/... ./cmd/av/...` - [x] Locally upgraded to git 2.54.0 and can confirm `av sync` now invokes the `pre-av-sync` hook instead of failing with `unknown hook event`. - [ ] CI green
2 participants
Summary
Git 2.54 tightened
git hook runto reject non-native hook names unless--allow-unknown-hook-nameis passed. Since av invokesgit hook run pre-av-prandgit hook run pre-av-sync— both custom names — everyav prandav syncfails on git 2.54+ with:This is closely related to #724, but that PR passes the flag unconditionally.
--allow-unknown-hook-namewas only introduced in git 2.44, so on older gits the flag itself would be rejected. This PR gates the flag on a parsedgit --version.Changes
internal/git/version.go: addsRepo.Version(ctx)which parsesgit --versioninto(major, minor), andRepo.HookRunArgs(ctx, hookName)which builds thegit hook runarg list and conditionally appends--allow-unknown-hook-namewhen git >= 2.44. If version detection fails, the flag is still included so modern git keeps working.internal/git/version_test.go: unit tests covering stable, patch, Apple-suffixed, newline-trailed, and malformed version strings.cmd/av/pr.go:runPRHookusesrepo.HookRunArgs(ctx, "pre-av-pr").cmd/av/sync.go:preAvSyncHookModel.Initusesm.repo.HookRunArgs(ctx, "pre-av-sync").Behavior by git version:
< 2.44— flag omitted (avoids "unknown option" errors on old git).2.44–2.53— flag included, accepted but not required.>= 2.54— flag included and required; prevents the "unknown hook event" failure.Test plan
go build ./...go tool golangci-lint run ./internal/git/... ./cmd/av/...go test --vet=all ./internal/git/... ./cmd/av/...