fix!: fix signed-off-by detection

[CJKay]

Description

This change replaces the manual parsing of the commit message when looking for Signed-off-by: trailers with an invocation of git interpret-trailers, which parses the trailers of the commit message passed via stdin. It's not as robust as I'd like it to be - it should ideally take into account the local Git configuration - but it at least resolves a major issue blocking my team from using the signed-off-by rule.

One known issue with this approach is that BREAKING CHANGE - if it occurs grouped alongside the sign-off trailers - will break detection. This appears to be due to the fact that Git does not accept trailers with spaces, instead treating them as part of the body (this seems to be an issue of the Conventional Commits specification more than anything). This does not occur with BREAKING-CHANGE, which is considered a valid trailer.

Motivation and Context

The signed-off-by rule does not correctly detect Signed-off-by: trailers if they are not the final non-comment line of the commit. This change ensures that sign-off trailers are correctly detected according to the formatting rules Git uses to insert them.

For example, the following commit message breaks today:

fix: commit subject

Commit body.

Signed-off-by: Chris Kay <chris.kay@arm.com>
Change-Id: I71af06c09d95c2c58e3fd766c4a61c5652637151

How Has This Been Tested?

This was tested by updating and running the existing signed-off-by rule tests to ensure that trailers after the Signed-off-by: trailer are now accepted.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[CJKay]

Hm... that's unfortunate. This does require at least Git 2.15.4 (2019-12-06).

[escapedcat]

Hey, thanks for the PR!

I'm not exactly sure where the git version for circle-ci is being set. I'm guessing maybe here:

• https://github.com/conventional-changelog/commitlint/blob/master/.circleci/config.yml
• https://github.com/conventional-changelog/commitlint/blob/master/docker-compose.yml#L5
  https://hub.docker.com/r/marionebl/commitlint-cubicle
  Not sure when this is being used

In general I think we could update the required git version by now to support your PR. Just need to figure out how/where to do this (apart from the change in the README).

You have an idea, where we could update the git version?

[AdeAttwood]

Ref: conventional-changelog/conventional-changelog#773

[AdeAttwood]

I also don't think updating git is an issue. If I am reading the code correctly then it's only a required to have git 2.15.4 for this rule. Then IMO not a braking change because all existing configs will be not break.

I do have and issue with the name of the rule "signed-off-by". The name "signed-off-by" to me suggests validating the email of the Signed-off-by: trailer. This only validates that the trailer Signed-off-by: is in the commit footer. In fact its even more general than that, even the name trailer-exists exists I think would be better due to the value param. This can be used to validate the trailer Co-authored-by: trailer and would work.

...
rules: {
    'trailer-exists': [1, 'always', 'Co-authored-by:']
}
...

[CJKay]

I also don't think updating git is an issue. If I am reading the code correctly then it's only a required to have git 2.15.4 for this rule. Then IMO not a braking change because all existing configs will be not break.

The breaking change here is that you can no longer use BREAKING CHANGE: alongside other trailers, e.g.:

fix: add a bugfix

Lorem ipsum.

BREAKING CHANGE: A breaking change.
Signed-off-by: Chris Kay <chris.kay@arm.com>

This would have worked before, but because Git does not interpret "BREAKING CHANGE" as a valid trailer, to correctly detect the sign-off you would now have to use either:

fix: add a bugfix

Lorem ipsum.

BREAKING-CHANGE: A breaking change.
Signed-off-by: Chris Kay <chris.kay@arm.com>

Or:

fix: add a bugfix

Lorem ipsum.

BREAKING CHANGE: A breaking change.

Signed-off-by: Chris Kay <chris.kay@arm.com>

I do have and issue with the name of the rule "signed-off-by". The name "signed-off-by" to me suggests validating the email of the Signed-off-by: trailer. This only validates that the trailer Signed-off-by: is in the commit footer. In fact its even more general than that, even the name trailer-exists exists I think would be better due to the value param. This can be used to validate the trailer Co-authored-by: trailer and would work.

...
rules: {
    'trailer-exists': [1, 'always', 'Co-authored-by:']
}
...

This could be a useful tool for us, so long as we had some level of flexibility with the importance of multiple individual trailers trailers - we use Gerrit, so all our commits have both a Signed-off-by: trailer and a Change-Id: trailer. At the moment these are rejected at the remote side.

We'd need to be able to support multiple trailer specs though, so we would need behaviour akin to:

...
rules: {
    'trailer-exists': [2, 'always', 'Signed-off-by:'],
    'trailer-exists': [1, 'always', 'Change-Id:']
}
...

Not sure how you'd do that.

[AdeAttwood]

My feeling is that we create a new rule "trailer-exists" that has this implementation. Then "signed-off-by" rule stays the same.

If you want more trailers you can add a local plugin to duplicate the rule

const {"trailer-exists": trailerExists} = require('@commitlint/rules').default;

module.exports = {
    plugins: [
        {
            rules: {
                'signed-off-by-exists': trailerExists,
                'change-id-exists': trailerExists,
            },
        },
    ],
    rules: {
        'signed-off-by-exists': [2, 'always', 'Signed-off-by:'],
        'change-id-exists': [1, 'always', 'Change-Id:'],
    },
};

@escapedcat @CJKay what are your thoughts

[CJKay]

That's neat; I think that would resolve my own use-case.

[escapedcat]

Sounds good to me. @CJKay will you change this PR accordingly?

[CJKay]

Sounds good to me. @CJKay will you change this PR accordingly?

I'm no JavaScript developer, but I reckon I can figure it out.

[CJKay]

Closing in favour of #2578.