[PATCH] Fix sed syntax in t7502-commit for OSX

[PATCH] Fix sed syntax in t7502-commit for OSX

[Marcel Koeppen]

The OSX version of sed interprets the command as argument to the -i option.

Signed-off-by: Marcel Koeppen <git-dev@marzelpan.de>
---
 t/t7502-commit.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 018060c..9a43104 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -166,7 +166,7 @@ test_expect_success 'author different from committer' '
 	test_cmp expect actual
 '
 
-sed -i '$d' expect
+sed -i -e '$d' expect
 echo "# Committer:
 #" >> expect
 unset GIT_COMMITTER_EMAIL
-- 
1.5.5.1.215.g64c0d

Re: [PATCH] Fix sed syntax in t7502-commit for OSX

[Junio C Hamano]

Marcel Koeppen <git-dev@marzelpan.de> writes:

> The OSX version of sed interprets the command as argument to the -i option.
>
> Signed-off-by: Marcel Koeppen <git-dev@marzelpan.de>
> ---
>  t/t7502-commit.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 018060c..9a43104 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -166,7 +166,7 @@ test_expect_success 'author different from committer' '
>  	test_cmp expect actual
>  '
>  
> -sed -i '$d' expect
> +sed -i -e '$d' expect
>  echo "# Committer:
>  #" >> expect
>  unset GIT_COMMITTER_EMAIL

Two comments and a half.

This patch does not make things worse, so I do not have much against it.

However, "sed -i $cmd" is also used in templates/hooks--prepare-commit-msg
and you would want to fix it as well.

But more importantly, as we are serious about portability, we should not
be using "sed -i" which is not even in POSIX to begin with.  Can we
rewrite these places without using the in-place extension?

Re: [PATCH] Fix sed syntax in t7502-commit for OSX

[Holger Schurig]

> > -sed -i '$d' expect
> > +sed -i -e '$d' expect

perl -pi -e 's/foo/bar/' ???

GIT already needs perl, so we can re-use it's features savely.

Re: [PATCH] Fix sed syntax in t7502-commit for OSX

[Marcel Koeppen]

Am 15.05.2008 um 10:58 schrieb Holger Schurig:

>>> -sed -i '$d' expect
>>> +sed -i -e '$d' expect
>
> perl -pi -e 's/foo/bar/' ???

This does not work with '$d', so we could use sed with a temp instead  
of the -i extension. Alternatively we could specify the full test  
fixture instead of editing an old one. This would make the expected  
results more obvious and the tests would work without assumptions  
about running order.

Marcel

Re: [PATCH] Fix sed syntax in t7502-commit for OSX

[Santi Béjar]

On Thu, May 15, 2008 at 10:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> But more importantly, as we are serious about portability, we should not
> be using "sed -i" which is not even in POSIX to begin with.

Is there a way (environment variable, flag) to test if it is POSIX? Or
we just have to know?

I'll send a patch to document the "sed -i" issue in
Documentation/CodingGuidelines.

>  Can we
> rewrite these places without using the in-place extension?

Yes, I'll do it soon if nobody beats me.

Santi

[PATCH] Replace in-place sed in t7502-commit

[Marcel Koeppen]

The in-place mode of sed used in t7502-commit is a non-POSIX extension.
That call of sed is replaced by a more portable version using a temporary file.

Signed-off-by: Marcel Koeppen <git-dev@marzelpan.de>
---
 t/t7502-commit.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 018060c..3531a99 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -166,7 +166,9 @@ test_expect_success 'author different from committer' '
 	test_cmp expect actual
 '
 
-sed -i '$d' expect
+mv expect expect.tmp
+sed '$d' < expect.tmp > expect
+rm -f expect.tmp
 echo "# Committer:
 #" >> expect
 unset GIT_COMMITTER_EMAIL
-- 
1.5.5.1.215.g64c0d

Re: [PATCH] Replace in-place sed in t7502-commit

[Jeff King]

On Fri, May 16, 2008 at 02:21:43AM +0200, Marcel Koeppen wrote:

> The in-place mode of sed used in t7502-commit is a non-POSIX extension.
> That call of sed is replaced by a more portable version using a
> temporary file.

Thanks, this fixes a similar issue on FreeBSD (which is probably the
same sed).

-Peff