{"thread":{"id":"34625","subject":"git rebase -i error message interprets \\t in commit message","startedAt":"2013-08-06T15:44:43Z","lastAt":"2013-08-07T09:48:20Z","messageCount":14,"participants":["David Kastrup","Ramkumar Ramachandra","Matthieu Moy","Junio C Hamano","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"224681","messageId":"87k3jy6cyc.fsf@fencepost.gnu.org","threadId":"34625","inReplyTo":null,"subject":"git rebase -i error message interprets \\t in commit message","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2013-08-06T15:44:43Z","receivedAt":"2013-08-06T15:44:43Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"\nHi,\n\nI just got the following error message:\n\ndak@lola:/usr/local/tmp/lilypond$ git rebase -i\nWaiting for Emacs...\nerror: could not apply 16de9d2... Make tempo range \\tempo 20~30 be input as \\tempo 20-30 instead\n\nWhen you have resolved this problem, run \"git rebase --continue\".\nIf you prefer to skip this patch, run \"git rebase --skip\" instead.\nTo check out the original branch and stop rebasing, run \"git rebase --abort\".\nCould not apply 16de9d2... Make tempo range \tempo 20~30 be input as \tempo 20-30 instead\ndak@lola:/usr/local/tmp/lilypond$ git --version\ngit version 1.8.1.2\n\nAs you can see, the first message starting with \"error: could not apply\"\noutputs a reasonable rendition of the commit summary line. However, the\nfinal \"Could not apply\" message (starting with a capital C) converts\ninstances of \\t to a tab.\n\nThis is on Ubuntu 13.04 with\n\nlrwxrwxrwx 1 root root 4 Aug 15 2012 /bin/sh -> dash\n\n-- \nDavid Kastrup\n"},{"id":"224682","messageId":"CALkWK0mg4FTONDieaaW+OV0cbUpzb46gj39NcNmBMXSyey_JKQ@mail.gmail.com","threadId":"34625","inReplyTo":"87k3jy6cyc.fsf@fencepost.gnu.org","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-08-06T16:01:04Z","receivedAt":"2013-08-06T16:01:04Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"David Kastrup wrote:\n> As you can see, the first message starting with \"error: could not apply\"\n> outputs a reasonable rendition of the commit summary line. However, the\n> final \"Could not apply\" message (starting with a capital C) converts\n> instances of \\t to a tab.\n\nTo get you started:\n\n $ git grep \"could not apply\"\n sequencer.c=475=static int do_pick_commit(struct commit *commit,\n sequencer.c:628: : _(\"could not apply %s... %s\"),\n\n $ git grep \"Could not apply\"\n git-rebase--interactive.sh=431=die_failed_squash() {\n git-rebase--interactive.sh:436: warn \"Could not apply $1... $2\"\n git-rebase--interactive.sh=460=do_pick () {\n git-rebase--interactive.sh:476: die_with_patch $1 \"Could not apply $1... $2\"\n git-rebase--interactive.sh:479: die_with_patch $1 \"Could not apply $1... $2\"\n\nSo, it's the shell script. Now, read about shell escaping [1] and\nsubmit a patch.\n\nThanks.\n\n[1]: http://tldp.org/LDP/abs/html/escapingsection.html\n"},{"id":"224683","messageId":"vpqli4ekdni.fsf@anie.imag.fr","threadId":"34625","inReplyTo":"87k3jy6cyc.fsf@fencepost.gnu.org","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-06T16:05:53Z","receivedAt":"2013-08-06T16:05:53Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"David Kastrup writes:\n\n> Could not apply 16de9d2... Make tempo range \tempo 20~30 be input as \tempo 20-30 instead\n\nIndeed. The source of the problem is that our \"die\" shell function\ninterprets \\t (because it uses \"echo\").\n\nA simple fix would be this:\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a964ad..97258d5 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -53,7 +53,7 @@ die () {\n die_with_status () {\n status=$1\n shift\n- echo >&2 \"$*\"\n+ printf >&2 \"%s\\n\" \"$*\"\n exit \"$status\"\n }\n \nIt does not sound crazy as the shell function \"say\" right below uses the\nsame printf \"%s\\n\" \"$*\", but I'm wondering whether this could have other\nbad implications (e.g. if there are escape sequences in the commit\nmessage, aren't we going to screw up the terminal?).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"224686","messageId":"87fvum694z.fsf@fencepost.gnu.org","threadId":"34625","inReplyTo":"vpqli4ekdni.fsf@anie.imag.fr","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2013-08-06T17:07:08Z","receivedAt":"2013-08-06T17:07:08Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Matthieu Moy writes:\n\n> David Kastrup writes:\n>\n>> Could not apply 16de9d2... Make tempo range empo 20~30 be input as\n>> empo 20-30 instead\n>\n> Indeed. The source of the problem is that our \"die\" shell function\n> interprets \\t (because it uses \"echo\").\n>\n> A simple fix would be this:\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 7a964ad..97258d5 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -53,7 +53,7 @@ die () {\n> die_with_status () {\n> status=$1\n> shift\n> - echo >&2 \"$*\"\n> + printf >&2 \"%s\\n\" \"$*\"\n> exit \"$status\"\n> }\n> \n> It does not sound crazy as the shell function \"say\" right below uses the\n> same printf \"%s\\n\" \"$*\",\n\nSounds reasonable, though I don't know off-hand (not having the source\nhere) whether using \"say\" inside of die_with_status (and thus not having\ntwo different places to maintain) might not be feasible instead. It's\nnot like die_with_status is going to be called often enough to become a\nperformance hog.\n\n> but I'm wondering whether this could have other bad implications\n> (e.g. if there are escape sequences in the commit message, aren't we\n> going to screw up the terminal?).\n\nOne person's escape sequences are another's multibyte characters. Less\ncheesy, it would seem that the above change is a strict improvement and\nrequires little thought to implement.\n\nNot interpreting escape sequences is orthogonal from output\nsanitization.\n\n-- \nDavid Kastrup\n"},{"id":"224687","messageId":"vpqd2pqka9e.fsf@anie.imag.fr","threadId":"34625","inReplyTo":"87fvum694z.fsf@fencepost.gnu.org","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-06T17:19:09Z","receivedAt":"2013-08-06T17:19:09Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"David Kastrup writes:\n\n>> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n>> index 7a964ad..97258d5 100644\n>> --- a/git-sh-setup.sh\n>> +++ b/git-sh-setup.sh\n>> @@ -53,7 +53,7 @@ die () {\n>> die_with_status () {\n>> status=$1\n>> shift\n>> - echo >&2 \"$*\"\n>> + printf >&2 \"%s\\n\" \"$*\"\n>> exit \"$status\"\n>> }\n>> \n>> It does not sound crazy as the shell function \"say\" right below uses the\n>> same printf \"%s\\n\" \"$*\",\n>\n> Sounds reasonable, though I don't know off-hand (not having the source\n> here) whether using \"say\" inside of die_with_status \n\nThe definition of say is:\n\nsay () {\n\tif test -z \"$GIT_QUIET\"\n\tthen\n\t\tprintf '%s\\n' \"$*\"\n\tfi\n}\n\nI don't think we want to disable die's output even when the caller\nrequested to be quiet. Currently, my patch is:\n\n>From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001\nFrom: Matthieu Moy \nDate: Tue, 6 Aug 2013 19:13:03 +0200\nSubject: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"\n\nAt least GNU echo interprets backslashes in its arguments.\n\nThis triggered at least one bug: the error message of \"rebase -i\" was\nturning \\t in commit messages into actual tabulations. There may be\nothers.\n\nUsing \"printf '%s\\n'\" instead avoids this bad behavior, and is the form\nused by the \"say\" function.\n\nNoticed-by: David Kastrup \nSigned-off-by: Matthieu Moy \n---\n git-sh-setup.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a964ad..e15be51 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -53,7 +53,7 @@ die () {\n die_with_status () {\n status=$1\n shift\n- echo >&2 \"$*\"\n+ printf >&2 '%s\\n' \"$*\"\n exit \"$status\"\n }\n \n-- \n1.8.3.3.797.gb72c616\n\nI'll resend properly for inclusion if no one objects.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"224693","messageId":"7v1u66hfsk.fsf@alter.siamese.dyndns.org","threadId":"34625","inReplyTo":"CALkWK0mg4FTONDieaaW+OV0cbUpzb46gj39NcNmBMXSyey_JKQ@mail.gmail.com","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T17:47:55Z","receivedAt":"2013-08-06T17:47:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> So, it's the shell script. Now, read about shell escaping [1] and\n> submit a patch.\n\nThis is not about shell escaping at all. I think the message is fed\nto echo as-is, or to printf as its first parameter.\n"},{"id":"224694","messageId":"7vwqnyg10v.fsf@alter.siamese.dyndns.org","threadId":"34625","inReplyTo":"vpqli4ekdni.fsf@anie.imag.fr","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T17:52:16Z","receivedAt":"2013-08-06T17:52:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> David Kastrup writes:\n>\n>> Could not apply 16de9d2... Make tempo range \tempo 20~30 be input as \tempo 20-30 instead\n>\n> Indeed. The source of the problem is that our \"die\" shell function\n> interprets \\t (because it uses \"echo\").\n>\n> A simple fix would be this:\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 7a964ad..97258d5 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -53,7 +53,7 @@ die () {\n> die_with_status () {\n> status=$1\n> shift\n> - echo >&2 \"$*\"\n> + printf >&2 \"%s\\n\" \"$*\"\n> exit \"$status\"\n> }\n> \n> It does not sound crazy as the shell function \"say\" right below uses the\n> same printf \"%s\\n\" \"$*\", but I'm wondering whether this could have other\n> bad implications (e.g. if there are escape sequences in the commit\n> message, aren't we going to screw up the terminal?).\n\nI gave a quick look at \"git grep -e 'die ' -- \\*.sh\" output, and I\ndo not think this change will break things (i.e. no caller expects\nthe non-portable behaviour of the shell such as \"\\c\" at the end to\nomit the trailing newline).\n\nThanks, care to roll it into a patch with a test or two?\n"},{"id":"224697","messageId":"1375813604-10565-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"34625","inReplyTo":"7vwqnyg10v.fsf@alter.siamese.dyndns.org","subject":"[PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-06T18:26:44Z","receivedAt":"2013-08-06T18:26:44Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"At least GNU echo interprets backslashes in its arguments.\n\nThis triggered at least one bug: the error message of \"rebase -i\" was\nturning \\t in commit messages into actual tabulations. There may be\nothers.\n\nUsing \"printf '%s\\n'\" instead avoids this bad behavior, and is the form\nused by the \"say\" function.\n\nNoticed-by: David Kastrup \nSigned-off-by: Matthieu Moy \n---\n git-sh-setup.sh | 2 +-\n t/t3404-rebase-interactive.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a964ad..e15be51 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -53,7 +53,7 @@ die () {\n die_with_status () {\n \tstatus=$1\n \tshift\n-\techo >&2 \"$*\"\n+\tprintf >&2 '%s\\n' \"$*\"\n \texit \"$status\"\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 49ccb38..074deb1 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '\n \ttest $(cat file1) = Z\n '\n \n+test_expect_success 'rebase -i error on commits with \\ in message' '\n+\tcurrent_head=$(git rev-parse HEAD)\n+\ttest_when_finished \"git rebase --abort; git reset --hard $current_head; rm -f error\" &&\n+\ttest_commit TO-REMOVE will-conflict old-content &&\n+\ttest_commit \"\\temp\" will-conflict new-content dummy &&\n+\t(\n+\tEDITOR=true &&\n+\texport EDITOR &&\n+\ttest_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error\n+\t) &&\n+\tgrep -v \"\t\" error\n+'\n+\n test_done\n-- \n1.8.3.3.797.gb72c616\n"},{"id":"224706","messageId":"87bo5a62te.fsf@fencepost.gnu.org","threadId":"34625","inReplyTo":"vpqd2pqka9e.fsf@anie.imag.fr","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2013-08-06T19:23:41Z","receivedAt":"2013-08-06T19:23:41Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Matthieu Moy writes:\n\n>>From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001\n> From: Matthieu Moy \n> Date: Tue, 6 Aug 2013 19:13:03 +0200\n> Subject: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"\n>\n> At least GNU echo interprets backslashes in its arguments.\n\nI think that's incorrect in several respects. For one thing, echo is\nnever called for most Bourne shells since echo is a builtin (might have\nbeen different for UNIX version 7 or so). For another, GNU echo would\nbehave like Bash:\n\nAnd GNU Bash does not interpret escapes unless you do echo -e. Escape\nsequence interpretation, however, happens for Dash:\n\ndak@lola:/usr/local/tmp/lilypond$ dash -c 'echo \"x\\tx\"'\nx\tx\ndak@lola:/usr/local/tmp/lilypond$ bash -c 'echo \"x\\tx\"'\nx\\tx\ndak@lola:/usr/local/tmp/lilypond$ /bin/echo \"x\\tx\"\nx\\tx\n\nSo replace \"GNU echo\" in your commit message with \"Dash's echo builtin\"\nand you get closer.\n\n-- \nDavid Kastrup\n"},{"id":"224707","messageId":"7vhaf2fw9c.fsf@alter.siamese.dyndns.org","threadId":"34625","inReplyTo":"87bo5a62te.fsf@fencepost.gnu.org","subject":"Re: git rebase -i error message interprets \\t in commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T19:35:11Z","receivedAt":"2013-08-06T19:35:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> Matthieu Moy writes:\n>\n>>>From 7962ac8d8f2cbc556f669fd97487f9d70edc4ea1 Mon Sep 17 00:00:00 2001\n>> From: Matthieu Moy \n>> Date: Tue, 6 Aug 2013 19:13:03 +0200\n>> Subject: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"\n>>\n>> At least GNU echo interprets backslashes in its arguments.\n>\n> I think that's incorrect in several respects. For one thing, echo is\n> never called for most Bourne shells since echo is a builtin (might have\n> been different for UNIX version 7 or so). For another, GNU echo would\n> behave like Bash:\n>\n> And GNU Bash does not interpret escapes unless you do echo -e. Escape\n> sequence interpretation, however, happens for Dash:\n>\n> dak@lola:/usr/local/tmp/lilypond$ dash -c 'echo \"x\\tx\"'\n> x\tx\n> dak@lola:/usr/local/tmp/lilypond$ bash -c 'echo \"x\\tx\"'\n> x\\tx\n> dak@lola:/usr/local/tmp/lilypond$ /bin/echo \"x\\tx\"\n> x\\tx\n>\n> So replace \"GNU echo\" in your commit message with \"Dash's echo builtin\"\n> and you get closer.\n\nI'll queue the attached.\n\nPOSIX makes it an implementation defined behaviour when the first\nargument is \"-n\", or any argument contains a backslas (X/Open System\nInterfaces wants to treat \"-n\" as literal and always interpret the\nbackslash sequence), so it is definitely safer to avoid running\n'echo' on any random string.\n\nThanks.\n\nAuthor: Matthieu Moy \nDate: Tue Aug 6 20:26:44 2013 +0200\n\n die_with_status: use \"printf '%s\\n'\", not \"echo\"\n \n Some implementations of 'echo' (e.g. dash's built-in) interpret\n backslash sequences in their arguments.\n \n This triggered at least one bug: the error message of \"rebase -i\" was\n turning \\t in commit messages into actual tabulations. There may be\n others.\n \n Using \"printf '%s\\n'\" instead avoids this bad behavior, and is the form\n used by the \"say\" function.\n \n Noticed-by: David Kastrup \n Signed-off-by: Matthieu Moy \n Signed-off-by: Junio C Hamano \n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a964ad..e15be51 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -53,7 +53,7 @@ die () {\n die_with_status () {\n \tstatus=$1\n \tshift\n-\techo >&2 \"$*\"\n+\tprintf >&2 '%s\\n' \"$*\"\n \texit \"$status\"\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 49ccb38..074deb1 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '\n \ttest $(cat file1) = Z\n '\n \n+test_expect_success 'rebase -i error on commits with \\ in message' '\n+\tcurrent_head=$(git rev-parse HEAD)\n+\ttest_when_finished \"git rebase --abort; git reset --hard $current_head; rm -f error\" &&\n+\ttest_commit TO-REMOVE will-conflict old-content &&\n+\ttest_commit \"\\temp\" will-conflict new-content dummy &&\n+\t(\n+\tEDITOR=true &&\n+\texport EDITOR &&\n+\ttest_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error\n+\t) &&\n+\tgrep -v \"\t\" error\n+'\n+\n test_done\n"},{"id":"224737","messageId":"871u65yj0v.fsf@linux-k42r.v.cablecom.net","threadId":"34625","inReplyTo":"1375813604-10565-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-07T08:58:24Z","receivedAt":"2013-08-07T08:58:24Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Matthieu Moy writes:\n\n> At least GNU echo interprets backslashes in its arguments.\n>\n> This triggered at least one bug: the error message of \"rebase -i\" was\n> turning \\t in commit messages into actual tabulations. There may be\n> others.\n>\n> Using \"printf '%s\\n'\" instead avoids this bad behavior, and is the form\n> used by the \"say\" function.\n>\n> Noticed-by: David Kastrup \n> Signed-off-by: Matthieu Moy \n[...]\n> +test_expect_success 'rebase -i error on commits with \\ in message' '\n> +\tcurrent_head=$(git rev-parse HEAD)\n> +\ttest_when_finished \"git rebase --abort; git reset --hard $current_head; rm -f error\" &&\n> +\ttest_commit TO-REMOVE will-conflict old-content &&\n> +\ttest_commit \"\\temp\" will-conflict new-content dummy &&\n> +\t(\n> +\tEDITOR=true &&\n> +\texport EDITOR &&\n> +\ttest_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error\n> +\t) &&\n> +\tgrep -v \"\t\" error\n\nUmm, doesn't that only test that _some_ line in the error does not\ncontain a tab?\n\nWhereas you need to test that _no_ line contains emp, or some\nsuch. Perhaps as\n\n ! grep -v \"\temp\" error\n\n> +'\n> +\n> test_done\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224740","messageId":"vpqy58dj1ly.fsf@anie.imag.fr","threadId":"34625","inReplyTo":"871u65yj0v.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-07T09:23:37Z","receivedAt":"2013-08-07T09:23:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Thomas Rast writes:\n\n>> +\tgrep -v \"\t\" error\n>\n> Umm, doesn't that only test that _some_ line in the error does not\n> contain a tab?\n\nIndeed. It does work as the error message is just a one-liner, but let's\nbe robust anyway.\n\n> Whereas you need to test that _no_ line contains emp, or some\n> such. Perhaps as\n>\n> ! grep -v \"\temp\" error\n\nErr, then the -v should be remove. Also, I'll use test_expect_code 1\ninstead of ! to catch other grep failures, just in case.\n\nThanks, new patch comming.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"224741","messageId":"1375867565-21127-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"34625","inReplyTo":"vpqy58dj1ly.fsf@anie.imag.fr","subject":"[PATCH v2] die_with_status: use \"printf '%s\\n'\", not \"echo\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2013-08-07T09:26:05Z","receivedAt":"2013-08-07T09:26:05Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Some implementations of 'echo' (e.g. dash's built-in) interpret\nbackslash sequences in their arguments.\n\nThis triggered at least one bug: the error message of \"rebase -i\" was\nturning \\t in commit messages into actual tabulations. There may be\nothers.\n\nUsing \"printf '%s\\n'\" instead avoids this bad behavior, and is the form\nused by the \"say\" function.\n\nNoticed-by: David Kastrup \nSigned-off-by: Matthieu Moy \nSigned-off-by: Junio C Hamano \n---\nChanged the \"grep\" function to be more accurate. The test still\ncatches the old failure and pass after the fix.\n\n git-sh-setup.sh | 2 +-\n t/t3404-rebase-interactive.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 7a964ad..e15be51 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -53,7 +53,7 @@ die () {\n die_with_status () {\n \tstatus=$1\n \tshift\n-\techo >&2 \"$*\"\n+\tprintf >&2 '%s\\n' \"$*\"\n \texit \"$status\"\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 49ccb38..4dbeddb 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '\n \ttest $(cat file1) = Z\n '\n \n+test_expect_success 'rebase -i error on commits with \\ in message' '\n+\tcurrent_head=$(git rev-parse HEAD)\n+\ttest_when_finished \"git rebase --abort; git reset --hard $current_head; rm -f error\" &&\n+\ttest_commit TO-REMOVE will-conflict old-content &&\n+\ttest_commit \"\\temp\" will-conflict new-content dummy &&\n+\t(\n+\tEDITOR=true &&\n+\texport EDITOR &&\n+\ttest_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error\n+\t) &&\n+\ttest_expect_code 1 grep \"\temp\" error\n+'\n+\n test_done\n-- \n1.8.3.3.797.gb72c616\n"},{"id":"224742","messageId":"87haf1vnkr.fsf@linux-k42r.v.cablecom.net","threadId":"34625","inReplyTo":"vpqy58dj1ly.fsf@anie.imag.fr","subject":"Re: [PATCH] die_with_status: use \"printf '%s\\n'\", not \"echo\"","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-07T09:48:20Z","receivedAt":"2013-08-07T09:48:20Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Matthieu Moy writes:\n\n> Thomas Rast writes:\n>\n>>> +\tgrep -v \"\t\" error\n>>\n>> Umm, doesn't that only test that _some_ line in the error does not\n>> contain a tab?\n>\n> Indeed. It does work as the error message is just a one-liner, but let's\n> be robust anyway.\n\nErr, right. I actually applied the patch and verified that the test\n\"erroneously\" passed without the fix, but that's simply because I use\nbash instead of dash. The error message indeed is only one line, the\nrest of the rebase output goes to stdout.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}