Regcomp with LC_COLLATE - #4560
Conversation
When searching for a configuration key for the diff driver, we construct the config key by modifying a buffer and then passing it to `git_config_get_multivar_foreach`. We do not check though whether the modification of the buffer actually succeded, so we could in theory end up passing the OOM buffer to the config function. Fix that by checking return codes. While at it, switch to use `git_buf_PUTS` to avoid repetition of the appended string to calculate its length.
Our user diff patterns make heavy use of range notation (e.g. "[a-z]")
which are being passed to `regcomp`. Problem is though that these ranges
heavily depend on locale value of LC_COLLATE, as these ranges might be
different depending on the language. This is why character classes like
[:alnum:], [:digit:] etc. exist for regular expressions, which easily
fix the problem in a locale-independent way.
Convert our user patterns to use these character classes instead.
There's two problems, though:
1. The character classes might include characters which are not allowed
in an actual function name. E.g. if the current language has any
weird UTF8 characters which belong to the alphabet of that language,
we may get invalid matches for these regexes.
2. We have inherited a catch-all word delimiter from git.git, which
catches multibyte characters not part of ASCII with the high-bit set.
git.git does this to ensure that unmatched words are not being
treated as whitespace-only changes and thus not be shown. With
LC_COLLATE set to a non-C locale, the pattern
"[\xc0-\xff][\x80-\xbf]+" will actually cause the regex compilation
to fail. See git.git, commit 664d44ee7 (userdiff: simplify word-diff
safeguard, 2011-01-11).
There are multiple possibilities here:
- Just bail on systems without `regcomp_l`. By default, we use
`regcomp_l` and pass it no locale, which instructs it to use the
default locale. This is the best-case scenario. Unfortunately,
`regcomp_l` is not specified by POSIX and thus is missing on many
systems.
- We could obviously open-code it and just write down the complete
range of characters, but this is _very_ unhandy.
- We could instruct callers to never set LC_COLLATE to something
other than the C locale. But this is simply won't work out, and
expecting callers to always do a `setlocale` dance around calls to
libgit2 is not going to fly well.
- Otherwise, there is not much to help us. The only option remaining
is to remove this range altogether and rely on the calling program
to correctly set up locales. In this case, we might be lucky and
all characters with the high-bit set will be matched by the
respective character classes. But... yeah.
Nothing is really satisfying at all. This commit uses the last
option, as it seems to be the least likely to upset everybody.
We cannot reliably use ranges, as these may not be usable depending on the locale. Let's just hope that there is no insane language which defines digits and hex-digits different than the C locale.
While the test asserts that the error value indcates a non-value, it is actually never getting assigned to. Fix this.
While we already have a test for `p_regexec` with `LC_CTYPE` being modified, `regexec` also alters behavior as soon as `LC_COLLATE` is being modified. Most importantly, `LC_COLLATE` changes the way how ranges are interpreted to just not handling them at all. Thus, add a test that is being guarded by `GIT_USE_REGCOMP_L`, as we can only succeed when using `regcomp_l`.
In order to make it easier adding more locale-related tests, add a generalized framework handling initial setup of languages as well as the cleanup of them afterwards.
In order to avoid us being unable to match characters which are part of the normal US alphabet in certain weird languages, add two tests to catch this behavior.
Naive question: it sounds like if we "open-code it and just write down the complete range of characters" that it will resolve this problem in full, regardless of the locale, calls to It has an additional drawback that we've now diverged from the git.git regexes, though we may have already done that. Can we add a function into the mix before compiling the regexes that does the expansion ourselves? I suspect that there are some crazy rules in regex character classes but since these are all constants in our code base, we probably don't need to support every crazy thing and have a limited subset to work with. |
|
This does seem like the least terrible option. We would ideally not diverge from git's regexes, if for no other reason that any divergence is going to be counted as a bug against us. So it would be great if we could get git to also use |
|
It's a bit hard to argue for git to use ":digit:", though. They don't care, as they will never use The translation system would probably be doable, but really inperformant. We'll have to use additional memory allocations to create the actual pattern, and this would not only be used for the userdiff patterns but also for stuff like querying configuration keys. By the way, I just noticed that my previous statement about glibc providing |
|
Circling back - seeing as we already have a regex that we ship as a dependency, could we simply always use our regex engine instead of using it as a fallback when the system doesn't have one? |
|
No, we can't, as deps/regex does not provide |
|
Closing in favor of #4935 |
This is inspired by #4528, where we do fail certain regexes when LC_COLLATE is being set to something other than the default. This is unfortunate, and I actually wasn't able to come up with a satisfying solution. Pulling the following message from my commits. Feedback and ideas would be highly appreciated.
Our user diff patterns make heavy use of range notation (e.g. "[a-z]")
which are being passed to
regcomp. Problem is though that these rangesheavily depend on locale value of LC_COLLATE, as these ranges might be
different depending on the language. This is why character classes like
[:alnum:], [:digit:] etc. exist for regular expressions, which easily
fix the problem in a locale-independent way.
Convert our user patterns to use these character classes instead.
There's two problems, though:
The character classes might include characters which are not allowed
in an actual function name. E.g. if the current language has any
weird UTF8 characters which belong to the alphabet of that language,
we may get invalid matches for these regexes.
We have inherited a catch-all word delimiter from git.git, which
catches multibyte characters not part of ASCII with the high-bit set.
git.git does this to ensure that unmatched words are not being
treated as whitespace-only changes and thus not be shown. With
LC_COLLATE set to a non-C locale, the pattern
"[\xc0-\xff][\x80-\xbf]+" will actually cause the regex compilation
to fail. See git.git, commit 664d44ee7 (userdiff: simplify word-diff
safeguard, 2011-01-11).
There are multiple possibilities here:
Just bail on systems without
regcomp_l. By default, we useregcomp_land pass it no locale, which instructs it to use thedefault locale. This is the best-case scenario. Unfortunately,
regcomp_lis not specified by POSIX and thus is missing on manysystems.
We could obviously open-code it and just write down the complete
range of characters, but this is very unhandy.
We could instruct callers to never set LC_COLLATE to something
other than the C locale. But this is simply won't work out, and
expecting callers to always do a
setlocaledance around calls tolibgit2 is not going to fly well.
Otherwise, there is not much to help us. The only option remaining
is to remove this range altogether and rely on the calling program
to correctly set up locales. In this case, we might be lucky and
all characters with the high-bit set will be matched by the
respective character classes. But... yeah.
Nothing is really satisfying at all. This commit uses the last
option, as it seems to be the least likely to upset everybody.