On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels <kgybels@xxxxxxxxxxxx> wrote:
> According to getenv(3)'s notes:
> [...]
> Since strings returned by getenv() are allowed to change on subsequent
> calls to getenv(), make sure to duplicate when caching external_diff_cmd
> from environment.
> [...]
> Signed-off-by: Kim Gybels <kgybels@xxxxxxxxxxxx>
> ---
> diff --git a/diff.c b/diff.c
> @@ -492,6 +492,9 @@ static const char *external_diff(void)
> external_diff_cmd = getenv("GIT_EXTERNAL_DIFF");
> if (!external_diff_cmd)
> external_diff_cmd = external_diff_cmd_cfg;
> + else
> + external_diff_cmd = xstrdup(external_diff_cmd);
Make sense.
Not shown in the context is that 'external_diff_cmd' is static, so
this is not (in the traditional sense) leaking the dup'd string.
I do find that the logic is obscured by doing the xstrdup() in the
'else' arm; it would be easier to grok if the condition was reversed
and xstrdup() done in the 'then' arm.
However, you might also consider using xstrdup_or_null(), like this:
external_diff_cmd = xstrdup_or_null(getenv(...));
if (!external_diff_cmd)
...as before...
> done_preparing = 1;
> return external_diff_cmd;
> }
