Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Martin Storsjö]

On Tue, 13 Feb 2024, Ridley Combs wrote:


It looks like checkout has different behavior from reset, and fate uses a
hard reset.
To test, I committed the change adding tests/ref/** -text,
unix2dos'd tests/ref/fate/sub-scc, then ran git -c core.autocrlf=true reset
--quiet --hard; this dos2unix'd the file as expected when run with a working
tree containing the .gitattributes change (but not otherwise).


Git doesn't have any "memory" of the CRLFiness of a file beyond the content
of the file itself (whether in the working tree or in committed blobs). It
just doesn't necessarily replace every file in checkout invocations when
they differ only in line endings. Windows was a mistake.


To rephrase; reset vs checkout doesn't make any difference here.

It seems to simply be the case, that as long as there are no changes to 
the file contents themselves between the relevant git commits, and the 
file isn't flagged as dirty in the stat cache of the local workdir, git 
never revisits the .gitattributes for this particular file.


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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Martin Storsjö]

On Tue, 13 Feb 2024, Ridley Combs wrote:


It looks like checkout has different behavior from reset, and fate uses a
hard reset.
To test, I committed the change adding tests/ref/** -text,
unix2dos'd tests/ref/fate/sub-scc, then ran git -c core.autocrlf=true reset
--quiet --hard; this dos2unix'd the file as expected when run with a working
tree containing the .gitattributes change (but not otherwise).


The difference here seems to be that you actively modify 
tests/ref/fate/sub-scc, which causes git to consider the file as needing 
to be restored when you run git reset.


When fate updates from one version to another, the files won't be locally 
modified, i.e. git's stat cache or similar has this file flagged as "not 
dirty".


So I suggest you retry your procedure by not manually modifying the file, 
but just letting git handle it, simulating exactly what happens on fate 
instances when updating from one version to another.


I.e., first check out 7bf1b9b3576~, nuke the file and check it out again, 
make sure that it contains CRLF. Then check out current master, which 
lacks attributes, but the local file in your workdir still contains CRLF. 
Then do any series of "git reset --hard", with/without "-c core.autocrlf", 
to commits on your experimental branch, and it won't change the line 
endings of the ref file, unless there actually are content changes to that 
particular file, between the git commits that you do check out.


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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Ridley Combs via ffmpeg-devel]

> On Feb 13, 2024, at 04:33, Martin Storsjö  wrote:
> 
> On Tue, 13 Feb 2024, Ridley Combs via ffmpeg-devel wrote:
> 
>>> On Feb 13, 2024, at 01:28, Anton Khirnov  wrote:
>>> Quoting Martin Storsjö (2024-02-12 12:31:29)
 On Mon, 12 Feb 2024, Hendrik Leppkes wrote:
> On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:
>>> diff --git a/.gitattributes b/.gitattributes
>>> index 5a19b963b6..a900528e47 100644
>>> --- a/.gitattributes
>>> +++ b/.gitattributes
>>> @@ -1,2 +1 @@
>>> *.pnm -diff -text
>>> -tests/ref/fate/sub-scc eol=crlf
>> This change seems to have had a tricky effect on the
>> tests/ref/fate/sub-scc file. Previously, when checked out, users got the
>> file with CRLF newlines. When updating to this git commit, or past it,
>> that file remains untouched, with CRLF still present, and the
>> fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
>> checkout tests/ref/fate/sub-scc", then the file does get restored with LR
>> newlines, and the test passes.
>> It's easy to do this change manually in the source checkout of a fate
>> runner, but I'm not sure how easily we get all fate instances fixed that
>> way - currently this test is failing in most of them.
> Can this be fixed by restoring the .gitattribute entry but with eol=lf?
> Not sure if Git would reset the file then.
 No, that doesn't seem to make any difference. Not sure if there are any 
 other straightforward/elegant fixes, short of renaming the file, which I 
 guess would require renaming the test itself.
>>> I'm fine with renaming the test, unless anyone has a better fix.
>> 
>> We could probably tweak the fate runner script to make sure this gets fixed 
>> up; can anyone try this patch on one of the affected machines? 
>> https://gist.github.com/rcombs/c2ad470bf36c5cbd3fc33e699330eb15
> 
> That doesn't seem to make any difference.
> 
> Also, updating fate.sh doesn't necessarily propagate automatically to runners 
> - in order to run fate, one needs to run fate.sh before it even clones/checks 
> out the directory where it fetches the latest source. So unless one later has 
> changed one's setup, to invoke a fate.sh from the checkout, most fate runners 
> just use whatever copy of fate.sh they had when it was set up.
> 
>> Alternately, we could set -text on all fate ref files, or explicitly set 
>> eol=of for them, to ensure their line endings never get rewritten like this 
>> regardless of git config. I think either of these solutions would fix this 
>> in fate, but only after the fix commit gets checked out *followed by* at 
>> least one other commit.
> 
> Neither of those seem to make any difference either.
> 
> It's quite easy to test for one self:
> 
> $ git checkout -b experiment
> $ 
> $ 
> 
> $ git checkout 7bf1b9b3576~ # Reset original state, for testing
> $ rm tests/ref/fate/sub-scc; git checkout tests/ref/fate/sub-scc
> $ vi tests/ref/fate/sub-scc # inspect that the file originally has CRLF
> $ git checkout experiment~ # check out the commit setting attributes
> $ git checkout experiment # check out the next commit, with the new 
> attributes set
> $ vi tests/ref/fate/sub-scc # observe that the file still has CRLF
> 
> $ git checkout --detach
> $ git -c core.autocrlf=false reset --hard 7bf1b9b3576
> $ vi tests/ref/fate/sub-scc # observe that the file still has CRLF

It looks like checkout has different behavior from reset, and fate uses a hard 
reset.
To test, I committed the change adding tests/ref/** -text, unix2dos'd 
tests/ref/fate/sub-scc, then ran git -c core.autocrlf=true reset --quiet 
--hard; this dos2unix'd the file as expected when run with a working tree 
containing the .gitattributes change (but not otherwise).

> 
> 
> It seems to me (I haven't trid to dig into manuals) that the attribute gets 
> stuck in whatever form it was when the file was first created in the workdir. 
> E.g. doing a "git checkout d1df72a702~" (the commit before the file was 
> originally added) followed by "git checkout 7bf1b9b3576" does fix it. This is 
> at least observed with git 2.25.1. Not sure if this is intended behaviour or 
> a bug from git's side.

Git doesn't have any "memory" of the CRLFiness of a file beyond the content of 
the file itself (whether in the working tree or in committed blobs). It just 
doesn't necessarily replace every file in checkout invocations when they differ 
only in line endings. Windows was a mistake.

> 
> // Martin

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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Martin Storsjö]

On Tue, 13 Feb 2024, Ridley Combs via ffmpeg-devel wrote:


On Feb 13, 2024, at 01:28, Anton Khirnov  wrote:

Quoting Martin Storsjö (2024-02-12 12:31:29)

On Mon, 12 Feb 2024, Hendrik Leppkes wrote:


On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:


diff --git a/.gitattributes b/.gitattributes
index 5a19b963b6..a900528e47 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,2 +1 @@
*.pnm -diff -text
-tests/ref/fate/sub-scc eol=crlf


This change seems to have had a tricky effect on the
tests/ref/fate/sub-scc file. Previously, when checked out, users got the
file with CRLF newlines. When updating to this git commit, or past it,
that file remains untouched, with CRLF still present, and the
fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
checkout tests/ref/fate/sub-scc", then the file does get restored with LR
newlines, and the test passes.

It's easy to do this change manually in the source checkout of a fate
runner, but I'm not sure how easily we get all fate instances fixed that
way - currently this test is failing in most of them.



Can this be fixed by restoring the .gitattribute entry but with eol=lf?
Not sure if Git would reset the file then.


No, that doesn't seem to make any difference. Not sure if there are any 
other straightforward/elegant fixes, short of renaming the file, which I 
guess would require renaming the test itself.


I'm fine with renaming the test, unless anyone has a better fix.


We could probably tweak the fate runner script to make sure this gets 
fixed up; can anyone try this patch on one of the affected machines? 
https://gist.github.com/rcombs/c2ad470bf36c5cbd3fc33e699330eb15


That doesn't seem to make any difference.

Also, updating fate.sh doesn't necessarily propagate automatically to 
runners - in order to run fate, one needs to run fate.sh before it even 
clones/checks out the directory where it fetches the latest source. So 
unless one later has changed one's setup, to invoke a fate.sh from the 
checkout, most fate runners just use whatever copy of fate.sh they had 
when it was set up.


Alternately, we could set -text on all fate ref files, or explicitly set 
eol=of for them, to ensure their line endings never get rewritten like 
this regardless of git config. I think either of these solutions would 
fix this in fate, but only after the fix commit gets checked out 
*followed by* at least one other commit.


Neither of those seem to make any difference either.

It's quite easy to test for one self:

$ git checkout -b experiment
$ 
$ 

$ git checkout 7bf1b9b3576~ # Reset original state, for testing
$ rm tests/ref/fate/sub-scc; git checkout tests/ref/fate/sub-scc
$ vi tests/ref/fate/sub-scc # inspect that the file originally has CRLF
$ git checkout experiment~ # check out the commit setting attributes
$ git checkout experiment # check out the next commit, with the new attributes 
set
$ vi tests/ref/fate/sub-scc # observe that the file still has CRLF

$ git checkout --detach
$ git -c core.autocrlf=false reset --hard 7bf1b9b3576
$ vi tests/ref/fate/sub-scc # observe that the file still has CRLF


It seems to me (I haven't trid to dig into manuals) that the attribute 
gets stuck in whatever form it was when the file was first created in the 
workdir. E.g. doing a "git checkout d1df72a702~" (the commit before the 
file was originally added) followed by "git checkout 7bf1b9b3576" does fix 
it. This is at least observed with git 2.25.1. Not sure if this is 
intended behaviour or a bug from git's side.


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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Ridley Combs via ffmpeg-devel]

> On Feb 13, 2024, at 01:28, Anton Khirnov  wrote:
> 
> Quoting Martin Storsjö (2024-02-12 12:31:29)
>> On Mon, 12 Feb 2024, Hendrik Leppkes wrote:
>> 
>>> On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:
> 
> diff --git a/.gitattributes b/.gitattributes
> index 5a19b963b6..a900528e47 100644
> --- a/.gitattributes
> +++ b/.gitattributes
> @@ -1,2 +1 @@
> *.pnm -diff -text
> -tests/ref/fate/sub-scc eol=crlf
 
 This change seems to have had a tricky effect on the
 tests/ref/fate/sub-scc file. Previously, when checked out, users got the
 file with CRLF newlines. When updating to this git commit, or past it,
 that file remains untouched, with CRLF still present, and the
 fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
 checkout tests/ref/fate/sub-scc", then the file does get restored with LR
 newlines, and the test passes.
 
 It's easy to do this change manually in the source checkout of a fate
 runner, but I'm not sure how easily we get all fate instances fixed that
 way - currently this test is failing in most of them.
 
>>> 
>>> Can this be fixed by restoring the .gitattribute entry but with eol=lf?
>>> Not sure if Git would reset the file then.
>> 
>> No, that doesn't seem to make any difference. Not sure if there are any 
>> other straightforward/elegant fixes, short of renaming the file, which I 
>> guess would require renaming the test itself.
> 
> I'm fine with renaming the test, unless anyone has a better fix.

We could probably tweak the fate runner script to make sure this gets fixed up; 
can anyone try this patch on one of the affected machines? 
https://gist.github.com/rcombs/c2ad470bf36c5cbd3fc33e699330eb15

Alternately, we could set -text on all fate ref files, or explicitly set eol=of 
for them, to ensure their line endings never get rewritten like this regardless 
of git config. I think either of these solutions would fix this in fate, but 
only after the fix commit gets checked out *followed by* at least one other 
commit.

> 
> -- 
> Anton Khirnov
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org 
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org  with 
> subject "unsubscribe".

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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Anton Khirnov]

Quoting Martin Storsjö (2024-02-12 12:31:29)
> On Mon, 12 Feb 2024, Hendrik Leppkes wrote:
> 
> > On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:
> >> >
> >> > diff --git a/.gitattributes b/.gitattributes
> >> > index 5a19b963b6..a900528e47 100644
> >> > --- a/.gitattributes
> >> > +++ b/.gitattributes
> >> > @@ -1,2 +1 @@
> >> > *.pnm -diff -text
> >> > -tests/ref/fate/sub-scc eol=crlf
> >>
> >> This change seems to have had a tricky effect on the
> >> tests/ref/fate/sub-scc file. Previously, when checked out, users got the
> >> file with CRLF newlines. When updating to this git commit, or past it,
> >> that file remains untouched, with CRLF still present, and the
> >> fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
> >> checkout tests/ref/fate/sub-scc", then the file does get restored with LR
> >> newlines, and the test passes.
> >>
> >> It's easy to do this change manually in the source checkout of a fate
> >> runner, but I'm not sure how easily we get all fate instances fixed that
> >> way - currently this test is failing in most of them.
> >>
> >
> > Can this be fixed by restoring the .gitattribute entry but with eol=lf?
> > Not sure if Git would reset the file then.
> 
> No, that doesn't seem to make any difference. Not sure if there are any 
> other straightforward/elegant fixes, short of renaming the file, which I 
> guess would require renaming the test itself.

I'm fine with renaming the test, unless anyone has a better fix.

-- 
Anton Khirnov
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Martin Storsjö]

On Mon, 12 Feb 2024, Hendrik Leppkes wrote:


On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:

>
> diff --git a/.gitattributes b/.gitattributes
> index 5a19b963b6..a900528e47 100644
> --- a/.gitattributes
> +++ b/.gitattributes
> @@ -1,2 +1 @@
> *.pnm -diff -text
> -tests/ref/fate/sub-scc eol=crlf

This change seems to have had a tricky effect on the
tests/ref/fate/sub-scc file. Previously, when checked out, users got the
file with CRLF newlines. When updating to this git commit, or past it,
that file remains untouched, with CRLF still present, and the
fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
checkout tests/ref/fate/sub-scc", then the file does get restored with LR
newlines, and the test passes.

It's easy to do this change manually in the source checkout of a fate
runner, but I'm not sure how easily we get all fate instances fixed that
way - currently this test is failing in most of them.



Can this be fixed by restoring the .gitattribute entry but with eol=lf?
Not sure if Git would reset the file then.


No, that doesn't seem to make any difference. Not sure if there are any 
other straightforward/elegant fixes, short of renaming the file, which I 
guess would require renaming the test itself.


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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Hendrik Leppkes]

On Mon, Feb 12, 2024 at 11:22 AM Martin Storsjö  wrote:
> >
> > diff --git a/.gitattributes b/.gitattributes
> > index 5a19b963b6..a900528e47 100644
> > --- a/.gitattributes
> > +++ b/.gitattributes
> > @@ -1,2 +1 @@
> > *.pnm -diff -text
> > -tests/ref/fate/sub-scc eol=crlf
>
> This change seems to have had a tricky effect on the
> tests/ref/fate/sub-scc file. Previously, when checked out, users got the
> file with CRLF newlines. When updating to this git commit, or past it,
> that file remains untouched, with CRLF still present, and the
> fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git
> checkout tests/ref/fate/sub-scc", then the file does get restored with LR
> newlines, and the test passes.
>
> It's easy to do this change manually in the source checkout of a fate
> runner, but I'm not sure how easily we get all fate instances fixed that
> way - currently this test is failing in most of them.
>

Can this be fixed by restoring the .gitattribute entry but with eol=lf?
Not sure if Git would reset the file then.

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

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/assenc: normalize line endings to \n

[Martin Storsjö]

On Mon, 12 Feb 2024, rcombs wrote:


ffmpeg | branch: master | rcombs  | Sun Jan 28 14:27:17 2024 
-0800| [7bf1b9b35769b37684dd2f18a54f01d852a540c8] | committer: rcombs

lavf/assenc: normalize line endings to \n

Previously, we produced output with either \r\n or mixed line endings.
This was undesirable unto itself, but also made working with patches affecting
FATE output particularly challenging, especially via the mailing list.

Everything that consumes the SSA/ASS format is line-ending-agnostic,
so \n is selected to simplify git/ML usage in FATE.

Extra \r characters at the end of a packet are dropped. These are always
ignored by the renderer anyway.


http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=7bf1b9b35769b37684dd2f18a54f01d852a540c8

---

.gitattributes  |   1 -
libavformat/assenc.c|  22 ++--
tests/ref/fate/sub-aqtitle  |  94 
tests/ref/fate/sub-ass-to-ass-transcode | 124 ++---
tests/ref/fate/sub-cc   |  32 +++---
tests/ref/fate/sub-cc-realtime  |  44 
tests/ref/fate/sub-cc-scte20|  34 +++---
tests/ref/fate/sub-charenc  | 128 +++---
tests/ref/fate/sub-jacosub  |  50 -
tests/ref/fate/sub-microdvd |  48 -
tests/ref/fate/sub-movtext  |  34 +++---
tests/ref/fate/sub-mpl2 |  36 +++
tests/ref/fate/sub-mpsub|  70 ++--
tests/ref/fate/sub-mpsub-frames |  32 +++---
tests/ref/fate/sub-pjs  |  34 +++---
tests/ref/fate/sub-realtext |  38 +++
tests/ref/fate/sub-sami |  46 
tests/ref/fate/sub-sami2| 186 
tests/ref/fate/sub-srt  | 102 +-
tests/ref/fate/sub-srt-badsyntax|  48 -
tests/ref/fate/sub-ssa-to-ass-remux | 168 ++---
tests/ref/fate/sub-stl  |  62 +--
tests/ref/fate/sub-subviewer|  34 +++---
tests/ref/fate/sub-subviewer1   |  48 -
tests/ref/fate/sub-vplayer  |  34 +++---
tests/ref/fate/sub-webvtt   |  58 +-
tests/ref/fate/sub-webvtt2  |  52 -
27 files changed, 831 insertions(+), 828 deletions(-)

diff --git a/.gitattributes b/.gitattributes
index 5a19b963b6..a900528e47 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,2 +1 @@
*.pnm -diff -text
-tests/ref/fate/sub-scc eol=crlf


This change seems to have had a tricky effect on the 
tests/ref/fate/sub-scc file. Previously, when checked out, users got the 
file with CRLF newlines. When updating to this git commit, or past it, 
that file remains untouched, with CRLF still present, and the 
fate-sub-scc test fails. If one does "rm tests/ref/fate/sub-scc; git 
checkout tests/ref/fate/sub-scc", then the file does get restored with LR 
newlines, and the test passes.


It's easy to do this change manually in the source checkout of a fate 
runner, but I'm not sure how easily we get all fate instances fixed that 
way - currently this test is failing in most of them.


// Martin

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

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