Re: [PATCH 1/5] tests: shell running test scripts is now named AM_TEST_RUNNER_SHELL

[Stefano Lattarini]

Hi Eric, thank you very much for the review.

On 05/07/2012 04:34 PM, Eric Blake wrote:
> On 05/01/2012 10:04 AM, Stefano Lattarini wrote:
>> This is just a preparatory refactoring for future changes.
> 
> Sorry for my delay in reviewing; I'm now looking at this series.
> 
>>
>> * configure.ac (AM_TEST_RUNNER_SHELL): New variable, defined
>> to $SHEL', and AC_SUBST'd.
> 
> s/SHEL/SHELL/
> 
>
Oops, fixed.

>> @@ -105,16 +105,16 @@ case ${AM_TESTS_REEXEC-yes} in
>>        *x*) opts=-x;;
>>        *) opts=;;
>>      esac
>> -    echo $me: exec $SHELL $opts "$0" "$*"
>> -    exec $SHELL $opts "$0" ${1+"$@"} || {
>> -      echo "$me: failed to re-execute with $SHELL" >&2
>> +    echo $me: exec $AM_TEST_RUNNER_SHELL $opts "$0" "$*"
> 
> Can $me ever contain spaces?
>
No.

> If so, shouldn't this be:
> 
> echo "$me: exec $AM_TEST_RUNNER_SHELL $opts $0 $*"
>
This would leave an extra white space between the name of the shell and
the path of the script whenever '$opts' is empty (as it usually is).
Bikeshedding of course, but since the pre-existing code behaved like
this already, I'd rather not change it.

> 
>> +++ b/t/self-check-cleanup.tap
>> @@ -58,7 +58,8 @@ do_clean ()
>>  # the cleanup code not to be run, so that the temporary directories
>>  # are left on disk.
>>  command_ok_ '"keep_testdirs=yes" causes testdir to be kept around' eval '
>> -     keep_testdirs=yes $SHELL -c ". ./defs && echo okok >foo" t/dummy.sh \
>> +     env keep_testdirs=yes \
>> +       $AM_TEST_RUNNER_SHELL -c ". ./defs && echo okok >foo" t/dummy.sh \
> 
> Why did you add an env wrapper?
>
Because I'd find it confusing if the assignment "keep_testdirs=yes" were
on a line different from the one of the command whose environment it
modifies.  An explicit use of 'env' removes this confusion.

> I guess it doesn't hurt, but it is one more layer of execution.
>
> This patch looks mostly mechanical.  I didn't check if you had any
> missed conversions,
>
I think not :-)

> but the ones you made are sane.
> 
Good.  Patch pushed then.

Regards,
  Stefano