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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Oct 25, 2017 at 7:39 AM, Andrey Okoshkin <a.okoshkin@samsung.com> 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 <a.okoshkin@samsung.com>
> Reviewed-by: Stefan Beller <sbeller@google.com>

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(&o->obuf, 0);
> --
> 2.14.3