Closed mrexox closed 2 months ago
Hey @tdesveaux, I've implemented a slightly different approach but it should solve the issue.
git lfs
hooks whether it uses it or not. I think this is better to provide it always and avoid being responsible for managin STDIN.Do you have some time to test this code for your case? I've done some testing locally but I'd like to hear some feedback from you since this fix should solve your issue.
This is a far better approach, thanks for the fix. I'll test try to test this today.
From a cursory review of the code:
Thank you for the review. Yes, it seems like Git LFS error is a crucial thing so it should fail the whole hook.
I will consider using file for caching. I think it makes sense when STDIN has about 1+ mb of data. Do you think such cases happen often?
https://git-scm.com/docs/githooks#_pre_push
I might have been far too cautious in thinking file buffering might be needed. There is one line per ref pushed (branch, tag, etc). For it to attain >1mb, it would take a very large repository being pushed in it's entirety. Not sure someone doing this would have lefthook enabled.
You can disregard this IMHO, this would add complexity to the code for no real benefit.
@mrexox Just tested this branch. This fix the issue I reproduced.
FYI, here is what I did to reproduce.
git init
a repositorypre-push
hook. (tested with a simple run: echo pre-push_hook
initially)test.bin
file with echo test >> test.bin
git lfs track test.bin
lefthook.yml
and .gitattributes
test.bin
I also tested your branch with a cheeky:
pre-push:
commands:
cat-stdin
run: cat
use_stdin: true
and it works.
Closes https://github.com/evilmartians/lefthook/issues/511 Closes https://github.com/evilmartians/lefthook/issues/611 Closes https://github.com/evilmartians/lefthook/issues/147
:zap: Summary
Right now git lfs pre-push hook does not receive STDIN. We should pass it there but some other hooks may also use it.
For this purpose I've added a
cachedReader
that reads from STDIN once and cached the read data. Not only forpre-push
but for all hooks that specifyuse_stdin
option thecachedReader
will be used.However with
parallel: true
the read data may be incorrect because of random reads from different commands that run cuncurrently. So, when you useuse_stdin
for more that one command/script, please consider not usingparallel: true
.:ballot_box_with_check: Checklist