Re: [PATCH v2] merge-recursive: check GIT_MERGE_VERBOSITY only once
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.
Show 33 quoted lines
> ---
> 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
Andrey Okoshkin
