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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2014, 21:05 UTC
Message-ID: <xmqqfvl3ycwk.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140423205533.GA20582@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Wed, Apr 23, 2014 at 01:48:05PM -0700, Junio C Hamano wrote:
>
>> > I don't think so. The point is that we _must_ use bash here, not any
>> > POSIX shell.
>> 
>> Sorry, but I do not understand.  Isn't what you want "any POSIX
>> shell with 'ulimit -s 64' supported"?
>
> Sure, that would be fine, but the original patch which started this
> thread claimed that bash was required. I had assumed that to be true,
> but it seems like it's not:
>
>>     $ dash -c 'ulimit -s && ulimit -s 64 && ulimit -s'
>>     8192
>>     64
>
> If we are just using the same shell we are already running, then why
> invoke it by name in the first place? IOW, why not:
>
>   run_with_limited_stack () {
> 	(ulimit -s 64 && "$@")
>   }

That is certainly more preferrable than an explicit "run this piece with $SHELL_PATH".

I think the choice between "Any bash that is on user's PATH" vs "The shell the user told us to use when working with Git" is a trade-off between

 - those who choose a shell that does not support "ulimit -s" to
   work with Git (which is fine, because our scripted Porcelains
   would not have any need for that); for these people, this test
   would be skipped unnecessarily if we insist on SHELL_PATH; and

 - those who run on a box without any bash on their PATH, chose a
   shell that is not bash but does support "ulimit -s" as their
   SHELL_PATH to build Git with; for these people, this test would
   be skipped unnecessarily if we insist on "bash".

and I do not think of a good reason to favor one over the other.

If I have to pick, I'd take your "don't name any shell, and let the current one run it" approach, solely for the simplicity of the solution (it ends up favoring the latter class of people as a side-effect, though).