Bug: git-rebase goofs up \n in commit messages

Bug: git-rebase goofs up \n in commit messages

[Szekeres Istvan]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

Hello,

while playing with git I found the following bug: if a commit message
contains "\n" (as a string, not as a character), git-rebase changes this
string into a literal newline character.

This is how to reproduce it:

mkdir tmp
cd tmp
git init-db
echo xx > xx.txt
git add xx.txt
git commit -m foo
echo xx >> xx.txt
git add xx.txt
git commit -m foo
git branch other 'HEAD^'
git checkout other
echo yy > yy.txt
git add yy.txt
git commit -m 'foo \\n bar'
git log 'HEAD^..'              [1]
git rebase master
git log 'HEAD^..'              [2]

The output of [1] is the following (correctly):

commit 694daa542b83dc1bbd6c070630f73c9a111f6e40
Author: Istvan Szekeres <szekeres@iii.hu>
Date:   Fri May 25 23:09:32 2007 +0200

    foo \n bar


The output of [2] is the following (wrong!):
commit 68ba80d2927d4e21c7a1d1d758f9023dbe063bde
Author: Istvan Szekeres <szekeres@iii.hu>
Date:   Fri May 25 22:58:28 2007 +0200

    foo
     bar
....

I think this is a bug.

Best regards,
Istvan






