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

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

[David Kastrup]

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

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

[Ramkumar Ramachandra]

David Kastrup wrote:
> 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.

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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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.

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

[Matthieu Moy]

David Kastrup <dak@gnu.org> writes:

> Could not apply 16de9d2... Make tempo range 	empo 20~30 be input as 	empo 20-30 instead

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

A simple fix would be this:

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..97258d5 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
        status=$1
        shift
-       echo >&2 "$*"
+       printf >&2 "%s\n" "$*"
        exit "$status"
 }
 
It does not sound crazy as the shell function "say" right below uses the
same printf "%s\n" "$*", but I'm wondering whether this could have other
bad implications (e.g. if there are escape sequences in the commit
message, aren't we going to screw up the terminal?).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Could not apply 16de9d2... Make tempo range empo 20~30 be input as
>> empo 20-30 instead
>
> Indeed. The source of the problem is that our "die" shell function
> interprets \t (because it uses "echo").
>
> A simple fix would be this:
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7a964ad..97258d5 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -53,7 +53,7 @@ die () {
>  die_with_status () {
>         status=$1
>         shift
> -       echo >&2 "$*"
> +       printf >&2 "%s\n" "$*"
>         exit "$status"
>  }
>  
> It does not sound crazy as the shell function "say" right below uses the
> same printf "%s\n" "$*",

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.

> but I'm wondering whether this could have other bad implications
> (e.g. if there are escape sequences in the commit message, aren't we
> going to screw up the terminal?).

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

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

[Matthieu Moy]

David Kastrup <dak@gnu.org> writes:

>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
>> index 7a964ad..97258d5 100644
>> --- a/git-sh-setup.sh
>> +++ b/git-sh-setup.sh
>> @@ -53,7 +53,7 @@ die () {
>>  die_with_status () {
>>         status=$1
>>         shift
>> -       echo >&2 "$*"
>> +       printf >&2 "%s\n" "$*"
>>         exit "$status"
>>  }
>>  
>> It does not sound crazy as the shell function "say" right below uses the
>> same printf "%s\n" "$*",
>
> Sounds reasonable, though I don't know off-hand (not having the source
> here) whether using "say" inside of die_with_status 

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 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001
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(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..e15be51 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
        status=$1
        shift
-       echo >&2 "$*"
+       printf >&2 '%s\n' "$*"
        exit "$status"
 }
 
-- 
1.8.3.3.797.gb72c616

I'll resend properly for inclusion if no one objects.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[David Kastrup]

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

>>From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001
> 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.

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

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

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>>From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001
>> 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.
>
> 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.

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>

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..e15be51 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
 	status=$1
 	shift
-	echo >&2 "$*"
+	printf >&2 '%s\n' "$*"
 	exit "$status"
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 49ccb38..074deb1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '
 	test $(cat file1) = Z
 '
 
+test_expect_success 'rebase -i error on commits with \ in message' '
+	current_head=$(git rev-parse HEAD)
+	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
+	test_commit TO-REMOVE will-conflict old-content &&
+	test_commit "\temp" will-conflict new-content dummy &&
+	(
+	EDITOR=true &&
+	export EDITOR &&
+	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
+	) &&
+	grep -v "	" error
+'
+
 test_done

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

[Junio C Hamano]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Could not apply 16de9d2... Make tempo range 	empo 20~30 be input as 	empo 20-30 instead
>
> Indeed. The source of the problem is that our "die" shell function
> interprets \t (because it uses "echo").
>
> A simple fix would be this:
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7a964ad..97258d5 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -53,7 +53,7 @@ die () {
>  die_with_status () {
>         status=$1
>         shift
> -       echo >&2 "$*"
> +       printf >&2 "%s\n" "$*"
>         exit "$status"
>  }
>  
> It does not sound crazy as the shell function "say" right below uses the
> same printf "%s\n" "$*", but I'm wondering whether this could have other
> bad implications (e.g. if there are escape sequences in the commit
> message, aren't we going to screw up the terminal?).

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?

[PATCH] die_with_status: use "printf '%s\n'", not "echo"

[Matthieu Moy]

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

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..e15be51 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
 	status=$1
 	shift
-	echo >&2 "$*"
+	printf >&2 '%s\n' "$*"
 	exit "$status"
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 49ccb38..074deb1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '
 	test $(cat file1) = Z
 '
 
+test_expect_success 'rebase -i error on commits with \ in message' '
+	current_head=$(git rev-parse HEAD)
+	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
+	test_commit TO-REMOVE will-conflict old-content &&
+	test_commit "\temp" will-conflict new-content dummy &&
+	(
+	EDITOR=true &&
+	export EDITOR &&
+	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
+	) &&
+	grep -v "	" error
+'
+
 test_done
-- 
1.8.3.3.797.gb72c616

Re: [PATCH] die_with_status: use "printf '%s\n'", not "echo"

[Thomas Rast]

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

> 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>
[...]
> +test_expect_success 'rebase -i error on commits with \ in message' '
> +	current_head=$(git rev-parse HEAD)
> +	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
> +	test_commit TO-REMOVE will-conflict old-content &&
> +	test_commit "\temp" will-conflict new-content dummy &&
> +	(
> +	EDITOR=true &&
> +	export EDITOR &&
> +	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
> +	) &&
> +	grep -v "	" error

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

> +'
> +
>  test_done

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] die_with_status: use "printf '%s\n'", not "echo"

[Matthieu Moy]

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

>> +	grep -v "	" error
>
> Umm, doesn't that only test that _some_ line in the error does not
> contain a tab?

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

> Whereas you need to test that _no_ line contains <TAB>emp, or some
> such.  Perhaps as
>
>   ! grep -v "	emp" error

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2] die_with_status: use "printf '%s\n'", not "echo"

[Matthieu Moy]

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

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..e15be51 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
 	status=$1
 	shift
-	echo >&2 "$*"
+	printf >&2 '%s\n' "$*"
 	exit "$status"
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 49ccb38..4dbeddb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '
 	test $(cat file1) = Z
 '
 
+test_expect_success 'rebase -i error on commits with \ in message' '
+	current_head=$(git rev-parse HEAD)
+	test_when_finished "git rebase --abort; git reset --hard $current_head; rm -f error" &&
+	test_commit TO-REMOVE will-conflict old-content &&
+	test_commit "\temp" will-conflict new-content dummy &&
+	(
+	EDITOR=true &&
+	export EDITOR &&
+	test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
+	) &&
+	test_expect_code 1 grep  "	emp" error
+'
+
 test_done
-- 
1.8.3.3.797.gb72c616

Re: [PATCH] die_with_status: use "printf '%s\n'", not "echo"

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>>> +	grep -v "	" error
>>
>> Umm, doesn't that only test that _some_ line in the error does not
>> contain a tab?
>
> Indeed. It does work as the error message is just a one-liner, but let's
> be robust anyway.

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.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch