{"thread":{"id":"57622","subject":"[PATCH] set LC_TIME even if locale dir is not present","startedAt":"2022-03-27T08:58:17Z","lastAt":"2022-03-27T11:12:52Z","messageCount":2,"participants":["Matthias Aßhauer via GitGitGadget","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452426","messageId":"pull.1189.git.1648371489398.gitgitgadget@gmail.com","threadId":"57622","inReplyTo":null,"subject":"[PATCH] set LC_TIME even if locale dir is not present","fromName":"Matthias Aßhauer via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-27T08:58:09Z","receivedAt":"2022-03-27T08:58:17Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n\nSince Commit aa1462c (introduce \"format\" date-mode, 2015-06-25) git log can\npass user specified format strings directly to strftime(). One special\nformat string we explicitly mention in our documentation is %c, which\ndepends on the system locale. To accommodate for %c we added a call to\nsetlocale() in git_setup_gettext().\n\nIn Commit cc5e1bf (gettext: avoid initialization if the locale dir is not\npresent, 2018-04-21) we added an early exit to git_setup_gettext() in case\nno textdomain directory is present. This early exit is so early, that we\ndon't even set the locale for %c in that case, despite strftime() not\nneeding the textdomain directory at all.\n\nThis leads to a subtle bug where `git log --date=format:%c` will use C\nlocale instead of the system locale on systems without a valid textdomain\ndirectory.\n\nThis fixes https://github.com/git-for-windows/git/issues/2959\n\nSigned-off-by: Matthias Aßhauer <mha1993@live.de>\n---\n    [RFC] set LC_TIME even if locale dir is not present\n    \n    This is a small bug fix with a large and unwieldy regression test. The\n    whole prepare_time_locale() bit and and the Makefile change is obviously\n    based on prepare_utf8_locale(), but I'm not really happy with it. I'm\n    not even sure how to fully put the issues I have with the test in words.\n    \n     * I feel like it's not really testing anything on builds without\n       gettext, but adding a GETTEXT prerequisite to a test that something\n       works without gettext is very counter intuitive.\n    \n     * I'm also not exactly happy about how I choose a locale, but can't\n       think of a better way. It's a reasonable assumption that C locale\n       uses a US date format on most, if not all supported systems, but I\n       have no good way to make sure that the selected locale actually\n       formats dates differently. Defining a custom locale would solve this,\n       but seems like a convoluted way to go about things.\n    \n     * I'm not entirely happy with testing the output of git log\n       -format=date:%c against the output of the exact same command. I've\n       tried a version of the test based on date(1) and got it working with\n       the GNU version, but looking at the BSD version for our OS X based CI\n       builds and the POSIX spec for that command, they share barely more\n       than their name.\n    \n    So, looking at the points above, I expect this to take a few re-rolls to\n    get into a reasonable shape.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1189%2Frimrul%2Fdate-format-without-gettext-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1189/rimrul/date-format-without-gettext-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1189\n\n Makefile                      |  7 +++++++\n gettext.c                     |  3 ++-\n t/t4205-log-pretty-formats.sh | 31 +++++++++++++++++++++++++++++++\n 3 files changed, 40 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex e8aba291d7f..ddca29b550b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -410,6 +410,10 @@ include shared.mak\n # If it isn't set, fallback to $LC_ALL, $LANG or use the first utf-8\n # locale returned by \"locale -a\".\n #\n+# Define GIT_TEST_TIME_LOCALE to preferred non-us locale for testing.\n+# If it isn't set, fallback to $LC_ALL, $LANG or use the first non-us\n+# locale returned by \"locale -a\".\n+#\n # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime.\n #\n # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.\n@@ -2862,6 +2866,9 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n endif\n ifdef GIT_TEST_UTF8_LOCALE\n \t@echo GIT_TEST_UTF8_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_UTF8_LOCALE)))'\\' >>$@+\n+endif\n+ifdef GIT_TEST_TIME_LOCALE\n+\t@echo GIT_TEST_TIME_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_TIME_LOCALE)))'\\' >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\ndiff --git a/gettext.c b/gettext.c\nindex bb5ba1fe7cc..2b614c2b8c6 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -107,6 +107,8 @@ void git_setup_gettext(void)\n \tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n \tchar *p = NULL;\n \n+\tsetlocale(LC_TIME, \"\");\n+\n \tif (!podir)\n \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n@@ -117,7 +119,6 @@ void git_setup_gettext(void)\n \n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n-\tsetlocale(LC_TIME, \"\");\n \tinit_gettext_charset(\"git\");\n \ttextdomain(\"git\");\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e448ef2928a..01a1e61ecea 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -25,6 +25,29 @@ commit_msg () {\n \tfi\n }\n \n+prepare_time_locale () {\n+\tif test -z \"$GIT_TEST_TIME_LOCALE\"\n+\tthen\n+\t\tcase \"${LC_ALL:-$LANG}\" in\n+\t\tC | C.* | POSIX | POSIX.* | en_US | en_US.* )\n+\t\t\tGIT_TEST_TIME_LOCALE=$(locale -a | sed -n '/^\\(C\\|POSIX\\|en_US\\)/I !{\n+\t\t\t\tp\n+\t\t\t\tq\n+\t\t\t}')\n+\t\t\t;;\n+\t\t*)\n+\t\t\tGIT_TEST_TIME_LOCALE=\"${LC_ALL:-$LANG}\"\n+\t\t\t;;\n+\t\tesac\n+\tfi\n+\tif test -n \"$GIT_TEST_TIME_LOCALE\"\n+\tthen\n+\t\ttest_set_prereq TIME_LOCALE\n+\telse\n+\t\tsay \"# No non-us locale available, some tests are skipped\"\n+\tfi\n+}\n+\n test_expect_success 'set up basic repos' '\n \t>foo &&\n \t>bar &&\n@@ -544,6 +567,14 @@ test_expect_success '--date=human %ad%cd is the same as %ah%ch' '\n \ttest_cmp expected actual\n '\n \n+prepare_time_locale\n+test_expect_success TIME_LOCALE '--date=format:%c does not need gettext' '\n+\trm -fr no-such-dir &&\n+\tLC_ALL=$GIT_TEST_TIME_LOCALE git log --date=format:%c HEAD^1..HEAD >expected &&\n+\tGIT_TEXTDOMAINDIR=no-such-dir LC_ALL=$GIT_TEST_TIME_LOCALE git log --date=format:%c HEAD^1..HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # get new digests (with no abbreviations)\n test_expect_success 'set up log decoration tests' '\n \thead1=$(git rev-parse --verify HEAD~0) &&\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452428","messageId":"220327.867d8ffy37.gmgdl@evledraar.gmail.com","threadId":"57622","inReplyTo":"pull.1189.git.1648371489398.gitgitgadget@gmail.com","subject":"Re: [PATCH] set LC_TIME even if locale dir is not present","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-27T10:13:31Z","receivedAt":"2022-03-27T11:12:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 27 2022, Matthias Aßhauer via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <mha1993@live.de>\n>\n> Since Commit aa1462c (introduce \"format\" date-mode, 2015-06-25) git log can\n> pass user specified format strings directly to strftime(). One special\n> format string we explicitly mention in our documentation is %c, which\n> depends on the system locale. To accommodate for %c we added a call to\n> setlocale() in git_setup_gettext().\n>\n> In Commit cc5e1bf (gettext: avoid initialization if the locale dir is not\n> present, 2018-04-21) we added an early exit to git_setup_gettext() in case\n> no textdomain directory is present. This early exit is so early, that we\n> don't even set the locale for %c in that case, despite strftime() not\n> needing the textdomain directory at all.\n\nThanks for tracking this down. This commit & end-state looks good to me,\nI just have comments about the implementation below, i.e. you're doing\nmore work than you need to, some of this we have test infrastructure\nalready...\n\n> diff --git a/Makefile b/Makefile\n> index e8aba291d7f..ddca29b550b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -410,6 +410,10 @@ include shared.mak\n>  # If it isn't set, fallback to $LC_ALL, $LANG or use the first utf-8\n>  # locale returned by \"locale -a\".\n>  #\n> +# Define GIT_TEST_TIME_LOCALE to preferred non-us locale for testing.\n> +# If it isn't set, fallback to $LC_ALL, $LANG or use the first non-us\n> +# locale returned by \"locale -a\".\n> +#\n>  # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime.\n>  #\n>  # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.\n> @@ -2862,6 +2866,9 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n>  endif\n>  ifdef GIT_TEST_UTF8_LOCALE\n>  \t@echo GIT_TEST_UTF8_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_UTF8_LOCALE)))'\\' >>$@+\n> +endif\n> +ifdef GIT_TEST_TIME_LOCALE\n> +\t@echo GIT_TEST_TIME_LOCALE=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_TIME_LOCALE)))'\\' >>$@+\n>  endif\n>  \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n>  ifdef GIT_PERF_REPEAT_COUNT\n\nYou won't need this, more later...\n\n> diff --git a/gettext.c b/gettext.c\n> index bb5ba1fe7cc..2b614c2b8c6 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -107,6 +107,8 @@ void git_setup_gettext(void)\n>  \tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n>  \tchar *p = NULL;\n>  \n> +\tsetlocale(LC_TIME, \"\");\n> +\n>  \tif (!podir)\n>  \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n>  \n> @@ -117,7 +119,6 @@ void git_setup_gettext(void)\n>  \n>  \tbindtextdomain(\"git\", podir);\n>  \tsetlocale(LC_MESSAGES, \"\");\n> -\tsetlocale(LC_TIME, \"\");\n>  \tinit_gettext_charset(\"git\");\n>  \ttextdomain(\"git\");\n>  \n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index e448ef2928a..01a1e61ecea 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -25,6 +25,29 @@ commit_msg () {\n>  \tfi\n>  }\n>  \n> +prepare_time_locale () {\n> +\tif test -z \"$GIT_TEST_TIME_LOCALE\"\n> +\tthen\n> +\t\tcase \"${LC_ALL:-$LANG}\" in\n> +\t\tC | C.* | POSIX | POSIX.* | en_US | en_US.* )\n> +\t\t\tGIT_TEST_TIME_LOCALE=$(locale -a | sed -n '/^\\(C\\|POSIX\\|en_US\\)/I !{\n> +\t\t\t\tp\n> +\t\t\t\tq\n> +\t\t\t}')\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tGIT_TEST_TIME_LOCALE=\"${LC_ALL:-$LANG}\"\n> +\t\t\t;;\n> +\t\tesac\n> +\tfi\n> +\tif test -n \"$GIT_TEST_TIME_LOCALE\"\n> +\tthen\n> +\t\ttest_set_prereq TIME_LOCALE\n> +\telse\n> +\t\tsay \"# No non-us locale available, some tests are skipped\"\n> +\tfi\n> +}\n> +\n\nAnd this setup we do already elsewhere.\n\nI think the below would be a good candidate to squash into this. The C\nchange doesn't change the behavior from your version, but I think it's\ngood to take the change here about being explicit what we do and don't\nseup with/without a podir.\n\nYour prepare_time_locale() is then duplicating lib-gettext.sh, the below\nshows a replacement for your test piggy-backing on existing test infra.\n\nI was then worried that we'd introduced a regression in cc5e1bf in\nassuming that we didn't need to setlocale(LC_MESSAGES) just because we\ndidn't have a podir, because the C library might have some, but the\nbelow test passe for me on linux+glibc.\n\nMaybe there's still a regression there, I'm not sure. Perhaps it's also\ngood to drop that \"optimization\" on non-Windows platforms?\n\ndiff --git a/gettext.c b/gettext.c\nindex bb5ba1fe7cc..9b46c224230 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -102,25 +102,34 @@ static void init_gettext_charset(const char *domain)\n \t\tsetlocale(LC_CTYPE, \"C\");\n }\n \n+static void git_setup_gettext_no_podir(void)\n+{\n+\tsetlocale(LC_TIME, \"\");\n+}\n+\n+static void git_setup_gettext_podir(const char *podir)\n+{\n+\tbindtextdomain(\"git\", podir);\n+\tsetlocale(LC_MESSAGES, \"\");\n+\tinit_gettext_charset(\"git\");\n+\ttextdomain(\"git\");\n+}\n+\n void git_setup_gettext(void)\n {\n \tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n \tchar *p = NULL;\n \n+\tgit_setup_gettext_no_podir();\n+\n \tif (!podir)\n \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n-\tif (!is_directory(podir)) {\n-\t\tfree(p);\n-\t\treturn;\n-\t}\n-\n-\tbindtextdomain(\"git\", podir);\n-\tsetlocale(LC_MESSAGES, \"\");\n-\tsetlocale(LC_TIME, \"\");\n-\tinit_gettext_charset(\"git\");\n-\ttextdomain(\"git\");\n+\tif (!is_directory(podir))\n+\t\tgoto done;\n \n+\tgit_setup_gettext_podir(podir);\n+done:\n \tfree(p);\n }\n \ndiff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh\nindex 0ce1f22eff6..69facd2f8ed 100755\n--- a/t/t0203-gettext-setlocale-sanity.sh\n+++ b/t/t0203-gettext-setlocale-sanity.sh\n@@ -23,4 +23,49 @@ test_expect_success GETTEXT_LOCALE 'git show a ISO-8859-1 commit under a UTF-8 l\n \tgrep -q \"iso-utf8-commit\" out\n '\n \n+test_expect_success GETTEXT_LOCALE 'the %c date format works even without a localedir (LC_TIME)' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tmkdir empty &&\n+\tLANGUAGE=is LC_ALL=\"$is_IS_locale\" GIT_TEXTDOMAINDIR=\"$PWD/empty\" \\\n+\t\tgit log --pretty=format:%ad --date=format:%c HEAD^1..HEAD >actual &&\n+\n+\t# Avoid testing the raw format (it might differ?). But\n+\t# Thursday is Fimmtudagur in Icelandic, so grepping \"fim\" is\n+\t# pretty certain to test that the locale was used.\n+\tgrep -iF fim actual\n+'\n+\n+test_lazy_prereq GETTEXT_HAVE_STRERROR_TRANSLATED '\n+\ttest_have_prereq GETTEXT_LOCALE &&\n+\ttest_have_prereq POSIXPERM &&\n+\n+\ttest_when_finished \"rm -f file\" &&\n+\t>file &&\n+\n+\t# German is more likely to have a strerror() translation\n+\ttest_must_fail git init file 2>loc-C &&\n+\tgrep \"cannot mkdir\" loc-C &&\n+\ttest_must_fail env \\\n+\t\tLANGUAGE=de LC_ALL=\"de_DE.utf8\" \\\n+\t\tgit init file 2>loc-de &&\np+\t! grep \"cannot mkdir\" loc-de\n+\n+'\n+\n+test_expect_success GETTEXT_LOCALE,GETTEXT_HAVE_STRERROR_TRANSLATED \\\n+\t'LC_MESSAGES is set up without a localedir (for strerror())' '\n+\ttest_when_finished \"rm -f file\" &&\n+\t>file &&\n+\ttest_when_finished \"rm -rf no-domain\" &&\n+\tmkdir no-domain &&\n+\n+\ttest_must_fail git init file 2>loc-C &&\n+\ttest_must_fail env \\\n+\t\tLANGUAGE=de LC_ALL=\"de_DE.utf8\" \\\n+\t\tNO_SET_GIT_TEXTDOMAINDIR=StopDoingThatInBinWrappers \\\n+\t\tGIT_TEXTDOMAINDIR=\"$PWD/no-domain\" \\\n+\t\tgit init file 2>loc-de-no-textdomain &&\n+\t! test_cmp loc-C loc-de-no-textdomain\n+'\n+\n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 515b1af7ed4..886d260082d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1428,6 +1428,8 @@ else # normal case, use ../bin-wrappers only unless $with_dashes:\n \t\t\tfi\n \t\t\twith_dashes=t\n \t\tfi\n+\t\tGIT_RUNING_TEST_LIB_SH=t\n+\t\texport GIT_RUNING_TEST_LIB_SH\n \t\tPATH=\"$git_bin_dir:$PATH\"\n \tfi\n \tGIT_EXEC_PATH=$GIT_BUILD_DIR\ndiff --git a/wrap-for-bin.sh b/wrap-for-bin.sh\nindex 95851b85b6b..3089bcad37c 100644\n--- a/wrap-for-bin.sh\n+++ b/wrap-for-bin.sh\n@@ -15,10 +15,14 @@ else\n \texport GIT_TEMPLATE_DIR\n fi\n GITPERLLIB='@@BUILD_DIR@@/perl/build/lib'\"${GITPERLLIB:+:$GITPERLLIB}\"\n-GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'\n+if test -z \"$NO_SET_GIT_TEXTDOMAINDIR\"\n+then\n+\tGIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'\n+\texport GIT_TEXTDOMAINDIR\n+fi\n PATH='@@BUILD_DIR@@/bin-wrappers:'\"$PATH\"\n \n-export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR\n+export GIT_EXEC_PATH GITPERLLIB PATH\n \n case \"$GIT_DEBUGGER\" in\n '')\n"}]}