Re: svn commit: r1743236 - in /subversion/trunk/subversion/tests/cmdline: merge_tests.py svntest/actions.py svntest/tree.py

[Evgeny Kotkov <ev...@visualsvn.com>]

Stefan Fuhrmann <st...@apache.org> writes:

> * subversion/tests/cmdline/svntest/actions.py
>   (set_prop): If we write a bytes string to a prop, treat it as binary
>               that can't be passed directly via command line argument.

[...]

> -  if value and (value[0] == '-' or '\x00' in value or sys.platform == 'win32'):
> +  if value and (isinstance(value, bytes) or
> +                (value[0] == '-' or '\x00' in value or sys.platform == 'win32')):
>      from tempfile import mkstemp
>      (fd, value_file_path) = mkstemp()
>      os.close(fd)

The new condition looks fairly suspicious.

What if someone calls set_prop('foo', '-') under Python 3?  Or something like
set_prop('foo', 'bar'), but on Windows with Python 3?  Is it going to raise
an error, because we'd try to pass a string to file.write() that expects
bytes?


Regards,
Evgeny Kotkov

Re: svn commit: r1743236 - in /subversion/trunk/subversion/tests/cmdline: merge_tests.py svntest/actions.py svntest/tree.py

[Stefan Fuhrmann <st...@apache.org>]

On 13.05.2016 19:51, Evgeny Kotkov wrote:
> Stefan Fuhrmann<st...@apache.org>  writes:
>
>>> The new condition looks fairly suspicious.
>> Sure, but no worse than before.
> I think it's actually worse than before, as now it looks like this function
> was updated for Python 3, whereas in fact it gets all the special cases
> like "value[0] == '-'" wrong.  And we will probably trip over this in the
> future.
Well, I only wanted to say that py3 wasn't supported
before either.
> How about doing it as in the attached patch?
That seems to work. Committed in r1743956.

Thanks for the patch!

-- Stefan^2.

Re: svn commit: r1743236 - in /subversion/trunk/subversion/tests/cmdline: merge_tests.py svntest/actions.py svntest/tree.py

[Evgeny Kotkov <ev...@visualsvn.com>]

Stefan Fuhrmann <st...@apache.org> writes:

>> The new condition looks fairly suspicious.
>
> Sure, but no worse than before.

I think it's actually worse than before, as now it looks like this function
was updated for Python 3, whereas in fact it gets all the special cases
like "value[0] == '-'" wrong.  And we will probably trip over this in the
future.

How about doing it as in the attached patch?


Regards,
Evgeny Kotkov

Re: svn commit: r1743236 - in /subversion/trunk/subversion/tests/cmdline: merge_tests.py svntest/actions.py svntest/tree.py

[Stefan Fuhrmann <st...@apache.org>]

On 12.05.2016 23:49, Evgeny Kotkov wrote:
> Stefan Fuhrmann <st...@apache.org> writes:
>
>> * subversion/tests/cmdline/svntest/actions.py
>>    (set_prop): If we write a bytes string to a prop, treat it as binary
>>                that can't be passed directly via command line argument.
> [...]
>
>> -  if value and (value[0] == '-' or '\x00' in value or sys.platform == 'win32'):
>> +  if value and (isinstance(value, bytes) or
>> +                (value[0] == '-' or '\x00' in value or sys.platform == 'win32')):
>>       from tempfile import mkstemp
>>       (fd, value_file_path) = mkstemp()
>>       os.close(fd)
> The new condition looks fairly suspicious.
Sure, but no worse than before.
> What if someone calls set_prop('foo', '-') under Python 3?  Or something like
> set_prop('foo', 'bar'), but on Windows with Python 3?  Is it going to raise
> an error, because we'd try to pass a string to file.write() that expects
> bytes?
Right now this is not a problem with all tests passing
under Linux and the Windows tests being fundamentally
broken. If you have a good idea how to improve that
code, please go ahead and patch it.

-- Stefan^2.