Re: [PATCH] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@gmail.com writes: 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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Junio C Hamano gits...@pobox.com writes: Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@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. 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.
-- 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 jrnie...@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
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Jonathan Nieder jrnie...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 jrnie...@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
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
On Mar 15, Johannes Sixt wrote: - 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Jonathan Nieder jrnie...@gmail.com writes:
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 u...@ibr.ch
Reviewed-by: Jonathan Nieder jrnie...@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; }
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib.sh: use printf instead of echo
Am 15.03.2014 00:57, schrieb Uwe Storbeck:
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 u...@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; }
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 u...@ibr.ch
Reviewed-by: Jonathan Nieder jrnie...@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/^/# /'
test $immediate = || { GIT_EXIT_OK=t; exit 1; }
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
