[v2,0/8] Change <non-empty?> GIT_TEST_* variables to <boolean>

Message

Ævar Arnfjörð Bjarmason June 20, 2019, 9:09 p.m. UTC

This v2 fixes tricky bugs I noticed after sending v1 in calling
gettext so early in the setup, and the t0016 name clash with pu Junio
pointed out.

I didn't change the "env--helper" interface as suggested because I
already had this ready and figured I'd send a v2 for review of the
stuff I have now.

The interface suggested in
<[e-mail]> is indeed prettier. FWIW
I did things like --mode-bool instead of --type=bool because it's
easier to do just with the getopt framework, likewise --variable=X
instead of "X" being last on the argv since you can do it all with the
flag parsing doing the work for you, and since it's an internal-only
command I figured it didn't matter much.

Ævar Arnfjörð Bjarmason (8):
  config tests: simplify include cycle test
  env--helper: new undocumented builtin wrapping git_env_*()
  config.c: refactor die_bad_number() to not call gettext() early
  t6040 test: stop using global "script" variable
  tests: make GIT_TEST_GETTEXT_POISON a boolean
  tests README: re-flow a previously changed paragraph
  tests: replace test_tristate with "git env--helper"
  tests: make GIT_TEST_FAIL_PREREQS a boolean

 .gitignore                |  1 +
 Makefile                  |  1 +
 builtin.h                 |  1 +
 builtin/env--helper.c     | 74 +++++++++++++++++++++++++++++++++
 ci/lib.sh                 |  2 +-
 config.c                  | 28 +++++++++----
 gettext.c                 |  6 +--
 git-sh-i18n.sh            |  4 +-
 git.c                     |  1 +
 po/README                 |  2 +-
 t/README                  | 12 +++---
 t/lib-git-daemon.sh       |  7 ++--
 t/lib-git-svn.sh          | 11 ++---
 t/lib-httpd.sh            | 15 ++++---
 t/t0000-basic.sh          | 10 ++---
 t/t0017-env-helper.sh     | 86 +++++++++++++++++++++++++++++++++++++++
 t/t0205-gettext-poison.sh |  7 +++-
 t/t1305-config-include.sh | 21 ++++------
 t/t5512-ls-remote.sh      |  3 +-
 t/t6040-tracking-info.sh  |  6 +--
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib-functions.sh   | 58 ++++----------------------
 t/test-lib.sh             | 33 +++++++++++----
 24 files changed, 266 insertions(+), 127 deletions(-)
 create mode 100644 builtin/env--helper.c
 create mode 100755 t/t0017-env-helper.sh

Range-diff:
-:  ---------- > 1:  c3483c37a1 config tests: simplify include cycle test
1:  8da3cd4240 ! 2:  e689759f7c env--helper: new undocumented builtin wrapping git_env_*()
    @@ -154,10 +154,10 @@
      	{ "fetch", cmd_fetch, RUN_SETUP },
      	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
     
    - diff --git a/t/t0016-env-helper.sh b/t/t0016-env-helper.sh
    + diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
      new file mode 100755
      --- /dev/null
    - +++ b/t/t0016-env-helper.sh
    + +++ b/t/t0017-env-helper.sh
     @@
     +#!/bin/sh
     +
