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

2014-04-24 Thread Stepan Kasal
Thanks for all suggestions and explanations. The diff against PATCH v2 is below, PATCH v3 follows. Have a nice day, Stepan Subject: [PATCH] fixup! git tag --contains : avoid stack overflow --- t/t7004-tag.sh | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git

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

2014-04-23 Thread Stepan Kasal
From: Jean-Jacques Lafay jeanjacques.la...@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

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

2014-04-23 Thread Johannes Schindelin
Hi, On Wed, 23 Apr 2014, Stepan Kasal wrote: I have found out that ulimit -s does not work on Windows. Adding this as a prerequisite, we will skip the test there. The interdiff can be seen here: https://github.com/msysgit/git/commit/c68e27d5 Ciao, Johannes -- To unsubscribe from

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

2014-04-23 Thread Stepan Kasal
Hello, On Wed, Apr 23, 2014 at 04:28:39PM +0200, Johannes Schindelin wrote: The interdiff can be seen here: https://github.com/msysgit/git/commit/c68e27d5 not exatly, is also changes the number of commits in the deep repo from 1000 to 4000, as peff proposed. Stepan -- To unsubscribe

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

2014-04-23 Thread Jeff King
On Wed, Apr 23, 2014 at 12:12:14PM -0700, Junio C Hamano wrote: +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.

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

2014-04-23 Thread Jeff King
On Wed, Apr 23, 2014 at 09:53:25AM +0200, Stepan Kasal wrote: I have found out that ulimit -s does not work on Windows. Adding this as a prerequisite, we will skip the test there. I found this bit weird, as the test originated on Windows. Did it never actually cause a failure there (i.e., the

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

2014-04-23 Thread Stepan Kasal
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

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

2014-04-23 Thread Junio C Hamano
Jeff King p...@peff.net writes: On Wed, Apr 23, 2014 at 12:12:14PM -0700, Junio C Hamano wrote: +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

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

2014-04-23 Thread Jeff King
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

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

2014-04-23 Thread Junio C Hamano
Jeff King p...@peff.net writes: 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,

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

2014-04-23 Thread Johannes Schindelin
Hi Peff, On Wed, 23 Apr 2014, Jeff King wrote: On Wed, Apr 23, 2014 at 09:53:25AM +0200, Stepan Kasal wrote: I have found out that ulimit -s does not work on Windows. Adding this as a prerequisite, we will skip the test there. I found this bit weird, as the test originated on Windows.