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

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Junio,

Le 2022-10-21 à 17:06, Junio C Hamano a écrit :
> "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.

Isn't the fact that the test suite passes with this change enough for us
to be confident that nothing is broken by it ?

In any case, I did a quick manual grep of each of the variables and could not
find a test that uses these names, so I think we are safe.

Thanks,
Philippe.