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

fix: share STDIN across different commands on pre-push hook #732

Merged
merged 7 commits into from
May 30, 2024

Conversation

mrexox
Copy link
Member

@mrexox mrexox commented May 29, 2024

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 for pre-push but for all hooks that specify use_stdin option the cachedReader 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 use use_stdin for more that one command/script, please consider not using parallel: true.

☑️ Checklist

  • Check locally
  • Add tests
  • Add documentation

tdesveaux and others added 4 commits May 27, 2024 17:32
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.
@mrexox
Copy link
Member Author

mrexox commented May 29, 2024

Hey @tdesveaux, I've implemented a slightly different approach but it should solve the issue.

  1. STDIN is passed to 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.
  2. But the STDIN is wrapped in a struct that caches all read bytes and allows to re-read them again and again. This solves issues when you have your own script/command that utilizes stdin together with Git LFS.

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.

@tdesveaux
Copy link
Contributor

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:

  • You should consider writing the buffered Stdin to a file if it's size is large (pretty edge case, but a big push would receive lots of Stdin). I think this can be an noted improvement for later.
  • You removed the error escalation if LFS command fails. I'd like to re-iterate how dangerous this is IMHO.

@mrexox
Copy link
Member Author

mrexox commented May 29, 2024

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?

@tdesveaux
Copy link
Contributor

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.

@tdesveaux tdesveaux mentioned this pull request May 29, 2024
3 tasks
@tdesveaux
Copy link
Contributor

@mrexox Just tested this branch. This fix the issue I reproduced.

FYI, here is what I did to reproduce.

  • git init a repository
  • setup lefthook with a pre-push hook. (tested with a simple run: echo pre-push_hook initially)
  • add a dummy test.bin file with echo test >> test.bin
  • git lfs track test.bin
  • commit lefthook.yml and .gitattributes
  • commit test.bin
  • push

I also tested your branch with a cheeky:

pre-push:
  commands:
    cat-stdin
      run: cat
      use_stdin: true

and it works.

@mrexox mrexox merged commit afc1125 into master May 30, 2024
16 of 19 checks passed
@mrexox mrexox deleted the fix/lfs-pre-push branch July 22, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants