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

Accept revert commit titles that are elongated by git #97

Closed
RaisinTen opened this issue May 2, 2022 · 2 comments · Fixed by #99
Closed

Accept revert commit titles that are elongated by git #97

RaisinTen opened this issue May 2, 2022 · 2 comments · Fixed by #99

Comments

@RaisinTen
Copy link
Contributor

In nodejs/node#42934, I was trying to revert nodejs/node#27371, so git revert automatically created this commit title for me:

Revert "bootstrap: delay the instantiation of maps in per-context scripts"

which has 74 characters. Since it exceeds our maximum limit of 72 characters, the commit message linter failed on CI - https://github.com/nodejs/node/runs/6248053065?check_suite_focus=true#step:6:15

Run echo "::add-matcher::.github/workflows/commit-lint-problem-matcher.json"
npm WARN exec The following package was not found and will be installed: core-validate-commit
# b04fe688d5859f707cf1a5e0206967268118bf7a
ok 1 co-authored-by-is-trailer: no Co-authored-by metadata
ok 2 fixes-url: skipping fixes-url # SKIP
ok 3 line-after-title: blank line after title
ok 4 line-length: line-lengths are valid
ok 5 subsystem: valid subsystems [bootstrap]
ok 6 title-format: Title is formatted correctly.
Error: not ok 7 title-length: Title must be <= 72 columns.
  ---
    {
      found: 74,
      compare: '<=',
      wanted: 72,
      at: {
        line: 0,
        column: 72,
        body: 'Revert "bootstrap: delay the instantiation of maps in per-context scripts"'
      }
    }
  ...

0..7
# tests 7
# pass  5
# fail  1
# Please review the commit message guidelines:
# https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

As a workaround, I had to change the commit title to

bootstrap: stop delaying instantiation of maps in per-context scripts

which has 69 characters (< 72) but it isn't satisfying anymore. :/

Instead of failing, IMO the linter should accept standard revert commit titles regardless of the size.

@tniessen
Copy link
Member

tniessen commented Jun 2, 2022

I think we used to restrict the title to 50 chars, which is conventional and works well with other tools, but now that the limit is softer, this can happen. So I guess the only solution is indeed to also relax this limit, but probably only if it matches /^Revert ".*"$/.

RaisinTen added a commit to RaisinTen/core-validate-commit that referenced this issue Jun 26, 2022
This change ensures that the commit message linter doesn't reject
revert commits that originally had commit titles with an acceptable
length but were elongated during the revert process by running
`git revert`.

Fixes: nodejs#97
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen
Copy link
Contributor Author

Agreed, sent a PR to implement this - #99

RaisinTen added a commit to RaisinTen/core-validate-commit that referenced this issue Jun 26, 2022
This change ensures that the commit message linter doesn't reject
revert commits that originally had commit titles with an acceptable
length but were elongated during the revert process by running
`git revert`.

Fixes: nodejs#97
Signed-off-by: Darshan Sen <[email protected]>
RaisinTen added a commit that referenced this issue Jul 3, 2022
This change ensures that the commit message linter doesn't reject
revert commits that originally had commit titles with an acceptable
length but were elongated during the revert process by running
`git revert`.

Fixes: #97
Signed-off-by: Darshan Sen <[email protected]>
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 a pull request may close this issue.

2 participants