Add Git LFS pre-push hook #479
Conversation
Git LFS installs pre-push hook which: - checks that git-lfs utility is on PATH, and if no, warns that Git LFS hook should probably be removed - calls it by executing `git lfs pre-push <remote_name> <remote_url>` The introduced hook is a Ruby port of the same algorithm modulo hook disabling note in the warning message.
| result = execute(['git', 'lfs', 'pre-push', remote_name, remote_url]) | ||
| return :fail, result.stderr unless result.success? | ||
|
|
||
| :pass |
There was a problem hiding this comment.
Not sure: is it possible to output some information in case the hook passes?
In fact, ideally it'd be cool to be able to output something while the hook works cause uploading LFS files might take a while. The original LFS hook reports status interactively, something like:
$ git push
Git LFS: (0 of 1 files) 3.80 MB / 6.85 MB
There was a problem hiding this comment.
I don't know of a good way. Would it make sense to instead of actually uploading just check if there is something to upload? The overcommit hook would then output something like
Checking for files that need to be uploaded..........................[GitLFS] FAILED
./some-file.large hasn't been uploaded. Run `git lfs pre-push ... something` to upload.
I'm not a lfs user myself, so this idea might not be possible at all. Or it might just be annoying to have to run the upload manually.
There was a problem hiding this comment.
This would require some refactoring of the display logic for Overcommit. In the past we've kicked around the idea of supporting progress bars or similar, but this would require a much more complicated display controller that utilized more advanced cursor features so you could update multiple lines at the same time (since hooks run in parallel).
There was a problem hiding this comment.
Definitely open to the idea, but I think it's well outside the scope of what you're trying to accomplish with this change.
There was a problem hiding this comment.
Would it make sense to instead of actually uploading just check if there is something to upload?
Not really, you'd just want it to be uploaded just with git push, without additionally calling git lfs .... Also, I'd not make overcommit behaviour different from standard (meaning what is installed by LFS client by default).
It's well outside the scope of what you're trying to accomplish with this change.
I totally agree, I was wondering if there's a way to do that, but if not, it's fine. It might be just mentioned in the doc since GitLfs hook is disabled by default and requires the user to add it in the config yaml.
|
Hey @sds, how do I trigger AppVeyor build again? Looks like its failure is unrelated. Also I'd love to get your feedback and get this merged eventually. Thanks! |
| # @see https://git-lfs.github.com/ | ||
| class GitLfs < Base | ||
| def run | ||
| result = execute(['command', '-v', 'git-lfs']) |
There was a problem hiding this comment.
Is it always safe to assume that a non-zero exit code means git-lfs wasn't installed? We could run a which git-lfs first for better precision.
There was a problem hiding this comment.
I simply followed what the original hook does, for consistency (original hook is installed by Git LFS client). I mentioned that before, here's its entire content:
#!/bin/sh
command -v git-lfs >/dev/null 2>&1 || { echo >&2 "\nThis repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting .git/hooks/pre-push.\n"; exit 2; }
git lfs pre-push "$@"But yeah, in_path looks like a safe replacement, will update.
| result = execute(['git', 'lfs', 'pre-push', remote_name, remote_url]) | ||
| return :fail, result.stderr unless result.success? | ||
|
|
||
| :pass |
There was a problem hiding this comment.
I don't know of a good way. Would it make sense to instead of actually uploading just check if there is something to upload? The overcommit hook would then output something like
Checking for files that need to be uploaded..........................[GitLFS] FAILED
./some-file.large hasn't been uploaded. Run `git lfs pre-push ... something` to upload.
I'm not a lfs user myself, so this idea might not be possible at all. Or it might just be annoying to have to run the upload manually.
|
Overall looks good, @detouched. Let's switch to using |
- Use `in_path` to check LFS binary presence on PATH - Fix newline in the warning message when LFS binary is not on PATH
detouched
force-pushed
the
detouched/git-lfs-pre-push-hook
branch
from
22b8ef5 to
ba71f99
Compare
|
As discussed, changed the way LFS binary is looked up to |
3 participants
Git LFS installs pre-push hook which:
git lfs pre-push <remote_name> <remote_url>The introduced hook is a Ruby port of the same algorithm
modulo hook disabling note in the warning message.
For the reference, here's the original hook script: