Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Spellcheck commit-msg hook #162

Merged
merged 3 commits into from
Apr 6, 2015
Merged

Add Spellcheck commit-msg hook #162

merged 3 commits into from
Apr 6, 2015

Conversation

jawshooah
Copy link
Collaborator

This hook uses hunspell to check the commit message for potential misspellings, and suggests alternate spellings if available.


def run
# Remove comments from commit message file
update_commit_message(commit_message)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing now that calling a destructive operation on the commit message file as part of a hook run is dangerous if we were to ever parallelize hook runs.

I realize you didn't add this helper, but I also realize that this helper isn't actually used anywhere (until now).

Ideally, we would create a temporary file with the comment lines removed and run the hunspell tool against that.

However, I also acknowledge that commit-msg hooks would want to be able to modify the commit message at some point.

For now, since this hook doesn't modify the commit message, let's run hunspell against a separate temporary file. We still need to think about the GerritChangeId hook, which modifies the commit message file directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I've updated the hook accordingly.

As for parallelizing hook runs, any hooks that directly modify the commit message file will probably have to be run serially, which can be specified in the config as discussed in #144.

@sds sds merged commit 35ee050 into sds:master Apr 6, 2015
@sds sds added the enhancement label Apr 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants