Re: [PATCH 4/8] perf/run: use $default_value instead of $4

[Christian Couder]

On Wed, Dec 13, 2017 at 9:54 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> If you want to be able to use this helper to specify a default value
>> of an empty string (which the orignal that used $4 did), then the
>> previous hunk must be corrected so that it does not unconditionally
>> set default_value to $4.  Perhaps like
>>
>>   if test -n "${4+x}"
>>   then
>>   default_value=$4
>>   else
>>   unset default_value || :
>>   fi
>>
>> or something.
>
> And if you do not care about passing an empty string as the default
> value, then the earlier hunk can stay the same
>
> default_value=$4
>
> but the eval part should become something like
>
> test -n "$default_value" && eval ...
>
> Given that you are planning to add yet another optional argument to
> the helper, and when you pass that variable, you'd probably need to
> pass "" as the "default" that is not exported, perhaps this "give up
> ability to pass an empty string as the default" approach may be the
> only thing you could do.

Yeah, thanks for the explanations and suggestions.

Re: [PATCH 4/8] perf/run: use $default_value instead of $4

[Christian Couder]

On Wed, Dec 13, 2017 at 9:40 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Signed-off-by: Christian Couder 
>> ---
>>  t/perf/run | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/perf/run b/t/perf/run
>> index 43e4de49ef..bbd703dc4f 100755
>> --- a/t/perf/run
>> +++ b/t/perf/run
>> @@ -105,7 +105,7 @@ get_var_from_env_or_config () {
>>   env_var="$1"
>>   conf_sec="$2"
>>   conf_var="$3"
>> - # $4 can be set to a default value
>> + default_value="$4" # optional
>>
>>   # Do nothing if the env variable is already set
>>   eval "test -z \"\${$env_var+x}\"" || return
>> @@ -123,7 +123,7 @@ get_var_from_env_or_config () {
>>   conf_value=$(git config -f "$GIT_PERF_CONFIG_FILE" "$var") &&
>>   eval "$env_var=\"$conf_value\"" && return
>>
>> - test -n "${4+x}" && eval "$env_var=\"$4\""
>> + test -n "${default_value+x}" && eval "$env_var=\"$default_value\""
>
> This conversion changes the behaviour.  Because default_value is
> always set by your change in the previous hunk, we end up always
> doing this eval.
>
> The original says "If $4 is set, then ${4+x} becomes x and if $4 is
> not set, ${4+x} is empty, so let's check if ${4+x} is a non-empty
> string to see if $4 is set.  If ${4+x} is a non-empty string, that
> means $4 was set so we do the eval.
>
> If you want to be able to use this helper to specify a default value
> of an empty string (which the orignal that used $4 did), then the
> previous hunk must be corrected so that it does not unconditionally
> set default_value to $4.  Perhaps like
>
> if test -n "${4+x}"
> then
> default_value=$4
> else
> unset default_value || :
> fi
>
> or something.

Yeah, you are right I will fix this.

Re: [PATCH 4/8] perf/run: use $default_value instead of $4

[Junio C Hamano]

Junio C Hamano  writes:

> If you want to be able to use this helper to specify a default value
> of an empty string (which the orignal that used $4 did), then the
> previous hunk must be corrected so that it does not unconditionally
> set default_value to $4.  Perhaps like
>
>   if test -n "${4+x}"
>   then
>   default_value=$4
>   else
>   unset default_value || :
>   fi
>
> or something.

And if you do not care about passing an empty string as the default
value, then the earlier hunk can stay the same

default_value=$4

but the eval part should become something like

test -n "$default_value" && eval ...

Given that you are planning to add yet another optional argument to
the helper, and when you pass that variable, you'd probably need to
pass "" as the "default" that is not exported, perhaps this "give up
ability to pass an empty string as the default" approach may be the
only thing you could do.

Re: [PATCH 4/8] perf/run: use $default_value instead of $4

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  t/perf/run | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/perf/run b/t/perf/run
> index 43e4de49ef..bbd703dc4f 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -105,7 +105,7 @@ get_var_from_env_or_config () {
>   env_var="$1"
>   conf_sec="$2"
>   conf_var="$3"
> - # $4 can be set to a default value
> + default_value="$4" # optional
>  
>   # Do nothing if the env variable is already set
>   eval "test -z \"\${$env_var+x}\"" || return
> @@ -123,7 +123,7 @@ get_var_from_env_or_config () {
>   conf_value=$(git config -f "$GIT_PERF_CONFIG_FILE" "$var") &&
>   eval "$env_var=\"$conf_value\"" && return
>  
> - test -n "${4+x}" && eval "$env_var=\"$4\""
> + test -n "${default_value+x}" && eval "$env_var=\"$default_value\""

This conversion changes the behaviour.  Because default_value is
always set by your change in the previous hunk, we end up always
doing this eval.

The original says "If $4 is set, then ${4+x} becomes x and if $4 is
not set, ${4+x} is empty, so let's check if ${4+x} is a non-empty
string to see if $4 is set.  If ${4+x} is a non-empty string, that
means $4 was set so we do the eval.

If you want to be able to use this helper to specify a default value
of an empty string (which the orignal that used $4 did), then the
previous hunk must be corrected so that it does not unconditionally
set default_value to $4.  Perhaps like

if test -n "${4+x}"
then
default_value=$4
else
unset default_value || :
fi

or something.

>  }
>  
>  run_subsection () {