[FFmpeg-devel] [PATCH 1/2] avcodec/{ass, webvttdec}: fix handling of backslashes

[Oneric]

On Tue, Feb 01, 2022 at 20:41:37 +0000, Soft Works wrote:
> > On Tue, Feb 01, 2022 at 19:44:24 +0000, Soft Works wrote:
> > > > On Sun, Jan 16, 2022 at 19:16:54 +0100, Oneric wrote:
> > > >
> > > > In case anyone is wondering why patchwork fails to apply the second patch,
> > > > this is probably once again because the patch updates one of FATE's ASS
> > > > reference files which use CRLF line-endings.
> > > > Locally git am applies both without a hitch for me on top of current master
> > > > (and FATE passes after applying each patch).
> > >
> > >
> > > You can add a .gitattributes file to tests/ref/fate/ which includes the line
> > >
> > > sub-webvtt2 -diff
> > >
> > > Then your local git format-patch will create a binary diff for the file.
> > 
> > Thanks for your suggestion. However, a binary diff would look like this which
> > isn't great for seeing what's going on during review:
> > 
> >   [...]
> 
> Yes, I know, but the question is probably what's more important..
> 
> ..that it can be applied or that the text is viewable?

It CAN be applied (as I've now written twice) and
of course I verified this with the mail received from the list.

> > Also as noted, locally plain `git am` has no issues applying the regular
> > (non-binary) patch; 
> 
> Yes, because it hasn't been sent around via e-mail yet. SMTP requires
> CRLF line endings, so essentially it depends on the receiving e-mail client
> whether it outputs LF or CRLF. [...]

The mail content is base64 encoded during client-server and server-server 
transfer. Perhaps the raw base64 text is using all CRLF line-endings, but 
this doesn't matter. Once decoded it will byte-for-byte match the format-patch 
I created locally except for the ffmpeg-devel footer and additional headers in 
the mail version — but git am will just ignore those.
See the headers of the second patch's mail:

  Content-Type: text/plain; charset="utf-8"
  Content-Transfer-Encoding: base64

For comparison, here are the same headers for your first reply:

  Content-Type: text/plain; charset="us-ascii"
  Content-Transfer-Encoding: 7bit

If you tried to send a patch with your mail client by pasting in the diff,
then yes, presumably the line-endings would indeed get mangled. That's why 
doc/developer.texi tells you not to do this (instead recommending
git send-email or binary (i.e. base64-encoded) attachments).
  
> The most extreme example is sub-textenc (and there was another one iirc).
> It has mixed line endings - some LF and some CRLF. At least at that point
> there's no way out, because those endings will get unrecoverably lost 
> as soon as it is sent around via e-mail as text-diff.
> 
> That's how I came to the binary diff as only working option.
> (or you just don't send patches around via e-mail at all ;-)

Mixed line-endings also aren't a problem with a base64-encoded mail body
as eg git send-email will automatically use.


There's one caveat specific to (ffmpeg's) patchwork
I didn't know about before:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220116181655.6407-2-oneric@oneric.de/

If I download the “diff” from the above page, CRLF and LF line-endings are 
preserved as they should be. However, the files served from the “mbox” and 
“series” buttons only contain LF line-endings. As everyone can test, the mail 
I sent evidently does use mixed CRLF and LF line-endings matching the file 
being patched (just like the result of the “diff” button).
I believe this is an issue separate from patchwork using --no-keep-cr and 
should be fixed on patchwork's side.


Tl;dr:
Mixed line-endings in a diff are no technical issue,
provided the patch is sent correctly.
The issue is purely with patchwork, which:
 a) uses --no-keep-cr or similar presumably to protect against CRLF sneaking 
    into regular source files. (This is a minor annoyance when CRLF is 
    actually required, but otherwise harmless)
 b) mangles the line-endings for the “mbox” and “series” options but does the 
    correct thing for the “diff” option. This is harmful and imho should be 
    fixed by patchwork. Nonetheless, the patch can still be correctly applied 
    from the _mail_ on the list.

The patch on the list can easily be applied from the received mail by a plain 
`git am` or `git am --keep-cr` if the value of am.keepcr is set to false.
(In my case am.keepcr is not set at all and plain git am does the right thing.)
If someone wants to apply the patches and has trouble with getting the patch 
out of the mailclient I can on request resend the patch with a binary diff for 
the reference file.