Re: [PATCH 0/4] Test name-rev with small stack

From: Michael J Gruber <git@grubix.eu>
Date: Sep 8, 2017, 12:33 UTC
Message-ID: <9b3275a2-7b47-9ed8-6f1f-dac999c3b46a@grubix.eu>
In-Reply-To: <20170907145423.wz3iqxxz2yvxq5lm@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 07.09.2017 16:54:

Show 22 quoted lines

> On Thu, Sep 07, 2017 at 04:02:19PM +0200, Michael J Gruber wrote:
> 
>> name-rev segfaults for me in emacs.git with the typical 8102 stack size.
>> The reason is the recursive walk that name-rev uses.
>>
>> This series adds a test to mark this as known failure, after some
>> clean-ups.
> 
> These all look reasonable to me. The size of the test case in the final
> one is presumably arbitrary and just copied from t7004. I don't know if
> it's worth trying to shrink it. It could shorten a rather expensive
> test. OTOH, if we shorten it too much then we might get a false pass
> (e.g., if the algorithm remains recursive but has a smaller stack
> footprint).
> 
>> Michael J Gruber (4):
>>   t7004: move limited stack prereq to test-lib
>>   t6120: test name-rev --all and --stdin
>>   t6120: clean up state after breaking repo
>>   t6120: test describe and name-rev with deep repos
> 
> Now comes the hard part: rewriting the C code. :)

Looking at it more closely, the solution in cbc60b6720 ("git tag --contains: avoid stack overflow", 2014-04-24) seems to be a bit "ad hoc" to me:

First of all, there is more than "tag --contains" that may exceed the stack by recursion. So I would expect the solution to be more general, and not localised and specialised to builtin/tag.c

Second, this is a walk, so I'm wondering whether our revision walk machinery should be the place to add missing functionality (if any). That way, everything would benefit from possible or existing improvements there. For example, I think some of our "extra walkers" don't heed object replacements. (I need to test more.)

Michael