Re: [PATCH 2/2] ci: restore running httpd tests

["SZEDER Gábor" <szeder.dev@gmail.com>]

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@gmail.com/T/