Merge lp:~mterry/unity-scopes-api/turkish into lp:unity-scopes-api/devel

[Michael Terry]

Fix reading scope metadata in a Turkish locale.

I don't know of any existing bug symptom caused by this. I came across this while investigating a crash due to a misconfigured locale.

We're using std::locale("") to handle lowercasing user-input (well developer-input) scope metadata keys into standardized key names. This has two effects:

1) If the environment-specified locale is not set up correctly, an exception will be thrown. Fair enough I guess, but still a harsh possible side effect of just trying to lowercase some ASCII characters.

2) Using a locale-sensitive lowercasing algorithm has some odd side effects. Most notably in Turkish. See [1] and other such sites if you google "turkish i problem." Basically, if your problem domain is ASCII and/or you are not dealing with strings that the user will see, you probably don't want to be converting characters with the user's locale.

I've added a test that demonstrates the issue. With the old code, if you don't have the tr_TR.UTF-8 locale installed, the test will crash. If you do, it will fail. With the new code, it will pass in both cases. I've added language-pack-tr to the Build-Depends to make sure the locale is valid during normal build tests (the i problem is more interesting to test than the invalid locale problem, which is probably expected behavior anyway).

[1] http://haacked.com/archive/2012/07/05/turkish-i-problem-and-why-you-should-care.aspx/

[Marcus Tomlinson (marcustomlinson)]

Excellent work adding a test case for this! LGTM

review: Approve

[Michael Terry (mterry)]

I retargeted to devel, please re-approve.

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:372
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/51/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/890
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/897
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/703/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/703
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/703/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/51/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Ah, my bad for missing that

review: Approve

=== modified file 'debian/control'
--- debian/control	2016-09-09 06:57:47 +0000
+++ debian/control	2016-10-17 08:54:00 +0000
@@ -18,6 +18,7 @@
                exuberant-ctags,
                google-mock,
                graphviz,
+               language-pack-tr,
                libaccounts-glib-dev,
                libapparmor-dev,
                libboost-filesystem-dev,

=== modified file 'src/scopes/internal/ConfigBase.cpp'
--- src/scopes/internal/ConfigBase.cpp	2014-08-28 00:20:56 +0000
+++ src/scopes/internal/ConfigBase.cpp	2016-10-17 08:54:00 +0000
@@ -219,7 +219,7 @@
 
 void ConfigBase::to_lower(string & str)
 {
-    locale locale("");
+    locale locale("C"); // Use "C" to avoid the Turkish I problem
     const ctype<char>& ct = use_facet<ctype<char> >(locale);
     transform(str.begin(), str.end(), str.begin(),
             bind1st(std::mem_fun(&ctype<char>::tolower), &ct));

=== modified file 'src/scopes/internal/Utils.cpp'
--- src/scopes/internal/Utils.cpp	2015-11-25 08:55:19 +0000
+++ src/scopes/internal/Utils.cpp	2016-10-17 08:54:00 +0000
@@ -113,7 +113,7 @@
 
 string uncamelcase(string const& str)
 {
-    const locale loc("");
+    const locale loc("C"); // Use "C" to avoid the Turkish I problem
     if (str.size() == 0)
     {
         return str;

=== modified file 'test/gtest/scopes/internal/Utils/Utils_test.cpp'
--- test/gtest/scopes/internal/Utils/Utils_test.cpp	2015-09-15 07:36:09 +0000
+++ test/gtest/scopes/internal/Utils/Utils_test.cpp	2016-10-17 08:54:00 +0000
@@ -37,6 +37,17 @@
     EXPECT_EQ("foo-bar", uncamelcase("foo-Bar"));
 }
 
+TEST(Utils, uncamelcase_turkish)
+{
+    char *old_locale = strdup(getenv("LC_ALL"));
+    setenv("LC_ALL", "tr_TR.UTF-8", 1);
+
+    EXPECT_EQ("small-i", uncamelcase("smallI"));
+
+    setenv("LC_ALL", old_locale, 1);
+    free(old_locale);
+}
+
 TEST(Utils, convert_to)
 {
     {