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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>
> 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?
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]