What about git LFS? - Githubissues

AlexGoris-Cashforce commented 5 years ago

I'm preparing a pull request with some changes which would make this project better suited for our internal use, however I've stumbled across an issue:

we have some repositories using git-lfs (large file storage), which als uses hooks to push lfs objects (matched through filters specific in .gitattributes) to the LFS storage which corresponds to the current repository. These hooks effectively get overwritten when installing this project. So should we include the LFS hooks in .githooks/* on all repo's which use LFS, or is there a better/other solution?

rycus86 commented 5 years ago

Hm, I haven't really used git-lfs yet, so I'm not sure how this should work. We could add a test for it and see what happens, I'd imagine it might need some fixes.

Thanks for bringing this here!

gabyx commented 5 years ago

We also use git lfs? How does git lfs install install files in the .git/hooks directory? I have not seen any?. Isnt git-lfs invoked by git itself through the settings in the global config (installed with git lfs install) For every commit git pushed the files itself to LFS , the ones which match the .gitattributes files...??

rycus86 commented 5 years ago

If either of you could set up a sample repo to test on, I could look into adding a test for it.

AlexGoris-Cashforce commented 5 years ago

I've set up a test repo with a 10MB.zip file added through git-lfs https://github.com/AlexGoris-Cashforce/githooks-lfs-test

The documentation around git-lfs seems to be a bit iffy... In the manpage you can read that git lfs install, amon other things:

Install a pre-push hook to run git-lfs-pre-push(1) for the current repository, if run from inside one. If "core.hooksPath" is configured in any Git configuration (and supported, i.e., the installed Git version is at least 2.9.0), then the pre-push hook will be installed to that directory instead.

Now it does more than just installing a pre-push hooks, if I run git lfs install in a clean repo, I get 4 new hooks:

post-checkout
post-commit
post-merge
pre-push

When installing githooks (this project), those hooks get replaced and the lfs ones get renamed to .replaced.githook, essentially breaking git-lfs. The dangerous thing is that the command line git client will not warn you about this. Because of the .gitattributes file which contains e.g. *.zip filter=lfs diff=lfs merge=lfs -text any matched files (*.zip in this case) will still be converted to lfs objects, and the file in the git repository replaced by an ASCII file containing a pointed to the LFS object, but the actual LFS object won't get pushed to the LFS host. It's only if someone else tries to clone the repo (and they are on a branch which contains LFS objects which have not been pushed) that you would start seeing warnings about lfs not being able to find the file.

rycus86 commented 5 years ago

Awesome, thanks @AlexGoris-Cashforce ! Yeah, this does sound like a bug, those replaced hooks should still run I believe. Will try to look into it this weekend. 👍

gabyx commented 5 years ago

Ohh, thanks for this analysis, this is also very important for our use case as well.

rycus86 commented 5 years ago

OK, so we should definitely run those hooks, see https://github.com/rycus86/githooks/blob/master/base-template.sh#L100 If it doesn't happen, could you try adding a set -x at the beginning of the new .git/hooks/pre-push file, so we could see what actually happens when it should run the new and old hooks? If not, I'll try to test it myself when I have a bit more time.

Thanks!

gabyx commented 5 years ago

The other important thing I noted is. (latest git): Note: git lfs install needs to be run once on the system, it will install itself into repos when cloning...

I cloned and checked out a master branch which does not have lfs configured. Probably no .gitattributes file, or lfs pointers. The hooks from lfs get not installed. If I switch however to another branch which has lfs configured -> the hooks get overwritten silently, this is kind of stupid ... I dont know what happens when we set core.hooksPath. Will test this...

gabyx commented 5 years ago

Ok with the core.hooksPath -> on branch switch to a branch where lfs is "active" -> lfs doesnt touch the .git/hooks folder obviously, but it silently overwrites the hooks in core.hooksPath.

gabyx commented 5 years ago

So it seems, that lfs (once installed with git lfs install, just checks if there are no hooks files as described by @AlexGoris-Cashforce : it newly creates them either inside core.hooksPath or .git/hooks.

When we clone a repository

The templates from dir ~/.githooks/templates get installed
If you optionaly run git lfs install it warns you because we have already a hook
```
To resolve this, either:
1: run `git lfs update --manual` for instructions on how to merge hooks.
2: run `git lfs update --force` to overwrite your hook.
```
I say optionaly because the user should not really be used to do this as the documentation of lfs says.

Might it be the best to include the lfs code in the ~/.githooks/templates but that makes the lfs hooks set for all repositories we use githooks with...

What do you think?

rycus86 commented 5 years ago

Yeah, I was thinking about something like this, since lfs wants to hooks to itself according to its source code. I could check in our hooks if you have any lfs filters in .gitattributes and run the lfs hooks if there are, but it wouldn't get rid of the warning. I still haven't tested this properly, but the source code of lfs seems to just do an exact equals test for the hooks content. (Maybe the first 1024 characters of it or something like that)

So in short, I can try to run the lfs hooks from this project, but won't be a 100% user friendly experience...

gabyx commented 5 years ago

Where did you find that line about checking the file in lfs?

Its a solution to run the lfs hooks if there is a .gitattribute file. Would that break something or is the existence of this file enough? hm... And when we copy the lfs command into this project, isnt that prone to furture changes in lfs? ...

gabyx commented 5 years ago

Its definitely not so nice, that lfs silently overwrites stuff... >The install procedure is a bit wacky... I dont know..

AlexGoris-Cashforce commented 5 years ago

What I've done for our repositories using LFS, I've just added the lfs scripts into the ./githooks/* directory for every repository. I think that should be enough for most users of this project, but we should probably document it in the README.md so that users are made aware they need to do this change

gabyx commented 5 years ago

Ah ok, thats also a solution.

gabyx commented 5 years ago

I still have some issues with the LFS: IF we have the LFS hooks in a shared repo and include them in a repo .shared file everythin works. We have the flag githooks.failOnNotAvailableSharedHooks which I set to true, such that if the hooks are not there, any action needing toe execute these hooks will fail.

There is still one issue. The user is still prompted to trust the hooks reference in .shared (where the LFS hooks are placed).

Do you you accept the changes? (Yes, all, no, disable)

He can then deny it (or even disable the hook :-|). which will refuse to execute the hook (meaning no LFS hook is run). How can we avoid this?

Suggestion 1: Honour the flag file .githooks/trust-all also in shared/global repos. All prompts will be skipped.

Of course the user can always hack things and accidentially not execute LFS hooks placed in shared hook repos. But thats another thing. I just would like that the user cannot accidentally disable stuff by giving them the option to do so...

Can I provide a PR for this simple fix?

rycus86 commented 5 years ago

No, sorry, I don't want to support hooks without the user accepting them.

We could potentially special case LFS handling if we detect that the current repo has it enabled. Do you know any good way of working that out?

gabyx commented 5 years ago

Checking for .gitattribute files in the whole repo. Or Looking if something like

[filter "lfs"]
    clean = git-lfs clean -- %f
    smudge = git-lfs smudge -- %f
    process = git-lfs filter-process
    required = true

is placed in the config...

gabyx commented 5 years ago

I think it needs both, first check if .gitattributes files are present somewhere in the repo (this is expensive), and then if a lfs filter is configured either locally or globaly -> RUN LFS hooks...

No, sorry, I don't want to support hooks without the user accepting them.

Ok also the .githooks/trust-all asks you once if you want to trust etc...

rycus86 commented 5 years ago

Cool, thanks. Is there an easy way to test LFS? Never used or set it up before.

gabyx commented 5 years ago

there is git-lfs the executable this needs to be on the path or setup... but that does not mean the repo uses it. Can we quickly test for a file .gitattributes in the index?

LFS is used if there is a .gitattributes file somewhere in the repo, and it contains a line like
```
<pattern> filter=lfs diff=lfs merge=lfs -text
```

gabyx commented 5 years ago

git ls-files ':(attr:filter=lfs)'

List files with that filter...

gabyx commented 5 years ago

But whats wrong by provinding a shared repo with the LFS hooks in place and have them in .githooks/.shared. We just need to be sure this .shared repo cannot be disregarded, that was my approach it looks pretty clean :-)

gabyx commented 5 years ago

My suggestion:

Integrate all original lfs hooks into the base-template.sh(those are oneliners...) and run them always if git-lfs --version is available (it does not matter if you do not use LFS, the hooks will pass through). In that way we also dont need to trust the lfs-hooks if these are files somewhere, because we integrated them into our template.
Provide an option (local or global) (git config --get githooks.disableInternalLFSHooks) with which we can disable running the integrated lfs hooks, if its needed. Generally you always want these hooks to be run, so disabling the option is only for pros who know what they do....

Is that an acceptable procedure? In this way we dont need to do stupid checks and such (checking gitattributes file etc...).

rycus86 commented 5 years ago

Yep, that sounds sensible, I was thinking along these lines. Don't know if having the version check is enough or not, if LFS hooks are really no-op, and they're not too slow on a repo that doesn't use LFS, then yeah, we can do it like that.

gabyx commented 5 years ago

git-lfs --version check only tells you that LFS is available. If its not available, and the user uses a repo where he should use lfs, then are we responsible to tell him that the repository should use LFS? I guess not (clearly it would be nice) but git also does not do that. If you install LFS on your machine, then every git clone you do will install the LFS Hooks, so meaning they already run even if you dont use the feature. So I think it should be fine.

rycus86 commented 5 years ago

Cool, thanks for the explanation! :+1:

rycus86 commented 5 years ago

This is now merged in.

gabyx commented 5 years ago

Awesome thx!

rycus86 / githooks

What about git LFS? #17