You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <rh...@sharpsvn.net> on 2009/04/05 07:58:22 UTC
RE: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
> -----Original Message-----
> From: Arfrever Frehtes Taifersar Arahesis
> [mailto:Arfrever.FTA@GMail.Com]
> Sent: Sunday, April 05, 2009 3:18 AM
> To: svn@subversion.tigris.org
> Subject: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
>
> Author: arfrever
> Date: Sat Apr 4 18:17:40 2009
> New Revision: 37008
>
> Log:
> Follow-up to r37006:
>
> * subversion/tests/cmdline/svntest/wc.py
> (State.from_wc): # Normalize line endings on Windows.
>
> Modified:
> trunk/subversion/tests/cmdline/svntest/wc.py
>
> Modified: trunk/subversion/tests/cmdline/svntest/wc.py
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest
> /wc.py?pathrev=37008&r1=37007&r2=37008
> =======================================================================
> =======
> --- trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> 17:25:19 2009 (r37007)
> +++ trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> 18:17:40 2009 (r37008)
> @@ -492,6 +492,8 @@ class State:
> node = os.path.join(dirpath, name)
> if os.path.isfile(node):
> contents = open(node, 'rb').read()
> + if sys.platform == 'win32':
> + contents = contents.replace('\r\n', '\n')
> try:
> contents = contents.decode()
> except UnicodeDecodeError:
-1 on this approach of hiding newline problems. (Not the first time I
mention this)
On windows in text a "\r\n" is good, a "\n" is an error. And the test suite
must error on this condition like it always did.
This hides all these possible errors in our test suite. Each line MUST END
with a "\r\n", or most other tools will fail.
E.g. you would hide errors like the ones you introduced by merging the svn
patch branch..
or with properties that contain invalid end of line sequences by just
ignoring the "\r"..
or by svn diff that sometimes uses the wrong line endings (Some cases with
svn:eol-style native).
You are still trying to fix things for a handful of python 3 users, by
reducing the usefulness of the test suite for millions of Windows users.
The approach of moving the tests to Unicode, instead of comparing the actual
bytes from svn is the wrong approach.. The output must match byte after
byte, in the right character set.. or all scripts depending on subversion
will break.
This breaks another class of usages of our test suite. (character set
differences)
Bert
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1549300
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Apr 5, 2009 at 14:29, Arfrever Frehtes Taifersar Arahesis
<Ar...@gmail.com> wrote:
> 2009-04-05 09:58:22 Bert Huijben napisał(a):
>> > -----Original Message-----
>> > From: Arfrever Frehtes Taifersar Arahesis
>> > [mailto:Arfrever.FTA@GMail.Com]
>> > Sent: Sunday, April 05, 2009 3:18 AM
>> > To: svn@subversion.tigris.org
>> > Subject: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
>...
>> > +++ trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
>> > 18:17:40 2009 (r37008)
>> > @@ -492,6 +492,8 @@ class State:
>> > node = os.path.join(dirpath, name)
>> > if os.path.isfile(node):
>> > contents = open(node, 'rb').read()
>> > + if sys.platform == 'win32':
>> > + contents = contents.replace('\r\n', '\n')
>> > try:
>> > contents = contents.decode()
>> > except UnicodeDecodeError:
>>
>>
>> -1 on this approach of hiding newline problems. (Not the first time I
>> mention this)
>>
>> On windows in text a "\r\n" is good, a "\n" is an error. And the test suite
>> must error on this condition like it always did.
>
> The following code should be sufficient:
>
> if sys.platform == 'win32':
> for line in contents.splitlines(True):
> if line[-2:] != '\r\n':
> raise ValueError("Invalid line ending: '%s'" % line)
> contents = contents.replace('\r\n', '\n')
No, that won't work. What if we have a test that is *supposed* to
generate \n, rather than \r\n? The above code would error. And the
.replace() would throw away information on what kind of newline was
generated.
>> This hides all these possible errors in our test suite. Each line MUST END
>> with a "\r\n", or most other tools will fail.
>>
>> E.g. you would hide errors like the ones you introduced by merging the svn
>> patch branch..
>> or with properties that contain invalid end of line sequences by just
>> ignoring the "\r"..
>> or by svn diff that sometimes uses the wrong line endings (Some cases with
>> svn:eol-style native).
>>
>> You are still trying to fix things for a handful of python 3 users, by
>> reducing the usefulness of the test suite for millions of Windows users.
>>
>>
>> The approach of moving the tests to Unicode, instead of comparing the actual
>> bytes from svn is the wrong approach..
>
> It would be very hard to maintain the branch with all "strings" replaced
> with b"strings".
Frankly? I really don't care at all. The test suite MUST WORK. Your
branch is completely secondary to that.
Personally, I don't see us *ever* merging that branch. I don't see
much impetus or benefit to that. Do what you want on the branch, but
there are limits to the impact you can make on trunk. This whole
"Python 3.0 compatibility" work on trunk has been introducing
complexity where it is not needed. Making the test suite less
effective, more difficult to work with, or limited in the kinds of
things it can do... in the name of Python 3.0 compatibility? No. That
just doesn't make sense.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1553743
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-04-05 09:58:22 Bert Huijben napisał(a):
> > -----Original Message-----
> > From: Arfrever Frehtes Taifersar Arahesis
> > [mailto:Arfrever.FTA@GMail.Com]
> > Sent: Sunday, April 05, 2009 3:18 AM
> > To: svn@subversion.tigris.org
> > Subject: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
> >
> > Author: arfrever
> > Date: Sat Apr 4 18:17:40 2009
> > New Revision: 37008
> >
> > Log:
> > Follow-up to r37006:
> >
> > * subversion/tests/cmdline/svntest/wc.py
> > (State.from_wc): # Normalize line endings on Windows.
> >
> > Modified:
> > trunk/subversion/tests/cmdline/svntest/wc.py
> >
> > Modified: trunk/subversion/tests/cmdline/svntest/wc.py
> > URL:
> > http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest
> > /wc.py?pathrev=37008&r1=37007&r2=37008
> > =======================================================================
> > =======
> > --- trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> > 17:25:19 2009 (r37007)
> > +++ trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> > 18:17:40 2009 (r37008)
> > @@ -492,6 +492,8 @@ class State:
> > node = os.path.join(dirpath, name)
> > if os.path.isfile(node):
> > contents = open(node, 'rb').read()
> > + if sys.platform == 'win32':
> > + contents = contents.replace('\r\n', '\n')
> > try:
> > contents = contents.decode()
> > except UnicodeDecodeError:
>
>
> -1 on this approach of hiding newline problems. (Not the first time I
> mention this)
>
> On windows in text a "\r\n" is good, a "\n" is an error. And the test suite
> must error on this condition like it always did.
The following code should be sufficient:
if sys.platform == 'win32':
for line in contents.splitlines(True):
if line[-2:] != '\r\n':
raise ValueError("Invalid line ending: '%s'" % line)
contents = contents.replace('\r\n', '\n')
> This hides all these possible errors in our test suite. Each line MUST END
> with a "\r\n", or most other tools will fail.
>
> E.g. you would hide errors like the ones you introduced by merging the svn
> patch branch..
> or with properties that contain invalid end of line sequences by just
> ignoring the "\r"..
> or by svn diff that sometimes uses the wrong line endings (Some cases with
> svn:eol-style native).
>
> You are still trying to fix things for a handful of python 3 users, by
> reducing the usefulness of the test suite for millions of Windows users.
>
>
> The approach of moving the tests to Unicode, instead of comparing the actual
> bytes from svn is the wrong approach..
It would be very hard to maintain the branch with all "strings" replaced
with b"strings".
$ grep -Er "('.*'|\".*\")" subversion/tests/cmdline | grep -v /\.svn/ | wc -l
24806
$ 2to3 -f unicode subversion/tests/cmdline
RefactoringTool: No files need to be modified.
$
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-04-06 15:04:45 Greg Stein napisał(a):
> 2009/4/6 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
> > 2009-04-06 01:03:02 Greg Stein napisał(a):
> >...
> >> >> You're making the tests less useful by hiding potential errors.
> >> >
> >> > What potential errors?
> >>
> >> Regressions in our code base. Potential/future errors.
> >>
> >> The point is, that you're breaking our test suite's ability to locate
> >> future problems in our codebase. Rewriting the output loses
> >> information that may be important to detecting bugs.
> >
> > I disagree, but what do you think about?:
> >
> > for dirpath, dirs, files in os.walk(base):
> > parent = path_to_key(dirpath, base)
> > if ignore_svn and dot_svn in dirs:
> > dirs.remove(dot_svn)
> > for name in dirs + files:
> > node = os.path.join(dirpath, name)
> > if os.path.isfile(node):
> > try:
> > contents = open(node, 'r').read()
> > except UnicodeDecodeError:
> > contents = open(node, 'rb').read()
> > if sys.platform == 'win32':
> > contents = contents.replace('\r\n', '\n')
> > else:
> > contents = None
> > desc[repos_join(parent, name)] = StateItem(contents=contents)
>
> Yeah... I just saw that recently. That is also broken. It means we
> cannot detect whether file contents have the correct style of newline
> in them.
We wasn't able to detect it also previously, so I committed this suggestion
in r37021.
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Greg Stein <gs...@gmail.com>.
2009/4/6 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
> 2009-04-06 01:03:02 Greg Stein napisał(a):
>...
>> >> You're making the tests less useful by hiding potential errors.
>> >
>> > What potential errors?
>>
>> Regressions in our code base. Potential/future errors.
>>
>> The point is, that you're breaking our test suite's ability to locate
>> future problems in our codebase. Rewriting the output loses
>> information that may be important to detecting bugs.
>
> I disagree, but what do you think about?:
>
> for dirpath, dirs, files in os.walk(base):
> parent = path_to_key(dirpath, base)
> if ignore_svn and dot_svn in dirs:
> dirs.remove(dot_svn)
> for name in dirs + files:
> node = os.path.join(dirpath, name)
> if os.path.isfile(node):
> try:
> contents = open(node, 'r').read()
> except UnicodeDecodeError:
> contents = open(node, 'rb').read()
> if sys.platform == 'win32':
> contents = contents.replace('\r\n', '\n')
> else:
> contents = None
> desc[repos_join(parent, name)] = StateItem(contents=contents)
Yeah... I just saw that recently. That is also broken. It means we
cannot detect whether file contents have the correct style of newline
in them.
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1561498
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-04-06 01:03:02 Greg Stein napisał(a):
> On Sun, Apr 5, 2009 at 23:10, Arfrever Frehtes Taifersar Arahesis
> <Ar...@gmail.com> wrote:
> > 2009-04-05 23:06:14 Branko Čibej napisał(a):
> >> Arfrever Frehtes Taifersar Arahesis wrote:
> >> > I have found a computer with Windows system, I installed Python on that system
> >> > and I tested opening some test files with different line endings.
> >> > When opening a file [1] with either "\r\n" or "\n" or mixed line endings in text mode,
> >> > and reading it, all line endings were automatically transformed into "\n". There
> >> > were no errors. When opening in binary mode [2], actual line endings were used.
> >> > Replacing of line endings [3] was working correctly.
> >> >
> >> > [1] open(file, "r").read()
> >> > [2] open(file, "rb").read()
> >> > [3] open(file, "rb").read().replace("\r\n", "\n")
> >> >
> >>
> >> Sorry, how is that relevant? Anyone who knows anything about how
> >> text-mode files work already knows this.
> >
> > Bert Huijben claimed that opening a file with "\n" line endings on Windows
> > would cause some errors (maybe exceptions throwed by 'raise' from Python internal
> > modules).
> >
> >> You're making the tests less useful by hiding potential errors.
> >
> > What potential errors?
>
> Regressions in our code base. Potential/future errors.
>
> The point is, that you're breaking our test suite's ability to locate
> future problems in our codebase. Rewriting the output loses
> information that may be important to detecting bugs.
I disagree, but what do you think about?:
for dirpath, dirs, files in os.walk(base):
parent = path_to_key(dirpath, base)
if ignore_svn and dot_svn in dirs:
dirs.remove(dot_svn)
for name in dirs + files:
node = os.path.join(dirpath, name)
if os.path.isfile(node):
try:
contents = open(node, 'r').read()
except UnicodeDecodeError:
contents = open(node, 'rb').read()
if sys.platform == 'win32':
contents = contents.replace('\r\n', '\n')
else:
contents = None
desc[repos_join(parent, name)] = StateItem(contents=contents)
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Peter Samuelson <pe...@p12n.org>.
[Arfrever Frehtes Taifersar Arahesis]
> Bert Huijben claimed that opening a file with "\n" line endings on Windows
> would cause some errors (maybe exceptions throwed by 'raise' from Python internal
> modules).
He said it would cause errors for "most other tools". I don't think he
specifically meant tools written in Python.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Apr 5, 2009 at 23:10, Arfrever Frehtes Taifersar Arahesis
<Ar...@gmail.com> wrote:
> 2009-04-05 23:06:14 Branko Čibej napisał(a):
>> Arfrever Frehtes Taifersar Arahesis wrote:
>> > I have found a computer with Windows system, I installed Python on that system
>> > and I tested opening some test files with different line endings.
>> > When opening a file [1] with either "\r\n" or "\n" or mixed line endings in text mode,
>> > and reading it, all line endings were automatically transformed into "\n". There
>> > were no errors. When opening in binary mode [2], actual line endings were used.
>> > Replacing of line endings [3] was working correctly.
>> >
>> > [1] open(file, "r").read()
>> > [2] open(file, "rb").read()
>> > [3] open(file, "rb").read().replace("\r\n", "\n")
>> >
>>
>> Sorry, how is that relevant? Anyone who knows anything about how
>> text-mode files work already knows this.
>
> Bert Huijben claimed that opening a file with "\n" line endings on Windows
> would cause some errors (maybe exceptions throwed by 'raise' from Python internal
> modules).
>
>> You're making the tests less useful by hiding potential errors.
>
> What potential errors?
Regressions in our code base. Potential/future errors.
The point is, that you're breaking our test suite's ability to locate
future problems in our codebase. Rewriting the output loses
information that may be important to detecting bugs.
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1555573
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-04-05 23:06:14 Branko Čibej napisał(a):
> Arfrever Frehtes Taifersar Arahesis wrote:
> > I have found a computer with Windows system, I installed Python on that system
> > and I tested opening some test files with different line endings.
> > When opening a file [1] with either "\r\n" or "\n" or mixed line endings in text mode,
> > and reading it, all line endings were automatically transformed into "\n". There
> > were no errors. When opening in binary mode [2], actual line endings were used.
> > Replacing of line endings [3] was working correctly.
> >
> > [1] open(file, "r").read()
> > [2] open(file, "rb").read()
> > [3] open(file, "rb").read().replace("\r\n", "\n")
> >
>
> Sorry, how is that relevant? Anyone who knows anything about how
> text-mode files work already knows this.
Bert Huijben claimed that opening a file with "\n" line endings on Windows
would cause some errors (maybe exceptions throwed by 'raise' from Python internal
modules).
> You're making the tests less useful by hiding potential errors.
What potential errors?
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Branko Cibej <br...@xbc.nu>.
Arfrever Frehtes Taifersar Arahesis wrote:
> I have found a computer with Windows system, I installed Python on that system
> and I tested opening some test files with different line endings.
> When opening a file [1] with either "\r\n" or "\n" or mixed line endings in text mode,
> and reading it, all line endings were automatically transformed into "\n". There
> were no errors. When opening in binary mode [2], actual line endings were used.
> Replacing of line endings [3] was working correctly.
>
> [1] open(file, "r").read()
> [2] open(file, "rb").read()
> [3] open(file, "rb").read().replace("\r\n", "\n")
>
Sorry, how is that relevant? Anyone who knows anything about how
text-mode files work already knows this. You're missing the point.
You're making the tests less useful by hiding potential errors. Don't do
that.
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1554575
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-04-05 09:58:22 Bert Huijben napisał(a):
> > -----Original Message-----
> > From: Arfrever Frehtes Taifersar Arahesis
> > [mailto:Arfrever.FTA@GMail.Com]
> > Sent: Sunday, April 05, 2009 3:18 AM
> > To: svn@subversion.tigris.org
> > Subject: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
> >
> > Author: arfrever
> > Date: Sat Apr 4 18:17:40 2009
> > New Revision: 37008
> >
> > Log:
> > Follow-up to r37006:
> >
> > * subversion/tests/cmdline/svntest/wc.py
> > (State.from_wc): # Normalize line endings on Windows.
> >
> > Modified:
> > trunk/subversion/tests/cmdline/svntest/wc.py
> >
> > Modified: trunk/subversion/tests/cmdline/svntest/wc.py
> > URL:
> > http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest
> > /wc.py?pathrev=37008&r1=37007&r2=37008
> > =======================================================================
> > =======
> > --- trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> > 17:25:19 2009 (r37007)
> > +++ trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
> > 18:17:40 2009 (r37008)
> > @@ -492,6 +492,8 @@ class State:
> > node = os.path.join(dirpath, name)
> > if os.path.isfile(node):
> > contents = open(node, 'rb').read()
> > + if sys.platform == 'win32':
> > + contents = contents.replace('\r\n', '\n')
> > try:
> > contents = contents.decode()
> > except UnicodeDecodeError:
>
>
> -1 on this approach of hiding newline problems. (Not the first time I
> mention this)
>
> On windows in text a "\r\n" is good, a "\n" is an error. And the test suite
> must error on this condition like it always did.
>
>
> This hides all these possible errors in our test suite. Each line MUST END
> with a "\r\n", or most other tools will fail.
I have found a computer with Windows system, I installed Python on that system
and I tested opening some test files with different line endings.
When opening a file [1] with either "\r\n" or "\n" or mixed line endings in text mode,
and reading it, all line endings were automatically transformed into "\n". There
were no errors. When opening in binary mode [2], actual line endings were used.
Replacing of line endings [3] was working correctly.
[1] open(file, "r").read()
[2] open(file, "rb").read()
[3] open(file, "rb").read().replace("\r\n", "\n")
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Apr 5, 2009 at 09:58, Bert Huijben <rh...@sharpsvn.net> wrote:
>> -----Original Message-----
>> From: Arfrever Frehtes Taifersar Arahesis
>> [mailto:Arfrever.FTA@GMail.Com]
>> Sent: Sunday, April 05, 2009 3:18 AM
>> To: svn@subversion.tigris.org
>> Subject: svn commit: r37008 - trunk/subversion/tests/cmdline/svntest
>>
>> Author: arfrever
>> Date: Sat Apr 4 18:17:40 2009
>> New Revision: 37008
>>
>> Log:
>> Follow-up to r37006:
>>
>> * subversion/tests/cmdline/svntest/wc.py
>> (State.from_wc): # Normalize line endings on Windows.
>>
>> Modified:
>> trunk/subversion/tests/cmdline/svntest/wc.py
>>
>> Modified: trunk/subversion/tests/cmdline/svntest/wc.py
>> URL:
>> http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/svntest
>> /wc.py?pathrev=37008&r1=37007&r2=37008
>> =======================================================================
>> =======
>> --- trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
>> 17:25:19 2009 (r37007)
>> +++ trunk/subversion/tests/cmdline/svntest/wc.py Sat Apr 4
>> 18:17:40 2009 (r37008)
>> @@ -492,6 +492,8 @@ class State:
>> node = os.path.join(dirpath, name)
>> if os.path.isfile(node):
>> contents = open(node, 'rb').read()
>> + if sys.platform == 'win32':
>> + contents = contents.replace('\r\n', '\n')
>> try:
>> contents = contents.decode()
>> except UnicodeDecodeError:
>
>
> -1 on this approach of hiding newline problems. (Not the first time I
> mention this)
>
> On windows in text a "\r\n" is good, a "\n" is an error. And the test suite
> must error on this condition like it always did.
>
>
> This hides all these possible errors in our test suite. Each line MUST END
> with a "\r\n", or most other tools will fail.
>
> E.g. you would hide errors like the ones you introduced by merging the svn
> patch branch..
> or with properties that contain invalid end of line sequences by just
> ignoring the "\r"..
> or by svn diff that sometimes uses the wrong line endings (Some cases with
> svn:eol-style native).
>
> You are still trying to fix things for a handful of python 3 users, by
> reducing the usefulness of the test suite for millions of Windows users.
>
>
> The approach of moving the tests to Unicode, instead of comparing the actual
> bytes from svn is the wrong approach.. The output must match byte after
> byte, in the right character set.. or all scripts depending on subversion
> will break.
>
> This breaks another class of usages of our test suite. (character set
> differences)
I concur with Bert. Please back out your encode/decode changes and the
line-ending rewriting.
Thanks,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1550815