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

["Martin Ågren" <martin.agren@gmail.com>]

On 24 October 2017 at 18:45, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Oct 24, 2017 at 12:28 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Tue, Oct 24, 2017 at 8:27 AM, Andrey Okoshkin <a.okoshkin@samsung.com> 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.

I'm having trouble wrapping my head around this. Under which
circumstances could the second call in the current code return NULL, but
the code after your patch behave in a well-defined (and correct) way?

> The distance between getenv() and the point where the value is
> actually used is a big concern due to not knowing what is or might be
> going on in called functions between the two points. According to [1],
> the value returned by getenv() could be invalidated by another call to
> getenv() (or setenv() or unsetenv() or putenv()), and we don't have
> guarantee that we're safe from such invalidation considering that this
> function calls out to others. For instance, after getenv() but before
> the value is used, init_merge_options() calls merge_recursive_config()
> which calls git_config() which calls git_xmerge_config(), and so on.
>
> For this reason, I have difficulty endorsing this change as-is.

Yeah. The call should be immediately before `merge_verbosity` is used.
Then, if a compiler wants to move the call, it has to do the work and
prove that it's ok.