[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Bug: git-rebase goofs up \n in commit messages

[Jeff King]

On Fri, May 25, 2007 at 11:11:26PM +0200, Szekeres Istvan wrote:

> while playing with git I found the following bug: if a commit message
> contains "\n" (as a string, not as a character), git-rebase changes this
> string into a literal newline character.

Hmm. The culprit seems to be git-am.sh, line 313:

  SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"

and even more exciting, it seems to be a bash vs dash thing. Try this:

$ cat >content-with-escapes <<'EOF'
foo \n bar
EOF

$ bash
bash$ test=$(cat content-with-escapes)
bash$ echo $test
foo \n bar

$ dash
dash$ test=$(cat content-with-escapes)
dash$ echo $test
foo
 bar


Hmm. It even happens with this:

bash$ export test=$(echo foo \\n bar)
bash$ dash
dash$ echo $test
foo
 bar

I'm not sure what the best workaround is.  I am cc'ing Herbert Xu to see
if he has any helpful comments.

-Peff

Re: Bug: git-rebase goofs up \n in commit messages

[Herbert Xu]

On Fri, May 25, 2007 at 08:40:28PM -0400, Jeff King wrote:
> 
> Hmm. It even happens with this:
> 
> bash$ export test=$(echo foo \\n bar)
> bash$ dash
> dash$ echo $test
> foo
>  bar
> 
> I'm not sure what the best workaround is.  I am cc'ing Herbert Xu to see
> if he has any helpful comments.

If you need to echo something that may have escapes in it, the portable
way to do it is

printf '%s\n' "$test"

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bug: git-rebase goofs up \n in commit messages

[Jeff King]

On Sat, May 26, 2007 at 11:10:36AM +1000, Herbert Xu wrote:

> If you need to echo something that may have escapes in it, the portable
> way to do it is
> 
> printf '%s\n' "$test"

Ah, I see. I had thought the problem was coming from some dash
interpolation magic, but yes, it's just echo doing the conversion. And
POSIX is very clear that this is an implementation defined behavior.
Thanks very much for the response, Herbert.

Junio, patch is below. I have no idea how prevalent this issue is within
our scripts, but this at least fixes the reported bug.

-- >8 --
git-am: use printf instead of echo on user-supplied strings

Under some implementations of echo (such as that provided by
dash), backslash escapes are recognized without any other
options. This means that echo-ing user-supplied strings may
cause any backslash sequences in them to be converted. Using
printf resolves the ambiguity.

This bug can be seen when using git-am to apply a patch
whose subject contains the character sequence "\n"; the
characters are converted to a literal newline. Noticed by
Szekeres Istvan.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-am.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c9f66e2..543efd0 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -331,7 +331,7 @@ do
 		ADD_SIGNOFF=
 	    fi
 	    {
-		echo "$SUBJECT"
+		printf '%s\n' "$SUBJECT"
 		if test -s "$dotest/msg-clean"
 		then
 			echo
@@ -394,7 +394,7 @@ do
 	fi
 
 	echo
-	echo "Applying '$SUBJECT'"
+	printf 'Applying %s\n' "$SUBJECT"
 	echo
 
 	case "$resolved" in
-- 
1.5.2.818.g9b59-dirty

Re: Bug: git-rebase goofs up \n in commit messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, May 26, 2007 at 11:10:36AM +1000, Herbert Xu wrote:
>
>> If you need to echo something that may have escapes in it, the portable
>> way to do it is
>> 
>> printf '%s\n' "$test"
>
> Ah, I see. I had thought the problem was coming from some dash
> interpolation magic, but yes, it's just echo doing the conversion. And
> POSIX is very clear that this is an implementation defined behavior.
> Thanks very much for the response, Herbert.
>
> Junio, patch is below. I have no idea how prevalent this issue is within
> our scripts, but this at least fixes the reported bug.

Gaah.  Ok, dash uses "echo -e" behaviour by default.

I guess we need to hunt allmost all "echo", as I suspect most of
them (except the ones we use to do "echo $SHA1") have user
strings somewhere.

What a mess, but that is not your fault nor Herbert's.

Thanks.

Re: Bug: git-rebase goofs up \n in commit messages

[Junio C Hamano]

It turns out that git-commit also shares the same problem under
dash.

-- >8 --
git-commit: use printf "%s\n" instead of echo on user-supplied strings

This adds a test to verify the earlier fix to git-am by Jeff
King, and fixes the same issue in git-commit that is exposed by
the new test while at it.

Cleverly enough, this commit's log message is a good test case
at the same time.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I suspect we would declare either "war on echo" or "harder push
   for builtins" triggered by these.

 git-commit.sh           |    8 ++++----
 t/t4014-format-patch.sh |    6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 292cf96..f3f9a35 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -376,12 +376,12 @@ t,)
 			rm -f "$TMP_INDEX"
 		fi || exit
 
-		echo "$commit_only" |
+		printf "%s\n" "$commit_only" |
 		GIT_INDEX_FILE="$TMP_INDEX" \
 		git-update-index --add --remove --stdin &&
 
 		save_index &&
-		echo "$commit_only" |
+		printf "%s\n" "$commit_only" |
 		(
 			GIT_INDEX_FILE="$NEXT_INDEX"
 			export GIT_INDEX_FILE
@@ -432,7 +432,7 @@ fi
 
 if test "$log_message" != ''
 then
-	echo "$log_message"
+	printf "%s\n" "$log_message"
 elif test "$logfile" != ""
 then
 	if test "$logfile" = -
@@ -475,7 +475,7 @@ if test -f "$GIT_DIR/MERGE_HEAD" && test -z "$no_edit"; then
 	echo "#"
 	echo "# It looks like you may be committing a MERGE."
 	echo "# If this is not correct, please remove the file"
-	echo "#	$GIT_DIR/MERGE_HEAD"
+	printf "%s\n" "#	$GIT_DIR/MERGE_HEAD"
 	echo "# and try again"
 	echo "#"
 fi >>"$GIT_DIR"/COMMIT_EDITMSG
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 4795872..42aa9e4 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -16,16 +16,16 @@ test_expect_success setup '
 
 	for i in 1 2 5 6 A B C 7 8 9 10; do echo "$i"; done >file &&
 	git update-index file &&
-	git commit -m "Side change #1" &&
+	git commit -m "Side changes #1" &&
 
 	for i in D E F; do echo "$i"; done >>file &&
 	git update-index file &&
-	git commit -m "Side change #2" &&
+	git commit -m "Side changes #2" &&
 	git tag C2 &&
 
 	for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >file &&
 	git update-index file &&
-	git commit -m "Side change #3" &&
+	git commit -m "Side changes #3 with \\n backslash-n in it." &&
 
 	git checkout master &&
 	git diff-tree -p C2 | git apply --index &&

Re: Bug: git-rebase goofs up \n in commit messages

[Jeff King]

On Fri, May 25, 2007 at 09:59:43PM -0700, Junio C Hamano wrote:

>  * I suspect we would declare either "war on echo" or "harder push
>    for builtins" triggered by these.

Cry havoc! More fixes below (just a diff -- maybe we want to aggregate
these into a single commit?).

These are the ones I noticed that use commit messages (which are
probably the most likely to use backslash). There are _tons_ of uses for
heads and filenames. I think we either should stop with commit messages,
or go all-out and simply remove all uses of echo (because there are
literally hundreds otherwise).

> -		echo "$commit_only" |
> +		printf "%s\n" "$commit_only" |

Is "\n" portable to all shells (i.e., do you need '\n')? It works with
bash and dash, which are by far the most common, but who knows what evil
lurks in the heart of Sun?

---
diff --git a/git-am.sh b/git-am.sh
index 543efd0..8b57129 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -18,7 +18,7 @@ stop_here () {
 
 stop_here_user_resolve () {
     if [ -n "$resolvemsg" ]; then
-	    echo "$resolvemsg"
+	    printf '%s\n' "$resolvemsg"
 	    stop_here $1
     fi
     cmdline=$(basename $0)
@@ -146,7 +146,7 @@ do
 	git_apply_opt="$git_apply_opt $1"; shift ;;
 
 	--resolvemsg=*)
-	resolvemsg=$(echo "$1" | sed -e "s/^--resolvemsg=//"); shift ;;
+	resolvemsg=${1#--resolvemsg=}; shift ;;
 
 	--)
 	shift; break ;;
diff --git a/git-commit.sh b/git-commit.sh
diff --git a/git-merge.sh b/git-merge.sh
index 44e9b70..981d69d 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -335,7 +335,7 @@ f,*)
 		then
 			echo "Wonderful."
 			result_commit=$(
-				echo "$merge_msg" |
+				printf '%s\n' "$merge_msg" |
 				git-commit-tree $result_tree -p HEAD -p "$1"
 			) || exit
 			finish "$result_commit" "In-index merge"
@@ -440,7 +440,7 @@ done
 if test '' != "$result_tree"
 then
     parents=$(git-show-branch --independent "$head" "$@" | sed -e 's/^/-p /')
-    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents) || exit
+    result_commit=$(printf '%s\n' "$merge_msg" | git-commit-tree $result_tree $parents) || exit
     finish "$result_commit" "Merge made by $wt_strategy."
     dropsave
     exit 0
@@ -479,7 +479,7 @@ else
 	do
 		echo $remote
 	done >"$GIT_DIR/MERGE_HEAD"
-	echo "$merge_msg" >"$GIT_DIR/MERGE_MSG"
+	printf '%s\n' "$merge_msg" >"$GIT_DIR/MERGE_MSG"
 fi
 
 if test "$merge_was_ok" = t
diff --git a/git-tag.sh b/git-tag.sh
index 4a0a7b6..6f0b7a7 100755
--- a/git-tag.sh
+++ b/git-tag.sh
@@ -126,7 +126,7 @@ if [ "$annotate" ]; then
           echo "#" ) > "$GIT_DIR"/TAG_EDITMSG
         ${VISUAL:-${EDITOR:-vi}} "$GIT_DIR"/TAG_EDITMSG || exit
     else
