RFR: 1236: Add jcheck option to check for large binary files

[Erik Joelsson]

On Wed, 1 Dec 2021 11:35:13 GMT, Guoxiong Li <gli at openjdk.org> wrote:

> > It looks like you have taken the existing "binary" check and changed it to be a size check.
> 
> Because when I dove into the class `PullRequestCheckIssueVisitor`, I fould that the `BinaryIssue` is never used and the check result has no effect in the PR. So I think `BinaryIssue` is not necessary and rename it to `BinaryFileTooLargeIssue`.
> 
> ```
> // class PullRequestCheckIssueVisitor
>     @Override
>     public void visit(BinaryIssue issue) {
>         log.fine("ignored: binary file");
>         // <---- miss to add the failed message to the `failedChecks`.
>     }
> ```
> 
> But now I find the `JCheckCLIVisitor` use the `BinaryIssue` which means the `BinaryIssue` is effective. So now I think the `PullRequestCheckIssueVisitor` just has a small bug that it miss to add the failed message to the `failedChecks`. And actually, the result of the `binary` check in the PR has never activated in the past.
> 
> ```
> // class JCheckCLIVisitor
>     public void visit(BinaryIssue i) {
>         if (!ignore.contains(i.check().name())) {
>             println(i, "adds binary file: " + i.path().toString());
>             hasDisplayedErrors = true;
>         }
>     }
> ```
> 
> I will solve this small bug of the `PullRequestCheckIssueVisitor` in this patch and will keep the `BinaryIssue` instead of removing `BinaryIssue`.

I found the original PR where this was added https://github.com/openjdk/skara/pull/159. It looks like they never got around to implementing handling in the PR visitor. I think it's good if you fix that.

-------------

PR: https://git.openjdk.java.net/skara/pull/1247