search

gnustavo / Git-Hooks

Framework for implementing Git (and Gerrit) hooks
http://search.cpan.org/dist/Git-Hooks/
41 stars 17 forks source link

Fix bug: CheckCommit commit object #71

Closed mikkoi closed 2 years ago

mikkoi commented 2 years ago

The earlier PR https://github.com/gnustavo/Git-Hooks/pull/69 introduced a new problem because the check_post_commit() subroutine was passing the commit parameter as a hash. To fix both problems, I reverted the earlier fix and changed now both subroutines check_post_commit() and signature_errors().

I also created a test. It is unfortunately somewhat complicated and prone to break as it depends on a peculiarity of gpg program, possibly a bug. I tested with an earlier version of gpg and I could not run the test successfully. But with gpg version 2.2.27 the tests work. If the local version of gpg is not suitable, the tests will skip that part and the test suite will not fail. It tests signature in both post-commit (client) and update (server) hooks.

mikkoi commented 2 years ago

We could also leave the unit test out of the bug fix and only include the other commits?

mikkoi commented 2 years ago

This case is still open. I get tired of using CheckCommit=0 when I commit. :grin:

gnustavo commented 2 years ago

Hi Mikko. I'm sorry for the delay...

Thanks again for spotting yet another bug!

I understood the problem and I'm going to incorporate your solution. I'm going to cherry-pick your commits instead of merging the whole PR because it does more than one thing, OK?

mikkoi commented 2 years ago

I'm going to cherry-pick your commits instead of merging the whole PR because it does more than one thing, OK?

No problem. That pull request is a bit of a monster, especially with the big test file.

gnustavo commented 2 years ago

I just pushed a few commits to the next branch. They implement all of yours but the one adding test cases. I'm going to review it later.

Note that commit 91948b35 re-implements your fix using the Git::Repository::Plugin::GitHooks::get_commit routine, as it's already capable of converting a revision string into a Git::Repository::Log object. Also, I noticed that there was another place needing that fix.

What do you think?

mikkoi commented 2 years ago

It is better to pass objects than revision hash strings.

gnustavo commented 2 years ago

Good!