patch, 3 partsclone: Free a few paths

> and second arguments to strlcpy() the same, making the test fail when
> run under valgrind.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> This patch assumes the path returned by make_absolute_path() is never
> longer than PATH_MAX, which I think is a safe assumption.

>
>  abspath.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 91ca00f..9149a98 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)
>  	char *last_elem = NULL;
>  	struct stat st;
>  
> -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> +	if (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
>  		die ("Too long path: %.*s", 60, path);
>  
>  	while (depth--) {

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Sometimes (at least in t-0001-init.sh test 12), the return value of
> > make_absolute_path() is passed to it as an argument, making the first
> 
> I don't think it is a bad idea per-se to avoid a copy from the same memory
> location into the same memory location, but independent of the necessity
> of fixes at the low-level, shouldn't we fix the callers that do not check
> if what they have is already absolute?

> 
> > and second arguments to strlcpy() the same, making the test fail when
> > run under valgrind.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> >
> > This patch assumes the path returned by make_absolute_path() is never
> > longer than PATH_MAX, which I think is a safe assumption.
> 
> >
> >  abspath.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/abspath.c b/abspath.c
> > index 91ca00f..9149a98 100644
> > --- a/abspath.c
> > +++ b/abspath.c
> > @@ -24,7 +24,7 @@ const char *make_absolute_path(const char *path)
> >  	char *last_elem = NULL;
> >  	struct stat st;
> >  
> > -	if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> > +	if (buf != path && strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> >  		die ("Too long path: %.*s", 60, path);
> >  
> >  	while (depth--) {

>> I don't think it is a bad idea per-se to avoid a copy from the same memory
>> location into the same memory location, but independent of the necessity
>> of fixes at the low-level, shouldn't we fix the callers that do not check
>> if what they have is already absolute?
>
> If we'd like the semantics to be "whatever I had, I now know what the
> absolute path is" then we could make the check in the beginning of the
> function, to centralise the check. If the semantics should be "I don't
> have an absolute path, so I need to figure out what it is", then there
> should be a check before calling make_absolute_path() (the name suggests
> the second).

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >> I don't think it is a bad idea per-se to avoid a copy from the same memory
> >> location into the same memory location, but independent of the necessity
> >> of fixes at the low-level, shouldn't we fix the callers that do not check
> >> if what they have is already absolute?
> >
> > If we'd like the semantics to be "whatever I had, I now know what the
> > absolute path is" then we could make the check in the beginning of the
> > function, to centralise the check. If the semantics should be "I don't
> > have an absolute path, so I need to figure out what it is", then there
> > should be a check before calling make_absolute_path() (the name suggests
> > the second).
> 
> Good thinking, and I think the former semantics would be easier to use.

> 
> >  There is however the extra functionality the function offers, namely
> > resolving links. It might be good to split it into two functions so each
> > caller can specify what it wants.
> 
> Probably.

> > 
> > >  There is however the extra functionality the function offers, namely
> > > resolving links. It might be good to split it into two functions so each
> > > caller can specify what it wants.
> > 
> > Probably.
> 
>  With the changes mentioned earlier, if you want an absolute pathname,
> you'd call absolute_path/make_nonrelative_path and if you want to make
> sure you have the real path of the target file, you'd use real_path just
> as you'd use realpath on a sane system, with

> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:
>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:
>> > Carlos Martín Nieto <cmn@elego.de> writes:
> [...]
>> > 
>> > >  There is however the extra functionality the function offers, namely
>> > > resolving links. It might be good to split it into two functions so each
>> > > caller can specify what it wants.
>> > 
>> > Probably.
>> 
>>  With the changes mentioned earlier, if you want an absolute pathname,
>> you'd call absolute_path/make_nonrelative_path and if you want to make
>> sure you have the real path of the target file, you'd use real_path just
>> as you'd use realpath on a sane system, with
>
>  ... a comment on the functions and maybe some documentation in
> Documentation/techncal, as it doesn't seem to exist yet.

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:
> >> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:
> >> > Carlos Martín Nieto <cmn@elego.de> writes:
> > [...]
> >> > 
> >> > >  There is however the extra functionality the function offers, namely
> >> > > resolving links. It might be good to split it into two functions so each
> >> > > caller can specify what it wants.
> >> > 
> >> > Probably.
> >> 
> >>  With the changes mentioned earlier, if you want an absolute pathname,
> >> you'd call absolute_path/make_nonrelative_path and if you want to make
> >> sure you have the real path of the target file, you'd use real_path just
> >> as you'd use realpath on a sane system, with
> >
> >  ... a comment on the functions and maybe some documentation in
> > Documentation/techncal, as it doesn't seem to exist yet.
> 
> We probably should involve Nguyễn in this thread as his fingers are
> everywhere on the codepaths related to setup.
> 

> I've been changing this a bit, trying to make all the paths normalized,
> but it'll take a bit longer. I'll send a partial patch when I've
> finished something worth seeing (for the moment, the test fail if there
> is a symlink somewhere in the tree, as I've mixed
> real_path/make_absolute_path and absolute_path/make_nonrelative_path a
> bit).
>
>  Is it a good idea to normalize the paths? Otherwise, everything could
> be replaced by real_path/make_absolute_path (as most calls already are).
> As it's transitive and these paths aren't stored permanently (other than
> with clone), as long as we agree on one representation, it should be
> fine.

> 2011/3/16 Carlos Martín Nieto <cmn@elego.de>:
> > I've been changing this a bit, trying to make all the paths normalized,
> > but it'll take a bit longer. I'll send a partial patch when I've
> > finished something worth seeing (for the moment, the test fail if there
> > is a symlink somewhere in the tree, as I've mixed
> > real_path/make_absolute_path and absolute_path/make_nonrelative_path a
> > bit).
> >
> >  Is it a good idea to normalize the paths? Otherwise, everything could
> > be replaced by real_path/make_absolute_path (as most calls already are).
> > As it's transitive and these paths aren't stored permanently (other than
> > with clone), as long as we agree on one representation, it should be
> > fine.
> 
> I think the question is whether it's _necessary_ to do that. Any gain?
> make_absolute_path() calls are not in critical path, I don't think we
> should bother much, unless there are bugs like one you fixed in your
> patch.

> 
> >  Is there a performance hit if we resolve links all the time? If we run
> > everything through normalize_path(_copy), is it slower than resolving
> > links?
> 
> What paths are you talking about? If they are inside $GIT_DIR, we
> touch them quite often. But there are not many of them (unless you
> spread loose objects all over the place), resolving links should not
> be an issue.

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:
>>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:
>>> > Carlos Martín Nieto <cmn@elego.de> writes:
>> [...]
>>> >
>>> > >  There is however the extra functionality the function offers, namely
>>> > > resolving links. It might be good to split it into two functions so each
>>> > > caller can specify what it wants.
>>> >
>>> > Probably.
>>>
>>>  With the changes mentioned earlier, if you want an absolute pathname,
>>> you'd call absolute_path/make_nonrelative_path and if you want to make
>>> sure you have the real path of the target file, you'd use real_path just
>>> as you'd use realpath on a sane system, with
>>
>>  ... a comment on the functions and maybe some documentation in
>> Documentation/techncal, as it doesn't seem to exist yet.
>
> We probably should involve Nguyễn in this thread as his fingers are
> everywhere on the codepaths related to setup.

> On Wed, Mar 16, 2011 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Carlos Martín Nieto <cmn@elego.de> writes:
> >
> >> On mar, 2011-03-15 at 12:59 +0100, Carlos Martín Nieto wrote:
> >>> On lun, 2011-03-14 at 15:58 -0700, Junio C Hamano wrote:
> >>> > Carlos Martín Nieto <cmn@elego.de> writes:
> >> [...]
> >>> >
> >>> > >  There is however the extra functionality the function offers, namely
> >>> > > resolving links. It might be good to split it into two functions so each
> >>> > > caller can specify what it wants.
> >>> >
> >>> > Probably.
> >>>
> >>>  With the changes mentioned earlier, if you want an absolute pathname,
> >>> you'd call absolute_path/make_nonrelative_path and if you want to make
> >>> sure you have the real path of the target file, you'd use real_path just
> >>> as you'd use realpath on a sane system, with
> >>
> >>  ... a comment on the functions and maybe some documentation in
> >> Documentation/techncal, as it doesn't seem to exist yet.
> >
> > We probably should involve Nguyễn in this thread as his fingers are
> > everywhere on the codepaths related to setup.
> 
> Thanks, my attempt to fix up setup code leaves more traces that I expect.
> 
> Splitting functions is fine, but is there any use of
> absolute_path/make_nonrelative_path alone? Most setup code uses
> make_absolute_path() for $GIT_{DIR,WORK_TREE}. For GIT_DIR, a
> resolved/normalized path is preferred. For GIT_WORK_TREE, I'm not
> sure. I tend to treat it the same way as GIT_DIR, but you guys may
> have a special case.

> @@ -99,10 +99,13 @@ void setup_path(void)
>  {
>  	const char *old_path = getenv("PATH");
>  	struct strbuf new_path = STRBUF_INIT;
> +	char *exec_path = (char *) git_exec_path();

> On Mon, Mar 14, 2011 at 08:18:37PM +0100, Carlos Martín Nieto wrote:
> 
> > Make sure the pointer git_exec_path() returns is in the heap and free
> > it after it's no longer needed.
> 
> Could you explain the motivation a bit more? From looking at the code,
> it looks like the situation is that git_exec_path sometimes returns a
> newly allocated string and sometimes not, so the caller may leak in the
> former case.

> 
> That seems to be the fault of system_path, which sometimes allocates and
> sometimes does not. Should we also be fixing system_path, which is a
> source of leaks? Or instead converting system_path to use a static
> buffer?

> 
> > @@ -99,10 +99,13 @@ void setup_path(void)
> >  {
> >  	const char *old_path = getenv("PATH");
> >  	struct strbuf new_path = STRBUF_INIT;
> > +	char *exec_path = (char *) git_exec_path();
> 
> Ick. If it is now returning an allocated buffer that the caller is
> responsible for free-ing, then its declaration should drop the const in
> the return value.

> Make system_path behave like the other path functions by using a
> static buffer, fixing a memory leak.
>
> Also make sure the prefix pointer is always initialized to either
> PREFIX or NULL.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> This fixes the leak I was trying to fix with my original patch, but
> this seems much cleaner
>
>  exec_cmd.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 38545e8..12ce017 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -9,11 +9,11 @@ static const char *argv0_path;
>  const char *system_path(const char *path)
>  {
>  #ifdef RUNTIME_PREFIX
> -       static const char *prefix;
> +       static const char *prefix = NULL;
>  #else
>        static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static char buf[PATH_MAX+1];

> @@ -33,9 +33,8 @@ const char *system_path(const char *path)
>        }
>  #endif
>
> -       strbuf_addf(&d, "%s/%s", prefix, path);
> -       path = strbuf_detach(&d, NULL);
> -       return path;
> +       snprintf(buf, PATH_MAX, "%s/%s", prefix, path);

> On Wed, Mar 16, 2011 at 12:26 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> > Make system_path behave like the other path functions by using a
> > static buffer, fixing a memory leak.
> >
> > Also make sure the prefix pointer is always initialized to either
> > PREFIX or NULL.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> >
> > This fixes the leak I was trying to fix with my original patch, but
> > this seems much cleaner
> >
> >  exec_cmd.c |    9 ++++-----
> >  1 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 38545e8..12ce017 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -9,11 +9,11 @@ static const char *argv0_path;
> >  const char *system_path(const char *path)
> >  {
> >  #ifdef RUNTIME_PREFIX
> > -       static const char *prefix;
> > +       static const char *prefix = NULL;
> >  #else
> >        static const char *prefix = PREFIX;
> >  #endif
> > -       struct strbuf d = STRBUF_INIT;
> > +       static char buf[PATH_MAX+1];
> 
> Why PATH_MAX + 1? POSIX says that PATH_MAX includes the null-termination...

> 
> > @@ -33,9 +33,8 @@ const char *system_path(const char *path)
> >        }
> >  #endif
> >
> > -       strbuf_addf(&d, "%s/%s", prefix, path);
> > -       path = strbuf_detach(&d, NULL);
> > -       return path;
> > +       snprintf(buf, PATH_MAX, "%s/%s", prefix, path);
> 
> Perhaps "snprintf(buf, sizeof(buf) - 1, "%s/%s", prefix, path);" instead?

> Make system_path behave like the other path functions by using a
> static buffer, fixing a memory leak.
>
> Also make sure the prefix pointer is always initialized to either
> PREFIX or NULL.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---

> 
> > Make system_path behave like the other path functions by using a
> > static buffer, fixing a memory leak.
> >
> > Also make sure the prefix pointer is always initialized to either
> > PREFIX or NULL.
> >
> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> > ---
> 
> Have you made sure all the callers are Ok with this change?
> 
> If somebody called system_path(GIT_EXEC_PATH), saved the result in a
> variable without copying, and then called system_path(ETC_GITATTRIBUTES),
> his variable may now have a value unrelated to GIT_EXEC_PATH, and you
> would fix all such callers to save the value away with strdup().

> +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> +	if (ret >= sizeof(buf))
> +		die("system path too long for %s", path);
> +	else if (ret < 0)
> +		die_errno("encoding error");

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > +	if (ret >= sizeof(buf))
> > +		die("system path too long for %s", path);
> > +	else if (ret < 0)
> > +		die_errno("encoding error");
> 
> POSIX says snprintf() should set errno in this case, and your use of
> die_errno() would show that information, but what is "encoding error"?
> 
> Just being curious, as I suspect that "snprintf() returned an error" may
> be more appropriate, if the answer is "I don't know what kind of error it
> is, but snprintf() found something faulty while encoding so I chose to
> call it encoding error".

> On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> > Carlos Martín Nieto <cmn@elego.de> writes:
> > 
> > > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > > +	if (ret >= sizeof(buf))
> > > +		die("system path too long for %s", path);
> > > +	else if (ret < 0)
> > > +		die_errno("encoding error");
> > 
> > POSIX says snprintf() should set errno in this case, and your use of
> > die_errno() would show that information, but what is "encoding error"?
> > 
> > Just being curious, as I suspect that "snprintf() returned an error" may
> > be more appropriate, if the answer is "I don't know what kind of error it
> > is, but snprintf() found something faulty while encoding so I chose to
> > call it encoding error".
> 
>  My manpage says snprintf returns -1 if there was an output or encoding
> error. As there couldn't be an output error because it's writing to
> memory and we can't output what snprintf chocked on because whatever
> die_errno uses will also choke on it, I just put "encoding error". I'd
> put "error assembling system path" as the actual error message, I guess.

> On Mon, Mar 21, 2011 at 10:56:19AM +0100, Carlos Martín Nieto wrote:
> 
> > On vie, 2011-03-18 at 00:25 -0700, Junio C Hamano wrote:
> > > Carlos Martín Nieto <cmn@elego.de> writes:
> > > 
> > > > +	ret = snprintf(buf, sizeof(buf), "%s/%s", prefix, path);
> > > > +	if (ret >= sizeof(buf))
> > > > +		die("system path too long for %s", path);
> > > > +	else if (ret < 0)
> > > > +		die_errno("encoding error");
> > > 
> > > POSIX says snprintf() should set errno in this case, and your use of
> > > die_errno() would show that information, but what is "encoding error"?
> > > 
> > > Just being curious, as I suspect that "snprintf() returned an error" may
> > > be more appropriate, if the answer is "I don't know what kind of error it
> > > is, but snprintf() found something faulty while encoding so I chose to
> > > call it encoding error".
> > 
> >  My manpage says snprintf returns -1 if there was an output or encoding
> > error. As there couldn't be an output error because it's writing to
> > memory and we can't output what snprintf chocked on because whatever
> > die_errno uses will also choke on it, I just put "encoding error". I'd
> > put "error assembling system path" as the actual error message, I guess.
> 
> FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
> calls just ignore the return value, and some even directly use the
> return value to add to a length. The one place that actually does check
> for the error is strbuf_vaddf, which just says "your vsnprintf is
> broken" and dies.

> 
> So I'm not sure how much we really care about this error code path. If
> anything, we should be replacing all of the calls with something like:
> 
>   static const char buggy_sprintf_msg[] =
>   "BUG: vsnprintf returned %d; either we fed it a bogus format string\n"
>   "(our bug) or your libc is buggy and returns an error when it should\n"
>   "tell us how much space is needed. The format string was:\n"
>   "%s\n";
>   int xsnprintf(char *out, size_t size, const char *fmt, ...)
>   {
>           va_list ap;
>           int r;
> 
>           va_start(ap, fmt);
>           r = vsnprintf(out, size, fmt, ap);
>           va_end(ap);
> 
>           if (r < 0)
>                   die(buggy_sprintf_msg, r, fmt);
>           return r;
>   }

> > FWIW, we don't catch snprintf failures in 99% of the calls in git. Most
> > calls just ignore the return value, and some even directly use the
> > return value to add to a length. The one place that actually does check
> > for the error is strbuf_vaddf, which just says "your vsnprintf is
> > broken" and dies.
> 
>  It's not actually likely we'll ever meet this error if the only one
> allowed to set the format string is the programmer (and to do otherwise
> is a security risk).

>  Or we could overload (#define) snprintf and replace it with the
> paranoid. It'd go nicely with the vsnprintf that tries to work around
> the Windows implementation.
> 
>  I don't feel that strongly we should have the extra check there, seeing
> how it's rare and not checked anywhere else.

> Yeah, I am happy to just drop it. AFAICT, an error return from snprintf
> is a bug in your program[1] or a bug in libc. In either case, it should
> fail or produce bogus output on the first run, and we don't need to
> worry about checking errors all the time. Noting the error helps a
> little with detection, but since we already install our own vsnprintf on
> known-buggy platforms, I haven't seen a single complaint.

>  #ifdef RUNTIME_PREFIX
> -       static const char *prefix;
> +       static const char *prefix = NULL;
>  #else
>        static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static char buf[PATH_MAX];
> +       int ret;

> Hi,
>
> Nguyen Thai Ngoc Duy wrote:
>
>> It was pointed out elsewhere [1] that PATH_MAX only specifies max
>> length of a path element, not full path. I think we'd need to stay
>> away from preallocated PATH_MAX-sized arrays.
>
> No, PATH_MAX is actually the maximum length of a path, and when you
> use, say, open(2), it will fail if your path is longer than that.  The
> maximum length of a path component on most filesytems is 255 or 256;
> PATH_MAX on Linux is 4096.

> Hi,
> 
> Nguyen Thai Ngoc Duy wrote:
> 
> > It was pointed out elsewhere [1] that PATH_MAX only specifies max
> > length of a path element, not full path. I think we'd need to stay
> > away from preallocated PATH_MAX-sized arrays.
> 
> No, PATH_MAX is actually the maximum length of a path, and when you
> use, say, open(2), it will fail if your path is longer than that.  The
> maximum length of a path component on most filesytems is 255 or 256;
> PATH_MAX on Linux is 4096.
> 
> It is indeed possible to have paths with length longer than that.  The
> way to support that is to use relative paths wherever possible, which

> On vie, 2011-03-18 at 06:38 -0500, Jonathan Nieder wrote:
>> Hi,
>>
>> Nguyen Thai Ngoc Duy wrote:
>>
>> > It was pointed out elsewhere [1] that PATH_MAX only specifies max
>> > length of a path element, not full path. I think we'd need to stay
>> > away from preallocated PATH_MAX-sized arrays.
>>
>> No, PATH_MAX is actually the maximum length of a path, and when you
>> use, say, open(2), it will fail if your path is longer than that.  The
>> maximum length of a path component on most filesytems is 255 or 256;
>> PATH_MAX on Linux is 4096.
>>
>> It is indeed possible to have paths with length longer than that.  The
>> way to support that is to use relative paths wherever possible, which
>
>  So what PATH_MAX describes is the maximum length of a string
> representing a path, but not necessarily the length of the path itself.

> It is indeed possible to have paths with length longer than that.  The
> way to support that is to use relative paths wherever possible, which
> does sound to me like an interesting long-term goal (mostly because I
> suspect the result would be easier to read and, especially, to reason
> about with respect to race conditions).

> Make sure the pointer git_exec_path() returns is in the heap and free
> it after it's no longer needed.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>  exec_cmd.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)

> diff --git a/exec_cmd.c b/exec_cmd.c
> index 38545e8..c16c3d4 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -73,11 +73,11 @@ const char *git_exec_path(void)
>  	const char *env;
>  
>  	if (argv_exec_path)
> -		return argv_exec_path;
> +		return xstrdup(argv_exec_path);
>  
>  	env = getenv(EXEC_PATH_ENVIRONMENT);
>  	if (env && *env) {
> -		return env;
> +		return xstrdup(env);
>  	}
>  
>  	return system_path(GIT_EXEC_PATH);
> @@ -99,10 +99,13 @@ void setup_path(void)
>  {
>  	const char *old_path = getenv("PATH");
>  	struct strbuf new_path = STRBUF_INIT;
> +	char *exec_path = (char *) git_exec_path();
>  
> -	add_path(&new_path, git_exec_path());
> +	add_path(&new_path, exec_path);
>  	add_path(&new_path, argv0_path);
>  
> +	free(exec_path);
> +
>  	if (old_path)
>  		strbuf_addstr(&new_path, old_path);
>  	else

> Carlos Martín Nieto <cmn@elego.de> writes:
> [...]
> 
>     Update only one caller setup_path() to follow the new API, but leave
>     other callers to leak even in normal cases.  The caller in git.c exits
>     immediately after using it, so we don't care about the leak there
>     anyway.  Also help.c has a few calls to it but the number of calls to
>     the function is small and bounded, so the leak is small and we don't
>     care.

> 
> 
> And then reviewers can agree or disagree if the small leaks in git.c (just
> one string allocation that immediately is followed by exit after its use)
> and help.c (in list_commands() and load_commands_list(), neither of which
> is called millions of times anyway) are OK to accept.
> 
> I tend to think they are Ok, but then I also tend to think one leak of
> exec-path return value in setup_path() is perfectly fine for the same
> reason, so in that sense I don't see a point in this patch...

> 
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 38545e8..c16c3d4 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -73,11 +73,11 @@ const char *git_exec_path(void)
> >  	const char *env;
> >  
> >  	if (argv_exec_path)
> > -		return argv_exec_path;
> > +		return xstrdup(argv_exec_path);
> >  
> >  	env = getenv(EXEC_PATH_ENVIRONMENT);
> >  	if (env && *env) {
> > -		return env;
> > +		return xstrdup(env);
> >  	}
> >  
> >  	return system_path(GIT_EXEC_PATH);
> > @@ -99,10 +99,13 @@ void setup_path(void)
> >  {
> >  	const char *old_path = getenv("PATH");
> >  	struct strbuf new_path = STRBUF_INIT;
> > +	char *exec_path = (char *) git_exec_path();
> >  
> > -	add_path(&new_path, git_exec_path());
> > +	add_path(&new_path, exec_path);
> >  	add_path(&new_path, argv0_path);
> >  
> > +	free(exec_path);
> > +
> >  	if (old_path)
> >  		strbuf_addstr(&new_path, old_path);
> >  	else

> > I tend to think they are Ok, but then I also tend to think one leak of
> > exec-path return value in setup_path() is perfectly fine for the same
> > reason, so in that sense I don't see a point in this patch...
> 
>  Which brings us to the matter of whether we actually care about memory
> leaks, as the processes are short-lived and the system is going to clean
> up after us. Do we, unless the leaks are huge? As there is built-in
> valgrind support in the test suite, I went in with the assumption that
> we did. It seems however that hardly any code paths free their memory,
> other than when using strbuf.
> 
>  In case we don't, valgrind should be told not to bother reporting leaks
> (and maybe mention in some document that small leaks are not an issue).

> diff --git a/t/README b/t/README
> index 78c9e65..2a73fc3 100644
> --- a/t/README
> +++ b/t/README
> @@ -98,6 +98,13 @@ appropriately before running "make".
>  	not see any output, this option implies --verbose.  For
>  	convenience, it also implies --tee.
>  
> +	*NOTE*: As the git process is short-lived and some errors are
> +	not interesting, valgrind is run with (among others) the
> +	option --leak-check=no. In order to run a single command under
> +	the same conditions manually, you should set GIT_VALGRIND to
> +	point to the 't/valgrind/' directory and use the commands
> +	under 't/valgrind/bin/'.

> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> >  As Jeff pointed out, the test suite does use --leak-check=no. I was
> > using valgrind manually as I was chasing a different error that does
> > show up. How about adding this to the README?
> 
> > diff --git a/t/README b/t/README
> > index 78c9e65..2a73fc3 100644
> > --- a/t/README
> > +++ b/t/README
> > @@ -98,6 +98,13 @@ appropriately before running "make".
> >  	not see any output, this option implies --verbose.  For
> >  	convenience, it also implies --tee.
> >  
> > +	*NOTE*: As the git process is short-lived and some errors are
> > +	not interesting, valgrind is run with (among others) the
> > +	option --leak-check=no. In order to run a single command under
> > +	the same conditions manually, you should set GIT_VALGRIND to
> > +	point to the 't/valgrind/' directory and use the commands
> > +	under 't/valgrind/bin/'.
> 
> I think what the text says is a good addition.  If I were writing this
> myself, I would rephrase "*NOTE*:" to say "Note that ...", though, as that
> seems to be more in line with the other parts of the document.

> +	free(dir);
> +	free(repo);
> +	free(path);
>  	strbuf_release(&reflog_msg);
>  	strbuf_release(&branch_top);
>  	strbuf_release(&key);