> -----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.
Bert
Received on 2013-04-22 13:00:56 CEST