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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2014, 19:12 UTC
Message-ID: <xmqqeh0nzwq9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140423075325.GA7268@camelia.ucw.cz>

Stepan Kasal <kasal@ucw.cz> writes:

[Administrivia: please refrain from using Mail-Followup-To to deflect an attempt to directly respond to you; it will waste time of other people while it may be saving your time].

Show 23 quoted lines

> From: Jean-Jacques Lafay <jeanjacques.lafay@gmail.com>
>
> 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.
>
> See also this thread on the msysGit list:
>
> 	https://groups.google.com/d/topic/msysgit/FqT6boJrb2g/discussion
>
> [jes: re-written to imitate the original recursion more closely]
>
> Thomas Braun pointed out several documentation shortcomings.
>
> Tests are run only if ulimit -s is available.  This means they cannot
> be run on Windows.
>
> Signed-off-by: Jean-Jacques Lafay <jeanjacques.lafay@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Tested-by: Stepan Kasal <kasal@ucw.cz>

Thanks.

Show 10 quoted lines

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 143a8ea..db82f6d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1423,4 +1423,27 @@ EOF
>  	test_cmp expect actual
>  '
>  
> +ulimit_stack="ulimit -s 64"
> +test_lazy_prereq ULIMIT 'bash -c "'"$ulimit_stack"'"'

With this implementaion, ULIMIT implies bash, and we use bash that appears on user's PATH that may not be the one the user chose to run git with. Can't we fix both of them by using $SHELL_PATH?

> +>expect

Move this inside test_expect_success?

Show 16 quoted lines

> +# we require bash and ulimit, this excludes Windows
> +test_expect_success ULIMIT '--contains works in a deep repo' '
> +	i=1 &&
> +	while test $i -lt 4000
> +	do
> +		echo "commit refs/heads/master
> +committer A U Thor <author@example.com> $((1000000000 + $i * 100)) +0200
> +data <<EOF
> +commit #$i
> +EOF"
> +		test $i = 1 && echo "from refs/heads/master^0"
> +		i=$(($i + 1))
> +	done | git fast-import &&
> +	git checkout master &&
> +	git tag far-far-away HEAD^ &&
> +	bash -c "'"$ulimit_stack"' && git tag --contains HEAD >actual" &&

So this runs a separate "bash", the only thing which does is to run a small script that gives a small stack to itself and exit, and then run "git tag" in the original shell?

Ahh, no, I am mis-pairing the quotes.

How about doing it along this line instead?

	run_with_limited_stack () {
		"$SHELL_PATH" -c "ulimit -s 64 && $*"
	}

	test_lazy_prereq ULIMIT "run_with_limited_stack true"

	test_expect_success ULIMIT '...' '
        	>expect &&
		i=1 &&
                ...
                done | git-fast-import &&
                git tag far-far-away HEAD^ &&
                run_with_limited_stack "git tag --contains HEAD" >actual &&
		test_cmp expect actual
	'