Re: [PATCH] exec_cmd: system_path memory leak fix

[Junio C Hamano <gitster@pobox.com>]

Alex Kuleshov <kuleshovmail@gmail.com> writes:

> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>

So this one does not change the contract between the caller and the
callee.  It does not change the fact that you need to explain your
change, though.  The story should read something like this:

    system_path(): always return a volatile result

    The function sometimes returns a newly allocated string and
    sometimes returns a borrowed string, the latter of which the
    callers must not free().

    The existing callers all assume that the return value belongs to
    the callee and most of them copy it with strdup() when they want
    to keep it around.  They end up leaking the returned copy when
    the callee returned a new string.

    Make sure all callers receive a volatile result, so that they do
    not have to be worried about freeing the returned string.

    There however are existing callers that are already broken, for
    example, ...

    Fixing these callers are done as separate patches, that can be
    applied either before or after this patch.

Personally, as I already said, I think the approach the previous
version took (but not the implementation) to change the contract
between the callers and this function is probably a good idea in
the longer term.  Leaking a bit by forgetting to convert a few
callers to free the returned values will not affect the correctness,
but making the returned value consistently more volatile will
expose existing breakage more often and will break codepaths that
happened to be working by accident.

Thanks.