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

Stepan Kasal [off-list ref] 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].

From: Jean-Jacques Lafay <redacted>

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 <redacted>
Signed-off-by: Johannes Schindelin <redacted>
Tested-by: Stepan Kasal <redacted>

Thanks.

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?

+# 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 [off-list ref] $((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
	'

On Wed, Apr 23, 2014 at 12:12:14PM -0700, Junio C Hamano wrote:

quoted

+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?

I don't think so. The point is that we _must_ use bash here, not any
POSIX shell. So my $SHELL_PATH is /bin/sh, which is dash, and would not
run the test.

We want to run "some bash" if we can. We may pick a bash on the user's
PATH that is not what they put into $SHELL_PATH, but that should be
relatively rare. And the consequence is that either that bash works fine
and we run the test, or it does not, and we skip the test.

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"

That's a much more direct test. I like it (aside from the $SHELL_PATH
thing as described above).

-Peff

Hi,

On Wed, Apr 23, 2014 at 12:12:14PM -0700, Junio C Hamano wrote:

[Administrivia: please refrain from using Mail-Followup-To to
deflect an attempt to directly respond to you;

thanks a lot for telling me.
Actually, this was a mistake: I added git to the list of discussion
lists, without realizing the consequences.
I'm glad to have separate copies of "my threads" that do not fall
to the git-list folder.

quoted

+>expect

Move this inside test_expect_success?

Of course.  Had this in mind, then forgot.

So this runs a separate "bash", [...]
run "git tag" in the original shell?

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

Point taken.  I admire how nicely you explained that!

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

Elegant. But I agree with Peff that we shall run "a bash" instead.

I'll mail an updated patch tomorrow.

Stepan