-:  ---------- > 3:  f759d5e91e config.c: refactor die_bad_number() to not call gettext() early
2:  8315c4ecdc = 4:  1ac798e8ce t6040 test: stop using global "script" variable
3:  f1ee208d70 ! 5:  d7d6e6c874 tests: make GIT_TEST_GETTEXT_POISON a boolean
    @@ -7,7 +7,30 @@
     
         Since it needed to be checked in both C code and shellscript (via test
         -n) it was one of the remaining shellscript-like variables. Now that
    -    we have "git env--helper" we can change that.
    +    we have "env--helper" we can change that.
    +
    +    There's a couple of tricky edge cases that arise because we're using
    +    git_env_bool() early, and the config-reading "env--helper".
    +
    +    If GIT_TEST_GETTEXT_POISON is set to an invalid value die_bad_number()
    +    will die, but to do so it would usually call gettext(). Let's detect
    +    the special case of GIT_TEST_GETTEXT_POISON and always emit that
    +    message in the C locale, lest we infinitely loop.
    +
    +    As seen in the updated tests in t0016-env-helper.sh there's also a
    +    caveat related to "env--helper" needing to read the config for trace2
    +    purposes.
    +
    +    Since the C_LOCALE_OUTPUT prerequisite is lazy and relies on
    +    "env--helper" we could get invalid results if we failed to read the
    +    config (e.g. because we'd loop on includes) when combined with
    +    e.g. "test_i18ngrep" wanting to check with "env--helper" if
    +    GIT_TEST_GETTEXT_POISON was true or not.
    +
    +    I'm crossing my fingers and hoping that a test similar to the one I
    +    removed in the earlier "config tests: simplify include cycle test"
    +    change in this series won't happen again, and testing for this
    +    explicitly in "env--helper"'s own tests.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    @@ -24,6 +47,26 @@
      esac
      
     
    + diff --git a/config.c b/config.c
    + --- a/config.c
    + +++ b/config.c
    +@@
    + 	if (!value)
    + 		value = "";
    + 
    ++	if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
    ++		/*
    ++		 * We explicitly *don't* use _() here since it would
    ++		 * cause an infinite loop with _() needing to call
    ++		 * use_gettext_poison(). This is why marked up
    ++		 * translations with N_() above.
    ++		 */
    ++		die(bad_numeric, value, name, error_type);
    ++
    + 	if (!(cf && cf->name))
    + 		die(_(bad_numeric), value, name, _(error_type));
    + 
    +
      diff --git a/gettext.c b/gettext.c
      --- a/gettext.c
      +++ b/gettext.c
    @@ -84,6 +127,31 @@
      prerequisite when adding more strings for translation. See "Testing
      marked strings" in po/README for details.
     
    + diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
    + --- a/t/t0017-env-helper.sh
    + +++ b/t/t0017-env-helper.sh
    +@@
    + 	test_cmp expected actual
    + '
    + 
    ++test_expect_success 'env--helper reads config thanks to trace2' '
    ++	mkdir home &&
    ++	git config -f home/.gitconfig include.path cycle &&
    ++	git config -f home/cycle include.path .gitconfig &&
    ++
    ++	test_must_fail \
    ++		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
    ++		git config -l 2>err &&
    ++	grep "exceeded maximum include depth" err &&
    ++
    ++	test_must_fail \
    ++		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
    ++		git -C cycle env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet 2>err &&
    ++	grep "# GETTEXT POISON #" err
    ++'
    ++
    + test_done
    +
      diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
      --- a/t/t0205-gettext-poison.sh
      +++ b/t/t0205-gettext-poison.sh
    @@ -96,6 +164,29 @@
      export GIT_TEST_GETTEXT_POISON
      . ./lib-gettext.sh
      
    +@@
    +     test_cmp expect actual
    + '
    + 
    ++test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
    ++	test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
    ++	grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
    ++"
    ++
    + test_done
    +
    + diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
    + --- a/t/t1305-config-include.sh
    + +++ b/t/t1305-config-include.sh
    +@@
    + 	git -C cycle config include.path cycle &&
    + 	git config -f cycle/cycle include.path config &&
    + 	test_must_fail \
    +-		env GIT_TEST_GETTEXT_POISON= \
    ++		env GIT_TEST_GETTEXT_POISON=false \
    + 		git -C cycle config --get-all test.value 2>stderr &&
    + 	grep "exceeded maximum include depth" stderr
    + '
     
      diff --git a/t/t7201-co.sh b/t/t7201-co.sh
      --- a/t/t7201-co.sh
    @@ -130,10 +221,14 @@
      	unset GIT_TEST_GETTEXT_POISON_ORIG
      fi
      
    +-# Can we rely on git's output in the C locale?
    +-if test -z "$GIT_TEST_GETTEXT_POISON"
    +-then
    +-	test_set_prereq C_LOCALE_OUTPUT
    +-fi
     +test_lazy_prereq C_LOCALE_OUTPUT '
     +	! git env--helper --mode-bool --variable=GIT_TEST_GETTEXT_POISON --default=0 --exit-code --quiet
     +'
    -+
    - # Can we rely on git's output in the C locale?
    - if test -z "$GIT_TEST_GETTEXT_POISON"
    + 
    + if test -z "$GIT_TEST_CHECK_CACHE_TREE"
      then
4:  f1e28dff36 = 6:  954428b3cd tests README: re-flow a previously changed paragraph
5:  f046cf21fb = 7:  79b41cf01b tests: replace test_tristate with "git env--helper"
6:  e2c68e0239 = 8:  a9aa166b66 tests: make GIT_TEST_FAIL_PREREQS a boolean