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

Validate: Stop validation at first comment, supports verbose output #19

Merged
merged 1 commit into from
Jun 5, 2014

Conversation

jzaefferer
Copy link
Owner

This causes the validation to accept anything after the first comment line,
which might cause more problems later. I don't see a better way to support
the --verbose option, since there is no delimiter or even indication that the
option is in use.

Fixes #15

Ping @markelog - what do you think? Is that worth the trouble?

@jzaefferer
Copy link
Owner Author

@markelog can you look into this?

@markelog
Copy link

I use verbose flag a lot, so for me it would preferable, although

anything after the first comment line

is a fragile logic and you already can think of some edge cases, but i can live with that.

@jzaefferer
Copy link
Owner Author

I wonder if this workaround would do the trick for you: http://stackoverflow.com/a/5207536/2671

Use git's prepare-commit-msg hook to comment out the diff, which would then allow commitplease to ignore it like any other comment.

@markelog
Copy link

It might, sure, if this problem was only mine to deal with, but other users could face that as well, like this might be a popular way to commit, if that true, we probably should merge this pull.

Using default functionality of some tool is always better then write some hacks for that tool i suppose.

@jzaefferer
Copy link
Owner Author

Good news! I looked into this again and discussed with Scott. Poking at git long enough turned up a much saner solution that git actually implements itself since 1.8.5. Starting with that version, a "scissor" line is included as a comment, which I'm now looking for. Regular comments are just ignored as before. @markelog, can you give that a try? Make sure you have an up-to-date git version installed.

@jzaefferer
Copy link
Owner Author

@markelog ping

@markelog
Copy link

markelog commented Jun 1, 2014

It works, but on newer version than 1.8.5. Temporary, i would leave solution you had previously, but use that code path only if you would not find scissor line.

message.split(/\n/).forEach(function ( line, index ) {
// Skip comments
// Can't cancel forEach, so abuse every to stop at first comment
message.split(/\n/).every(function ( line, index ) {
Copy link

Choose a reason for hiding this comment

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

You follow jquery code style right? Than i would change

message.split(/\n/)

to

message.split( /\n/ )

and

function ( line, index ) {

to

function( line, index ) {

@jzaefferer
Copy link
Owner Author

1.8.5 was released about half a year ago. As far as I can tell they're doing a very good job with backwards compability, so I don't see any reason for not upgrading to at least newer versions before 2.0 (haven't checked what's new/changed in 2.0).

The previous solution would break down when doing an interactive rebase with squashing of commits, where each commit message is interleaved with comments, and even the first line is a comment line.

So I'm going to fix the style issues you mentioned, then merge and do a new release.

jzaefferer added a commit that referenced this pull request Jun 5, 2014
Requires git 1.8.5+ for the scissor line to be included before the verbose
output.

See git/git@1a72cfd

Fixes #15
Closes #19
Requires git 1.8.5+ for the scissor line to be included before the verbose
output.

See git/git@1a72cfd

Fixes #15
Closes #19
@mgol
Copy link

mgol commented Jun 5, 2014

The problem is default Git on OS X 10.8 is 1.7. I also assume on Windows
people rarely update their Git versions since it doesn't prompt for it etc.
Therefore, it's hard to tell how many people it'd break.

If we want to fail on older Gits, we should make sure the user gets a clear
message why it happened and that they need to update. Otherwise we're going
to confuse a lot of people.

@qfox
Copy link

qfox commented Jun 5, 2014

@mzgol 👍 Git on Win can be easily updated to the latest version. So clear message would be fine solution for it.
Same for mac.

@jzaefferer jzaefferer merged commit 92b2c0f into master Jun 5, 2014
@jzaefferer jzaefferer deleted the ignore-verbose-output branch June 5, 2014 10:39
@jzaefferer
Copy link
Owner Author

8db07ce updates the readme to mention the recommended/required git version.

Since this problem only occurs when using --verbose, which the commit-msg hook can't detect, checking for the git version would have to always happen, which doesn't seem too useful either.

If someone wants to look into implementing that, I would review and land it, but I don't think its worth the effort.

@jzaefferer
Copy link
Owner Author

I've published 1.9.0 to npm, which deals with --verbose for git 1.8.5+.

jzaefferer pushed a commit to qunitjs/qunit that referenced this pull request Oct 8, 2014
commitplease had a bug before 1.9.0 where it didn't properly handle the
verbose flag `git commit -v`.

jzaefferer/commitplease#19 (comment)

This would lead to git diff for preview getting subjected to commit
message validation, which could abort commits if it failed.

Closes #679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verbose flag
4 participants