Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 21, 2022, 21:06 UTC
- Message-ID
- <xmqq8rl8kht3.fsf@gitster.g>
- In-Reply-To
- <832e910aadc47309392feed69927c2e8693cbfc5.1666365220.git.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 10 quoted lines
> 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.
Show 39 quoted lines
> 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"