Skip to content

fix: use pre-push stdin for push file detection#1368

Merged
mrexox merged 5 commits intoevilmartians:masterfrom
supitsdu:fixes-1367
Apr 3, 2026
Merged

fix: use pre-push stdin for push file detection#1368
mrexox merged 5 commits intoevilmartians:masterfrom
supitsdu:fixes-1367

Conversation

@supitsdu
Copy link
Copy Markdown
Contributor

@supitsdu supitsdu commented Apr 2, 2026

Closes #1367

Context

pre-push changed-file detection fails on the initial push in repositories using sha256 object format.

Before this change, Lefthook tried to infer {push_files} from HEAD @{push} and, when no upstream existed yet, fell back to a hardcoded SHA-1 empty-tree object:

4b825dc642cb6eb9a060e54bf8d69288fbee4904

That object does not exist in sha256 repositories, so Lefthook failed before running configured pre-push commands.

I validated this both with automated coverage and against a real empty sha256 GitLab repository:

git@gitlab.com:supitsdu/lefthook-issue-1367.git

Real-world comparison:

  • issue-1367 branch reproduces the failure:

    git diff --name-only HEAD 4b825dc642cb6eb9a060e54bf8d69288fbee4904
    fatal: ambiguous argument '4b825dc642cb6eb9a060e54bf8d69288fbee4904'
    echo PUSH {push_files} (skip) error replacing {push_files}: exit status 128
    error: failed to push some refs
    
  • fixes-1367 branch succeeds against the same empty remote:

    PUSH README.md lefthook.yml
    To gitlab.com:supitsdu/lefthook-issue-1367.git
     * [new branch]      main -> main
    branch 'main' set up to track 'origin/main'
    

Changes

  • Use Git's pre-push stdin ref updates as the source of truth for {push_files} when available.
  • Parse stdin lines in the form:
    <local ref> <local oid> <remote ref> <remote oid>
    
  • For new remote refs where remote oid is all zeroes, resolve pushed files with:
    git ls-tree -r --name-only <local oid>
    
  • For existing remote refs, resolve pushed files with:
    git diff --name-only <remote oid> <local oid>
    
  • Keep a fallback path for runs without pre-push stdin, but replace the hardcoded SHA-1 empty-tree fallback with:
    git ls-tree -r --name-only HEAD
    
  • Add unit coverage for stdin-based push file resolution and the initial-push fallback path.
  • Add an integration regression test for a sha256 repository performing its first push.

Validation performed:

  • mise x go@1.26.0 -- make test
  • targeted integration regression for pre_push_sha256_initial
  • real push validation against an empty sha256 GitLab remote using binaries built from:

@supitsdu supitsdu requested a review from mrexox as a code owner April 2, 2026 07:01
Copy link
Copy Markdown
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this PR provides two things:

  • Fixes issue for SHA256 repos
  • Adds custom PushFiles

I don't see why you're passing PushFiles and usePushFiles when the idea is to get the push files on pre-push hook in the command/build.go. Could you please explain why this approach does not work?

@supitsdu
Copy link
Copy Markdown
Contributor Author

supitsdu commented Apr 2, 2026

I went with PushFiles and UsePushFiles to inject the exact pre-push list from the hook’s stdin into the existing builder flow.

The reason for both fields is that a simple slice is ambiguous. An empty slice could mean "the pushed file set is empty," or it could mean "no override was provided." UsePushFiles acts as a sentinel so the builder knows whether to use the injected result or fall back to the default Repository behavior.

I'm happy to clean up the plumbing if this feels too explicit. A few ideas:

  • Use a *[]string to handle the "missing vs. empty" distinction.
  • Use an (files, ok) style helper.
  • Move the stdin resolution closer to command/build.go to remove the override entirely.

I ain't married with it, tho. Just a approach. Let me know if you have any approach on it or if my approach is wrong in anyway.

@supitsdu
Copy link
Copy Markdown
Contributor Author

supitsdu commented Apr 2, 2026

I took a swing at a more minimal approach, and honestly, it’s much cleaner. You can check it out here: [fix: simplify pre-push sha256 fallback](5892b2b).

I realized I was probably over-engineering things with all that PushFiles plumbing. I stripped it all out and just updated the empty-tree fallback:

return r.FindExistingFiles(append(cmdPushFilesTreeBase, "HEAD"), "")

This keeps the fix focused on the actual bug and saves us from wrestling with pre-push stdin or TTY ghosts. It still solves the SHA-256 initial-push issue without the extra baggage.

I ran the usual make lint/test/test-integration gauntlet, and even tested it against a blank SHA-256 GitLab repo. It works! The old hardcoded hash is officially dead, and the simplified version actually passes.

@supitsdu supitsdu requested a review from mrexox April 2, 2026 10:44
reVersion = regexp.MustCompile(`\d+\.\d+\.(\d+|\w+)`)
cmdPushFilesBase = []string{"git", "diff", "--name-only", "HEAD", "@{push}"}
cmdPushFilesHead = []string{"git", "diff", "--name-only", "HEAD"}
cmdPushFilesTreeBase = []string{"git", "ls-tree", "-r", "--name-only"}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name it cmdLsTreeAllFiles?

@mrexox
Copy link
Copy Markdown
Member

mrexox commented Apr 3, 2026

Awesome! Thank you for cleaning up the PR. I thin I can merge it and address my comment myself

@mrexox mrexox merged commit f3fc175 into evilmartians:master Apr 3, 2026
3 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pre-push fails on the initial push in a Git repository using sha256 object format.

2 participants