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

fix!: fix signed-off-by detection #2567

Closed
wants to merge 1 commit into from
Closed

fix!: fix signed-off-by detection #2567

wants to merge 1 commit into from

Conversation

CJKay
Copy link
Contributor

@CJKay CJKay commented Apr 28, 2021

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 <[email protected]>
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.

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.

The change replaces the manual parsing of the commit message with an
invocation of `git interpret-trailers`, which parses the trailers of
the commit message passed via stdin.

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.

BREAKING CHANGE: `BREAKING CHANGE` in the footers block will break `Signed-off-by:` detection.

Signed-off-by: Chris Kay <[email protected]>
@CJKay
Copy link
Contributor Author

CJKay commented Apr 28, 2021

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

@escapedcat
Copy link
Member

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:

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
Copy link
Member

@AdeAttwood
Copy link
Member

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
Copy link
Contributor Author

CJKay commented Apr 29, 2021

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 <[email protected]>

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 <[email protected]>

Or:

fix: add a bugfix

Lorem ipsum.

BREAKING CHANGE: A breaking change.

Signed-off-by: Chris Kay <[email protected]>

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
Copy link
Member

AdeAttwood commented Apr 29, 2021

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
Copy link
Contributor Author

CJKay commented Apr 29, 2021

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

@escapedcat
Copy link
Member

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

@CJKay
Copy link
Contributor Author

CJKay commented Apr 30, 2021

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 CJKay mentioned this pull request May 4, 2021
7 tasks
@CJKay
Copy link
Contributor Author

CJKay commented May 4, 2021

Closing in favour of #2578.

@CJKay CJKay closed this May 4, 2021
@CJKay CJKay deleted the interpret-trailers branch November 3, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants