Re: leaky cherry-pick

[Jeff King <peff@peff.net>]

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

> 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