-        echo "$message" >"$GIT_DIR"/TAG_EDITMSG
+        printf '%s\n' "$message" >"$GIT_DIR"/TAG_EDITMSG
     fi
 
     grep -v '^#' <"$GIT_DIR"/TAG_EDITMSG |

Re: Bug: git-rebase goofs up \n in commit messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 25, 2007 at 09:59:43PM -0700, Junio C Hamano wrote:
>
>>  * I suspect we would declare either "war on echo" or "harder push
>>    for builtins" triggered by these.
>
> Cry havoc! More fixes below (just a diff -- maybe we want to aggregate
> these into a single commit?).
>
> These are the ones I noticed that use commit messages (which are
> probably the most likely to use backslash). There are _tons_ of uses for
> heads and filenames. I think we either should stop with commit messages,
> or go all-out and simply remove all uses of echo (because there are
> literally hundreds otherwise).
>
>> -		echo "$commit_only" |
>> +		printf "%s\n" "$commit_only" |
>
> Is "\n" portable to all shells (i.e., do you need '\n')? It works with
> bash and dash, which are by far the most common, but who knows what evil
> lurks in the heart of Sun?

Gaah, you are right.

Re: Bug: git-rebase goofs up \n in commit messages

[Herbert Xu]

On Sat, May 26, 2007 at 02:07:48AM -0400, Jeff King wrote:
>
> > -		echo "$commit_only" |
> > +		printf "%s\n" "$commit_only" |
> 
> Is "\n" portable to all shells (i.e., do you need '\n')? It works with
> bash and dash, which are by far the most common, but who knows what evil
> lurks in the heart of Sun?

