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

[Andrey Okoshkin]

Thanks, Eric, indeed it's better to change the commit message.

25.10.2017 14:53, Eric Sunshine wrote:
> On Wed, Oct 25, 2017 at 7:39 AM, Andrey Okoshkin  
> wrote:
>> Check 'GIT_MERGE_VERBOSITY' environment variable only once in
>> init_merge_options().
>> Consequential call of getenv() may return NULL pointer.
> 
> It would be particularly nice to have a more detailed explanation in
> the commit message of the potential problem this patch is trying to
> solve. Given the amount of discussion, thus far, surrounding such a
> simple patch, this cryptic warning about getenv() returning NULL upon
> second invocation is insufficient to explain why this patch is
> desirable; it merely leads to a lot of head-scratching.
> 
>> However the stored pointer to the obtained getenv() result may be invalidated
>> by some other getenv() call as getenv() is not thread-safe.
> 
> This is even more cryptic, as it appears to be arguing for or against
> _something_ (it's not clear what) and it seems to be talking about a
> facet of the existing code, rather than explaining why the updated
> code consumes its 'merge_verbosity' value as early as possible after
> being assigned. Perhaps this part could be reworded something like
> this:
> 
> Instead, call getenv() only once, storing its value and
> consulting it as many times as needed. This update takes care
> to consume the value returned by getenv() without any
> intervening calls to getenv(), setenv(), unsetenv(), or
> putenv(), any of which might invalidate the pointer returned
> by the initial call.
> 
>> Signed-off-by: Andrey Okoshkin 
>> Reviewed-by: Stefan Beller 
> 
> As this patch is semantically quite different from the original to
> which Stefan gave his Reviewed-by: (and which other people argued
> against), it might be better omit this footer and let him re-give it
> if he so desires.
> 
>> ---
>> Changes since the previous patch:
>> * no actions are taken between the merge_verbosity assignment and check.
>>  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);
>> --
>> 2.14.3
> 
> 
> 

-- 
Best regards,
Andrey Okoshkin

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

[Eric Sunshine]

On Wed, Oct 25, 2017 at 7:39 AM, Andrey Okoshkin  wrote:
> Check 'GIT_MERGE_VERBOSITY' environment variable only once in
> init_merge_options().
> Consequential call of getenv() may return NULL pointer.

It would be particularly nice to have a more detailed explanation in
the commit message of the potential problem this patch is trying to
solve. Given the amount of discussion, thus far, surrounding such a
simple patch, this cryptic warning about getenv() returning NULL upon
second invocation is insufficient to explain why this patch is
desirable; it merely leads to a lot of head-scratching.

> However the stored pointer to the obtained getenv() result may be invalidated
> by some other getenv() call as getenv() is not thread-safe.

This is even more cryptic, as it appears to be arguing for or against
_something_ (it's not clear what) and it seems to be talking about a
facet of the existing code, rather than explaining why the updated
code consumes its 'merge_verbosity' value as early as possible after
being assigned. Perhaps this part could be reworded something like
this:

Instead, call getenv() only once, storing its value and
consulting it as many times as needed. This update takes care
to consume the value returned by getenv() without any
intervening calls to getenv(), setenv(), unsetenv(), or
putenv(), any of which might invalidate the pointer returned
by the initial call.

> Signed-off-by: Andrey Okoshkin 
> Reviewed-by: Stefan Beller 

As this patch is semantically quite different from the original to
which Stefan gave his Reviewed-by: (and which other people argued
against), it might be better omit this footer and let him re-give it
if he so desires.

> ---
> Changes since the previous patch:
> * no actions are taken between the merge_verbosity assignment and check.
>  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);
> --
> 2.14.3

[PATCH v2] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Andrey Okoshkin]

Check 'GIT_MERGE_VERBOSITY' environment variable only once in
init_merge_options().
Consequential call of getenv() may return NULL pointer.
However the stored pointer to the obtained getenv() result may be invalidated
by some other getenv() call as getenv() is not thread-safe.

Signed-off-by: Andrey Okoshkin 
Reviewed-by: Stefan Beller 
---
Changes since the previous patch:
* no actions are taken between the merge_verbosity assignment and check.
 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);
-- 
2.14.3