Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Simple prepare-commit-msg hook #170

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcin-lawrowski
Copy link
Contributor

Fixes #107

It is WIP. I implemented only Xmllint reporting

@marcin-lawrowski
Copy link
Contributor Author

@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"
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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 😄

Copy link
Contributor Author

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 ;-)

Copy link
Contributor Author

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?

Copy link
Contributor

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).

@westonruter
Copy link
Contributor

@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:

  1. Generate a default commit message (if a default one isn't already present, e.g. for a merge commit)
  2. Add validation that the commit message entered is in a good format (e.g. short summary line).

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. feature/ABC-123-add-awesome-feature. This could result in a default commit message that consists of:

ABC-123:

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.

@marcin-lawrowski
Copy link
Contributor Author

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:
bugfix/issue-123-something

@westonruter
Copy link
Contributor

@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")
Copy link
Contributor

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.

Copy link
Contributor

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.

@marcin-lawrowski
Copy link
Contributor Author

OK, I moved #131 features in a separate PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants