What do you think if we create int variable, something like
given_config_must_free = 0; and will set up it to 1 in cases where it
will be allocated, and than we can free it in the end of cmd_config?
I'm reading git source code to understanding git internals and found
little memory leak in cat-file, so i fixed it as i described above.
2014-11-26 15:42 GMT+06:00 Alexander Kuleshov <kuleshovm...@gmail.com>:
> Maybe we will discard this patch, because i looked on it and tested
> with different places, it brings more leaks than before?
>
> 2014-11-26 9:53 GMT+06:00 Alexander Kuleshov <kuleshovm...@gmail.com>:
>>>
>>> Comparing this with what I sent out...
>>>
>>> > builtin/help.c | 10 +++++++---
>>> > exec_cmd.c | 17 +++++++++--------
>>> > exec_cmd.h | 4 ++--
>>> > git.c | 16 ++++++++++++----
>>> > 4 files changed, 30 insertions(+), 17 deletions(-)
>>> >
>>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
>>> > static void show_info_page(const char *git_cmd)
>>> > {
>>> > const char *page = cmd_to_page(git_cmd);
>>> > - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
>>> > + char *git_info_path = system_path(GIT_INFO_PATH);
>>> > + setenv("INFOPATH", git_info_path, 1);
>>> > + free(git_info_path);
>>>
>>> We are just about to exec; does this warrant the code churn?
>>
>> hmm... Can't understand what's the problem here? We get git_info_path
>> from system_path which returns pointer which will need to free, set it in
>> environment var and than free it...
>>
>>>
>>> > execlp("info", "info", "gitman", page, (char *)NULL);
>>> > die(_("no info viewer handled the request"));
>>>
>>> > @@ -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?
>>
>>
>> strbuf_addf(&d, "%s/%s", prefix, path);
>> path = strbuf_detach(&d, NULL);
>> return (char*)path;
>>
>> Or something else?
>>
>>>
>>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
>>> >
>>> >
>>> > /* Returns the highest-priority, location to look for git programs. */
>>> > -const char *git_exec_path(void)
>>> > +char *git_exec_path(void)
>>> > {
>>> > const char *env;
>>> >
>>> > if (argv_exec_path)
>>> > - return argv_exec_path;
>>> > + return strdup(argv_exec_path);
>>> >
>>> > env = getenv(EXEC_PATH_ENVIRONMENT);
>>> > if (env && *env) {
>>> > - return env;
>>> > + return strdup(env);
>>> > }
>>>
>>> Now you are making callers of git_exec_path() responsible for
>>> freeing the result they receive.
>>>
>>> git_exec_path() may be called quite a lot, which means we may end up
>>> calling system_path() many times during the life of a process
>>> without freeing its return value, so this change may be worth doing,
>>> but this patch is insufficient, isn't it?
>>>
>>> You just added load_command_list() in help.c a new leak or two, for
>>> example. There probably are other callers of this function but I
>>> don't have time to look at all of them myself right now.
>>
>> Yes, need to do that all git_exec_path() callers free result of
>> git_exec_path.
>>
>>>
>>> > @@ -95,8 +94,10 @@ void setup_path(void)
>>> > {
>>> > const char *old_path = getenv("PATH");
>>> > struct strbuf new_path = STRBUF_INIT;
>>> > + char* exec_path = git_exec_path();
>>> >
>>> > - add_path(&new_path, git_exec_path());
>>> > + add_path(&new_path, exec_path);
>>> > + free(exec_path);
>>> > add_path(&new_path, argv0_path);
>>>
>>> This part by itself is good, provided if we make it the caller's
>>> responsiblity to free string returned by git_exec_path().
>>>
>>> > diff --git a/git.c b/git.c
>>> > index 82d7a1c..d01c4f1 100644
>>> > --- a/git.c
>>> > +++ b/git.c
>>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int
>>> > *argc, int *envchanged)
>>> > if (*cmd == '=')
>>> > git_set_argv_exec_path(cmd + 1);
>>> > else {
>>> > - puts(git_exec_path());
>>> > + char *exec_path = git_exec_path();
>>> > + puts(exec_path);
>>> > + free(exec_path);
>>> > exit(0);
>>> > }
>>> > } else if (!strcmp(cmd, "--html-path")) {
>>> > - puts(system_path(GIT_HTML_PATH));
>>> > + char *git_html_path = system_path(GIT_HTML_PATH);
>>> > + puts(git_html_path);
>>> > + free(git_html_path);
>>> > exit(0);
>>> > } else if (!strcmp(cmd, "--man-path")) {
>>> > - puts(system_path(GIT_MAN_PATH));
>>> > + char *git_man_path = system_path(GIT_MAN_PATH);
>>> > + puts(git_man_path);
>>> > + free(git_man_path);
>>> > exit(0);
>>> > } else if (!strcmp(cmd, "--info-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?
>
>
>
> --
> _________________________
> 0xAX
--
_________________________
0xAX
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
