[FFmpeg-devel] FATE-Suite Data Test Data

[Andreas Rheinhardt]

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Friday, December 10, 2021 8:21 PM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] FATE-Suite Data Test Data
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
>>>> Rheinhardt
>>>> Sent: Friday, December 10, 2021 8:00 PM
>>>> To: ffmpeg-devel at ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] FATE-Suite Data Test Data
>>>>
>>>> Soft Works:
>>>>> Hi,
>>>>>
>>>>> can we add the attached file (SubRip_capability_tester2.srt) to the fate-
>>>> suite data?
>>>>>
>>>>> Compared to SubRip_capability_tester.srt, it has one line removed that
>>>> would
>>>>> cause (legitimate) trailing whitespace in some ref data files.
>>>>>
>>>>> It hasn’t been an issue so far due to a bug in the encoders which didn’t
>>>> properly
>>>>> convert ASS hard-space tags (\h) and incorrectly emitted them as “\h”
>> text.
>>>>> After fixing this, encoders to formats which don’t have a concept of
>> “hard
>>>> spaces”,
>>>>> will output \h as regular spaces, and in the case of that single source
>>>> line
>>>>> that I have removed in the source file, this would lead to trailing
>> spaces
>>>> in the
>>>>> ref files.
>>>>>
>>>>> As I don’t know how, when or whether at all this can be fixed in a way
>> that
>>>> it
>>>>> could work with patches transported via e-mail, this single line change
>>>> seems
>>>>> to be the best quick solution to the problem.
>>>>>
>>>>> Thanks,
>>>>> softworkz
>>>>>
>>>>
>>>> I don't think that this will help. Trailing whitespace should actually
>>>> only lead to a warning when applying (depending upon core.whitespace).
>>>>
>>>> Your patch is broken: There is a line "\ N is a forced line break" in
>>>> the current ref file which ends with \r\n. This line is not marked as
>>>> changed in your patchfile, but it is part of the surrounding context of
>>>> a diff; and said line of context uses only \n and is therefore not
>>>> recognized.
>>>>
>>>> And when I create a diff where the patch file has the proper context, it
>>>> still fails; I need to add the "--keep-cr" option to git am to make it
>>>> work. But then it works, whereas this option didn't help if the patch
>>>> file only has \n instead of \r\n.
>>>>
>>>> - Andreas
>>>
>>> Thanks for looking into this.
>>>
>>> I'm simply going after the patchwork error from my recent submnission:
>>>
>>> https://patchwork.ffmpeg.org/check/47132/
>>> --------------------------
>>> 	.git/rebase-apply/patch:131: trailing whitespace.
>>> A letter followed by 05 hard spaces: A
>>> .git/rebase-apply/patch:192: trailing whitespace.
>>> A letter followed by 05 hard spaces: A
>>> error: patch failed: tests/ref/fate/sub-textenc:160
>>> error: tests/ref/fate/sub-textenc: patch does not apply
>>> error: Did you hand edit your patch?
>>> It does not apply to blobs recorded in its index.
>>> --------------------------
>>>
>>> It fails exactly at the line that I have removed in
>>> SubRip_capability_tester2.srt
>>
>> No, it does not. Look at the error: "error: patch failed:
>> tests/ref/fate/sub-textenc:160"
>> Line 160 is "\ N is a forced line break", the first line of context with
>> \r\n. It has nothing to do with the line you removed. The
>> trailing-whitespace stuff just leads to warnings, not errors.
> 
> OK, seems you are right. I understood the 160 as indication 
> of the hunk start line which failed to apply rather than the line of failure. 
> 
> To summarize:
> - The file in the repo has mixed endings
> - My patch contains all CRLF
> - The MBOX has all LF
> 
> When we look at the previous patch submitted for the file:
> 
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20170729192751.26379-1-u@pkh.me/
> 
> It also as only LF (the MBOX), but the file I the repo, 
> with the sequences that were changed by the patch have
> mixed line endings.
> 
> So, that's kind of a miracle then, how that patch got
> applied to have mixed endings in the repo?
> 

That's easy: Just don't let it be touched as email. git
push/pull/fetch/clone would have no issue at all. You also had no
problem committing the updated file locally.

(git am uses git mailsplit to split mbox files into smaller files (one
per email, I think, even if the mbox file contained only one email);
during this process, \r\n is just treated as newline and replaced by \n
if it is run without --keep-cr. And therefore the patch does no longer
apply later when the file to be changed really uses \r\n. So the fix for
this issue would be for patchwork to use said option (which I think
doesn't have any downside (patches using \r\n somewhere except a +-line
could no longer be applied to files using \n, but that is not a downside
as it indicates wrong patches)); furthermore, the eventual committer
would also have to use said option if he receives the mail by email.)

- Andreas