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

From: Stefan Beller <sbeller@google.com>
Date: Oct 24, 2017, 16:28 UTC
Message-ID: <CAGZ79kaSZG9WriqX0SLbTnEDRr-4YdFRuK17+9wnxyrftfnMmg@mail.gmail.com>
In-Reply-To: <3aed764b-388c-d163-08fc-32b294c6b9d3@samsung.com>

On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin <a.okoshkin@samsung.com> wrote:

Show 5 quoted lines

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

But do we have other threads running at the time?
Inspecting the four callsites:
* sequencer.c:
  The prior lines to hold the index lock suggests we're not in a threaded
  environment
* builtin/merge-recursive.c:
  In cmd_merge_recursive and we're fiddling with argv/argc, which
  suggests we in a main function, not having threads around
* builtin/am.c: fall_back_threeway called by am_run.
  (am is not threaded)
* builtin/merge:
  In try_merge_strategy called from the main function, also index locking
* builtin/checkout.c:
  merge_working_tree also locks the index.

So I think this function is never called from within a threaded environment in git.

Show 15 quoted lines

>
> Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com>
> ---
>  merge-recursive.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 1494ffdb8..eaac98145 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 = getenv("GIT_MERGE_VERBOSITY");

Despite not being in a threaded environment, I wonder if we want to minimize the time between calling getenv and the use of the result, i.e. declare merge_verbosity here, but assign it later, just before the condition?

(The compiler may shuffle stuff around anyway, so this is a moot suggestion; It gears mostly towards making the code more readable/maintainable when presenting this part of the code to the user.)

With or without this change:
Reviewed-by: Stefan Beller <sbeller@google.com>

Show 17 quoted lines

>         memset(o, 0, sizeof(struct merge_options));
>         o->verbosity = 2;
>         o->buffer_output = 1;
> @@ -2171,9 +2172,8 @@ 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);
> +       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