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
cachedReaderthat reads from STDIN once and cached the read data. Not only forpre-pushbut for all hooks that specifyuse_stdinoption thecachedReaderwill be used.However with
parallel: truethe read data may be incorrect because of random reads from different commands that run cuncurrently. So, when you useuse_stdinfor more that one command/script, please consider not usingparallel: true.☑️ Checklist