You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2013/04/16 12:15:17 UTC
log --search test failures on trunk and 1.8.x
Hi,
I got following failures when running Debug build with VC 2010:
FAIL: log_tests.py 40: test 'svn log -g --search'
FAIL: log_tests.py 39: 'svn log --search'
They fail because of assertion in VC runtime:
[[[
W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
failed: (unsigned)(c + 1) <= 256
]]]
This happens because apr_fnmatch does not handle UTF8 character and
pass UTF lead char to tolower() call. There is already XXX comment in
apr\strings\apr_fnmatch.c:121
[[
/* Look at only well-formed range patterns;
* "x-]" is not allowed unless escaped ("x-\]")
* XXX: Fix for locale/MBCS character width
*/
if (((*pattern)[1] == '-') && ((*pattern)[2] != ']'))
{
startch = *pattern;
*pattern += (escape && ((*pattern)[2] == '\\')) ? 3 : 2;
/* NOT a properly balanced [expr] pattern, EOS terminated
* or ranges containing a slash in FNM_PATHNAME mode pattern
* fall out to to the rewind and test '[' literal code path
*/
if (!**pattern || (slash && (**pattern == '/')))
break;
/* XXX: handle locale/MBCS comparison, advance by MBCS
char width */
if ((**string >= *startch) && (**string <= **pattern))
result = 0;
else if (nocase && (isupper(**string) || isupper(*startch)
|| isupper(**pattern))
&& (tolower(**string) >= tolower(*startch))
&& (tolower(**string) <= tolower(**pattern)))
result = 0;
++*pattern;
continue;
}
/* XXX: handle locale/MBCS comparison, advance by MBCS char width */
if ((**string == **pattern))
result = 0;
else if (nocase && (isupper(**string) || isupper(**pattern))
&& (tolower(**string) == tolower(**pattern)))
result = 0;
]]
Easy solution could be use apr_fnmatch without APR_FNM_CASE_BLIND and
make svn --search case-sensitive. Alternative solution is convert
search pattern and search string to lower case before passing to
apr_fnmatch. And third option is to fix apr_fnmatch().
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Wed, Apr 24, 2013 at 1:34 AM, Branko Čibej <br...@wandisco.com> wrote:
> If you consider all this, the easiest approach by far might be to simply
> add a Lucene index of all log messages to the server, then you can and
> any number of bells and whistles including language-specific stemming.
> I'd consider that a better solution then any homegrown full-text search
> facility; these are never easy.
Ha! That might actually be easier than importing all of the Unicode
nastiness. =) -- justin
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 23.04.2013 14:51, Stefan Sperling wrote:
> On Tue, Apr 23, 2013 at 02:27:08PM +0200, Branko Čibej wrote:
>> You're missing the point. tolower() works on individual characters, not
>> whole strings; so it in general /cannot/ do correct locale-specific
> Do you really mean characters, or bytes?
> It sounds like you mean bytes. tolower() works on individual bytes.
It *does not matter* whether it's bytes or characters, it still cannot
do correct local-specific lowercasing.
[...]
>> Trying to retrofit anything less
>> smart onto apr_fnmatch will not work correctly.
> That depends on whether an fnmatch implementation is willing to live
> with the limitations of the locale mechanism (one opaque charset
> supported, any charset not in the current locale can give errors).
For the case we're considering, we don't care about conversion from
UTF-8, since we require log messages to be in UTF-8 anyway.
> It seems that some people do think fnmatch() should do it this way:
> http://opensource.apple.com/source/Libc/Libc-583/gen/FreeBSD/fnmatch.c
> (Caution: This implementation has the out-of-bounds recursion bug
> which made Bill rewrite fnmatch for APR...)
>
> Subversion already assumes it can convert strings from UTF-8 to the
> locale's character set for output. We could also assume that we can
> convert log messages from UTF-8 to the current locale charset, and
> write something that performs case-insensitive matching with wchar_t.
> However, that's clearly out of scope for 1.8 as well :)
Let me say again: comparing single characters is not correct case
folding. German is a good example of why that doesn't work: it does not
just have the ß == SS equivalence; for case-insensitive search, I'd also
expect ö == OE/oe == Ö etc. to be equivalent.
If you consider all this, the easiest approach by far might be to simply
add a Lucene index of all log messages to the server, then you can and
any number of bells and whistles including language-specific stemming.
I'd consider that a better solution then any homegrown full-text search
facility; these are never easy.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Apr 23, 2013 at 02:27:08PM +0200, Branko Čibej wrote:
> You're missing the point. tolower() works on individual characters, not
> whole strings; so it in general /cannot/ do correct locale-specific
Do you really mean characters, or bytes?
It sounds like you mean bytes. tolower() works on individual bytes.
> lowercasing. What you're looking for is something like strcoll(), except
> that it should be case-insensitive.
There are many existing functions that could be used to build
something fairly useful (wcscasecmp for instance).
> The long and short of it is that you need a complete locale-aware
> implementation of the Unicode standard to do proper case-insensitive
> comparisons; ICU is one such example.
Yes.
> Trying to retrofit anything less
> smart onto apr_fnmatch will not work correctly.
That depends on whether an fnmatch implementation is willing to live
with the limitations of the locale mechanism (one opaque charset
supported, any charset not in the current locale can give errors).
It seems that some people do think fnmatch() should do it this way:
http://opensource.apple.com/source/Libc/Libc-583/gen/FreeBSD/fnmatch.c
(Caution: This implementation has the out-of-bounds recursion bug
which made Bill rewrite fnmatch for APR...)
Subversion already assumes it can convert strings from UTF-8 to the
locale's character set for output. We could also assume that we can
convert log messages from UTF-8 to the current locale charset, and
write something that performs case-insensitive matching with wchar_t.
However, that's clearly out of scope for 1.8 as well :)
> (N.B., the svn_utf__glob function on the wc-collate-path branch is
> explicitly case-sensitive, it only deals with UTF normalization
> differences.)
>
>
> We can of course document the restriction that 'log --search' will give
> unexpected results when log messages are anything but ASCII. I'd even
> accept that as a marginal band-aid provided we promis to (at least
> partially) fix it in 1.9. Now, we can't really make that promise, can we. :)
I don't know. Perhaps just removing case-insensitive search is the
best option. We can always add it back later and do it right (under
some reasonable definition of "right" which, as far as I'm concerned,
remains to be determined).
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 23.04.2013 08:53, Stefan Sperling wrote:
> On Mon, Apr 22, 2013 at 01:13:43PM +0200, Branko Čibej wrote:
>> On 22.04.2013 12:59, Bert Huijben wrote:
>>> 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)
> OK Bert, I can see how, for example, a tolower() implementation which runs
> in a latin1 locale could convert parts of a UTF-8 string which contains
> bytes that are part of a multibyte character, if such bytes happen to have
> the same value as some upper case letter from the latin1 symbol range [128-255].
You're missing the point. tolower() works on individual characters, not
whole strings; so it in general /cannot/ do correct locale-specific
lowercasing. What you're looking for is something like strcoll(), except
that it should be case-insensitive.
The long and short of it is that you need a complete locale-aware
implementation of the Unicode standard to do proper case-insensitive
comparisons; ICU is one such example. Trying to retrofit anything less
smart onto apr_fnmatch will not work correctly.
(N.B., the svn_utf__glob function on the wc-collate-path branch is
explicitly case-sensitive, it only deals with UTF normalization
differences.)
We can of course document the restriction that 'log --search' will give
unexpected results when log messages are anything but ASCII. I'd even
accept that as a marginal band-aid provided we promis to (at least
partially) fix it in 1.9. Now, we can't really make that promise, can we. :)
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 22, 2013 at 01:13:43PM +0200, Branko Čibej wrote:
> On 22.04.2013 12:59, Bert Huijben wrote:
> > 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)
OK Bert, I can see how, for example, a tolower() implementation which runs
in a latin1 locale could convert parts of a UTF-8 string which contains
bytes that are part of a multibyte character, if such bytes happen to have
the same value as some upper case letter from the latin1 symbol range [128-255].
Users could be running --search for one or more such upper-case characters
and get results that match at the byte-level (after tolower() conversion)
but don't make any sense because the characters matched are entirely different.
E.g. they get could get some log message which contains only Chinese
characters when they run 'svn log --search Ö', in case the byte used
for Ö matches part of the UTF-8 representation of a Chinese character.
I can see that this kind of behaviour could be confusing, and that it
is "undefined" in the sense that it is locale-dependent. But I find
your notion that it is entirely buggy somewhat exaggerating, given that
apr_fnmatch() makes no claims to support multibyte characters in the
first place. So please provide an example for the kinds of bugs you
are thinking of -- are they any worse than my example?
In case you're making an argument based on the assumption that tolower()
could try to do something intelligent with UTF-8 characters, let's agree
that tolower() cannot use multibyte characters. It only supports single byte
character sets where symbols are taken from the range [0-255], and it also
supports the special symbol EOF (represented by a -1 in some implementations)
which is why it accepts an 'int'. There is a towlower() function which accepts
'wchar_t' instead of 'int' and could thus be used to support multibyte
characters. But apr_fnmatch() doesn't use towlower().
Perhaps fnmatch() simply isn't a good interface to use for the purposes
of log --search. But it is all that APR provides for us.
> > 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.
>
Hard-coding a conversion from a-z to A-Z before passing strings
to apr_fnmatch() would indeed provide all functionality we can expect
from apr_fnmatch() with the CASE_BLIND flag set.
But this would really be a workaround to avoid the CASE_BLIND flag.
If we're going to say that CASE_BLIND should not be used, I'd like
to point out that the autoprops code is also using it and could thus
suffer similar problems (e.g. wrongly apply properties to files with
certain multibyte-character names in certain locales).
Do we need to do anything there?
> 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.
Branko,
we haven't documented how 'svn log --search' behaves in this regard.
As far as I'm concerned its behaviour can change between releases.
It's a feature of the command line client, not the API, and it's
meant to be a convenient function for humans, not scripts.
If 1.8 matches English characters in a case-insensitive way, and characters
from other languages in a case-sensitive way, and we fix that in 1.9 or
later e.g. by switching away from fnmatch() to something better, then
I don't see any problem.
If this were implemented as an API, I'd have more concerns, of course.
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 22.04.2013 12:59, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: 'Stefan Sperling' [mailto:stsp@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:stsp@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
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: 'Stefan Sperling' [mailto:stsp@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:stsp@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.
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by 'Stefan Sperling' <st...@elego.de>.
On Mon, Apr 22, 2013 at 11:22:11AM +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: Stefan Sperling [mailto:stsp@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).
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@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?
This doesn't make apr_fnmatch use proper case folding, as that needs proper
UTF-8 processing and following locale rules.
The hack will make apr_fnmatch whatever is the current locale casing and
current chararacter encoding rules on the platform for our UTF-8 characters
in the log message. (Which are known to not always be strict utf-8). And
even there it ignores multibyte characters by handing them to tolower one at
a time.
This is just stacking undefined behaviour.
If you would first convert the characters back to the system encoding and
then pass things to apr_fnmatch it would be good enough for an 'svn'
implementation for me, but probably not for our libraries.
Like Ivan suggested: for our libraries we want strict behavior over all
platforms, no undefined behavior.
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@elego.de>.
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?
Re: log --search test failures on trunk and 1.8.x
Posted by Mattias Engdegård <ma...@bredband.net>.
21 apr 2013 kl. 20.07 skrev Branko Čibej:
> Yes, the obvious ones are German (ß == SS) equivalence and turkic (i
> ==
> İ) and (ı == I) equivalences (and that's aready three characters);
> but
> then in French, lowercase accented letters are equivalent to uppercase
> unaccented letters, whereas for example in Spanish that's not the
> case.
> And that's just looking at European and West Asian Latin scripts.
> There
> are at least 7 distinct Cyrillic scripts in roughly the same area that
> I'm aware of, and I certainly don't know the case-folding rules for
> all
> of them.
Not only is the above true, one should also be careful to distinguish
case conversion from case-insensitive matching; these follow different
rules.
For instance, converting lower-case letters to upper case in French
will retain the accents (most of the time - this is locale-dependent),
but they are generally expected to be ignored when searching. By
contrast, it would be an error to match "a" with "ä" in Swedish when
searching, or to drop the dots in a case conversion.
Clearly a case- and accent-sensitive search is much easier to
implement, but would benefit from normalisation. Bytewise matching is
on the lowest rung.
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Apr 21, 2013 at 10:50 PM, Bert Huijben <be...@qqmail.nl> wrote:
> Note that Unicode 3.2 (where these links point to) is over 10 years old. The
> current version is 6.2... and things haven’t got easier with newer versions.
>
Good point. But it seems there is no big difference in this area.
> A different route would be to convert the characters from our utf-8 to the
> native platform encoding (using our existing apis for that) and then make
> the platform do the case folding for us
In general I prefer to avoid platform specific code in Subversion: it
makes testing and maintenance more complicated due different
configurations.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
Note that Unicode 3.2 (where these links point to) is over 10 years old. The current version is 6.2... and things haven’t got easier with newer versions.
A different route would be to convert the characters from our utf-8 to the native platform encoding (using our existing apis for that) and then make the platform do the case folding for us before apr does the comparison/search.
Bert
Sent from Windows Mail
From: Ivan Zhakov
Sent: Sunday, April 21, 2013 8:44 PM
To: Branko Čibej
Cc: dev@subversion.apache.org
On Sun, Apr 21, 2013 at 10:07 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.04.2013 17:11, Ivan Zhakov wrote:
>> On Sun, Apr 21, 2013 at 4:48 PM, Branko Čibej <br...@wandisco.com> wrote:
>>> On 21.04.2013 14:05, Stefan Sperling wrote:
>>>> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>>>>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
>>>> What's the issue with the current implementation apart from the
>>>> test failures on Windows?
>>>>
>>>> The behaviour of 'svn log --search' regarding case-sensitivity
>>>> isn't even documented, so we're not really prosmising anything.
>>>>
>>>> It is possible that some users who are using languages other than
>>>> English will complain, since ASCII is being matched case-insensitively,
>>>> and all other characters are being matched case-sensitively.
>>>> But this is due to a missing feature in APR's implemention of fnmatch().
>>>>
>>>> Provided we can fix the 1.8.x tests on Windows I see no reason to
>>>> change our implementation of log --search. We can simply wait for
>>>> APR to grow the necessary support for multibyte strings.
>>> The wc-collate-path branch has an svn_utf__glob function that's mainly
>>> intended for use by SQLite, however, it can be a replacement for
>>> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
>>> to Unicode normalization form D, which keeps diacriticals separate from
>>> the base letters. In other words, we could easily extend that to do
>>> completely diacritical-agnostic case-folding matching for Latin
>>> alphabets (and probably also for Cyrillic scripts).
>>>
>>> The idea to manually hack things to work with western Latin alphabets
>>> seems completely wrong-headed to me.
>>>
>>> But yes; in general, case folding is locale-specific. If we wanted to
>>> support that, we'd need ICU instead of utf8proc. I can imagine that
>>> eventually being an option, but not a mandatory dependency.
>>>
>> According to Unicode case folding data [1] the only two characters
>> uses locale specific case-folding.
>
> How on earth did you come to that conclusion?
>
> Yes, the obvious ones are German (ß == SS) equivalence and turkic (i ==
> İ) and (ı == I) equivalences (and that's aready three characters); but
> then in French, lowercase accented letters are equivalent to uppercase
> unaccented letters, whereas for example in Spanish that's not the case.
> And that's just looking at European and West Asian Latin scripts. There
> are at least 7 distinct Cyrillic scripts in roughly the same area that
> I'm aware of, and I certainly don't know the case-folding rules for all
> of them.
>
I've just read Unicode specs, but I didn't read all of them :)
According to the link I provided [1] there are 4 types of characters
in terms of case folding:
[[[
# The status field is:
# C: common case folding, common mappings shared by both simple and
full mappings.
# F: full case folding, mappings that cause strings to grow in length.
Multiple characters are separated by spaces.
# S: simple case folding, mappings to single characters where different from F.
# T: special case for uppercase I and dotted uppercase I
# - For non-Turkic languages, this mapping is normally not used.
# - For Turkic languages (tr, az), this mapping can be used instead
of the normal mapping for these characters.
]]]
In CaseFolding-3.2.0.txt the only 'T' chars needs locale depended handling.
BUT there is another document that describes special case-folding
rules [2] which list cases like ß == SS. I missed it. That's my fault.
[1] http://www.unicode.org/Public/3.2-Update/CaseFolding-3.2.0.txt
[2] http://www.unicode.org/Public/3.2-Update/SpecialCasing-3.2.0.txt
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Apr 21, 2013 at 10:07 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.04.2013 17:11, Ivan Zhakov wrote:
>> On Sun, Apr 21, 2013 at 4:48 PM, Branko Čibej <br...@wandisco.com> wrote:
>>> On 21.04.2013 14:05, Stefan Sperling wrote:
>>>> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>>>>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
>>>> What's the issue with the current implementation apart from the
>>>> test failures on Windows?
>>>>
>>>> The behaviour of 'svn log --search' regarding case-sensitivity
>>>> isn't even documented, so we're not really prosmising anything.
>>>>
>>>> It is possible that some users who are using languages other than
>>>> English will complain, since ASCII is being matched case-insensitively,
>>>> and all other characters are being matched case-sensitively.
>>>> But this is due to a missing feature in APR's implemention of fnmatch().
>>>>
>>>> Provided we can fix the 1.8.x tests on Windows I see no reason to
>>>> change our implementation of log --search. We can simply wait for
>>>> APR to grow the necessary support for multibyte strings.
>>> The wc-collate-path branch has an svn_utf__glob function that's mainly
>>> intended for use by SQLite, however, it can be a replacement for
>>> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
>>> to Unicode normalization form D, which keeps diacriticals separate from
>>> the base letters. In other words, we could easily extend that to do
>>> completely diacritical-agnostic case-folding matching for Latin
>>> alphabets (and probably also for Cyrillic scripts).
>>>
>>> The idea to manually hack things to work with western Latin alphabets
>>> seems completely wrong-headed to me.
>>>
>>> But yes; in general, case folding is locale-specific. If we wanted to
>>> support that, we'd need ICU instead of utf8proc. I can imagine that
>>> eventually being an option, but not a mandatory dependency.
>>>
>> According to Unicode case folding data [1] the only two characters
>> uses locale specific case-folding.
>
> How on earth did you come to that conclusion?
>
> Yes, the obvious ones are German (ß == SS) equivalence and turkic (i ==
> İ) and (ı == I) equivalences (and that's aready three characters); but
> then in French, lowercase accented letters are equivalent to uppercase
> unaccented letters, whereas for example in Spanish that's not the case.
> And that's just looking at European and West Asian Latin scripts. There
> are at least 7 distinct Cyrillic scripts in roughly the same area that
> I'm aware of, and I certainly don't know the case-folding rules for all
> of them.
>
I've just read Unicode specs, but I didn't read all of them :)
According to the link I provided [1] there are 4 types of characters
in terms of case folding:
[[[
# The status field is:
# C: common case folding, common mappings shared by both simple and
full mappings.
# F: full case folding, mappings that cause strings to grow in length.
Multiple characters are separated by spaces.
# S: simple case folding, mappings to single characters where different from F.
# T: special case for uppercase I and dotted uppercase I
# - For non-Turkic languages, this mapping is normally not used.
# - For Turkic languages (tr, az), this mapping can be used instead
of the normal mapping for these characters.
]]]
In CaseFolding-3.2.0.txt the only 'T' chars needs locale depended handling.
BUT there is another document that describes special case-folding
rules [2] which list cases like ß == SS. I missed it. That's my fault.
[1] http://www.unicode.org/Public/3.2-Update/CaseFolding-3.2.0.txt
[2] http://www.unicode.org/Public/3.2-Update/SpecialCasing-3.2.0.txt
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 21.04.2013 17:11, Ivan Zhakov wrote:
> On Sun, Apr 21, 2013 at 4:48 PM, Branko Čibej <br...@wandisco.com> wrote:
>> On 21.04.2013 14:05, Stefan Sperling wrote:
>>> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>>>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
>>> What's the issue with the current implementation apart from the
>>> test failures on Windows?
>>>
>>> The behaviour of 'svn log --search' regarding case-sensitivity
>>> isn't even documented, so we're not really prosmising anything.
>>>
>>> It is possible that some users who are using languages other than
>>> English will complain, since ASCII is being matched case-insensitively,
>>> and all other characters are being matched case-sensitively.
>>> But this is due to a missing feature in APR's implemention of fnmatch().
>>>
>>> Provided we can fix the 1.8.x tests on Windows I see no reason to
>>> change our implementation of log --search. We can simply wait for
>>> APR to grow the necessary support for multibyte strings.
>> The wc-collate-path branch has an svn_utf__glob function that's mainly
>> intended for use by SQLite, however, it can be a replacement for
>> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
>> to Unicode normalization form D, which keeps diacriticals separate from
>> the base letters. In other words, we could easily extend that to do
>> completely diacritical-agnostic case-folding matching for Latin
>> alphabets (and probably also for Cyrillic scripts).
>>
>> The idea to manually hack things to work with western Latin alphabets
>> seems completely wrong-headed to me.
>>
>> But yes; in general, case folding is locale-specific. If we wanted to
>> support that, we'd need ICU instead of utf8proc. I can imagine that
>> eventually being an option, but not a mandatory dependency.
>>
> According to Unicode case folding data [1] the only two characters
> uses locale specific case-folding.
How on earth did you come to that conclusion?
Yes, the obvious ones are German (ß == SS) equivalence and turkic (i ==
İ) and (ı == I) equivalences (and that's aready three characters); but
then in French, lowercase accented letters are equivalent to uppercase
unaccented letters, whereas for example in Spanish that's not the case.
And that's just looking at European and West Asian Latin scripts. There
are at least 7 distinct Cyrillic scripts in roughly the same area that
I'm aware of, and I certainly don't know the case-folding rules for all
of them.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Apr 21, 2013 at 4:48 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.04.2013 14:05, Stefan Sperling wrote:
>> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
>> What's the issue with the current implementation apart from the
>> test failures on Windows?
>>
>> The behaviour of 'svn log --search' regarding case-sensitivity
>> isn't even documented, so we're not really prosmising anything.
>>
>> It is possible that some users who are using languages other than
>> English will complain, since ASCII is being matched case-insensitively,
>> and all other characters are being matched case-sensitively.
>> But this is due to a missing feature in APR's implemention of fnmatch().
>>
>> Provided we can fix the 1.8.x tests on Windows I see no reason to
>> change our implementation of log --search. We can simply wait for
>> APR to grow the necessary support for multibyte strings.
>
> The wc-collate-path branch has an svn_utf__glob function that's mainly
> intended for use by SQLite, however, it can be a replacement for
> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
> to Unicode normalization form D, which keeps diacriticals separate from
> the base letters. In other words, we could easily extend that to do
> completely diacritical-agnostic case-folding matching for Latin
> alphabets (and probably also for Cyrillic scripts).
>
> The idea to manually hack things to work with western Latin alphabets
> seems completely wrong-headed to me.
>
> But yes; in general, case folding is locale-specific. If we wanted to
> support that, we'd need ICU instead of utf8proc. I can imagine that
> eventually being an option, but not a mandatory dependency.
>
According to Unicode case folding data [1] the only two characters
uses locale specific case-folding.
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
apr_fnmatch(svn_utf__casefold(pattern), svn_utf__casefold(string))
[1] http://www.unicode.org/Public/3.2-Update/CaseFolding-3.2.0.txt
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Sun, Apr 21, 2013 at 9:37 AM, Bert Huijben <be...@qqmail.nl> wrote:
> Do we want case folding (or at least case variant compare) support in our
> libraries for 1.8?
>
No.
> Or is this 1.9+ scope?
>
Yes. =)
My $.02. -- justin
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 21.04.2013 15:37, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@wandisco.com]
>> Sent: zondag 21 april 2013 14:48
>> To: dev@subversion.apache.org
>> Subject: Re: log --search test failures on trunk and 1.8.x
>>
>> On 21.04.2013 14:05, Stefan Sperling wrote:
>>> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>>>> I'd rather pull the case insensitive search part of this new in 1.8 search
>> feature and do it right in 1.9.
>>> What's the issue with the current implementation apart from the
>>> test failures on Windows?
>>>
>>> The behaviour of 'svn log --search' regarding case-sensitivity
>>> isn't even documented, so we're not really prosmising anything.
>>>
>>> It is possible that some users who are using languages other than
>>> English will complain, since ASCII is being matched case-insensitively,
>>> and all other characters are being matched case-sensitively.
>>> But this is due to a missing feature in APR's implemention of fnmatch().
>>>
>>> Provided we can fix the 1.8.x tests on Windows I see no reason to
>>> change our implementation of log --search. We can simply wait for
>>> APR to grow the necessary support for multibyte strings.
>> The wc-collate-path branch has an svn_utf__glob function that's mainly
>> intended for use by SQLite, however, it can be a replacement for
>> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
>> to Unicode normalization form D, which keeps diacriticals separate from
>> the base letters. In other words, we could easily extend that to do
>> completely diacritical-agnostic case-folding matching for Latin
>> alphabets (and probably also for Cyrillic scripts).
>>
>> The idea to manually hack things to work with western Latin alphabets
>> seems completely wrong-headed to me.
>>
>> But yes; in general, case folding is locale-specific. If we wanted to
>> support that, we'd need ICU instead of utf8proc. I can imagine that
>> eventually being an option, but not a mandatory dependency.
> Summarizing: What would it help to include utf8proc on trunk now for this issue?
>
> Your conclusion is (similar to mine) that we need more for case folding than what we have now and/or what utf8proc will offer us.
>
> Do we want case folding (or at least case variant compare) support in our libraries for 1.8?
>
> Or is this 1.9+ scope?
It's 1.9+ scope. It would be madness to merge such an invasive change to
trunk at this late date (IMHO).
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: zondag 21 april 2013 14:48
> To: dev@subversion.apache.org
> Subject: Re: log --search test failures on trunk and 1.8.x
>
> On 21.04.2013 14:05, Stefan Sperling wrote:
> > On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
> >> I'd rather pull the case insensitive search part of this new in 1.8 search
> feature and do it right in 1.9.
> > What's the issue with the current implementation apart from the
> > test failures on Windows?
> >
> > The behaviour of 'svn log --search' regarding case-sensitivity
> > isn't even documented, so we're not really prosmising anything.
> >
> > It is possible that some users who are using languages other than
> > English will complain, since ASCII is being matched case-insensitively,
> > and all other characters are being matched case-sensitively.
> > But this is due to a missing feature in APR's implemention of fnmatch().
> >
> > Provided we can fix the 1.8.x tests on Windows I see no reason to
> > change our implementation of log --search. We can simply wait for
> > APR to grow the necessary support for multibyte strings.
>
> The wc-collate-path branch has an svn_utf__glob function that's mainly
> intended for use by SQLite, however, it can be a replacement for
> apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
> to Unicode normalization form D, which keeps diacriticals separate from
> the base letters. In other words, we could easily extend that to do
> completely diacritical-agnostic case-folding matching for Latin
> alphabets (and probably also for Cyrillic scripts).
>
> The idea to manually hack things to work with western Latin alphabets
> seems completely wrong-headed to me.
>
> But yes; in general, case folding is locale-specific. If we wanted to
> support that, we'd need ICU instead of utf8proc. I can imagine that
> eventually being an option, but not a mandatory dependency.
Summarizing: What would it help to include utf8proc on trunk now for this issue?
Your conclusion is (similar to mine) that we need more for case folding than what we have now and/or what utf8proc will offer us.
Do we want case folding (or at least case variant compare) support in our libraries for 1.8?
Or is this 1.9+ scope?
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 21.04.2013 14:05, Stefan Sperling wrote:
> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
> What's the issue with the current implementation apart from the
> test failures on Windows?
>
> The behaviour of 'svn log --search' regarding case-sensitivity
> isn't even documented, so we're not really prosmising anything.
>
> It is possible that some users who are using languages other than
> English will complain, since ASCII is being matched case-insensitively,
> and all other characters are being matched case-sensitively.
> But this is due to a missing feature in APR's implemention of fnmatch().
>
> Provided we can fix the 1.8.x tests on Windows I see no reason to
> change our implementation of log --search. We can simply wait for
> APR to grow the necessary support for multibyte strings.
The wc-collate-path branch has an svn_utf__glob function that's mainly
intended for use by SQLite, however, it can be a replacement for
apr_fnmatch. It uses apr_fnmatch internally, but decomposes the inputs
to Unicode normalization form D, which keeps diacriticals separate from
the base letters. In other words, we could easily extend that to do
completely diacritical-agnostic case-folding matching for Latin
alphabets (and probably also for Cyrillic scripts).
The idea to manually hack things to work with western Latin alphabets
seems completely wrong-headed to me.
But yes; in general, case folding is locale-specific. If we wanted to
support that, we'd need ICU instead of utf8proc. I can imagine that
eventually being an option, but not a mandatory dependency.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: zondag 21 april 2013 14:05
> To: Bert Huijben
> Cc: 'Branko Čibej'; dev@subversion.apache.org
> Subject: Re: log --search test failures on trunk and 1.8.x
>
> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
> > I'd rather pull the case insensitive search part of this new in 1.8
search
> feature and do it right in 1.9.
>
> What's the issue with the current implementation apart from the
> test failures on Windows?
What I try to avoid is adding the new api's in libsvn_subr which we would
have to maintain indefinitely even though we might not use them from
anywhere except 'svn'
>
> The behaviour of 'svn log --search' regarding case-sensitivity
> isn't even documented, so we're not really prosmising anything.
>
> It is possible that some users who are using languages other than
> English will complain, since ASCII is being matched case-insensitively,
> and all other characters are being matched case-sensitively.
> But this is due to a missing feature in APR's implemention of fnmatch().
>
> Provided we can fix the 1.8.x tests on Windows I see no reason to
> change our implementation of log --search. We can simply wait for
> APR to grow the necessary support for multibyte strings.
We currently trigger undefined behavior...This is something completely
different than 'trying to fix some tests'
Undefined behavior might be that it works on the buildbot, but formats every
users harddisk.
An easy fix could be that we perform case normalization in 'svn' itself by
just converting [a-z] to [A-Z] in all the strings before passing them to
apr. (Normalizing to upper case is in general much safer then normalizing to
lower case. Not normalizing and using a proper compare function is the best
fix)
Note that this is unrelated from the utf normalization problems what we
tried to use utf8proc for. (Which is a problem that is resolvable by a fixed
rules table, while case folding via a locale isn't)
I don't think we should bump the required apr version to fix this problem
and should try to fix this in 'svn' instead.
APR currently doesn't document a specific character encoding and/or locale
for its fnmatch function, so a patch to make it comply to a specific
character encoding and locale is probably not the right fix.
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sun, Apr 21, 2013 at 4:05 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
>> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
>
> What's the issue with the current implementation apart from the
> test failures on Windows?
>
Search is case-insensitive for only latin characters. This is really confusing.
> The behaviour of 'svn log --search' regarding case-sensitivity
> isn't even documented, so we're not really prosmising anything.
>
Ok, what about make 'log --search' case-sensitive in 1.8 and implement
'log --isearch' in 1.9 when we merge utf8proc stuff to trunk?
> It is possible that some users who are using languages other than
> English will complain, since ASCII is being matched case-insensitively,
> and all other characters are being matched case-sensitively.
> But this is due to a missing feature in APR's implemention of fnmatch().
>
Users do not care whether this APR or Subversion problem. They just use svn.exe.
> Provided we can fix the 1.8.x tests on Windows I see no reason to
> change our implementation of log --search. We can simply wait for
> APR to grow the necessary support for multibyte strings.
>
There is really small chance that this issue will be fixed in near
future. For proper fix APR should link utf8proc or alternative
library for true case-folding.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Apr 21, 2013 at 01:53:43PM +0200, Bert Huijben wrote:
> I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
What's the issue with the current implementation apart from the
test failures on Windows?
The behaviour of 'svn log --search' regarding case-sensitivity
isn't even documented, so we're not really prosmising anything.
It is possible that some users who are using languages other than
English will complain, since ASCII is being matched case-insensitively,
and all other characters are being matched case-sensitively.
But this is due to a missing feature in APR's implemention of fnmatch().
Provided we can fix the 1.8.x tests on Windows I see no reason to
change our implementation of log --search. We can simply wait for
APR to grow the necessary support for multibyte strings.
Does the patch to APR below fix the 1.8.x tests on Windows?
Index: strings/apr_fnmatch.c
===================================================================
--- strings/apr_fnmatch.c (revision 1470158)
+++ strings/apr_fnmatch.c (working copy)
@@ -135,8 +135,10 @@ leadingclosebrace:
/* XXX: handle locale/MBCS comparison, advance by MBCS char width */
if ((**string >= *startch) && (**string <= **pattern))
result = 0;
- else if (nocase && (isupper(**string) || isupper(*startch)
- || isupper(**pattern))
+ else if (nocase
+ && **string >= 0 && *startch >= 0 && **pattern >= 0
+ && (isupper(**string) || isupper(*startch)
+ || isupper(**pattern))
&& (tolower(**string) >= tolower(*startch))
&& (tolower(**string) <= tolower(**pattern)))
result = 0;
@@ -148,7 +150,8 @@ leadingclosebrace:
/* XXX: handle locale/MBCS comparison, advance by MBCS char width */
if ((**string == **pattern))
result = 0;
- else if (nocase && (isupper(**string) || isupper(**pattern))
+ else if (nocase && **string >= 0 && **pattern >= 0
+ && (isupper(**string) || isupper(**pattern))
&& (tolower(**string) == tolower(**pattern)))
result = 0;
@@ -175,7 +178,8 @@ leadingclosebrace:
/* XXX: handle locale/MBCS comparison, advance by the MBCS char width */
if (**string == **pattern)
result = 0;
- else if (nocase && (isupper(**string) || isupper(**pattern))
+ else if (nocase && **string >= 0 && **pattern >= 0
+ && (isupper(**string) || isupper(**pattern))
&& (tolower(**string) == tolower(**pattern)))
result = 0;
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: zaterdag 20 april 2013 18:40
> To: dev@subversion.apache.org
> Subject: Re: log --search test failures on trunk and 1.8.x
>
> On 20.04.2013 18:30, Ivan Zhakov wrote:
> > On Sat, Apr 20, 2013 at 8:04 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> On Sat, Apr 20, 2013 at 7:54 PM, Branko Čibej <br...@wandisco.com>
> wrote:
> >>> On 20.04.2013 17:42, Stefan Sperling wrote:
> >>>> On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
> >>>>> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org>
> wrote:
> >>>>>> How is lower-casing a multi-byte UTF-8 character going to help?
> >>>>>> Won't the lower-case equivalent still be a multibyte character
> >>>>>> and trigger the overflow check in Visual Studio?
> >>>>>>
> >>>>> Because in this case we can call apr_fnmatch without
> >>>>> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
> >>>>> match.
> >>>> Are we going to implement our own code to transform a string
> >>>> to lower case? Or do we already have such functions for UTF-8 strings?
> >> No, we don't have such functions. We can try to convert UTF-8 string
> >> to wchar_t and use towlower(), but probably it's overkill.
> >>
> >>> We aren't and we don't, on trunk; but the wc-collate-path branch does
> >>> have code that can do Unicode-aware case folding through utf8proc.
> >>>
> >> It looks like a best solution.
> >>
> > I've looked to wc-collate-path branch and it seems utf8proc stuff is
> > useful. Can we merge utf8proc to trunk?
>
> I was going to wait until the 1.8-RC specific changes get finished, to
> make merging easier. I'd also planned to replace a bunch of homegrown
> UTF-8 processing functions with utf8proc eventually.
What locales is utf8proc going to provide in-tree?
If we just get case folding for the common western cases we can just write some simple case folding in 'svn'. I would say adding a build requirement like this for such a specific 'svn'-client feature is a bit overkill in this stage of release.
Adding all the required plumbing now to make this a supported api for all future releases after branching 1.8...
I'd rather pull the case insensitive search part of this new in 1.8 search feature and do it right in 1.9.
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 20.04.2013 18:30, Ivan Zhakov wrote:
> On Sat, Apr 20, 2013 at 8:04 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Sat, Apr 20, 2013 at 7:54 PM, Branko Čibej <br...@wandisco.com> wrote:
>>> On 20.04.2013 17:42, Stefan Sperling wrote:
>>>> On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
>>>>> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
>>>>>> How is lower-casing a multi-byte UTF-8 character going to help?
>>>>>> Won't the lower-case equivalent still be a multibyte character
>>>>>> and trigger the overflow check in Visual Studio?
>>>>>>
>>>>> Because in this case we can call apr_fnmatch without
>>>>> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
>>>>> match.
>>>> Are we going to implement our own code to transform a string
>>>> to lower case? Or do we already have such functions for UTF-8 strings?
>> No, we don't have such functions. We can try to convert UTF-8 string
>> to wchar_t and use towlower(), but probably it's overkill.
>>
>>> We aren't and we don't, on trunk; but the wc-collate-path branch does
>>> have code that can do Unicode-aware case folding through utf8proc.
>>>
>> It looks like a best solution.
>>
> I've looked to wc-collate-path branch and it seems utf8proc stuff is
> useful. Can we merge utf8proc to trunk?
I was going to wait until the 1.8-RC specific changes get finished, to
make merging easier. I'd also planned to replace a bunch of homegrown
UTF-8 processing functions with utf8proc eventually.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sat, Apr 20, 2013 at 8:04 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sat, Apr 20, 2013 at 7:54 PM, Branko Čibej <br...@wandisco.com> wrote:
>> On 20.04.2013 17:42, Stefan Sperling wrote:
>>> On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
>>>> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
>>>>> How is lower-casing a multi-byte UTF-8 character going to help?
>>>>> Won't the lower-case equivalent still be a multibyte character
>>>>> and trigger the overflow check in Visual Studio?
>>>>>
>>>> Because in this case we can call apr_fnmatch without
>>>> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
>>>> match.
>>> Are we going to implement our own code to transform a string
>>> to lower case? Or do we already have such functions for UTF-8 strings?
>>
> No, we don't have such functions. We can try to convert UTF-8 string
> to wchar_t and use towlower(), but probably it's overkill.
>
>> We aren't and we don't, on trunk; but the wc-collate-path branch does
>> have code that can do Unicode-aware case folding through utf8proc.
>>
> It looks like a best solution.
>
I've looked to wc-collate-path branch and it seems utf8proc stuff is
useful. Can we merge utf8proc to trunk?
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sat, Apr 20, 2013 at 7:54 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 20.04.2013 17:42, Stefan Sperling wrote:
>> On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
>>> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
>>>> How is lower-casing a multi-byte UTF-8 character going to help?
>>>> Won't the lower-case equivalent still be a multibyte character
>>>> and trigger the overflow check in Visual Studio?
>>>>
>>> Because in this case we can call apr_fnmatch without
>>> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
>>> match.
>> Are we going to implement our own code to transform a string
>> to lower case? Or do we already have such functions for UTF-8 strings?
>
No, we don't have such functions. We can try to convert UTF-8 string
to wchar_t and use towlower(), but probably it's overkill.
> We aren't and we don't, on trunk; but the wc-collate-path branch does
> have code that can do Unicode-aware case folding through utf8proc.
>
It looks like a best solution.
> It's always been wrong to use tolower for case-insensitive comparisons
> anyway, but we've tended to ignore that.
>
Yes, I know about this but I think we can currently ignore it.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: log --search test failures on trunk and 1.8.x
Posted by Branko Čibej <br...@wandisco.com>.
On 20.04.2013 17:42, Stefan Sperling wrote:
> On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
>> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
>>> How is lower-casing a multi-byte UTF-8 character going to help?
>>> Won't the lower-case equivalent still be a multibyte character
>>> and trigger the overflow check in Visual Studio?
>>>
>> Because in this case we can call apr_fnmatch without
>> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
>> match.
> Are we going to implement our own code to transform a string
> to lower case? Or do we already have such functions for UTF-8 strings?
We aren't and we don't, on trunk; but the wc-collate-path branch does
have code that can do Unicode-aware case folding through utf8proc.
It's always been wrong to use tolower for case-insensitive comparisons
anyway, but we've tended to ignore that.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Apr 20, 2013 at 07:26:06PM +0400, Ivan Zhakov wrote:
> On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
> > How is lower-casing a multi-byte UTF-8 character going to help?
> > Won't the lower-case equivalent still be a multibyte character
> > and trigger the overflow check in Visual Studio?
> >
> Because in this case we can call apr_fnmatch without
> APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
> match.
Are we going to implement our own code to transform a string
to lower case? Or do we already have such functions for UTF-8 strings?
Or do you want to bypass APR and call the tolower() function
provided by the operating system directly? That could result
in similar issues if the current locale isn't using UTF-8.
> >> And third option is to fix apr_fnmatch().
> >
> > I'd very much like to do that, actually. Just have to find time.
> I think the problem that svn log --search is case-insensitive only for
> ASCII (latin) chars, while characters in other locales handled in
> case-sensitive way. This could confuse non-English users.
Yes, it can. But that's a limitation of APR, not Subversion.
Once APR is fixed it will work fine.
Re: log --search test failures on trunk and 1.8.x
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sat, Apr 20, 2013 at 2:27 PM, Stefan Sperling <st...@apache.org> wrote:
> On Tue, Apr 16, 2013 at 02:15:17PM +0400, Ivan Zhakov wrote:
>> Hi,
>>
>> I got following failures when running Debug build with VC 2010:
>> FAIL: log_tests.py 40: test 'svn log -g --search'
>> FAIL: log_tests.py 39: 'svn log --search'
>>
>> They fail because of assertion in VC runtime:
>> [[[
>> W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
>> failed: (unsigned)(c + 1) <= 256
>> ]]]
>
> Seems like a bogus check to me. Is there a notion of debug vs. release
> builds in Visual Studio, and if so, does this happen in release mode
> builds?
>
> tolower() is supposed to return the byte unchanged in case it cannot
> be converted to a lower-case representation. I would expect that
> to happen in your case.
>
> See http://pubs.opengroup.org/onlinepubs/9699919799/functions/tolower.html
> It doesn't say "fail if the value overflows".
It doesn't fails, it just asserts because caller violated API
restrictions. See Bert's reply.
>
>> This happens because apr_fnmatch does not handle UTF8 character and
>> pass UTF lead char to tolower() call. There is already XXX comment in
>> apr\strings\apr_fnmatch.c:121
>
> Yes, apr_fnmatch() doesn't support multi-byte characters yet.
> And it never has. But the fnmatch implementation was changed
> in APR-1.4.5, so it is possible that this problem didn't trigger
> with the old implementation.
>
>> Easy solution could be use apr_fnmatch without APR_FNM_CASE_BLIND and
>> make svn --search case-sensitive. Alternative solution is convert
>> search pattern and search string to lower case before passing to
>> apr_fnmatch.
>
> How is lower-casing a multi-byte UTF-8 character going to help?
> Won't the lower-case equivalent still be a multibyte character
> and trigger the overflow check in Visual Studio?
>
Because in this case we can call apr_fnmatch without
APR_FNM_CASE_BLIND, avoid tolower calls and get case-insensitive
match.
>> And third option is to fix apr_fnmatch().
>
> I'd very much like to do that, actually. Just have to find time.
I think the problem that svn log --search is case-insensitive only for
ASCII (latin) chars, while characters in other locales handled in
case-sensitive way. This could confuse non-English users.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
RE: log --search test failures on trunk and 1.8.x
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@apache.org]
> Sent: zaterdag 20 april 2013 12:28
> To: Ivan Zhakov
> Cc: Subversion Development
> Subject: Re: log --search test failures on trunk and 1.8.x
>
> On Tue, Apr 16, 2013 at 02:15:17PM +0400, Ivan Zhakov wrote:
> > Hi,
> >
> > I got following failures when running Debug build with VC 2010:
> > FAIL: log_tests.py 40: test 'svn log -g --search'
> > FAIL: log_tests.py 39: 'svn log --search'
> >
> > They fail because of assertion in VC runtime:
> > [[[
> > W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
> > failed: (unsigned)(c + 1) <= 256
> > ]]]
>
> Seems like a bogus check to me. Is there a notion of debug vs. release
> builds in Visual Studio, and if so, does this happen in release mode
> builds?
>
> tolower() is supposed to return the byte unchanged in case it cannot
> be converted to a lower-case representation. I would expect that
> to happen in your case.
>
> See
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/tolower.html
> It doesn't say "fail if the value overflows".
Looking at this reference you handed:
---
The tolower() [CX] [Option Start] and tolower_l() [Option End] functions
have as a domain a type int, the value of which is representable as an
unsigned char or the value of EOF. If the argument has any other value, the
behavior is undefined.
---
So theoretically it is allowed to eat your harddrive, kill some hardware, or
whatever if you pass it an invalid value.
Note that apr has a wrapper of this function which documents more of its
behavior.
Bert
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@apache.org>.
On Sat, Apr 20, 2013 at 12:54:45PM +0200, Mattias Engdegård wrote:
> 20 apr 2013 kl. 12.27 skrev Stefan Sperling:
>
> >>W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
> >>failed: (unsigned)(c + 1) <= 256
> >>]]]
> >
> >Seems like a bogus check to me. Is there a notion of debug vs. release
> >builds in Visual Studio, and if so, does this happen in release mode
> >builds?
>
> Sorry about barging in like this, but isn't this just a case of
> calling ctype functions/macros with a char argument, where char is
> signed by default? It's prohibited by the standard, although glibc
> is "nice" enough to allow values in the [-128, 255] range just
> because this mistake is so common.
Ah, yes! That explains the check. Thanks.
> Looking at apr_fnmatch.c, it indeed seems like that is what is going on.
Yes, since fnmatch() takes const char * arguments.
> >See http://pubs.opengroup.org/onlinepubs/9699919799/functions/tolower.html
> >It doesn't say "fail if the value overflows".
>
> That is not how the C standard works. When a library call is used
> with arguments for which its behaviour is not defined, this results
> in undefined behaviour.
I've refreshed my memory on signed/unsigned conversion rules (scary stuff).
The critical bit seems to be this one:
The tolower() and tolower_l() functions have as a domain a type int,
the value of which is representable as an unsigned char or the value of EOF.
So... should apr_fnmatch() perform a range check and not call tolower()
and similar ctype functions on char values outside the range [0x0, 0x7f]
to avoid problems where negative char values are converted to huge unsigned
integers? Seems to be a good way of avoiding the issue since the C locale
doesn't support printable characters outside the ASCII range anyway,
and apr_fnmatch() is currently not equipped to deal with other locales.
Re: log --search test failures on trunk and 1.8.x
Posted by Mattias Engdegård <ma...@bredband.net>.
20 apr 2013 kl. 12.27 skrev Stefan Sperling:
>> W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
>> failed: (unsigned)(c + 1) <= 256
>> ]]]
>
> Seems like a bogus check to me. Is there a notion of debug vs. release
> builds in Visual Studio, and if so, does this happen in release mode
> builds?
Sorry about barging in like this, but isn't this just a case of
calling ctype functions/macros with a char argument, where char is
signed by default? It's prohibited by the standard, although glibc is
"nice" enough to allow values in the [-128, 255] range just because
this mistake is so common.
Looking at apr_fnmatch.c, it indeed seems like that is what is going on.
> See http://pubs.opengroup.org/onlinepubs/9699919799/functions/tolower.html
> It doesn't say "fail if the value overflows".
That is not how the C standard works. When a library call is used with
arguments for which its behaviour is not defined, this results in
undefined behaviour.
Re: log --search test failures on trunk and 1.8.x
Posted by Stefan Sperling <st...@apache.org>.
On Tue, Apr 16, 2013 at 02:15:17PM +0400, Ivan Zhakov wrote:
> Hi,
>
> I got following failures when running Debug build with VC 2010:
> FAIL: log_tests.py 40: test 'svn log -g --search'
> FAIL: log_tests.py 39: 'svn log --search'
>
> They fail because of assertion in VC runtime:
> [[[
> W: f:\dd\vctools\crt_bld\self_x86\crt\src\isctype.c(68) : Assertion
> failed: (unsigned)(c + 1) <= 256
> ]]]
Seems like a bogus check to me. Is there a notion of debug vs. release
builds in Visual Studio, and if so, does this happen in release mode
builds?
tolower() is supposed to return the byte unchanged in case it cannot
be converted to a lower-case representation. I would expect that
to happen in your case.
See http://pubs.opengroup.org/onlinepubs/9699919799/functions/tolower.html
It doesn't say "fail if the value overflows".
> This happens because apr_fnmatch does not handle UTF8 character and
> pass UTF lead char to tolower() call. There is already XXX comment in
> apr\strings\apr_fnmatch.c:121
Yes, apr_fnmatch() doesn't support multi-byte characters yet.
And it never has. But the fnmatch implementation was changed
in APR-1.4.5, so it is possible that this problem didn't trigger
with the old implementation.
> Easy solution could be use apr_fnmatch without APR_FNM_CASE_BLIND and
> make svn --search case-sensitive. Alternative solution is convert
> search pattern and search string to lower case before passing to
> apr_fnmatch.
How is lower-casing a multi-byte UTF-8 character going to help?
Won't the lower-case equivalent still be a multibyte character
and trigger the overflow check in Visual Studio?
> And third option is to fix apr_fnmatch().
I'd very much like to do that, actually. Just have to find time.