commitlint icon indicating copy to clipboard operation
commitlint copied to clipboard

False-positive `body-leading-blank` warnings for GPG-signed commits.

Open mschilli87 opened this issue 3 years ago • 5 comments

This is a follow-up to https://github.com/conventional-changelog/commitlint/issues/876.

The upshot is that I am getting

âš  body must have leading blank line [body-leading-blank]

warnings for GPG-signed commits that actually do have leading blanks.

Furthermore, the very same commit messages do not trigger that warning when passed in from STDIN or when not GPG-signing the commits.

My guess is that the original issue might have been 'resolved' when switching to the other computer by different Git default configurations on the two systems involved (the first one GPG-signing by default, the second one not).

Expected Behavior

When a message contains a blank line between header and body it should not trigger a body-leading-blank warning, regardless whether the corresponding commit was GPG signed or not.

Current Behavior

When a message contains a blank line between header and body it does not trigger a body-leading-blank warning when passed in via STDIN or queried from the Git log via a non-GPG-signed commit, but does so when queried from the Git log via a GPG-signed commit.

Affected packages

  • [X] cli
  • [ ] core
  • [ ] prompt
  • [ ] config-angular

Steps to Reproduce (for bugs)

  1. Create test repository:

    mkdir test
    cd test
    git init --quiet
    
  2. Write test commit message to a text file.

    cat > commit-message << CommitMessage
    test: a totally valid commit message
    
    This message has a body that is separated from the header line by an
    empty line.
    CommitMessage
    
  3. Create default commitlint config:

    cat > commitlint.config.js << CommitlintConfig
    module.exports = {extends: ["@commitlint/config-conventional"]}
    CommitlintConfig
    
  4. Ensure that commit message does not trigger a body-leading-blank warning when passed in via STDIN:

    commitlint \
        --config=commitlint.config.js \
        < commit-message
    
  5. Create non-GPG-signed commit using that exact same message:

    git add .
    git commit \
        --quiet \
        --no-gpg-sign \
        --file=commit-message
    
  6. Ensure that commit message does not trigger a body-leading-blank warning when passed in via the non-GPG-signed commit:

    commitlint \
        --config=commitlint.config.js \
        --to=HEAD
    
  7. Create a temporary GnuPG home directory:

    export GNUPGHOME="$(pwd)/gpg"
    mkdir "$GNUPGHOME"
    chmod 700 "$GNUPGHOME"
    
  8. Generate a temporary GPG key:

    # WARNING: This is meant for testing purposes only!
    # This is not my recommended way of generating GPG keys for production
    # use. Please refer to the appropriate documentation when setting up GPG
    # to sign your actual commits.
    gpg \
        --quiet \
        --batch \
        --gen-key << GnuPGKeyGenConfig
    Key-Type: 1
    Key-Length: 2048
    Subkey-Type: 1
    Subkey-Length: 2048
    %no-protection
    Name-Real: Test User
    Name-Email: [email protected]
    Expire-Date: 0
    GnuPGKeyGenConfig
    
  9. Amend the commit to GPG-sign it without changing the commit message:

    git commit \
        --quiet \
        --amend \
        --gpg-sign='Test user <[email protected]>' \
        --no-edit
    
  10. Observe the false-positive body-leading-blank warning when queried from the Git log via the GPG-signed commit:

    commitlint \
        --config=commitlint.config.js \
        --to=HEAD
    
    â§—   input: test: a totally valid commit message
    âš    body must have leading blank line [body-leading-blank]
    
    âš    found 0 problems, 1 warnings
    ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
    
    
commitlint.config.js
module.exports = {extends: ["@commitlint/config-conventional"]}  

Context

See this blogpost by Mike Gerwitz for a nice writeup of why GPG-signing commits is a good idea. If I have to decide between using commitlint or GPG-signing my commits, I'll choose the latter.

Your Environment

This is on Gentoo using Node.js and Git from Portage but commitlint installed via

npm install --global '@commitlint/cli'

Furthermore, the Conventional Commits configuration module was installed via

npm install --global '@commitlint/config-conventional'

For it to be found, I also had to add the following to my shell environment:

export NODE_PATH="$HOME/.npm-global/lib64/node_modules"
Executable Version
commitlint --version @commitlint/[email protected]
git --version git version 2.34.1
node --version v14.17.6
``

mschilli87 avatar Feb 01 '22 14:02 mschilli87

Thanks for this! Before having a closer look, do you think these are related?:

  • https://github.com/conventional-changelog/commitlint/issues/321#issuecomment-428319119
  • https://github.com/conventional-changelog/commitlint/issues/59#issuecomment-350372055

escapedcat avatar Feb 03 '22 01:02 escapedcat

@escapedcat:

[...] [D]o you think these are related?:

I don't see how that one would be related beyond the fact that they both deal with signed commits. It is asking for a new feature allowing to enforce signed commits. I am just asking to fix a bug where an unrelated already existing check is broken when applied to signed commits.

That one is entirely unrelated: It is about signing off commits using -s/--signoff which simply adds a specific line to the commit message footer and is commoly used as some sort of 'signature' in the sense that the commiter confirms code ownership or such things. My issue is with commits signed cryptographically via -S/--gpg-signed to prevent untrusted parties from modifying the source code once the commit is published.

mschilli87 avatar Feb 03 '22 07:02 mschilli87

Alright, thanks for you feedback.
Would you be motivated and have time to look into this and maybe create a PR for this?

escapedcat avatar Feb 03 '22 08:02 escapedcat

@escapedcat

Would you be motivated and have time to look into this and maybe create a PR for this?

I am sorry but I have no experience with TypeScript or even simple JavaScript so I doubt I'll be able to come up with a viable solution by myself in a reasonable timeframe.

mschilli87 avatar Feb 03 '22 08:02 mschilli87

I am sorry but I have no experience with TypeScript or even simple JavaScript so I doubt I'll be able to come up with a viable solution by myself in a reasonable timeframe.

Alright, no worries

escapedcat avatar Feb 03 '22 09:02 escapedcat