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

On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin [off-list ref] wrote:

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.

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 <redacted>