-
Notifications
You must be signed in to change notification settings - Fork 58
Simple prepare-commit-msg hook #170
base: master
Are you sure you want to change the base?
Simple prepare-commit-msg hook #170
Conversation
@westonruter Please take a look at this. I would need to know if it is a good approach. I had no idea how to pass data from the first hook to the second. I used a temporary file. |
function add_commit_message { | ||
label=$1 | ||
value=$2 | ||
DEV_LIB_COMMIT_MESSAGE+="\n$label: $value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcin-lawrowski instead of appending to a variable, how about just appending it to the file?
echo "$label: $value" >> $DEV_LIB_COMMIT_MESSAGE_FILE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When pre-commit terminates with return code other than 0 the file should be cleaned as well. In this approach the file is created only if all tests passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but I don't think that's so important now, since the file is getting stored in /tmp
. It will automatically get garbage-collected with the next reboot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I wasn't aware that Bash supported the +=
operator. Thanks for enlightening me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pleasure is all mine ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary files will be garbage-collected with the next reboot, but what about users that use this tool in some server environment which isn't rebooted often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files are super tiny, so I'm not so concerned about it, since this is going to be used on dev environments not servers. Also, there are a lot of other files being written to /tmp
when running the pre-commit
hook and Travis CI builds (see usages of TEMP_DIRECTORY
in https://github.com/xwp/wp-dev-lib/blob/master/check-diff.sh).
@marcin-lawrowski thanks! Yeah, a temp file seems like a good way to go. I'm a bit confused by the Xmllint reporting part of this. Are you actually working on #131 instead of #107? These are closely related of course. I think what we're looking at here consists of two parts:
For the default commit message, I'd suggest it look at the current branch name and determine if it looks like it contains a JIRA ticket number, e.g.
The commit message validation hook can then be used to verify that they actually have entered a commit message after the ticket number, for example. There's some discovery inherent with this issue, so please explore what enhancements we could automate to the workflow and figure out what makes sense. |
Thanks @westonruter for review. Actually I was thinking on merging the two issues together because they are closely related. I choose Xmllint as a POC. Should I leave it as it is or prepare a separate PR? In the next commit I prepared detecting of Jira task signature in branch name. The same way we can detect GitHub isses: |
@marcin-lawrowski yeah, let's do the amending of the status checks (#131) in a separate PR. I think it's a more controversial thing to include and it will involve more logic. |
JIRA_TASK_REGEXP='.*([A-Z]{1,32}-[0-9]{1,32}).*' | ||
JIRA_BRANCH_REGEXP='([a-zA-Z]+-[0-9]{1,32})'; | ||
if [[ ! $(cat "$COMMIT_EDITMSG") =~ $JIRA_TASK_REGEXP ]]; then | ||
JIRA_TASK=$(git rev-parse --abbrev-ref HEAD | grep -oP "$JIRA_BRANCH_REGEXP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the -P
argument is not recognized on OSX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, make sure to include the -E
param to indicate that extended regular expressions are to be used in grep
. Otherwise, the +
repeater and bracket notation don't work on OSX.
OK, I moved #131 features in a separate PR. |
Fixes #107
It is WIP. I implemented only Xmllint reporting