[PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Ben Walton]

The space following the last / in a sed command caused Solaris'
xpg4/sed to fail, claiming the program was garbled and exit with
status 2:

% echo 'foo' | /usr/xpg4/bin/sed -e 's/foo/bar/ '
sed: command garbled: s/foo/bar/
% echo $?
2

Fix this by simply removing the unnecessary space.

Additionally, in 99094a7a, a trivial && breakage was fixed. This
exposed a problem with the test when run on Solaris with xpg4/sed that
had gone silently undetected since its introduction in
e4bd10b2. Solaris' sed executes the requested substitution but prints
a warning about the missing newline at the end of the file and exits
with status 2.

% echo "CHANGE_ME" | \
tr -d "\\012" | /usr/xpg4/bin/sed -e 's/CHANGE_ME/change_me/'
sed: Missing newline at end of file standard input.
change_me
% echo $?
2

To work around this, use perl to execute the substitution instead. By
using inplace replacement, we can subsequently drop the mv command.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
 t/t5601-clone.sh                       |    2 +-
 t/t9500-gitweb-standalone-no-errors.sh |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index fa6be3c..2583f84 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -445,7 +445,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
 #IPv6
 for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::1]:
 do
-	ehost=$(echo $tuah | sed -e "s/1]:/1]/ " | tr -d "\133\135")
+	ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "\133\135")
 	test_expect_success "clone ssh://$tuah/home/user/repo" "
 	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
 	"
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index e94b2f1..eb264f9 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -290,8 +290,7 @@ test_expect_success 'setup incomplete lines' '
 	echo "incomplete" | tr -d "\\012" >>file &&
 	git commit -a -m "Add incomplete line" &&
 	git tag incomplete_lines_add &&
-	sed -e s/CHANGE_ME/change_me/ <file >file+ &&
-	mv -f file+ file &&
+	perl -pi -e "s/CHANGE_ME/change_me/" file &&
 	git commit -a -m "Incomplete context line" &&
 	git tag incomplete_lines_ctx &&
 	echo "Dominus regit me," >file &&
-- 
1.7.10.4

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Eric Sunshine]

On Sat, Jul 18, 2015 at 11:21 AM, Ben Walton <bdwalton@gmail.com> wrote:
> The space following the last / in a sed command caused Solaris'
> xpg4/sed to fail, claiming the program was garbled and exit with
> status 2:
>
> % echo 'foo' | /usr/xpg4/bin/sed -e 's/foo/bar/ '
> sed: command garbled: s/foo/bar/
> % echo $?
> 2
>
> Fix this by simply removing the unnecessary space.
>
> Additionally, in 99094a7a, a trivial && breakage was fixed. This
> exposed a problem with the test when run on Solaris with xpg4/sed that
> had gone silently undetected since its introduction in
> e4bd10b2. Solaris' sed executes the requested substitution but prints
> a warning about the missing newline at the end of the file and exits
> with status 2.
>
> % echo "CHANGE_ME" | \
> tr -d "\\012" | /usr/xpg4/bin/sed -e 's/CHANGE_ME/change_me/'
> sed: Missing newline at end of file standard input.
> change_me
> % echo $?
> 2
>
> To work around this, use perl to execute the substitution instead. By
> using inplace replacement, we can subsequently drop the mv command.

This two unrelated fixes could be separate patches...

> Signed-off-by: Ben Walton <bdwalton@gmail.com>
> ---
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index fa6be3c..2583f84 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -445,7 +445,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
>  #IPv6
>  for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::1]:
>  do
> -       ehost=$(echo $tuah | sed -e "s/1]:/1]/ " | tr -d "\133\135")
> +       ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "\133\135")
>         test_expect_success "clone ssh://$tuah/home/user/repo" "
>           test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
>         "
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index e94b2f1..eb264f9 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -290,8 +290,7 @@ test_expect_success 'setup incomplete lines' '
>         echo "incomplete" | tr -d "\\012" >>file &&
>         git commit -a -m "Add incomplete line" &&
>         git tag incomplete_lines_add &&
> -       sed -e s/CHANGE_ME/change_me/ <file >file+ &&
> -       mv -f file+ file &&
> +       perl -pi -e "s/CHANGE_ME/change_me/" file &&
>         git commit -a -m "Incomplete context line" &&
>         git tag incomplete_lines_ctx &&
>         echo "Dominus regit me," >file &&
> --
> 1.7.10.4

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Sixt]

Am 18.07.2015 um 17:21 schrieb Ben Walton:
> The space following the last / in a sed command caused Solaris'
> xpg4/sed to fail, claiming the program was garbled and exit with
> status 2:
>
> % echo 'foo' | /usr/xpg4/bin/sed -e 's/foo/bar/ '
> sed: command garbled: s/foo/bar/
> % echo $?
> 2
>
> Fix this by simply removing the unnecessary space.
>
> Additionally, in 99094a7a, a trivial && breakage was fixed. This
> exposed a problem with the test when run on Solaris with xpg4/sed that
> had gone silently undetected since its introduction in
> e4bd10b2. Solaris' sed executes the requested substitution but prints
> a warning about the missing newline at the end of the file and exits
> with status 2.
>
> % echo "CHANGE_ME" | \
> tr -d "\\012" | /usr/xpg4/bin/sed -e 's/CHANGE_ME/change_me/'
> sed: Missing newline at end of file standard input.
> change_me
> % echo $?
> 2
>
> To work around this, use perl to execute the substitution instead. By
> using inplace replacement, we can subsequently drop the mv command.
>
> Signed-off-by: Ben Walton <bdwalton@gmail.com>
> ---
>   t/t5601-clone.sh                       |    2 +-
>   t/t9500-gitweb-standalone-no-errors.sh |    3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index fa6be3c..2583f84 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -445,7 +445,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
>   #IPv6
>   for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::1]:
>   do
> -	ehost=$(echo $tuah | sed -e "s/1]:/1]/ " | tr -d "\133\135")
> +	ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "\133\135")

