Re: [PATCH] git tag --contains : avoid stack overflow

> On Wed, Apr 16, 2014 at 04:15:19PM +0200, Stepan Kasal wrote:
> 
> > From: Jean-Jacques Lafay at Sat, 10 Nov 2012 18:36:10 +0100
> > 
> > In large repos, the recursion implementation of contains(commit,
> > commit_list) may result in a stack overflow. Replace the recursion
> > with a loop to fix it.
> > 
> > This problem is more apparent on Windows than on Linux, where the
> > stack is more limited by default.
> 
> I think this is a good thing to be doing, and it looks mostly good to
> me. A few comments:
> 
> > -static int contains_recurse(struct commit *candidate,
> > +/*
> > + * Test whether the candidate or one of its parents is contained in the list.
> > + * Do not recurse to find out, though, but return -1 if inconclusive.
> > + */
> > +static int contains_test(struct commit *candidate,
> >  			    const struct commit_list *want)
> 
> Can we turn this return value into
> 
>   enum {
> 	CONTAINS_UNKNOWN = -1,
> 	CONTAINS_NO = 0,
> 	CONTAINS_YES = 1,
>   } contains_result;
> 
> to make the code a little more self-documenting?

> [... detailed instructions what changes are implied by the enum ...]
> 
> > +>expect
> > +# ulimit is a bash builtin; we can rely on that in MinGW, but nowhere else
> > +test_expect_success MINGW '--contains works in a deep repo' '
> > +	ulimit -s 64
> 
> It would be nice to test this on Linux.
> 
> Can we do something like:
> 
>   test_lazy_prereq BASH 'bash --version'
> 
>   test_expect_success BASH '--contains works in a deep repo' '
> 	... setup repo ...
> 	bash -c "ulimit -s 64 && git tag --contains HEAD" >actual &&
> 	test_cmp expect actual
>   '
> 
> As a bonus, then our "ulimit" call does not pollute the environment of
> subsequent tests.