On 22.04.2013 12:59, Bert Huijben wrote: > >> -----Original Message----- >> From: 'Stefan Sperling' [mailto:s...@elego.de] >> Sent: maandag 22 april 2013 11:46 >> To: Bert Huijben >> Cc: 'Ivan Zhakov'; 'Branko Čibej'; dev@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:s...@elego.de] >>>> Sent: maandag 22 april 2013 00:08 >>>> To: Ivan Zhakov >>>> Cc: Branko Čibej; Bert Huijben; dev@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
