Re: [PATCH 1/1] change contract between system_path and it's callers
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 25, 2014, 21:13 UTC
- Message-ID
- <xmqqfvd7rnkb.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1416939854-29930-1-git-send-email-kuleshovmail@gmail.com>
Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 6 quoted lines
> Now system_path returns path which is allocated string to callers; > It prevents memory leaks in some places. All callers of system_path > are owners of path string and they must release it. > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> > ---
Comparing this with what I sent out...
Show 14 quoted lines
> 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?
> execlp("info", "info", "gitman", page, (char *)NULL);
> die(_("no info viewer handled the request"));Show 7 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().
Show 18 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -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().
Show 34 quoted lines
> 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.