From: Firmin Martin <firminmartin24@gmail.com>
Date: Mon, 17 May 2021 07:44:22 GMT
Subject: Re: [PATCH v2 1/2] t/README: document test_config
Message-ID: <87cztpg5yh.fsf@Inspiron.i-did-not-set--mail-host-address--so-tickle-me>
In-Reply-To: <5faaa5a6-8ebf-b2ef-805f-551a36ab7e6c@gmail.com>

Hi Bagas,

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 15/05/21 22.27, Firmin Martin wrote:
>>   
>> + - test_config <config-option> [<value>]
>> +
>> +   Set the configuration option <config-option> to <value>, and unset it at the
>> +   end of the current test. For a similar purpose, test_config_global for
>> +   global configuration is also available. Note, however, that test_config_*
>> +   must not be used in a subshell.
>> +

>  From the syntax above, when I omit <value>, default value for <config-option>
> will be set, right? 
Good remark. I thought the same as you, but no, it will "git config
<config-option>" (query the value of <config-option>) and then "git
config --unset <config-option>" which is dangerous. Doing so is more
likely an error and should be forbidden, as in this case, it is
equivalent to test_unconfig <config-option>. Will reflect this in v3.
> You forgot to mention it.
I'll thus drop square brackets.

>> +   Example:
>> +
>> +	test_config format.coverLetter auto
>> +
>> +   Is a concise way to write:
>> +	test_when_finished "git config --unset format.coverLetter" &&
>> +	git config format.coverLetter auto
>> +
>>   
>
> The description said "set the config, then unset it". But the expanded version
> said "unsetting the config is deferred to the end of test and set the config".
>
> The documentation for test_when_finished said:
>
>>    Prepend <script> to a list of commands to run to clean up
>>    at the end of the current test.  If some clean-up command
>>    fails, the test will not pass.
>
> Is my interpretation above correct?
Yes.

> Oh yeah, maybe it's better to say:
> "For example: <blah> have the same effect as <blah>."
I agree, because the two lines have not exactly the same behaviour as
test_config. Will fix wording.

Thanks,

Firmin