On Fri, Sep 06, 2019 at 03:13:01PM -0400, Jeff King wrote:
> On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:
>
> > Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> > exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> > that we run the httpd tests in the Linux Clang and GCC build jobs, or
> > error out if they can't be run for any reason [1].
>
> Yikes, good catch.
>
> I wonder if it would be possible for the test suite to catch this. I
> think env--helper would have written a message to stderr, but because we
> use --exit-code, we can't tell the difference between that and "false".
No, '--exit-code' only suppresses the printing of 'true' and 'false',
but it doesn't have any effect on the command's exit code. And if the
value of the environment variable is not a bool, then it does print an
error message to standard error, and, more importantly, exits with 128
(as opposed to 1 indicating false). So we could tell the difference,
but as it happens the command is invoked as 'if ! git env-helper ...',
which then interprets that 128 the same as on ordinary false.
$ VAR=false git env--helper --type=bool --default=false --exit-code VAR ; echo $?
1
$ VAR=true git env--helper --type=bool --default=false --exit-code VAR ; echo $?
0
$ VAR=YesPlease git env--helper --type=bool --default=false --exit-code VAR ; echo $?
fatal: bad numeric config value 'YesPlease' for 'VAR': invalid unit
128
> I think we'd have go back to something more like:
>
> test_tristate () {
The env var is supposed to be a bool, so there is no third state
anymore.
> bool=$(git env--helper --type=bool --default=true "$1") ||
Some callsites use '--default=false', so we need a second parameter to
specify the default.
> eval "error \"$1 is not a bool: \$$1\""
> test "$bool" = "true"
> }
> ...
> if test_tristate GIT_TEST_HTTPD
> then
> ... use httpd ...
> fi
>
> Not sure if it's worth it.
Well... On one hand, there are no other similar issues in our CI
scripts (there is still a GIT_TEST_CLONE_2GB=YesPlease, but it's only
ever checked with 'test -z' in 't5608-clone-2gb.sh', so it works as
expected, even though it's inconsistent, and
'GIT_TEST_CLONE_2GB=NoThanks' would do the wrong thing).
OTOH, some unsuspecting devs might still have a
'GIT_TEST_foo=YesPlease' in their 'config.mak' from the old days...
Furthermore, as for the "good catch", I just got lucky, and not only
once but several times in a row: that Perforce filehost outage the
other day errored one of my builds, so I came up with a fix [1], for
once took the extra effort and checked it on Azure Pipelines, and
since that was my first ever build over there, out of curiosity I
scrolled through the whole build output, by chance those "skipped:
Network testing disabled (unset GIT_TEST_HTTPD to enable)" lines
caught my eye, shrugged and thought that Dscho should better fix this,
but then an hour later got suspicious, so looked up a recent Travis CI
build, and... here we are.
I have to wonder how much longer it could have been remained
unnoticed, if the Perforce filehost hadn't failed or if I hadn't made
the gitgitgadget PR for my fix, or...
Anyway, this is just a long-winded way to say that I think we should
validate those bools properly and error loudly on an invalid value
even if it doesn't seem to worth it.
[1] https://public-inbox.org/git/20190906102711.6401-1-szeder.dev@xxxxxxxxx/T/
