gnustavo / Git-Hooks

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

CheckJira should be able to disregard some commits by their message contents #58

Open gnustavo opened 5 years ago

gnustavo commented 5 years ago

The CheckJira plugin looks for Jira keys in the commit messages and is usually configured to abort if it can't find any.

However, when it processes commits generated automatically (such as merge, squash, and revert commits) the messages are generated automatically and may not satisfy all CheckJira constraints.

There are situations in which these commits are generated at the git server (such as when you interate a pull request in Bitbucket server) and the user don't have a chance to amend the commit before it's fed to the hook.

So, it's useful to be able to detect and disregard such commits.

The idea is to implement a new directive called githooks.checkjira.skip-message which would accept one or more regexes. If any one of them maches the commit message, the commit should be skipped from consideration by the hook.

boudekerk commented 5 years ago

I think for most cases this can already be achieved by combining the CheckJira and CheckLog plugins:

githooks.checkjira.matchkey=\b[A-Z][A-Z]+\d?-\d+\b
githooks.checkjira.require=false
githooks.checklog.title-match=(\b[A-Z][A-Z]+\d?-\d+\b)|<CUSTOM COMMIT MESSAGE>
gnustavo commented 5 years ago

Clever, @boudekerk !

The error message would be very opaque to the user, though. I think this specific case deserves a specific solution in CheckJira.

Or perhaps a more general solution common to all plugins... Something like githooks.dont-check-commit-matching-message.

But your solution can solve the problem right now. Thanks!

boudekerk commented 2 years ago

We are actually running into the issue where people seeing it the first time aren't clear about the issue. So if the specific solution in CheckJira would be considered, that might be nice.

The general one I think would benefit from a second property specifying which plugins. I can imagine that you wouldn't want there to be a message to skip every check. For example ACL, but which ones might be different for everyone.

gnustavo commented 1 year ago

Hi @boudekerk ,

Can you try the new CheckJira's skip-logs directive I implemented in the commit bf81b1a3a2c79 to see if it works for you?

boudekerk commented 1 year ago

I just tested it, and that would work great for us. Thanks!

But I'm not sure skip-logs should have a default like that? It's good to have a sensible suggestion for a value in the documentation, but adding it by default. does break backwards compatibility.

And I probably wouldn't have the revert in there, that might very well warrant a Jira ticket with an explanation.

P.S. Any chance you could have a look at #77 ?

gnustavo commented 1 year ago

@boudekerk , I've rewritten that commit to remove the default and to mention it in the documentation.

I created PR #78. Can you comment on it?

Regarding #77 , I'm thinking about it. I'm not sure what's the best way to support more than one Jira server. We have two at work, but so far only one of them is used for software development, so that we've never found the need to mention the other one in our commits. But I can see that it would be useful. I'll comment on it when I have something sensible to say.

boudekerk commented 1 year ago

78 Looks good to me.

Regarding my PR #77. Are you doubtful about my chosen solution, or the way I implemented it? I know I'm not a developer, so it might not be the best and/or nicest code.