[PATCH] test-bzr: Do not use unportable sed "\+"

[PATCH] test-bzr: Do not use unportable sed "\+"

[Torsten Bögershausen]

Using sed -e '/[0-9]\+//' to find "one ore more" digits
is not portable.
Use the Basic Regular Expression '/[0-9][0-9]*//' instead

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 contrib/remote-helpers/test-bzr.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
index d9c32f4..5dfa070 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -328,7 +328,7 @@ test_expect_success 'strip' '
 
   echo four >> content &&
   bzr commit -m four &&
-  bzr log --line | sed -e "s/^[0-9]\+: //" > ../expected
+  bzr log --line | sed -e "s/^[0-9][0-9]*: //" > ../expected
   ) &&
 
   (cd gitrepo &&
-- 
1.8.2.1.614.g66d7af5

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Using sed -e '/[0-9]\+//' to find "one ore more" digits
> is not portable.
> Use the Basic Regular Expression '/[0-9][0-9]*//' instead
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
the tags?

> ---
>  contrib/remote-helpers/test-bzr.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
> index d9c32f4..5dfa070 100755
> --- a/contrib/remote-helpers/test-bzr.sh
> +++ b/contrib/remote-helpers/test-bzr.sh
> @@ -328,7 +328,7 @@ test_expect_success 'strip' '
>  
>    echo four >> content &&
>    bzr commit -m four &&
> -  bzr log --line | sed -e "s/^[0-9]\+: //" > ../expected
> +  bzr log --line | sed -e "s/^[0-9][0-9]*: //" > ../expected
>    ) &&
>  
>    (cd gitrepo &&

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
> the tags?

I think your sed will see the same breakage for the one in 5551 (my
sed is unfortunately GNU and ".\+" does not break it).  Could you
test this patch with:

     GIT_TEST_LONG=YesPlease GIT_TEST_HTTPD=YesPlease \
     ./t5551-http-fetch.sh

Thanks.

 t/t5551-http-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index b23efbb..4a3184e 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -209,7 +209,7 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 
 	# now assign tags to all the dangling commits we created above
 	tag=$("$PERL_PATH" -e "print \"bla\" x 30") &&
-	sed -e "s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/" <marks >>packed-refs
+	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
 	)
 '
 

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Torsten Bögershausen]

On 11.05.13 21:45, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Thanks.  Is there another one in t/t5551-http-fetch.sh that checks
>> the tags?
> 
> I think your sed will see the same breakage for the one in 5551 (my
> sed is unfortunately GNU and ".\+" does not break it).  Could you
> test this patch with:
> 
>      GIT_TEST_LONG=YesPlease GIT_TEST_HTTPD=YesPlease \
>      ./t5551-http-fetch.sh
> 
> Thanks.
> 
>  t/t5551-http-fetch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
> index b23efbb..4a3184e 100755
> --- a/t/t5551-http-fetch.sh
> +++ b/t/t5551-http-fetch.sh
> @@ -209,7 +209,7 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
>  
>  	# now assign tags to all the dangling commits we created above
>  	tag=$("$PERL_PATH" -e "print \"bla\" x 30") &&
> -	sed -e "s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/" <marks >>packed-refs
> +	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
>  	)

I did,
the interesting thing is that the test passes with and without your patch.
(After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)


Side note:
I added this line in in t/check-non-portable-shell.pl
  /sed[^"]+"[^"]+\\([+])/ and err "sed \\$1 is not portable)";

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> I did,
> the interesting thing is that the test passes with and without your patch.
> (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)

Strange.  Do you see differences between the produced packed-refs
file?

RE: [PATCH] test-bzr: Do not use unportable sed "\+"

[Torsten Bögershausen]

On 05/11/2013 03:25 PM, Torsten Bögershausen wrote:
Sorry, I forgot to mention that there is another test case that fails
in test-bzr.sh (Both Linux and MacOS):

Cloning into 'gitrepo'...
--- ../expected 2013-05-11 20:07:17.678360248 +0000
+++ actual      2013-05-11 20:07:21.510312073 +0000
@@ -1,3 +1,3 @@
-origin/HEAD
+origin
  origin/branch
  origin/trunk
not ok 11 - proper bzr repo

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Torsten Bögershausen]

On 11.05.13 22:09, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> I did,
>> the interesting thing is that the test passes with and without your patch.
>> (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)
> 
> Strange.  Do you see differences between the produced packed-refs
> file?

The original version seems to look like this:
:1 666527db455708922859283c673094002092910b
:2 1e2acf73c6db881cfb1d56d67662e3d9260be2cf
[snip]

The "fixed POSIX version" follows that style:
666527db455708922859283c673094002092910b refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-1
1e2acf73c6db881cfb1d56d67662e3d9260be2cf refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-2
[snip]

