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

Check for signed-off-by doesn't ignore comments #1809

Closed
1 of 4 tasks
JQrdan opened this issue Jun 2, 2020 · 6 comments · Fixed by #2098
Closed
1 of 4 tasks

Check for signed-off-by doesn't ignore comments #1809

JQrdan opened this issue Jun 2, 2020 · 6 comments · Fixed by #2098
Labels

Comments

@JQrdan
Copy link

JQrdan commented Jun 2, 2020

Expected Behavior

When using the rule "signed-off-by": [2, "always", "Signed-off-by:"], it doesn't seem to use the parsed commit message when reading in the lines. I would expect the values of lines to be ['subject', 'body', 'Signed-off-by: User [email protected]'].

Current Behavior

The current behaviour includes the comments that are part of .git/COMMIT_EDIT_MSG so it is checking for a 'Signed-off-by' line on the last comment rather than the last actual line of the message, causing it to always fail the check.

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Possible Solution

Use the correctly parsed input?

Steps to Reproduce (for bugs)

Make a commit using git commit -s with the setting "signed-off-by": [1, "always", "Signed-off-by:"].

Context

I'm trying to enforce signed commits in our repo.

Your Environment

Executable Version
commitlint --version 8.3.5
git --version 2.21.1
node --version v10.16.2
@escapedcat
Copy link
Member

Would you mind posting a full example message? Thanks

@JQrdan
Copy link
Author

JQrdan commented Jun 5, 2020

The commit I'm making looks like this (not done through -m):

fix: remove test file

Contributes to: #11

Signed-off-by: Jordan <[email protected]>

Which then gives the correct output in the console as:

⧗   input: fix: remove test file

Contributes to: #11

Signed-off-by: Jordan <[email protected]>

However, it gives the error: ✖ message must be signed off [signed-off-by]

When looking deeper into the code, I logged out what the value of lines is from this file:

const lines = toLines(parsed.raw).filter(Boolean);

And it gives the value as:

['fix: remove test file','Contributes to: #11','Signed-off-by: Jordan <[email protected]>','# Please enter the commit message for your changes. Lines starting','# with '#' will be ignored, and an empty message aborts the commit.,'#','# On branch actions','# Your branch is up to date with 'origin/actions'.','#','# Changes to be committed:','#       deleted:    test.js','#']

So, the value of last is # instead of the last actual line in the commit. So, it looks like it usually parses the comments out as when I disable this line:

opts.parserOpts.commentChar = '#';

The output in the console then shows all the comments as well. But, it doesn't use this parsed result when checking the last line for a Signed-off-bystring.

@escapedcat escapedcat added the bug label Jun 5, 2020
@escapedcat
Copy link
Member

If the # from the issue number is being interpreted as a comment this is a bug I guess.
Looks like the tests don't cover this use case yet.

Thanks for reporting!

@JQrdan
Copy link
Author

JQrdan commented Jun 5, 2020

The # is coming from the very last line that comes from the commit as when you edit a commit without -m you get this templated at the bottom:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch test
# Your branch is up to date with 'origin/test'.
#
# Changes to be committed:
#       new file:   test.js
#

Which seems to be ignored by other rules but not for signed-off-by.

@escapedcat
Copy link
Member

Oh I see. Thanks for the clarification!
Not sure when we can tackle this.

@bajtos
Copy link
Contributor

bajtos commented Sep 1, 2020

I opened a pull request to fix this issue, can you @escapedcat please review? See #2098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants