Re: log --search test failures on trunk and 1.8.x

On 22.04.2013 12:59, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: 'Stefan Sperling' [mailto:stsp_at_elego.de]
>> Sent: maandag 22 april 2013 11:46
>> To: Bert Huijben
>> Cc: 'Ivan Zhakov'; 'Branko Čibej'; dev_at_subversion.apache.org
>> Subject: Re: log --search test failures on trunk and 1.8.x
>>
>> On Mon, Apr 22, 2013 at 11:22:11AM +0200, Bert Huijben wrote:
>>>> -----Original Message-----
>>>> From: Stefan Sperling [mailto:stsp_at_elego.de]
>>>> Sent: maandag 22 april 2013 00:08
>>>> To: Ivan Zhakov
>>>> Cc: Branko Čibej; Bert Huijben; dev_at_subversion.apache.org
>>>> Subject: Re: log --search test failures on trunk and 1.8.x
>>>>
>>>> On Sun, Apr 21, 2013 at 07:11:14PM +0400, Ivan Zhakov wrote:
>>>>> So I propose the following plan:
>>>>> 1. Make 'log --search" case-sensitive in trunk and 1.8.x.
>>>>> 2. Merge utf8proc stuff to trunk
>>>>> 3. Implement svn_utf__casefold() using utf8proc
>>>>> 4. Implement 'log --isearch' using
>>>> No --isearch please. It did exist on trunk but we made --search
>>>> case-insensitive in r1388530 to avoid having too many options.
>>>>
>>>> Has anyone tried my APR patch on windows yet? I'd be interested to
>>>> know whether or not that helps... if you are running Windows and
>>>> care about this issue, please let me know if my APR patch helps so
>>>> that I can prepare a fix for APR and SVN 1.8.x. I think we can do
>>>> better than making --search case-sensitive just because of a bug in APR.
>>>>
>>>> I don't see why we couldn't ship a private and fixed version of
>>>> apr_fnmatch() for use by log --search in 1.8.x, to avoid undefined
>>>> behaviour within tolower() via apr_fnmatch() without requiring future
>>>> APR versions. Would this not be a good way of fixing this?
>>> What would this fix?
>> Our custom version of apr_fnmatch() would not pass values to tolower()
>> which cannot be represented as unsigned char, thus eliminating undefined
>> behaviour and avoiding assertion failures on Windows. We would put this
>> custom fnmatch() into 'svn' (not the libraries) to specifically address
>> the assertion failures caused by svn log --search.
>>
>> Is that more clear and does it make sense?
>>
>>> This doesn't make apr_fnmatch use proper case folding, as that needs
>> proper
>>> UTF-8 processing and following locale rules.
>> AFAIK, we are talking about assertion failures in the test suite on
>> Windows. The discussion has balooned into adding locale-specific
>> case-folding to apr_fnmatch() in order to make log --search truly
>> case-insensitive in all locales. But that is a separate issue from
>> assertion failures in tests. I want to fix the assertions, I do not
>> want to open up the can of worms required to have locale-aware
>> case-insensitive matching in apr_fnmatch() (at least I don't want
>> to open it right now -- perhaps later but it would be a change to APR,
>> not Subversion).
> The assertion shows a design problem which we should handle for future compatibility and you suggest just adding some bandages to patch/hide the test failure?
>
> The current code is broken and the suggestion you do is like the solution mostly vetoed by most of the responders in this thread: assuming there is only us-english, by using a function that has platform specific behavior.
>
> (tolower() is locale and platform character encoding dependent. You should never just pass individual UTF-8 bytes to it)
>
>
>
> Personally, I don't care if 'svn' does such an assumption about the world and leave the bugfixing to future releases, but libsvn_* should never have such assumptions in it.
>
> But then please just hardcode converting 'a-z' to 'A-Z' for the log messages instead of pretending tolower() handles everything else for you. This has strictly defined behavior and shows that we need a better implementation later. While using tolower() with a hack around it just ships undefined behavior.

I tend to agree. As it is, it would be better to make --search
case-sensitive, or remove it from 1.8, than to tell people it's
case-insensitive but just happens not to work as advertised.

-- Brane

-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com