git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Eric Wong" <e@80x24.org>,
	git@vger.kernel.org
Subject: Re: [PATCH 1/2] tests: add 'test_bool_env' to catch non-bool GIT_TEST_* values
Date: Mon, 25 Nov 2019 08:50:16 -0500
Message-ID: <20191125135016.GA632@sigill.intra.peff.net> (raw)
In-Reply-To: <20191122131437.25849-2-szeder.dev@gmail.com>

On Fri, Nov 22, 2019 at 02:14:36PM +0100, SZEDER Gábor wrote:

> Let's be more careful about what the test suite accepts as bool values
> in GIT_TEST_* environment variables, and error out loud and clear on
> invalid values instead of simply skipping tests.  Add the
> 'test_bool_env' helper function to encapsulate the invocation of 'git
> env--helper' and the verification of its exit code, and replace all
> invocations of that command in our test framework and test suite with
> a call to this new helper (except in 't0017-env-helper.sh', of
> course).
> 
>   $ GIT_TEST_GIT_DAEMON=YesPlease ./t5570-git-daemon.sh
>   fatal: bad numeric config value 'YesPlease' for 'GIT_TEST_GIT_DAEMON': invalid unit
>   error: test_bool_env requires bool values both for $GIT_TEST_GIT_DAEMON and for the default fallback

This patch looks good to me. A few musings below, but I'm not sure if
they're worth acting on.

> +test_bool_env () {
> +	if test $# != 2
> +	then
> +		BUG "test_bool_env requires two parameters (variable name and default value)"
> +	fi
> +
> +	git env--helper --type=bool --default="$2" --exit-code "$1"
> +	ret=$?
> +	case $ret in
> +	0|1)	# unset or valid bool value
> +		;;
> +	*)	# invalid bool value or something unexpected
> +		error >&7 "test_bool_env requires bool values both for \$$1 and for the default fallback"
> +		;;
> +	esac
> +	return $ret
> +}

The magic of exit code "1" is undocumented, but we have to rely on it
here. I suggested earlier that we could do:

  if ! val=$(git env--helper --type=bool --default="$2" "$1")
    error ...
  fi
  test "$val" = "true"

but as you noted, we exit with code 1 for "false" even without
--exit-code. IMHO this is a mis-design in the interface of env--helper.

I think it would be an option to change it. It's an undocumented
double-dashed internal helper, so I don't think we need to worry about
breaking compatibility. There's only one other caller that you didn't
touch in this patch, and it uses --exit-code (more on that in a second).

> +test_expect_success 'test_bool_env' '

These tests make sense. In fact, they're much more interesting than the
ones in t0017, since these cover a superset of the code that's actually
used in practice. t0017 covers non-exit-code and --ulong invocations,
but nobody uses them!

I'm wondering if this whole env--helper thing is kind of
over-engineered. Should it actually be a test-tool helper instead of a
shipped builtin? The only call outside of the test suite is this one in
git-sh-i18n:

  # First decide what scheme to use...
  GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
  if test -n "$GIT_TEST_GETTEXT_POISON" &&
              git env--helper --type=bool --default=0 --exit-code \
                  GIT_TEST_GETTEXT_POISON
  then
          GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
  elif test -n "@@USE_GETTEXT_SCHEME@@"
  ...

which suffers from the same problem your patch is fixing. But since this
is again a test-suite thing, it seems like it would be simpler for the
test suite to just set GIT_INTERNAL_GETTEXT_SH_SCHEME=poison itself
(with a little rearranging here to let that override the "fallthrough"
case).

That would make the remaining --exit-code problem go away, remove some
test cruft from production code, and remove the last non-test-suite
caller of env--helper.

At that point we could make it a test-tool builtin. Or even implement it
purely in shell, saving some processes (that would require duplicating
the internal bool logic, but that's way shorter than the boilerplate
needed to expose it via env--helper).

I do think env--helper _could_ be useful for user scripts. But then I
think we'd need to document and rename it to make it clear that it's
part of Git's plumbing that you can depend on.

