git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Uwe Storbeck <uwe@ibr.ch>, git@vger.kernel.org
Subject: Re: [PATCH] test-lib.sh: use printf instead of echo
Date: Tue, 18 Mar 2014 15:18:44 -0700	[thread overview]
Message-ID: <20140318221844.GA828@google.com> (raw)
In-Reply-To: <xmqq61nceidp.fsf@gitster.dls.corp.google.com>

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(-)

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
 }
 

  reply	other threads:[~2014-03-18 22:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-14 23:57 [PATCH] test-lib.sh: use printf instead of echo Uwe Storbeck
2014-03-15  0:18 ` Jonathan Nieder
2014-03-17 19:26   ` Junio C Hamano
2014-03-18 22:18     ` Jonathan Nieder [this message]
2014-03-19 17:17       ` Junio C Hamano
2014-03-19 17:22         ` David Kastrup
2014-03-19 19:24           ` Junio C Hamano
2014-03-20  0:17         ` Jonathan Nieder
2014-03-20 16:42           ` Junio C Hamano
2014-03-15  8:59 ` Johannes Sixt
2014-03-17 18:38   ` Uwe Storbeck
2014-03-17 18:39 ` [PATCH v2] " Uwe Storbeck
2014-03-18  0:14 ` [PATCH v3] test-lib.sh: do not "echo" externally supplied strings Uwe Storbeck
2014-03-18 18:47   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140318221844.GA828@google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=uwe@ibr.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).