(Just to verify the changes:)
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index b23efbb..68965f0 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -210,8 +210,11 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
        # now assign tags to all the dangling commits we created above
        tag=$("$PERL_PATH" -e "print \"bla\" x 30") &&
        sed -e "s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/" <marks >>packed-refs
+       sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >packed-refs.junio
+       sed -e "s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/" <marks >packed-refs.orig
        )
 '
+       exit 0

Re: [PATCH] test-bzr: Do not use unportable sed "\+"

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 11.05.13 22:09, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>>> I did,
>>> the interesting thing is that the test passes with and without your patch.
>>> (After enabling  GIT_TEST_LONG and GIT_TEST_HTTPD in both cases)
>> 
>> Strange.  Do you see differences between the produced packed-refs
>> file?
>
> The original version seems to look like this:
> :1 666527db455708922859283c673094002092910b
> :2 1e2acf73c6db881cfb1d56d67662e3d9260be2cf
> [snip]
>
> The "fixed POSIX version" follows that style:
> 666527db455708922859283c673094002092910b refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-1
> 1e2acf73c6db881cfb1d56d67662e3d9260be2cf refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-2
> [snip]

That means whatever the test that comes after that set-up step to
populate the packed-refs file does not need the packed-refs file.

In fact, the test only sees if "clone" succeeds, without checking
what refs are present in the resulting repository.  The original
repository lacks these 50000 tags due to the non-portable sed
construct (it has a corrupt packed-refs file) and it may or may not
have other refs, but because cloning an empty repository does not
error out these days, the test just passes.

Sloppy.

[PATCH] t5551: do not use unportable sed '\+'

[Junio C Hamano]

The set-up step to prepare a repository with 50000 tags used a
non-porable '\+' to match one-or-more.

The error was not caught because the next test that uses that
repository did not even bother to check if these expected tags were
actually cloned to the resulting repository.

Fix the sed construct to use BRE and update the "clone" test that
wanted to test cloning from such a repository with many refs to
check the resulting repository.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 47eb769..fe6637b 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -184,13 +184,17 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 
 	# now assign tags to all the dangling commits we created above
 	tag=$("$PERL_PATH" -e "print \"bla\" x 30") &&
-	sed -e "s/^:\(.\+\) \(.\+\)$/\2 refs\/tags\/$tag-\1/" <marks >>packed-refs
+	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
 	)
 '
 
 test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command line overflow' '
 	git clone $HTTPD_URL/smart/repo.git too-many-refs 2>err &&
-	test_line_count = 0 err
+	test_line_count = 0 err &&
+	(
+		cd too-many-refs &&
+		test $(git for-each-ref refs/tags | wc -l) = 50000
+	)
 '
 
 stop_httpd
-- 
1.8.3-rc1-278-g19c008b

Re: [PATCH] t5551: do not use unportable sed '\+'

[Torsten Bögershausen]

On 2013-05-13 00.50, Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t5551-http-fetch.sh | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
Thanks, works like a charm, tested on Mac OS.
/torsten

Re: [PATCH] t5551: do not use unportable sed '\+'

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2013-05-13 00.50, Junio C Hamano wrote:
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  t/t5551-http-fetch.sh | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
> Thanks, works like a charm, tested on Mac OS.
> /torsten

Thanks; I take it that you reverted the "sed fix" part and saw the
updated "clone" check fail with the platform "sed"?

Re: [PATCH] t5551: do not use unportable sed '\+'

[Torsten Bögershausen]

On 2013-05-13 18.30, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> On 2013-05-13 00.50, Junio C Hamano wrote:
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>  t/t5551-http-fetch.sh | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>> Thanks, works like a charm, tested on Mac OS.
>> /torsten
> 
> Thanks; I take it that you reverted the "sed fix" part and saw the
> updated "clone" check fail with the platform "sed"?
In my first test with the fix the test case passed.
Then the sed expression was only manipulated to verify that the TC failes.
And now I took even the original expression and have verfied it is failing.

The short version: Yes

Re: [PATCH] t5551: do not use unportable sed '\+'

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2013-05-13 18.30, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>>> On 2013-05-13 00.50, Junio C Hamano wrote:
>>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>>> ---
>>>>  t/t5551-http-fetch.sh | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>> Thanks, works like a charm, tested on Mac OS.
>>> /torsten
>> 
>> Thanks; I take it that you reverted the "sed fix" part and saw the
>> updated "clone" check fail with the platform "sed"?
> In my first test with the fix the test case passed.
> Then the sed expression was only manipulated to verify that the TC failes.
> And now I took even the original expression and have verfied it is failing.
>
> The short version: Yes

Thanks.