Alexander Kuleshov <kuleshovmail@xxxxxxxxx> writes:
>> > + 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.
>> > @@ -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"?
>> > ...
>> > - 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html