-Peff

  reply index

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-19  9:46 [PATCH] fetch: only run 'gc' once when fetching multiple remotes Nguyễn Thái Ngọc Duy
2019-06-19 10:26 ` [RFC/PATCH] gc: run more pre-detach operations under lock Ævar Arnfjörð Bjarmason
2019-06-19 12:51   ` Duy Nguyen
2019-06-19 18:01     ` Ævar Arnfjörð Bjarmason
2019-06-19 19:10       ` Jeff King
2019-06-19 22:49         ` Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 0/6] Change <non-empty?> GIT_TEST_* variables to <boolean> Ævar Arnfjörð Bjarmason
2019-06-20 18:13             ` Junio C Hamano
2019-06-20 21:00               ` Ævar Arnfjörð Bjarmason
2019-06-20 20:03             ` Junio C Hamano
2019-06-20 21:09             ` [PATCH v2 0/8] " Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 1/8] config tests: simplify include cycle test Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 2/8] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-21 17:07                 ` Junio C Hamano
2019-06-21 10:18               ` [PATCH v3 3/8] config.c: refactor die_bad_number() to not call gettext() early Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 4/8] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-24 16:47                 ` Junio C Hamano
2019-06-21 10:18               ` [PATCH v3 6/8] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-21 10:18               ` [PATCH v3 7/8] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-09-06 12:13                 ` [PATCH 1/2] t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP tests SZEDER Gábor
2019-09-06 12:13                   ` [PATCH 2/2] ci: restore running httpd tests SZEDER Gábor
2019-09-06 17:03                     ` Junio C Hamano
2019-09-06 19:13                     ` Jeff King
2019-09-07 10:16                       ` SZEDER Gábor
2019-11-22 13:14                         ` [PATCH 0/2] tests: catch non-bool GIT_TEST_* values SZEDER Gábor
2019-11-22 13:14                           ` [PATCH 1/2] tests: add 'test_bool_env' to " SZEDER Gábor
2019-11-25 13:50                             ` Jeff King [this message]
2019-11-22 13:14                           ` [PATCH 2/2] t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a bool SZEDER Gábor
2019-11-25 13:53                             ` Jeff King
2019-06-21 10:18               ` [PATCH v3 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 1/8] config tests: simplify include cycle test Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 2/8] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-20 22:11               ` Junio C Hamano
2019-06-20 22:21                 ` Junio C Hamano
2019-06-21  8:11                   ` Ævar Arnfjörð Bjarmason
2019-06-21 15:04                     ` Junio C Hamano
2019-06-20 21:09             ` [PATCH v2 3/8] config.c: refactor die_bad_number() to not call gettext() early Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 4/8] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 6/8] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 7/8] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-06-20 21:09             ` [PATCH v2 8/8] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 1/6] env--helper: new undocumented builtin wrapping git_env_*() Ævar Arnfjörð Bjarmason
2019-06-20 19:25             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 2/6] t6040 test: stop using global "script" variable Ævar Arnfjörð Bjarmason
2019-06-20 19:54             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 3/6] tests: make GIT_TEST_GETTEXT_POISON a boolean Ævar Arnfjörð Bjarmason
2019-06-20 20:00             ` Junio C Hamano
2019-06-19 23:30           ` [PATCH 4/6] tests README: re-flow a previously changed paragraph Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 5/6] tests: replace test_tristate with "git env--helper" Ævar Arnfjörð Bjarmason
2019-06-19 23:30           ` [PATCH 6/6] tests: make GIT_TEST_FAIL_PREREQS a boolean Ævar Arnfjörð Bjarmason
2019-06-20 10:26           ` [RFC/PATCH] gc: run more pre-detach operations under lock Duy Nguyen
2019-06-20 21:49             ` Ævar Arnfjörð Bjarmason
2019-06-20 10:43           ` Jeff King
2019-06-20 18:55         ` Junio C Hamano
2019-06-19 19:08     ` Jeff King
2019-06-19 18:59 ` [PATCH] fetch: only run 'gc' once when fetching multiple remotes Jeff King
2019-06-20 10:11   ` Duy Nguyen
2019-06-20 10:21     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191125135016.GA632@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git