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

[Erik Joelsson]

On Mon, 6 Dec 2021 16:51:45 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>>> I think size delta in the case of modified is fine. In my experience, it's very rare for binary patches to actually be partial in real situations. When replacing an image or archive, the whole file usually changes enough. If the binary patch is empty, due to file moved/rename, then we can't really do anything about that unfortunately. Have you looked at actual patch output from Git to see what it looks like for renamed binary files? Git doesn't track file renames explicitly, so I'm not sure what would actually happen here.
>> 
>> Moved file is identifed as `renamed`. The `renamed` diff information is shown below.
>> 
>> 
>> :100644 100644 c9eef64 c9eef64 R100     t/1.png t/2.png
>> 
>> diff --git a/t/1.png b/t/2.png
>> similarity index 100%
>> rename from t/1.png
>> rename to t/2.png
>> 
>> 
>> It only has the source file and target file and have no information about size.
>
>> > I think size delta in the case of modified is fine. In my experience, it's very rare for binary patches to actually be partial in real situations. When replacing an image or archive, the whole file usually changes enough. If the binary patch is empty, due to file moved/rename, then we can't really do anything about that unfortunately. Have you looked at actual patch output from Git to see what it looks like for renamed binary files? Git doesn't track file renames explicitly, so I'm not sure what would actually happen here.
>> 
>> Moved file is identifed as `renamed`. The `renamed` diff information is shown below.
>> 
>> ```
>> :100644 100644 c9eef64 c9eef64 R100     t/1.png t/2.png
>> 
>> diff --git a/t/1.png b/t/2.png
>> similarity index 100%
>> rename from t/1.png
>> rename to t/2.png
>> ```
>> 
>> It only has the source file and target file and have no information about size.
> 
> Then we can't check the size of files in such changes. We are limited to what Git will tell us here.

> I don't think this patch would be integrated in short time. So I advice that we can use another patch to enable the existing binary check firstly, which is a bug in the past. @erikj79 What's your opinion?

I agree that it's better to change PullRequestCheckIssueVisitor to react to the existing binary check in a separate change.

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

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