You mean the "\n" in printf? Yes that is specified in POSIX.
Without the "\n" printf will act like echo -n (which incidentally
is forbidden by POSIX).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bug: git-rebase goofs up \n in commit messages

[Jeff King]

On Sat, May 26, 2007 at 04:19:42PM +1000, Herbert Xu wrote:

> > Is "\n" portable to all shells (i.e., do you need '\n')? It works with
> > bash and dash, which are by far the most common, but who knows what evil
> > lurks in the heart of Sun?
> 
> You mean the "\n" in printf? Yes that is specified in POSIX.
> Without the "\n" printf will act like echo -n (which incidentally
> is forbidden by POSIX).

No, I meant would the shell, while interpolating a double-quoted string
"\n", always preserve the string and pass the backslash and 'n' to
printf?  Clearly \\ and \" get interpolated, but I don't know the rules
for "unrecognized" backslash sequences.

-Peff

Re: Bug: git-rebase goofs up \n in commit messages

[Herbert Xu]

On Sat, May 26, 2007 at 02:27:48AM -0400, Jeff King wrote:
> 
> No, I meant would the shell, while interpolating a double-quoted string
> "\n", always preserve the string and pass the backslash and 'n' to
> printf?  Clearly \\ and \" get interpolated, but I don't know the rules
> for "unrecognized" backslash sequences.

Yep.  Section 2.2.3 says that the backslash within double quotes will
only serve as an escape character for $, `, ", \ and <newline>.  Of
course, you can always use a single quote instead, i.e.,

printf '%s\n' "$VAR"

Then there can be no doubt :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bug: git-rebase goofs up \n in commit messages

[Jeff King]

On Sat, May 26, 2007 at 05:38:55PM +1000, Herbert Xu wrote:

> Yep.  Section 2.2.3 says that the backslash within double quotes will
> only serve as an escape character for $, `, ", \ and <newline>.  Of
> course, you can always use a single quote instead, i.e.,
> 
> printf '%s\n' "$VAR"
> 
> Then there can be no doubt :)

Right, I wasn't sure if the single quotes were necessary, but it looks
like Junio's version (with doublequotes) is also fine according to
POSIX. Thanks again.

-Peff

Re: Bug: git-rebase goofs up \n in commit messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 25, 2007 at 09:59:43PM -0700, Junio C Hamano wrote:
>
>>  * I suspect we would declare either "war on echo" or "harder push
>>    for builtins" triggered by these.
>
> Cry havoc! More fixes below (just a diff -- maybe we want to aggregate
> these into a single commit?).
>
> These are the ones I noticed that use commit messages (which are
> probably the most likely to use backslash). There are _tons_ of uses for
> heads and filenames. I think we either should stop with commit messages,
> or go all-out and simply remove all uses of echo (because there are
> literally hundreds otherwise).

At least the ones you did look very sane to me.  Will apply with
appropriate log message, credit to you.

Thanks.

I do not think we need to do all the uses of 'echo'.  Many of
them are clearly fixed string we know about, object names we
parsed out of plumbing output, refnames and refspecs, all of
which should be safe.

Other worrisome ones are pathnames, but (1) I do not think
anybody is insane enough to have slashed funnies in their
pathname components, (2) half the pathnames we deal with come
from plumbing output which use '/' as path component separator
even on Windows, (3) users can use forward slash as path
component separator in their input even on Windows, and (4) even
though we try to use -z output from plumbing and read it with -0
capable downstream in some of our pipelines, many pure-shell
scripts read non-z output using shell built-in "read" and do not
unquote c-quoted ones, so they do not work correctly if you have
HT or LF in your pathnames anyway (notable exception is that
pipelines between git plumbing, e.g. "ls-files | update-index
--stdin", are safe without -z, as the downstream knows how to
unquote c-quoted paths).

I would expect that by the time we run out of more important
things to worry about and start worrying about truly funny
pathnames, we would have rewritten more of the remaining
Porcelains shell scripts in C, which automatically would make
this problem go away.