Support for running Git LFS hooks :green_heart:

rycus86 commented 5 years ago

This is my attempt to solve #17 @gabyx @AlexGoris-Cashforce can you please have a look?

rycus86 commented 5 years ago

Unfortunately, Git LFS will still print a warning on install that the existing hook is not theirs, but otherwise should be working as far as I can tell.

gabyx commented 5 years ago

Nice. Maybe we should add a test. which without installing githooks inits a repo, and compares the output .git/hooks/* to the hook switch we are using inside base-template. In that way, we are sure we have the same hook-switch internally. Maybe we can also check that during installation (why not?) so if something goes wrong it happens during installation...

gabyx commented 5 years ago

Unfortunately, Git LFS will still print a warning on install that the existing hook is not theirs, but otherwise should be working as far as I can tell.

What is the output of that?

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 267

14 of 17 (82.35%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.08%) to 90.658%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
base-template.sh	14	17	82.35%
<!--	Total:	14	17	82.35%	-->

Totals
Change from base Build 253:	-0.08%
Covered Lines:	1611
Relevant Lines:	1777

💛 - Coveralls

rycus86 commented 5 years ago

What is the output of that?

It says:

$ git lfs install
Hook already exists: pre-push

        #!/bin/sh
        # Base Git hook template from https://github.com/rycus86/githooks
        #
        # It allows you to have a .githooks folder per-project that contains
        # its hooks to execute on various Git triggers.
        #
        # Version: 1907.110919-c0438b

        #####################################################
        # Execute the current hook,
        #   that in turn executes the hooks in the repo.
        #
        # Returns:
        #   0 when successfully finished, 1 otherwise
        #####################################################
        process_git_hook() {
        are_githooks_disabled && return 0
        set_main_variables
        export_staged_files
        check_for_updates_if_needed
        execute_old_hook_if_available "$@" || return 1
        execute_global_shared_hooks "$@" || return 1
        execute_local_shared_hooks "$@" || return 1
        execute_all_hooks_in "$(pwd)/.githooks" "$@" || return 1
        }

        #####################################################
        # Checks if Githooks is completely disabled
        #   for the current repository or globally.
        #   This can be done with Git config or using
        #   the ${GITH

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.

rycus86 commented 5 years ago

Maybe we should add a test. which without installing githooks inits a repo, and compares the output to the hook switch we are using inside base-template.

You mean 1) install LFS first with its hooks, then 2) install Githooks and 3) check Githooks hooks are run with also triggers git-lfs ?

gabyx commented 5 years ago

Ah that one, ah easy. git lfs install is done on a machine before githooks is installed. So it should be fine...

For the tests I mean:

git init should install all lfs hooks (when you have done git lfs install
Check the automatically installed lfs original hooks in .git/hooks/* against our function execute_lfs_hook_if_appropriate. Making sure we use lfs for the same hooks as Lfs automatically installs.

rycus86 commented 5 years ago

Oh OK, I think I see what you're saying. I'll try to have a look tomorrow/this week, time permitting. Thanks for the comments!

gabyx commented 5 years ago

Second test.

git install lfs
./install.sh
git init
git commit
check if post-commit lfs has been run maybe mock git-lfs by config:
```
[alias] 
lfs = our-mock-script
```

gabyx commented 5 years ago

Try to add the first test case.

rycus86 commented 5 years ago

Try to add the first test case.

I'm also looking into this, just got it working. Will push in a minute.

gabyx commented 5 years ago

See two commits on the branch : https://github.com/gabyx/githooks/tree/lfs-support There is also the first LFS test.

rycus86 commented 5 years ago

See two commits on the branch : https://github.com/gabyx/githooks/tree/lfs-support

Yep, similar approach, different implementation. :)

rycus86 commented 5 years ago

OK, added both tests now. Can you please have a look @gabyx ? Do you have anything extra on your branch you'd want me to consider?

gabyx commented 5 years ago

Plz have a look at this, can we include that. My branch commit : e6f01e9

rycus86 commented 5 years ago

Plz have a look at this, can we include that. My branch commit : e6f01e9

Why not just have a .githooks/pre-commit/require-lfs and provide your own script? I'm not against the extension, just wondering what's the added value? Less repetition? Easier setup?

gabyx commented 5 years ago

Ah jeah of course :). We should write a Readme, to include whats happning with LFS for the user! Maybe add this to an example/pre-commit/require-lfs

That would be nice.

gabyx commented 5 years ago

The PR is nice, so with that and #29 I can finally test the setup for the company wide Git Installation. Thx!

rycus86 commented 5 years ago

Ah jeah of course :). We should write a Readme, to include whats happning with LFS for the user!

Added #31 to track it.

rycus86 commented 5 years ago

Plz have a look at this, can we include that. My branch commit : e6f01e9

Can you check the PR now? I added support for the .githooks/.lfs-required file.

rycus86 commented 5 years ago

Sooo, bit of a complication with .lfs-required. Most LFS hooks are post-* which can't fail the operation since they run after that. So that leaves us with pre-push. Is that good enough to fail there? Would you think maybe we should check elsewhere too? Like pre-commit ?

In any case, I'll look into it tomorrow.

gabyx commented 5 years ago

git lfs hooks are:

Add the following to .git/hooks/pre-push:

        #!/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 "$@"

Add the following to .git/hooks/post-checkout:

        #!/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/post-checkout.\n"; exit 2; }
        git lfs post-checkout "$@"

Add the following to .git/hooks/post-commit:

        #!/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/post-commit.\n"; exit 2; }
        git lfs post-commit "$@"

Add the following to .git/hooks/post-merge:

        #!/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/post-merge.\n"; exit 2; }
        git lfs post-merge "$@"

Strange that they do not provide git lfs pre-push "$@" || exit 1 Hm... do you know why?

gabyx commented 5 years ago

Maybe only fail at pre-push, yes. And print a warning for the others if something happened? Why not also exit 1?

gabyx commented 5 years ago

For me that PR looks good, merge when ever you are ready. Thx!

rycus86 commented 5 years ago

I think it does exit with non-zero and we will too. But as https://git-scm.com/docs/githooks#_post_commit states:

This hook is meant primarily for notification, and cannot affect the outcome of git commit.

rycus86 commented 5 years ago

I'll try to wrap this up this evening and merge it in.

rycus86 commented 5 years ago

OK, added one more test, and will merge this once Travis is happy. Some good news: post-checkout fails both the initial git clone and also further git checkout if the .githooks/.lfs-required file is there but git-lfs is not found. :tada:

gabyx commented 5 years ago

ah perfect!

Von meinem iPhone gesendet

Am 01.08.2019 um 13:18 schrieb Viktor Adam notifications@github.com:

OK, added one more test, and will merge this once Travis is happy. Some good news: post-checkout fails both the initial git clone and also further git checkout if the .githooks/.lfs-required file is there but git-lfs is not found. 🎉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gabyx commented 5 years ago

But strange when you clone you clone the repo file .githooks/.lfs-required file and afterwards its fails?

Von meinem iPhone gesendet

Am 01.08.2019 um 13:18 schrieb Viktor Adam notifications@github.com:

OK, added one more test, and will merge this once Travis is happy. Some good news: post-checkout fails both the initial git clone and also further git checkout if the .githooks/.lfs-required file is there but git-lfs is not found. 🎉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gabyx commented 5 years ago

got it :)

Von meinem iPhone gesendet

Am 01.08.2019 um 14:12 schrieb Ganriel Nützi gnuetzi@gmail.com:

But strange when you clone you clone the repo file .githooks/.lfs-required file and afterwards its fails?

Von meinem iPhone gesendet

Am 01.08.2019 um 13:18 schrieb Viktor Adam notifications@github.com:

OK, added one more test, and will merge this once Travis is happy. Some good news: post-checkout fails both the initial git clone and also further git checkout if the .githooks/.lfs-required file is there but git-lfs is not found. 🎉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rycus86 / githooks

Support for running Git LFS hooks :green_heart: #30

Pull Request Test Coverage Report for Build 267

💛 - Coveralls