Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Junio C Hamano]

Andrey Okoshkin  writes:

> I think, the main benefits are:
> * Code is more readable, no duplicated calls with the same constant string
> argument.
> * Code is potentially safer, the second getenv() call may return another
> pointer value which could be NULL (and yes, this is an arguable point as it
> can be done only artificially).
>
>> For the sake of fairness, I would say that the resulting code may be
>> easier to follow and has one less instance of a constant string that
>> the compiler cannot statically check if we made a typo.  That's the
>> only benefit in this patch as far as I can see.
>> 
>> The original makes a call to see if the result is NULL, and then
>> makes the same call, expecting that we get the same result (not just
>> that it is not NULL, but it is the same verbosity request the end
>> user made via the environment as the one we checked earlier), and I
>> can understand that it feels a bit redundant and ugly.
>
> Yes, you absolutely right.

I am absolutely right when I say your "code is potentially safer" is
total BS.  The result from first getenv() call may be pointing at an
invalid piece of memory by the time it is used, if you are in a
situation in which not having the second getenv() matters
(i.e. somebody else is also mucking with getenv() at the same time).

So please update the log message so that the patch is not sold on
that basis.

Thanks.

Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Andrey Okoshkin]

31.10.2017 05:26, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>> That holds true for the code before or after this patch equally.  In
>> other words, that sounds like a justification for rejecting this
>> patch (i.e. explanation of why this change is not needed).
>>
>> If we are worried about another thread calling these functions after
>> the time we call getenv() and before the time we pass the result to
>> strtol(), then I do not think this patch gives a better protection
>> against such race, so I do not think that is why you are doing this.
>>
>> So... why do we want to do this change?  I am puzzled.

I think, the main benefits are:
* Code is more readable, no duplicated calls with the same constant string
argument.
* Code is potentially safer, the second getenv() call may return another
pointer value which could be NULL (and yes, this is an arguable point as it
can be done only artificially).

> For the sake of fairness, I would say that the resulting code may be
> easier to follow and has one less instance of a constant string that
> the compiler cannot statically check if we made a typo.  That's the
> only benefit in this patch as far as I can see.
> 
> The original makes a call to see if the result is NULL, and then
> makes the same call, expecting that we get the same result (not just
> that it is not NULL, but it is the same verbosity request the end
> user made via the environment as the one we checked earlier), and I
> can understand that it feels a bit redundant and ugly.

Yes, you absolutely right.
I believe this patch makes code more beautiful :-)

>>> Signed-off-by: Andrey Okoshkin 
>>> Reviewed-by: Stefan Beller 
>>> ---
>>> Added 'reviewed-by' field.
>>>  merge-recursive.c | 7 ---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/merge-recursive.c b/merge-recursive.c
>>> index 1494ffdb8..60084e3a0 100644
>>> --- a/merge-recursive.c
>>> +++ b/merge-recursive.c
>>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct 
>>> merge_options *o)
>>>  
>>>  void init_merge_options(struct merge_options *o)
>>>  {
>>> +   const char *merge_verbosity;
>>> memset(o, 0, sizeof(struct merge_options));
>>> o->verbosity = 2;
>>> o->buffer_output = 1;
>>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)
>>> o->renormalize = 0;
>>> o->detect_rename = 1;
>>> merge_recursive_config(o);
>>> -   if (getenv("GIT_MERGE_VERBOSITY"))
>>> -   o->verbosity =
>>> -   strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
>>> +   merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
>>> +   if (merge_verbosity)
>>> +   o->verbosity = strtol(merge_verbosity, NULL, 10);
>>> if (o->verbosity >= 5)
>>> o->buffer_output = 0;
>>> strbuf_init(>obuf, 0);
> 
> 
> 

-- 
Best regards,
Andrey Okoshkin

Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Junio C Hamano]

Junio C Hamano  writes:

> That holds true for the code before or after this patch equally.  In
> other words, that sounds like a justification for rejecting this
> patch (i.e. explanation of why this change is not needed).
>
> If we are worried about another thread calling these functions after
> the time we call getenv() and before the time we pass the result to
> strtol(), then I do not think this patch gives a better protection
> against such race, so I do not think that is why you are doing this.
>
> So... why do we want to do this change?  I am puzzled.

For the sake of fairness, I would say that the resulting code may be
easier to follow and has one less instance of a constant string that
the compiler cannot statically check if we made a typo.  That's the
only benefit in this patch as far as I can see.

The original makes a call to see if the result is NULL, and then
makes the same call, expecting that we get the same result (not just
that it is not NULL, but it is the same verbosity request the end
user made via the environment as the one we checked earlier), and I
can understand that it feels a bit redundant and ugly.


>> Signed-off-by: Andrey Okoshkin 
>> Reviewed-by: Stefan Beller 
>> ---
>> Added 'reviewed-by' field.
>>  merge-recursive.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 1494ffdb8..60084e3a0 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct 
>> merge_options *o)
>>  
>>  void init_merge_options(struct merge_options *o)
>>  {
>> +const char *merge_verbosity;
>>  memset(o, 0, sizeof(struct merge_options));
>>  o->verbosity = 2;
>>  o->buffer_output = 1;
>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)
>>  o->renormalize = 0;
>>  o->detect_rename = 1;
>>  merge_recursive_config(o);
>> -if (getenv("GIT_MERGE_VERBOSITY"))
>> -o->verbosity =
>> -strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
>> +merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
>> +if (merge_verbosity)
>> +o->verbosity = strtol(merge_verbosity, NULL, 10);
>>  if (o->verbosity >= 5)
>>  o->buffer_output = 0;
>>  strbuf_init(>obuf, 0);

Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Junio C Hamano]

Andrey Okoshkin  writes:

> Get 'GIT_MERGE_VERBOSITY' environment variable only once in
> init_merge_options() and store the pointer to its value for the further check.

OK, that is "what this thing does" description.

> No intervening calls to getenv(), putenv(), setenv() or unsetenv() are done
> between the initial getenv() call and the consequential result pass to 
> strtol()
> as these environment related functions could modify the string pointer 
> returned
> by the initial getenv() call.

That holds true for the code before or after this patch equally.  In
other words, that sounds like a justification for rejecting this
patch (i.e. explanation of why this change is not needed).

If we are worried about another thread calling these functions after
the time we call getenv() and before the time we pass the result to
strtol(), then I do not think this patch gives a better protection
against such race, so I do not think that is why you are doing this.

So... why do we want to do this change?  I am puzzled.


>
> Signed-off-by: Andrey Okoshkin 
> Reviewed-by: Stefan Beller 
> ---
> Added 'reviewed-by' field.
>  merge-recursive.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 1494ffdb8..60084e3a0 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options 
> *o)
>  
>  void init_merge_options(struct merge_options *o)
>  {
> + const char *merge_verbosity;
>   memset(o, 0, sizeof(struct merge_options));
>   o->verbosity = 2;
>   o->buffer_output = 1;
> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)
>   o->renormalize = 0;
>   o->detect_rename = 1;
>   merge_recursive_config(o);
> - if (getenv("GIT_MERGE_VERBOSITY"))
> - o->verbosity =
> - strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
> + merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
> + if (merge_verbosity)
> + o->verbosity = strtol(merge_verbosity, NULL, 10);
>   if (o->verbosity >= 5)
>   o->buffer_output = 0;
>   strbuf_init(>obuf, 0);