git rebase -i error message interprets \t in commit message

Hi,

I just got the following error message:

dak@lola:/usr/local/tmp/lilypond$ git rebase -i Waiting for Emacs... error: could not apply 16de9d2... Make tempo range \tempo 20~30 be input as \tempo 20-30 instead

When you have resolved this problem, run "git rebase --continue". If you prefer to skip this patch, run "git rebase --skip" instead. To check out the original branch and stop rebasing, run "git rebase --abort". Could not apply 16de9d2... Make tempo range empo 20~30 be input as empo 20-30 instead dak@lola:/usr/local/tmp/lilypond$ git --version git version 1.8.1.2

As you can see, the first message starting with "error: could not apply" outputs a reasonable rendition of the commit summary line. However, the final "Could not apply" message (starting with a capital C) converts instances of \t to a tab.

This is on Ubuntu 13.04 with

lrwxrwxrwx 1 root root 4 Aug 15  2012 /bin/sh -> dash

David Kastrup wrote:

To get you started:

  $ git grep "could not apply"
  sequencer.c=475=static int do_pick_commit(struct commit *commit,
  sequencer.c:628:                      : _("could not apply %s... %s"),

  $ git grep "Could not apply"
  git-rebase--interactive.sh=431=die_failed_squash() {
  git-rebase--interactive.sh:436: warn "Could not apply $1... $2"
  git-rebase--interactive.sh=460=do_pick () {
  git-rebase--interactive.sh:476: die_with_patch $1 "Could not apply $1... $2"
  git-rebase--interactive.sh:479: die_with_patch $1 "Could not apply $1... $2"

So, it's the shell script. Now, read about shell escaping [1] and submit a patch.

Thanks.

[1]: http://tldp.org/LDP/abs/html/escapingsection.html

Ramkumar Ramachandra <artagnon@gmail.com> writes:

This is not about shell escaping at all. I think the message is fed to echo as-is, or to printf as its first parameter.

David Kastrup <dak@gnu.org> writes:

Indeed. The source of the problem is that our "die" shell function interprets \t (because it uses "echo").

A simple fix would be this:

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Sounds reasonable, though I don't know off-hand (not having the source here) whether using "say" inside of die_with_status (and thus not having two different places to maintain) might not be feasible instead. It's not like die_with_status is going to be called often enough to become a performance hog.

One person's escape sequences are another's multibyte characters. Less cheesy, it would seem that the above change is a strict improvement and requires little thought to implement.

Not interpreting escape sequences is orthogonal from output sanitization.

David Kastrup <dak@gnu.org> writes:

The definition of say is:

say () {
	if test -z "$GIT_QUIET"
	then
		printf '%s\n' "$*"
	fi
}

I don't think we want to disable die's output even when the caller requested to be quiet. Currently, my patch is:

From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Tue, 6 Aug 2013 19:13:03 +0200
Subject: [PATCH] die_with_status: use "printf '%s\n'", not "echo"

At least GNU echo interprets backslashes in its arguments.

This triggered at least one bug: the error message of "rebase -i" was turning \t in commit messages into actual tabulations. There may be others.

Using "printf '%s\n'" instead avoids this bad behavior, and is the form used by the "say" function.

Noticed-by: David Kastrup <dak@gnu.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-sh-setup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

I think that's incorrect in several respects. For one thing, echo is never called for most Bourne shells since echo is a builtin (might have been different for UNIX version 7 or so). For another, GNU echo would behave like Bash:

And GNU Bash does not interpret escapes unless you do echo -e. Escape sequence interpretation, however, happens for Dash:

dak@lola:/usr/local/tmp/lilypond$ dash -c 'echo "x\tx"' x x dak@lola:/usr/local/tmp/lilypond$ bash -c 'echo "x\tx"' x\tx dak@lola:/usr/local/tmp/lilypond$ /bin/echo "x\tx" x\tx

So replace "GNU echo" in your commit message with "Dash's echo builtin" and you get closer.

David Kastrup <dak@gnu.org> writes:

I'll queue the attached.

POSIX makes it an implementation defined behaviour when the first argument is "-n", or any argument contains a backslas (X/Open System Interfaces wants to treat "-n" as literal and always interpret the backslash sequence), so it is definitely safer to avoid running 'echo' on any random string.

Thanks.

Author: Matthieu Moy <Matthieu.Moy@imag.fr>
Date:   Tue Aug 6 20:26:44 2013 +0200

    die_with_status: use "printf '%s\n'", not "echo"
    
    Some implementations of 'echo' (e.g. dash's built-in) interpret
    backslash sequences in their arguments.
    
    This triggered at least one bug: the error message of "rebase -i" was
    turning \t in commit messages into actual tabulations. There may be
    others.
    
    Using "printf '%s\n'" instead avoids this bad behavior, and is the form
    used by the "say" function.
    
    Noticed-by: David Kastrup <dak@gnu.org>
    Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

I gave a quick look at "git grep -e 'die ' -- \*.sh" output, and I do not think this change will break things (i.e. no caller expects the non-portable behaviour of the shell such as "\c" at the end to omit the trailing newline).

Thanks, care to roll it into a patch with a test or two?

At least GNU echo interprets backslashes in its arguments.

This triggered at least one bug: the error message of "rebase -i" was turning \t in commit messages into actual tabulations. There may be others.

Using "printf '%s\n'" instead avoids this bad behavior, and is the form used by the "say" function.

Noticed-by: David Kastrup <dak@gnu.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-sh-setup.sh               |  2 +-
 t/t3404-rebase-interactive.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

[...]

Umm, doesn't that only test that _some_ line in the error does not contain a tab?

Whereas you need to test that _no_ line contains <TAB>emp, or some such. Perhaps as

  ! grep -v "	emp" error

Thomas Rast <trast@inf.ethz.ch> writes:

Indeed. It does work as the error message is just a one-liner, but let's be robust anyway.

Err, then the -v should be remove. Also, I'll use test_expect_code 1 instead of ! to catch other grep failures, just in case.

Thanks, new patch comming.

Some implementations of 'echo' (e.g. dash's built-in) interpret backslash sequences in their arguments.

This triggered at least one bug: the error message of "rebase -i" was turning \t in commit messages into actual tabulations. There may be others.

Using "printf '%s\n'" instead avoids this bad behavior, and is the form used by the "say" function.

Noticed-by: David Kastrup <dak@gnu.org>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Changed the "grep" function to be more accurate. The test still
catches the old failure and pass after the fix.

 git-sh-setup.sh               |  2 +-
 t/t3404-rebase-interactive.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Err, right. I actually applied the patch and verified that the test "erroneously" passed without the fix, but that's simply because I use bash instead of dash. The error message indeed is only one line, the rest of the rebase output goes to stdout.