Hi Peff,
On Wed, 16 Apr 2014, Jeff King wrote:
> 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?
Good idea!
> [... 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.
That's a very good idea! We mulled it over a bit and did not come up with
this excellent solution.
Please see https://github.com/msysgit/git/c63d196 for the fixup, and
https://github.com/msysgit/git/compare/tag-contains%5E...tag-contains for
the updated patch.
Thanks,
Dscho
--
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
