Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run LFS hooks after checking if hooks are skipped #818

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

zachahn
Copy link
Contributor

@zachahn zachahn commented Sep 19, 2024

⚡ Summary

I noticed that the skip: true configuration I added to post-checkout was running LFS code. I also noticed that setting the environment variable LEFTHOOK=0 did not run any LFS code.

This PR moves r.Hook.DoSkip closer to the top of RunAll, basically running less code when asked to skip.

I didn't really know how to test this! runner_test.go doesn't seem to test any LFS behavior. I'm pretty new to Go, so I'm unlikely to be able to heavily modify tests.

☑️ Checklist

  • Check locally
  • Add tests
  • Add documentation

@mrexox
Copy link
Member

mrexox commented Sep 20, 2024

Hey! Thank you for creating this PR. Are you sure that LSF should be skipped when lefthook hooks are skipped? LFS seems to be like something independent from lefthook and is supposed to be executed on every pre-push hook regardless lefthook configuration.

Could you share your case, so we can discuss this?

@zachahn
Copy link
Contributor Author

zachahn commented Sep 20, 2024

Hello! Yes of course!

I'm working on a very large codebase that doesn't seem to use LFS. My understanding is that LFS is enabled on certain paths that are set via a .gitattributes file. Because LFS doesn't run automatically when using git without lefthook, I'm assuming it's a pretty safe change to skip LFS hooks along with all user-configured hooks when configured to skip.

Because of how large the repo is (3.47 GiB according to git count-objects -vH after a fresh gc), this LFS hook is really slow! I confirmed that this change greatly speeds up skip: true.

My benchmarks show an improvement from 2.6 seconds down to about 0.4 seconds

Before

❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.63s user 1.78s system 81% cpu 2.959 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.52s user 1.71s system 84% cpu 2.624 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.50s user 1.71s system 86% cpu 2.566 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.52s user 1.72s system 85% cpu 2.617 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.49s user 1.71s system 84% cpu 2.617 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.52s user 1.76s system 78% cpu 2.895 total

After

❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.14s user 0.29s system 97% cpu 0.441 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.10s user 0.24s system 83% cpu 0.411 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.11s user 0.25s system 81% cpu 0.434 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.11s user 0.25s system 83% cpu 0.428 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.11s user 0.25s system 84% cpu 0.429 total
❯ time git checkout .
Updated 0 paths from the index
git checkout .  0.11s user 0.24s system 83% cpu 0.421 total

Edit: I created a discussion to talk about potential next steps. But I feel like this PR is still helpful as is! #821

@zghorn
Copy link

zghorn commented Sep 25, 2024

FWIW, I've been using @zachahn fork for this past week with no incident. The speedup is noticeable

@mrexox
Copy link
Member

mrexox commented Sep 26, 2024

Ok, let's merge it 👍

@mrexox mrexox merged commit 6383ff3 into evilmartians:master Sep 26, 2024
19 checks passed
@zachahn zachahn deleted the run-lfs-after-skip-check branch September 26, 2024 14:21
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.

3 participants