Re: [PATCH 1/1] change contract between system_path and it's callers

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2014, 17:53 UTC
Message-ID: <xmqqy4qxrgq0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CANCZXo63q2YmTcYr9Vf99fAAnENNj7vE1nC18QJDYve=-OcXyQ@mail.gmail.com>

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

Show 8 quoted lines

>> > +     setenv("INFOPATH", git_info_path, 1);
>> > +     free(git_info_path);
>> >       execlp("info", "info", "gitman", page, (char *)NULL);
>> >       die(_("no info viewer handled the request"));
>>
>> We are just about to exec; does this warrant the code churn?
>
> hmm... Can't understand what's the problem here?

Adding a new variable to keep the allocated piece of memory so that you can free after using it, and freeing it. These are not "problem" per-se.

But immediately after doing so, execlp() wipes your process and starts a different program afresh. When that happens, all the heap memory you allocated and were using is automatically reclaimed by the system anyway.

So it may not be "wrong" to free, but doing anything extra is an unnecessary work.

If you did an "assign to a variable to keep allocated memory, use it, free it and then exec" sequence in a new code, I do not think it is worth fixing the code not to do so to reduce that unnecessary work---it is not worth another round of review exchange. But in the same way, make an existing code that is not doing an unnecessary work to do so is not worth it.

Show 14 quoted lines

>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>> >  #endif
>> >
>> >       strbuf_addf(&d, "%s/%s", prefix, path);
>> > -     path = strbuf_detach(&d, NULL);
>> > -     return path;
>> > +     return d.buf;
>>
>> These happens to be the same with the current strbuf implementation,
>> but it is a good manner to use strbuf_detach(&d, NULL) here.  We
>> don't know what other de-initialization tomorrow's implementation of
>> the strbuf API may have to do in strbuf_detach().
>
> How to do it in correct way?

Wouldn't "return strbuf_detach(&d, NULL);" work without any assignment to "path"?

Show 13 quoted lines

>> > ...
>> > -                     puts(system_path(GIT_INFO_PATH));
>> > +                     char *git_info_path = system_path(GIT_INFO_PATH);
>> > +                     puts(git_info_path);
>> > +                     free(git_info_path);
>> >                       exit(0);
>> >               } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>> >                       use_pager = 1;
>>
>> None of these warrant the code churn, I would say.
>
> Sorry, english is not my first language, what did you mean when saying:
> "code churn"? Code duplication or something else?

Step back a bit and _think_ why you want to avoid leaks. When a program is leaky, your process will bloat by accumulating unused cruft in your heap. Step back again and think the reason why you want to avoid that bloat.

It is because the parts of your program that want to allocate and use more memory will suffer _after_ leak happens, because leakers consume and keep memory that would be otherwise useful if they did not leak and freed instead. The later callers want to get more memory, but cannot get it because the program leaked memory before the control flow got to them.

Now, who do these calls to free() you added help? Whose desire to allocate more memory can be harmed by the allocated but not freed piece of memory held by the return values from system_path()?

Nobody--exactly because the next thing you do is to exit.

It is not wrong per-se to free immediately before exiting; it is just like freeing immediately before execing. It is an unnecessary thing to do, and a change to do so has no or very little value, even if it is not a wrong thing to do.