-
Notifications
You must be signed in to change notification settings - Fork 220
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
fix: share STDIN across different commands on pre-push hook #732
Conversation
Since Stdin from Git is not intercepted and forwared to each commands/scripts, this could lead to hang.
This is only used by LFS pre-push Hook which require Stdin.
Hey @tdesveaux, I've implemented a slightly different approach but it should solve the issue.
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. 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. 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.
I also tested your branch with a cheeky:
and it works. |
Closes #511
Closes #611
Closes #147
⚡ 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
.☑️ Checklist