Re: [PATCH 1/1] change contract between system_path and it's callers
From: "Alexander Kuleshov" Sent: Wednesday, November 26, 2014 3:53 AM 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? -- Hi Alexander, The term 'churn' is originally from British butter making. Churn: verb 1.shake (milk or cream) in a machine in order to produce butter. "the cream is ripened before it is churned" synonyms: stir, agitate; 2.(with reference to liquid) move or cause to move about vigorously. "the seas churned". For Code (used in a somewhat negative sense), it means that lots of bits are moved around a great deal for little apparent benefit. In the sense Junio used it, I believe it's suggesting that the balance between the amount of change and usefulness of the change had gone further than hoped. (Though Junio is usually open to receiving a well argued case) Philip see also http://en.wikipedia.org/wiki/Churn_rate -- 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
Re: [PATCH 1/1] change contract between system_path and it's callers
Alexander Kuleshov 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] change contract between system_path and it's callers
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 :
> 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 :
>>>
>>> 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);
>>> > + pu
Re: [PATCH 1/1] change contract between system_path and it's callers
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 :
>>
>> 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")) {
>> > -
Re: [PATCH 1/1] change contract between system_path and it's callers
>
> 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, "--pa
Re: [PATCH 1/1] change contract between system_path and it's callers
Alexander Kuleshov writes:
> 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
> ---
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?
> 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().
> @@ -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.
> @@ -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.
--
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
[PATCH 1/1] change contract between system_path and it's callers
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
---
builtin/help.c | 10 +++---
exec_cmd.c | 17 +
exec_cmd.h | 4 ++--
git.c | 16
4 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..544d1cc 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -321,12 +321,13 @@ static void setup_man_path(void)
{
struct strbuf new_path = STRBUF_INIT;
const char *old_path = getenv("MANPATH");
-
+ char *git_man_path = system_path(GIT_MAN_PATH);
/* We should always put ':' after our path. If there is no
* old_path, the ':' at the end will let 'man' to try
* system-wide paths after ours to find the manual page. If
* there is old_path, we need ':' as delimiter. */
- strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+ strbuf_addstr(&new_path, git_man_path);
+ free(git_man_path);
strbuf_addch(&new_path, ':');
if (old_path)
strbuf_addstr(&new_path, old_path);
@@ -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);
execlp("info", "info", "gitman", page, (char *)NULL);
die(_("no info viewer handled the request"));
}
@@ -392,6 +395,7 @@ static void get_html_page_path(struct strbuf *page_path,
const char *page)
strbuf_init(page_path, 0);
strbuf_addf(page_path, "%s/%s.html", html_path, page);
+ free((char*)html_path);
}
/*
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..e3ebdd6 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
static const char *argv_exec_path;
static const char *argv0_path;
-const char *system_path(const char *path)
+char *system_path(const char *path)
{
#ifdef RUNTIME_PREFIX
static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
struct strbuf d = STRBUF_INIT;
if (is_absolute_path(path))
- return path;
+ return strdup(path);
#ifdef RUNTIME_PREFIX
assert(argv0_path);
@@ -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;
}
const char *git_extract_argv0_path(const char *argv0)
@@ -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);
}
return system_path(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);
if (old_path)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
extern void git_set_argv_exec_path(const char *exec_path);
extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
extern void setup_path(void);
extern const char **prepare_git_cmd(const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
LAST_ARG_MUST_BE_NULL
extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
#endif /* GIT_EXEC_CMD_H */
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);
}
