test-lib.sh: use printf instead of echo
PATCH 1/1 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).
--- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/test-lib.sh +2 -2
@@ -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; } }
Signature · 1 line
1.9.0
Quoted 2 lines · Uwe Storbeck
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.
Quoted 1 line
Signed-off-by Uwe Storbeck <[email protected]>
(patch left unsnipped for reference)
Quoted 26 lines
--- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/test-lib.sh +2
-2@@ -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; } }
Quoted 30 lines · Jonathan Nieder
Quoted 2 lines · Uwe Storbeck
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.
Quoted 1 line
Signed-off-by Uwe Storbeck <[email protected]>Reviewed-by Jonathan Nieder <[email protected]>(patch left unsnipped for reference)
Quoted 17 lines
--- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/test-lib.sh +1
-1@@ -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
Quoted 6 lines
t/test-lib.sh test_failure_ () { · line 331
Quoted 6 lines
t/test-lib.sh +1
-1@@ -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?
Quoted 3 lines
Quoted 3 lines
t/test-lib.sh +0
-0test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; } }
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
Review
Quoted 1 line · Uwe Storbeck
(quoted diff) +1
-0+ 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.
--- git-sh-setup.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
git-sh-setup.sh +6 -2
@@ -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 }
Quoted 7 lines · Jonathan Nieder
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
Review
Quoted 1 line · Uwe Storbeck
(quoted diff) +1
-0+ printf '%s\n' "$@" | sed -e 's/^/# /'This is wrong, isn't it? Why do we want one line per item here?
Quoted 6 lines
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.
Quoted 60 lines
-- >8 --
Subject: git-sh-setup: introducesane_echohelperUsing '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_echohelper 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 <[email protected]>--- git-sh-setup.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)git-sh-setup.sh +6
-2@@ -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 }
Quoted 16 lines · Junio C Hamano
Quoted 7 lines · Jonathan Nieder
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
Review
Quoted 1 line · Uwe Storbeck
(quoted diff) +1
-0+ printf '%s\n' "$@" | sed -e 's/^/# /'This is wrong, isn't it? Why do we want one line per item here?
Quoted 6 lines
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:
Quoted 6 lines
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
Signature · 1 line
David Kastrup
Quoted 18 lines · David Kastrup
Quoted 16 lines · Junio C Hamano
Quoted 13 lines · Jonathan Nieder
Quoted 5 lines · Junio C Hamano
Quoted 3 lines
Review
Quoted 1 line · Uwe Storbeck
(quoted diff) +1
-0+ 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:
Quoted 8 lines
Quoted 6 lines
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.
Quoted 11 lines · Junio C Hamano
Quoted 4 lines · Jonathan Nieder
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.
Quoted 6 lines
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
Quoted 10 lines · Jonathan Nieder
Junio C Hamano wrote:
...Quoted 6 lines
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".
Quoted 4 lines
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?
Am 15.03.2014 00:57, schrieb Uwe Storbeck:
Quoted 24 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 <[email protected]>--- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/test-lib.sh +1
-1@@ -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
t/test-lib.sh test_failure_ () { · line 331
Quoted 5 lines
t/test-lib.sh +1
-1@@ -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/^/# /'
Quoted 4 lines
t/test-lib.sh +0
-0test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; } }
Quoted 6 lines · Johannes Sixt
t/test-lib.sh test_failure_ () { · line 331
Quoted 2 lines
t/test-lib.sh +1
-1- 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
PATCH v2 1/1 test-lib.sh: use printf instead of echo
In reply to Uwe Storbeckwhen variables may contain backslash sequences.
Backslash sequences are interpreted as control characters by the echo command of some shells (e.g. dash).
---
Changed $@ to $* in printf.
Thanks to Johannes Sixt to point that out.
t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/test-lib.sh +2 -2
@@ -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; } }
Signature · 1 line
1.9.0
PATCH v3 1/1 test-lib.sh: do not "echo" externally supplied strings
In reply to Uwe StorbeckIn 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.
---
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(-)
t/test-lib.sh +2 -2
@@ -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; } }
Signature · 1 line
1.9.0
Re: [PATCH v3] test-lib.sh: do not "echo" externally supplied strings
Quoted 11 lines · Uwe Storbeck
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 <[email protected]>---
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 <[email protected]> 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).
t/test-lib.sh +2 -2
@@ -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; } }