threads / patch / 36163

patchtest-lib.sh: use printf instead of echo

Subject: [PATCH] test-lib.sh: use printf instead of echo

## tl;dr

14 messages between Mar 14, 2014 and Mar 20, 2014. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Uwe Storbeck· Mar 14, 2014, 23:57 UTC · lore
when variables may contain backslash sequences.

Backslash sequences are interpreted as control characters by the echo command of some shells (e.g. dash).

Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
---
 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/test-lib.sh +2 −2
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..8209204 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error "Test script did not set test_description."
 
 if test "$help" = "t"
 then
-	echo "$test_description"
+	printf '%s\n' "$test_description"
 	exit 0
 fi
 
@@ -328,7 +328,7 @@ test_failure_ () {
 	test_failure=$(($test_failure + 1))
 	say_color error "not ok $test_count - $1"
 	shift
-	echo "$@" | sed -e 's/^/#	/'
+	printf '%s\n' "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }
 

-- 
1.9.0
Jonathan Nieder· Mar 15, 2014, 00:18 UTC · re: Uwe Storbeck · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Uwe Storbeck wrote:
> Backslash sequences are interpreted as control characters
> by the echo command of some shells (e.g. dash).

This has bothered me for a while but never enough to do anything about it. Thanks for fixing it.

> Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
(patch left unsnipped for reference)
Show 26 quoted lines
> ---
>  t/test-lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..8209204 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -277,7 +277,7 @@ error "Test script did not set test_description."
>  
>  if test "$help" = "t"
>  then
> -	echo "$test_description"
> +	printf '%s\n' "$test_description"
>  	exit 0
>  fi
>  
> @@ -328,7 +328,7 @@ test_failure_ () {
>  	test_failure=$(($test_failure + 1))
>  	say_color error "not ok $test_count - $1"
>  	shift
> -	echo "$@" | sed -e 's/^/#	/'
> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
>  	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
>  }
>
Junio C Hamano· Mar 17, 2014, 19:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 36 quoted lines
> Uwe Storbeck wrote:
>
>> Backslash sequences are interpreted as control characters
>> by the echo command of some shells (e.g. dash).
>
> This has bothered me for a while but never enough to do anything about
> it.  Thanks for fixing it.
>
>> Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> (patch left unsnipped for reference)
>> ---
>>  t/test-lib.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 1531c24..8209204 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -277,7 +277,7 @@ error "Test script did not set test_description."
>>  
>>  if test "$help" = "t"
>>  then
>> -	echo "$test_description"
>> +	printf '%s\n' "$test_description"
>>  	exit 0
>>  fi
>>  
>> @@ -328,7 +328,7 @@ test_failure_ () {
>>  	test_failure=$(($test_failure + 1))
>>  	say_color error "not ok $test_count - $1"
>>  	shift
>> -	echo "$@" | sed -e 's/^/#	/'
>> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
This is wrong, isn't it?  Why do we want one line per item here?
>>  	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
>>  }
>>
Jonathan Nieder· Mar 18, 2014, 22:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Junio C Hamano wrote:
>> Uwe Storbeck wrote:
>>> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
>
> This is wrong, isn't it?  Why do we want one line per item here?
Yes, Hannes caught the same, too.  Sorry for the sloppiness.

We currently use "echo" all over the place (e.g., 'echo "$path"' in git-sh-setup), and every time we fix it there is a chance of making mistakes. I wonder if it would make sense to add a helper to make the echo calls easier to replace:

-- >8 --
Subject: git-sh-setup: introduce sane_echo helper

Using 'echo' with arguments that might contain backslashes or "-e" or "-n" can produce confusing results that vary from platform to platform (e.g., dash always interprets \ escape sequences and echoes "-e" verbatim, whereas bash does not interpret \ escapes unless "-e" is passed as an argument to echo and suppresses the "-e" from its output).

Instead, we should use printf, which is more predictable:
	printf '%s\n' "$foo"; # Just prints $foo, on all platforms.

Blindly replacing echo with "printf '%s\n'" would not be good enough because that printf prints each argument on its own line. Provide a sane_echo helper that prints its arguments, space-delimited, on a single line, to make this easier to remember, and tweak 'say' and 'die_with_status' to illustrate how it is used.

No functional change intended.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-sh-setup.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Show changes to diff +6 −2
diff --git i/git-sh-setup.sh w/git-sh-setup.sh
index 256c89a..f35b5b9 100644
--- i/git-sh-setup.sh
+++ w/git-sh-setup.sh
@@ -43,6 +43,10 @@ git_broken_path_fix () {
 
 # @@BROKEN_PATH_FIX@@
 
+sane_echo () {
+	printf '%s\n' "$*"
+}
+
 die () {
 	die_with_status 1 "$@"
 }
@@ -50,7 +54,7 @@ die () {
 die_with_status () {
 	status=$1
 	shift
-	printf >&2 '%s\n' "$*"
+	sane_echo >&2 "$*"
 	exit "$status"
 }
 
@@ -59,7 +63,7 @@ GIT_QUIET=
 say () {
 	if test -z "$GIT_QUIET"
 	then
-		printf '%s\n' "$*"
+		sane_echo "$*"
 	fi
 }
Junio C Hamano· Mar 19, 2014, 17:17 UTC · re: Jonathan Nieder · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Junio C Hamano wrote:
>>> Uwe Storbeck wrote:
>
>>>> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
>>
>> This is wrong, isn't it?  Why do we want one line per item here?
>
> Yes, Hannes caught the same, too.  Sorry for the sloppiness.
>
> We currently use "echo" all over the place (e.g., 'echo "$path"' in
> git-sh-setup), and every time we fix it there is a chance of making
> mistakes.  I wonder if it would make sense to add a helper to make the
> echo calls easier to replace:

I agree that we would benefit from having a helper to print a single line, which we very often do, without having to worry about the boilerplate '%s\n' of printf or the portability gotcha of echo.

I am a bit reluctant to name the helper "sane_echo" to declare "echo that interprets backslashes in the string is insane", though. For these "print a single line" uses, we are only interested in using a subset of the features offered by 'echo', but that does not mean the other features we do not want to trigger in our use is of no use to any sane person. It very different from "sane_unset" that works around "unset" on an unset variable that can trigger an error when nobody sane is interested in that error condition. If somebody is interested if a variable is not yet set and behave differently, there are more direct ways to see the "set-ness" of a variable, and asking "unset" for that information is insane, hence I think the name "sane_unset" is justified. I do not feel the same way for "sane_echo".

I would have called it "say" if the name weren't taken.
Show 60 quoted lines
> -- >8 --
> Subject: git-sh-setup: introduce sane_echo helper
>
> Using 'echo' with arguments that might contain backslashes or "-e" or
> "-n" can produce confusing results that vary from platform to platform
> (e.g., dash always interprets \ escape sequences and echoes "-e"
> verbatim, whereas bash does not interpret \ escapes unless "-e" is
> passed as an argument to echo and suppresses the "-e" from its
> output).
>
> Instead, we should use printf, which is more predictable:
>
> 	printf '%s\n' "$foo"; # Just prints $foo, on all platforms.
>
> Blindly replacing echo with "printf '%s\n'" would not be good enough
> because that printf prints each argument on its own line.  Provide a
> sane_echo helper that prints its arguments, space-delimited, on a
> single line, to make this easier to remember, and tweak 'say'
> and 'die_with_status' to illustrate how it is used.
>
> No functional change intended.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  git-sh-setup.sh | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git i/git-sh-setup.sh w/git-sh-setup.sh
> index 256c89a..f35b5b9 100644
> --- i/git-sh-setup.sh
> +++ w/git-sh-setup.sh
> @@ -43,6 +43,10 @@ git_broken_path_fix () {
>  
>  # @@BROKEN_PATH_FIX@@
>  
> +sane_echo () {
> +	printf '%s\n' "$*"
> +}
> +
>  die () {
>  	die_with_status 1 "$@"
>  }
> @@ -50,7 +54,7 @@ die () {
>  die_with_status () {
>  	status=$1
>  	shift
> -	printf >&2 '%s\n' "$*"
> +	sane_echo >&2 "$*"
>  	exit "$status"
>  }
>  
> @@ -59,7 +63,7 @@ GIT_QUIET=
>  say () {
>  	if test -z "$GIT_QUIET"
>  	then
> -		printf '%s\n' "$*"
> +		sane_echo "$*"
>  	fi
>  }
>
David Kastrup· Mar 19, 2014, 17:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>>> Uwe Storbeck wrote:
>>
>>>>> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
>>>
>>> This is wrong, isn't it?  Why do we want one line per item here?
>>
>> Yes, Hannes caught the same, too.  Sorry for the sloppiness.
>>
>> We currently use "echo" all over the place (e.g., 'echo "$path"' in
>> git-sh-setup), and every time we fix it there is a chance of making
>> mistakes.  I wonder if it would make sense to add a helper to make the
>> echo calls easier to replace:
>
> I agree that we would benefit from having a helper to print a single
> line, which we very often do, without having to worry about the
> boilerplate '%s\n' of printf or the portability gotcha of echo.
>
> I am a bit reluctant to name the helper "sane_echo" to declare "echo
> that interprets backslashes in the string is insane", though.
raw_echo
-- 
David Kastrup
Junio C Hamano· Mar 19, 2014, 19:24 UTC · re: David Kastrup · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

David Kastrup <dak@gnu.org> writes:
Show 26 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> Junio C Hamano wrote:
>>>>> Uwe Storbeck wrote:
>>>
>>>>>> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
>>>>
>>>> This is wrong, isn't it?  Why do we want one line per item here?
>>>
>>> Yes, Hannes caught the same, too.  Sorry for the sloppiness.
>>>
>>> We currently use "echo" all over the place (e.g., 'echo "$path"' in
>>> git-sh-setup), and every time we fix it there is a chance of making
>>> mistakes.  I wonder if it would make sense to add a helper to make the
>>> echo calls easier to replace:
>>
>> I agree that we would benefit from having a helper to print a single
>> line, which we very often do, without having to worry about the
>> boilerplate '%s\n' of printf or the portability gotcha of echo.
>>
>> I am a bit reluctant to name the helper "sane_echo" to declare "echo
>> that interprets backslashes in the string is insane", though.
>
> raw_echo

Yeah, but the thing is, this is not even "raw" if you view it from the direction of knowing what "echo" does. That is why I repeated "helper to print a single line", which is a viewpoint from the user side. "We do not care how it is implemented, we just want a single line printed" is what we want to express, which "say" is perfectly in line with. "We use a subset semantics of 'echo' to implement it" is of secondary concern.

Jonathan Nieder· Mar 20, 2014, 00:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 15 quoted lines
>> We currently use "echo" all over the place (e.g., 'echo "$path"' in
>> git-sh-setup), and every time we fix it there is a chance of making
>> mistakes.  I wonder if it would make sense to add a helper to make the
>> echo calls easier to replace:
>
> I agree that we would benefit from having a helper to print a single
> line, which we very often do, without having to worry about the
> boilerplate '%s\n' of printf or the portability gotcha of echo.
>
> I am a bit reluctant to name the helper "sane_echo" to declare "echo
> that interprets backslashes in the string is insane", though.  For
> these "print a single line" uses, we are only interested in using a
> subset of the features offered by 'echo', but that does not mean the
> other features we do not want to trigger in our use is of no use to
> any sane person.
In a portable script, uncareful use of 'echo' is always insane.

In a script tailored to an environment where echo behaves consistently it is perfectly reasonable to use 'echo', but that's a different story. In the context of git, saying "Here is the thing you should always use instead of echo" is a good thing, in my opinion.

Hoping that clarifies, Jonathan

Junio C Hamano· Mar 20, 2014, 16:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> Junio C Hamano wrote:
> ...
>> I am a bit reluctant to name the helper "sane_echo" to declare "echo
>> that interprets backslashes in the string is insane", though.  For
>> these "print a single line" uses, we are only interested in using a
>> subset of the features offered by 'echo', but that does not mean the
>> other features we do not want to trigger in our use is of no use to
>> any sane person.
>
> In a portable script, uncareful use of 'echo' is always insane.

I agree that makes sense and I actually think that it is a bit stronger than that. If a script is meant to be portable, there is no way to use "echo" on a string whose contents is unknown sanely. There is no "careful use is OK".

> In a script tailored to an environment where echo behaves consistently
> it is perfectly reasonable to use 'echo', but that's a different
> story.  In the context of git, saying "Here is the thing you should
> always use instead of echo" is a good thing, in my opinion.

That is true in my opinion, but that thing is also what you should always use instead of "printf '%s\n'". A guideline more useful for the users is "Here is the thing you should always use when literally emitting a single line.", isn't it?

Johannes Sixt· Mar 15, 2014, 08:59 UTC · re: Uwe Storbeck · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

Am 15.03.2014 00:57, schrieb Uwe Storbeck:
Show 29 quoted lines
> when variables may contain backslash sequences.
> 
> Backslash sequences are interpreted as control characters
> by the echo command of some shells (e.g. dash).
> 
> Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
> ---
>  t/test-lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..8209204 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -277,7 +277,7 @@ error "Test script did not set test_description."
>  
>  if test "$help" = "t"
>  then
> -	echo "$test_description"
> +	printf '%s\n' "$test_description"
>  	exit 0
>  fi
>  
> @@ -328,7 +328,7 @@ test_failure_ () {
>  	test_failure=$(($test_failure + 1))
>  	say_color error "not ok $test_count - $1"
>  	shift
> -	echo "$@" | sed -e 's/^/#	/'
> +	printf '%s\n' "$@" | sed -e 's/^/#	/'
This should be
	printf '%s\n' "$*" | sed -e 's/^/#	/'
>  	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
>  }
>  
>
Uwe Storbeck· Mar 17, 2014, 18:38 UTC · re: Johannes Sixt · lore

Re: [PATCH] test-lib.sh: use printf instead of echo

On Mar 15, Johannes Sixt wrote:
Show 6 quoted lines
> > -	echo "$@" | sed -e 's/^/#	/'
> > +	printf '%s\n' "$@" | sed -e 's/^/#	/'
> 
> This should be
> 
> 	printf '%s\n' "$*" | sed -e 's/^/#	/'

Right, that should be $* to always be one argument for the format pattern.

Thanks
Uwe
Uwe Storbeck· Mar 17, 2014, 18:39 UTC · re: Uwe Storbeck · lore

[PATCH v2] test-lib.sh: use printf instead of echo

when variables may contain backslash sequences.

Backslash sequences are interpreted as control characters by the echo command of some shells (e.g. dash).

Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
---

Changed $@ to $* in printf. Thanks to Johannes Sixt to point that out.

 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/test-lib.sh +2 −2
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..3c7cb1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error "Test script did not set test_description."
 
 if test "$help" = "t"
 then
-	echo "$test_description"
+	printf '%s\n' "$test_description"
 	exit 0
 fi
 
@@ -328,7 +328,7 @@ test_failure_ () {
 	test_failure=$(($test_failure + 1))
 	say_color error "not ok $test_count - $1"
 	shift
-	echo "$@" | sed -e 's/^/#	/'
+	printf '%s\n' "$*" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }
 

-- 
1.9.0
Uwe Storbeck· Mar 18, 2014, 00:14 UTC · re: Uwe Storbeck · lore

[PATCH v3] test-lib.sh: do not "echo" externally supplied strings

In some places we "echo" a string that is supplied by the calling test script and may contain backslash sequences. The echo command of some shells, most notably "dash", interprets these backslash sequences (POSIX.1 allows this) which may scramble the test output.

Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
---

Commit message rewritten to avoid title continuation in the body. Thanks Junio C Hamano for the hint.

 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/test-lib.sh +2 −2
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..3c7cb1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error "Test script did not set test_description."
 
 if test "$help" = "t"
 then
-	echo "$test_description"
+	printf '%s\n' "$test_description"
 	exit 0
 fi
 
@@ -328,7 +328,7 @@ test_failure_ () {
 	test_failure=$(($test_failure + 1))
 	say_color error "not ok $test_count - $1"
 	shift
-	echo "$@" | sed -e 's/^/#	/'
+	printf '%s\n' "$*" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }
 

-- 
1.9.0
Junio C Hamano· Mar 18, 2014, 18:47 UTC · re: Uwe Storbeck · lore

Re: [PATCH v3] test-lib.sh: do not "echo" externally supplied strings

Uwe Storbeck <uwe@ibr.ch> writes:
Show 11 quoted lines
> In some places we "echo" a string that is supplied by the calling
> test script and may contain backslash sequences. The echo command
> of some shells, most notably "dash", interprets these backslash
> sequences (POSIX.1 allows this) which may scramble the test
> output.
>
> Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
> ---
>
> Commit message rewritten to avoid title continuation in the body.
> Thanks Junio C Hamano for the hint.

Here is what I queued yesterday. I was wrong to say "control characters"; a backslash sequence is not necessarily a control character (e.g. \c at the end that suppresses the final LF), so I'll replace it with your version.

The test titles are not externally supplied but under our control, so it is less problematic than the "rebase -i" case where we do get bitten by user supplied commit title string. Still it is a good idea to apply this change to protect ourselves.

Thanks.  
commit 215cd588caebe86fe77115bdda97930b4659aecd
Author: Uwe Storbeck <uwe@ibr.ch>
Date:   Sat Mar 15 00:57:36 2014 +0100
    test-lib.sh: do not "echo" test titles
    
    The test title could be a string with backslash in it, and can be
    interpreted as control characters by the echo command of some shells
    (e.g. dash).
    
    Signed-off-by: Uwe Storbeck <uwe@ibr.ch>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Show changes to t/test-lib.sh +2 −2
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..3c7cb1d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -277,7 +277,7 @@ error "Test script did not set test_description."
 
 if test "$help" = "t"
 then
-	echo "$test_description"
+	printf '%s\n' "$test_description"
 	exit 0
 fi
 
@@ -328,7 +328,7 @@ test_failure_ () {
 	test_failure=$(($test_failure + 1))
 	say_color error "not ok $test_count - $1"
 	shift
-	echo "$@" | sed -e 's/^/#	/'
+	printf '%s\n' "$*" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }

← back to recent threads