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
