Multi-line check approach is broken

[andy-maier]

Code of Conduct

• I agree to follow this project's Code of Conduct

Is there an existing issue for this?

• I have searched the existing issues

GitHub-hosted runner

ubuntu-latest

Additional runner information

The code currently invokes a single RegExp.test() call here: https://github.com/GsActions/commit-message-checker/blob/main/src/commit-message-checker.ts#L99

That approach does not work for the purpose of checking each line in a multi-line commit message or multi-line PR title+description.

Consider this Javascript example that checks for a maximum line length of 5:

const str = "tablex\ntable";
const regex = new RegExp("^.{0,5}$", "gm")
console.log(regex.test(str));  // output: true

One would expect that the regexp checks each line to be between 0 and 5 characters long, but what actually happens is that the entire multi-line string is searched for a line that is between 0 and 5 characters long. So it misses the first line which is supposed to make the check fail, and is happy by finding the second line that does match the condition.

Current Behavior

The current approach checks for the condition:

• There is at least one line in the multi-line string that matches the regexp.

Expected Behavior

The expected behavior is that the condition is checked:

• All lines in the multi-line string match the regexp.

This can be achieved by first splitting the multi-line string into a list of single-line strings, and then performing test() on each of these lines, failing if at least one check fails.

Steps To Reproduce

See above for standalone Javascript that shows the issue.

To reproduce with the checker:

1. Have a PR description with a title "tablex" and a description "table"
2. Run the checker with:

    - uses: gsactions/commit-message-checker@v2
      with:
        pattern: '^.{0,5}$'
        flags: gm
        error: PR title or description exceeds maximum line length of 5

The checker will succeed even though the "tablex" PR title exceeds the maximum line length of 5.

Anything else?

No response

[andy-maier]

Update: After experimenting a bit, I found the following regexp that correctly performs a line length check on each of the lines, with the current version of the checker:

    - name: Check Commit - Line length
      uses: gsactions/commit-message-checker@v2
      with:
        pattern: '^(.{0,80}\n)*.{0,80}$'
        flags: ''
        excludeDescription: true
        excludeTitle: true
        checkAllCommitMessages: true
        accessToken: ${{ secrets.GITHUB_TOKEN }}
        error: Commit message title or body exceeds maximum line length of 80

This treats the multi-line string as a single-line string (i.e. no "m" flag), and handles the lines in the expression itself. Since without multi-line matching, ^ and $ apply to the entire multi-line string, the regexp .{0,80} is applied to each line.

This may solve the issue in some situations, but may be inconvenient or impossible in others.

[dajaklie]

Coincidentally, I also started thinking about this "bug" yesterday. As opposed to @andy-maier, I would argue it is not a bug, merely using the standard javascript regex testing with incorrect and misleading examples. The library's choice to use the multiline regex flag by default is strange to say the least, but if we would like to avoid removing that to not break existing applications, the first two examples in the readme need to be changed to be correct. In addition to the problems with the second example, the first example is impressively misleading.

In the first example you use \[[^]]+\] .+$. As you haven't escaped the first "]" character, the string [B]] asdf passes, as B it is one character which is not, well, no thing, followed by at least two closing brackets. What you probably meant was \[[^\]]+\] .+$. However even this won't work as intended because of the multiline flag. With the multiline flag set, ^ and $ change from matching the start and end of the entire string, to the start and end of any line. This means when using ^, the match doesn't have to be on the first line, though in this case, the author didn't even add a ^ character at the start, so this is just all around strange.

• /\[[^]]+\] .+$/.test("[B]] asdf"); // true
• /\[[^]]+\] .+$/.test("[BUGFIX] asdf"); // false
• /\[[^\]]+\] .+$/.test("[BUGFIX] asdf"); // true
• /\[[^\]]+\] .+$/.test("[B]] asdf"); // false
• /\[[^\]]+\] .+$/.test("\n\n[BUGFIX] asdf"); // true
• /^\[[^\]]+\] .+$/gm.test("line1\nline2\n[BUGFIX] asdf"); // true
• /^\[[^\]]+\] .+$/.test("line1\nline2\n[BUGFIX] asdf"); // false

In the second example, you try to set an upper limit to the line lengths, but I would argue changing the library or adding some custom flag which checks the regex for each line in the commit seems uneccesarily complex. I'm a fan of the suggestion @andy-maier made above, by just dealing with that in regex, without the multiline flag set. In addition, as commented in #103, the line length check is just incorrect. The third example seems like it should work, given the intention of that line being anywhere in the commit message.