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

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Oneric
> Sent: Tuesday, February 1, 2022 9:07 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/{ass, webvttdec}: fix
> handling of backslashes
> 
> 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:
> 
>   diff --git a/tests/ref/fate/sub-webvtt2 b/tests/ref/fate/sub-webvtt2
>   index
> 357b8178ea1cf224ad47dcf78b24f1948ece6665..4cd1d86a9a58ccf65812131bf84a17531c2
> c6cfa 100644
>   GIT binary patch
>   delta 24
>   gcmeys^NnXiIV;bjhJHgMV-r)eM-6?GYgs=70DwpeRR910
> 
>   delta 18
>   Zcmeyy^MPkWIV+o?k+F%X+2m%{&j3Hw26O-b

Yes, I know, but the question is probably what's more important..

..that it can be applied or that the text is viewable?

I assume that a reviewer would often apply the patch locally anyway, then 
the text changes become visible again as this doesn't affect git log.
Given that it's just a handful files from a few thousands, this should 
be acceptable.


> 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. It's then up to git whether it ignores
or adjust the line ending when applying a patch. Maybe that's what is 
configured @patchwork, I can't tell.

 
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 ;-)


> I recall using some .gitattribute settings to force crlf (without making
> the file binary?) were discussed last time this happened, but deemed to be
> not worth it because of some other issues with it.

Doesn't work with mixed endings. BTW, I don't find it wrong to make such files
binary because logically these are not text files, but test reference results,
which are expected to precisely match (rather than "have the same text").

If you don't want to mark the files as binary, you could probably use
.gitattributes without commiting, so it would only affect format-patch 
generation.

Best,
softworkz



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".