Re: leaky cherry-pick

From: Jeff King <peff@peff.net>
Date: Jan 10, 2012, 19:50 UTC
Message-ID: <20120110195017.GA19961@sigill.intra.peff.net>
In-Reply-To: <CALkWK0nwE0c6qVvbauPrjmb3NX4NDeGSrvrC2ry2bjMeM4Hr0A@mail.gmail.com>

On Tue, Jan 10, 2012 at 10:58:45AM +0530, Ramkumar Ramachandra wrote:

Show 24 quoted lines

> Interesting- I wonder where .gitattributes parsing code fits into all
> this.  The purpose of bootstrap _attr_stack() is to populate
> attr_stack for its callers.  Lots of memory allocation happening in
> handle_attr_line() -- that information is returned to
> bootstrap_attr_stack().  We have to keep backtracking until that
> information is provably useless and free it.  Hm, convert_attrs() (in
> convert.c) is a common caller in both cases, but the populated
> attr_stack is local to attr.c; I wonder if this is the problem.  If my
> hunch is right, it should be a trivial fix (caution: untested):
> 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> diff --git a/attr.c b/attr.c
> index 76b079f..12e3824 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -745,6 +745,7 @@ int git_check_attr(const char *path, int num,
> struct git_attr_check *check)
>  		check[i].value = value;
>  	}
> 
> +	drop_attr_stack();
>  	return 0;
>  }

I don't think this is right. The attr_stack is intentionally kept in place after a lookup as a cache, because callers are very likely to lookup nearby filenames. The first thing we do is pop unrelated parts of the stack, keep the early bits, and then push any new needed directories.

So if you do a lookup for "foo/bar/baz/file1", the stack afterwards would be:

  $GIT_DIR/info/attributes
  foo/bar/baz/.gitattributes
  foo/bar/.gitattributes
  foo/.gitattributes
  .gitattributes
  [builtins]

If you then do a lookup for "foo/bar/baz/file2", it can use the exact same stack without looking for or reparsing the attribute files. If you then do a lookup for "foo/bar/bleep/file", it pops only the entry for "foo/bar/baz/.gitattributes", and pushes only the entry for "foo/bar/bleep/.gitattributes".

The calling code _could_ say "btw, I am done with attributes now, so free the memory". But we don't bother, since it's a small amount of memory, and other parts of the code may want it later.

-Peff