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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
David Kastrup d...@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/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
Matthieu Moy matthieu@grenoble-inp.fr writes:
David Kastrup d...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
David Kastrup d...@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@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 d...@gnu.org
Signed-off-by: Matthieu Moy matthieu@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/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
Ramkumar Ramachandra artag...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
Matthieu Moy matthieu@grenoble-inp.fr writes:
David Kastrup d...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
Matthieu Moy matthieu@grenoble-inp.fr writes: From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001 From: Matthieu Moy matthieu@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i error message interprets \t in commit message
David Kastrup d...@gnu.org writes:
Matthieu Moy matthieu@grenoble-inp.fr writes:
From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001
From: Matthieu Moy matthieu@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@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 d...@gnu.org
Signed-off-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Junio C Hamano gits...@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^^ 2error
+ )
+ grep -v error
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
