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

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

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

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

On Fri, May 25, 2007 at 08:40:28PM -0400, Jeff King wrote:

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

printf '%s\n' "$test"

Cheers,

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

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

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?

---

Jeff King <peff@peff.net> writes:

Gaah, you are right.

On Sat, May 26, 2007 at 02:07:48AM -0400, Jeff King wrote:

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,

On Sat, May 26, 2007 at 04:19:42PM +1000, Herbert Xu 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.

-Peff

On Sat, May 26, 2007 at 02:27:48AM -0400, Jeff King 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 :)

Cheers,

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

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

Jeff King <peff@peff.net> writes:

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.