Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'

[Junio C Hamano <gitster@pobox.com>]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Some variables in 'test_commit' have names that are common enough that
> it is very likely that test authors might use them in a test. If they do
> so and use 'test_commit' between setting such a variable and using it,
> the variable value from 'test_commit' will leak back into the test and
> most likely break it.
>
> Prevent that by marking all variables in 'test_commit' as 'local'. This
> allow a subsequent commit to use a 'tag' variable.

This is the right thing to do, if done onn day 1 of the project, and
it is the right thing to do for the longer term health of the
project.  But it is a bit scary thing to do in the middle.

I wonder if there is an easy way to detect that a set of callers of
test_commit are relying on the fact that calling test_commit without
passing --author option cleared their $author variable (similarly
for other variables that are cleared or set to a known value as a
side effect of calling test_commit).  If their correctness depends
on $author becoming empty after calling the script today, they will
get broken by this change.

While it is OK to argue that they deserve it, we would have to be
finding and fixing them ourselves after all.

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  t/test-lib-functions.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 527a7145000..adc0fb6330c 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -273,13 +273,13 @@ debug () {
>  # <file>, <contents>, and <tag> all default to <message>.
>  
>  test_commit () {
> -	notick= &&
> -	echo=echo &&
> -	append= &&
> -	author= &&
> -	signoff= &&
> -	indir= &&
> -	tag=light &&
> +	local notick= &&
> +	local echo=echo &&
> +	local append= &&
> +	local author= &&
> +	local signoff= &&
> +	local indir= &&
> +	local tag=light &&
>  	while test $# != 0
>  	do
>  		case "$1" in
> @@ -322,7 +322,7 @@ test_commit () {
>  		shift
>  	done &&
>  	indir=${indir:+"$indir"/} &&
> -	file=${2:-"$1.t"} &&
> +	local file=${2:-"$1.t"} &&
>  	if test -n "$append"
>  	then
>  		$echo "${3-$1}" >>"$indir$file"