On Wed, Oct 25, 2017 at 12:07:12AM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2017 at 9:48 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > Jeff King <peff@xxxxxxxx> 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