Can this not be rewritten as

	ehost=$(echo $tuah | sed -e "s/1]:/1]/" -e "s/[][]//g")

But I admit that it looks like black magic without a comment...

>   	test_expect_success "clone ssh://$tuah/home/user/repo" "
>   	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
>   	"
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index e94b2f1..eb264f9 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -290,8 +290,7 @@ test_expect_success 'setup incomplete lines' '
>   	echo "incomplete" | tr -d "\\012" >>file &&
>   	git commit -a -m "Add incomplete line" &&
>   	git tag incomplete_lines_add &&
> -	sed -e s/CHANGE_ME/change_me/ <file >file+ &&
> -	mv -f file+ file &&
> +	perl -pi -e "s/CHANGE_ME/change_me/" file &&

This is problematic. On Windows, perl -i fails when no backup file 
extension is specified because perl attempts to replace a file that is 
still open; that does not work on Windows. This should work, but I 
haven't tested, yet:

	perl -pi.bak -e "s/CHANGE_ME/change_me/" file &&

>   	git commit -a -m "Incomplete context line" &&
>   	git tag incomplete_lines_ctx &&
>   	echo "Dominus regit me," >file &&
>

-- Hannes

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Schindelin]

Hi,

On 2015-07-19 08:54, Johannes Sixt wrote:
> Am 18.07.2015 um 17:21 schrieb Ben Walton:
>>   	test_expect_success "clone ssh://$tuah/home/user/repo" "
>>   	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
>>   	"
>> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
>> index e94b2f1..eb264f9 100755
>> --- a/t/t9500-gitweb-standalone-no-errors.sh
>> +++ b/t/t9500-gitweb-standalone-no-errors.sh
>> @@ -290,8 +290,7 @@ test_expect_success 'setup incomplete lines' '
>>   	echo "incomplete" | tr -d "\\012" >>file &&
>>   	git commit -a -m "Add incomplete line" &&
>>   	git tag incomplete_lines_add &&
>> -	sed -e s/CHANGE_ME/change_me/ <file >file+ &&
>> -	mv -f file+ file &&
>> +	perl -pi -e "s/CHANGE_ME/change_me/" file &&
> 
> This is problematic. On Windows, perl -i fails when no backup file
> extension is specified because perl attempts to replace a file that is
> still open; that does not work on Windows.

Let's qualify this a bit better: it actually works with the SDK of Git for Windows 2.x. It is therefore incomplete and partially incorrect to say "that does not work on Windows". It is true that Git for Windows 1.x' perl bails out with "Can't do inplace edit".

> This should work, but I haven't tested, yet:
> 
> 	perl -pi.bak -e "s/CHANGE_ME/change_me/" file &&

This works, of course, but it leaves an extra file behind.

I really wonder why the previous ">file+ && mv -f file+ file" dance needs to be replaced?

Ciao,
Johannes

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Sixt]

Am 19.07.2015 um 09:37 schrieb Johannes Schindelin:
> On 2015-07-19 08:54, Johannes Sixt wrote:
>> Am 18.07.2015 um 17:21 schrieb Ben Walton:
>>> -	sed -e s/CHANGE_ME/change_me/ <file >file+ &&
>>> -	mv -f file+ file &&
>>> +	perl -pi -e "s/CHANGE_ME/change_me/" file &&
>>
>> This is problematic. On Windows, perl -i fails when no backup file
>> extension is specified because perl attempts to replace a file that is
>> still open; that does not work on Windows.
>
> Let's qualify this a bit better: it actually works with the SDK of
> Git  for Windows 2.x.

Good to know!

> I really wonder why the previous ">file+ && mv -f file+ file" dance
> needs to be replaced?

The sed must be replaced because some versions on Solaris choke on the 
incomplete last line in the file.

-- Hannes

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> I really wonder why the previous ">file+ && mv -f file+ file" dance
>> needs to be replaced?
>
> The sed must be replaced because some versions on Solaris choke on the
> incomplete last line in the file.

Switching from sed to perl is not being questioned.

I think Dscho is asking about the use of "-i", when the original
idiom ">target+ && mv -f target+ target" worked perfectly fine.

I.e.

	perl -p -e '...' file >file+ &&
        mv file+ file

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Schindelin]

Hi,

On 2015-07-20 18:07, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> I really wonder why the previous ">file+ && mv -f file+ file" dance
>>> needs to be replaced?
>>
>> The sed must be replaced because some versions on Solaris choke on the
>> incomplete last line in the file.
> 
> Switching from sed to perl is not being questioned.
> 
> I think Dscho is asking about the use of "-i", when the original
> idiom ">target+ && mv -f target+ target" worked perfectly fine.
> 
> I.e.
> 
> 	perl -p -e '...' file >file+ &&
>         mv file+ file

Thanks for translating from Dscho to English.

Ciao,
Dscho