Re: [PATCH] t: use user-specific utf-8 locale for testing

From: Taylor Blau <ttaylorr@github.com>
Date: Jun 2, 2021, 19:56 UTC
Message-ID: <YLfiYXxQqXL7RyHC@nand.local>
In-Reply-To: <20210602114646.17463-1-congdanhqx@gmail.com>

On Wed, Jun 02, 2021 at 06:46:46PM +0700, Đoàn Trần Công Danh wrote:

> Despite being required by POSIX, locale(1) is unavailable in some
> systems, e.g. Linux with musl libc.  Some of those systems support
> utf-8 locale out of the box.

Hmmph. I would have imagined that locale was available everywhere, but unfortunately not.

Show 10 quoted lines

> diff --git a/Makefile b/Makefile
> index c3565fc0f8..4b2c24e5ea 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -398,6 +398,9 @@ all::
>  # with a different indexfile format version.  If it isn't set the index
>  # file format used is index-v[23].
>  #
> +# Define GIT_TEST_UTF8_LOCALE to prefered utf-8 locale for testing.
> +# If it isn't set, use the first utf-8 locale returned by "locale -a".

s/prefered/preferred

Show 36 quoted lines

> +#
>  # Define HAVE_CLOCK_GETTIME if your platform has clock_gettime.
>  #
>  # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.
> @@ -2801,6 +2804,9 @@ ifdef GIT_TEST_CMP
>  endif
>  ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
>  	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+
> +endif
> +ifdef GIT_TEST_UTF8_LOCALE
> +	@echo GIT_TEST_UTF8_LOCALE=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_UTF8_LOCALE)))'\' >>$@+
>  endif
>  	@echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\' >>$@+
>  ifdef GIT_PERF_REPEAT_COUNT
> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
> index 547eb3c31a..df319593f7 100644
> --- a/t/lib-git-svn.sh
> +++ b/t/lib-git-svn.sh
> @@ -121,12 +121,15 @@ start_svnserve () {
>  		 --listen-host 127.0.0.1 &
>  }
>
> -prepare_a_utf8_locale () {
> -	a_utf8_locale=$(locale -a | sed -n '/\.[uU][tT][fF]-*8$/{
> -	p
> -	q
> -}')
> -	if test -n "$a_utf8_locale"
> +prepare_utf8_locale () {
> +	if test -z "$GIT_TEST_UTF8_LOCALE"
> +	then
> +		GIT_TEST_UTF8_LOCALE=$(locale -a | sed -n '/\.[uU][tT][fF]-*8$/{
> +		p
> +		q
> +	}')
> +	fi

OK, so we bind GIT_TEST_UTF8_LOCALE to the value of $a_utf8_locale in the pre-image, unless the user said otherwise.

> +	if test -n "$GIT_TEST_UTF8_LOCALE"

...Then we go on to handle things like before, except we read from "$GIT_TEST_UTF8_LOCALE" instead of "$a_utf8_locale". Makes sense to me.

Show 27 quoted lines

>  	then
>  		test_set_prereq UTF8
>  	else
> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
> index 1d3fdcc997..d5563ec35f 100755
> --- a/t/t9100-git-svn-basic.sh
> +++ b/t/t9100-git-svn-basic.sh
> @@ -4,21 +4,13 @@
>  #
>
>  test_description='git svn basic tests'
> -GIT_SVN_LC_ALL=${LC_ALL:-$LANG}
>
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
>  . ./lib-git-svn.sh
>
> -case "$GIT_SVN_LC_ALL" in
> -*.UTF-8)
> -	test_set_prereq UTF8
> -	;;
> -*)
> -	say "# UTF-8 locale not set, some tests skipped ($GIT_SVN_LC_ALL)"
> -	;;
> -esac
> +prepare_utf8_locale

This change (and the omitted ones below in later hunks) look like it isn't changing any behavior (and just running the same code behind the prepare_utf8_locale function instead of inlining it).

They all look right to me, but it may be helpful to either point it out in the commit message and/or prepare the separately. I'd probably err on the side of the former.

That said, this patch looks good to me with minor touch-ups (my only nits are the above and the spelling mistake in the Makefile).

Thanks, Taylor