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

From: Jeff King <peff@peff.net>
Date: Oct 25, 2017, 07:27 UTC
Message-ID: <20171025072717.7svdq4kqlfxlwszi@sigill.intra.peff.net>
In-Reply-To: <CAPig+cSjQd=p1CdizU5oUaz91z=j02UnWLtTguWzvkjS+v6ETA@mail.gmail.com>

On Wed, Oct 25, 2017 at 12:07:12AM -0400, Eric Sunshine wrote:

Show 10 quoted lines

> On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >> I definitely agree with the sentiment that as few things as possible
> >> should happen between calling getenv() and using its result. I've seen
> >> real bugs there from unexpected invalidation of the static buffer.
> >
> > Sure.  I do not think we mind xstrdup()ing the result and freeing
> > after we are done, though ;-)
> 
> More specifically, xstrdup_or_null(getenv(...)), if that route is chosen.

That would be the way to do it, but I do not see thta we need to record the string at all. The current code is calling strtol on it on it immediately.

So the options are:

  1. Save the result of getenv() in a variable. If it is non-NULL, then
     immediately call strtol() on it.

  2. Do nothing. The double-call to getenv() is actually fine in the
     real world as it will return consistent results.

But the patch under discussion, which calls getenv() then expects it to be correct after a call to merge_recursive_config(), introduces a problem.

-Peff