{"thread":{"id":"36153","subject":"Corner case bug caused by shell dependent behavior","startedAt":"2014-03-14T00:02:13Z","lastAt":"2014-03-14T23:53:00Z","messageCount":5,"participants":["Uwe Storbeck","Jonathan Nieder","Jeff King","David Kastrup"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236700","messageId":"20140314000213.GA3739@ibr.ch","threadId":"36153","inReplyTo":null,"subject":"Corner case bug caused by shell dependent behavior","fromName":"Uwe Storbeck","fromEmail":"uwe@ibr.ch","sentAt":"2014-03-14T00:02:13Z","receivedAt":"2014-03-14T00:02:13Z","isPatch":false,"sender":{"key":"uwe@ibr.ch","avatar":null},"body":"Hi\n\nWhen your system shell (/bin/sh) is a dash control sequences in\nstrings get interpreted by the echo command. A commit message\nwhich ends with the string '\\n' may result in a garbage line in\nthe todo list of an interactive rebase which causes the rebase\nto fail.\n\nTo reproduce the behavior (with dash as /bin/sh):\n\n  mkdir test && cd test && git init\n  echo 1 >foo && git add foo\n  git commit -m\"this commit message ends with '\\n'\"\n  echo 2 >foo && git commit -a --fixup HEAD\n  git rebase -i --autosquash --root\n\nNow the editor opens with garbage in line 3 which has to be\nremoved or the rebase fails.\n\nThe attached one-line patch fixes the bug. Be free to edit the\ncommit message when it's too long.\n\nMaybe there are more places where it would be more robust to use\nprintf instead of echo.\n\nUwe\n\n\n>From 53262bc8a7a3ec9d9a6b0e8ecaaea598257b87fe Mon Sep 17 00:00:00 2001\nFrom: Uwe Storbeck <uwe@ibr.ch>\nDate: Fri, 14 Mar 2014 00:28:33 +0100\nSubject: [PATCH] git-rebase--interactive: replace echo by printf\n\nto avoid shell dependent behavior.\n\nWhen your system shell (/bin/sh) is a dash control sequences in\nstrings get interpreted by the echo command. A commit message\nwhich ends with the string '\\n' may result in a garbage line in\nthe todo list of an interactive rebase which causes the rebase\nto fail.\n\nTo reproduce the behavior (with dash as /bin/sh):\n\n  mkdir test && cd test && git init\n  echo 1 >foo && git add foo\n  git commit -m\"this commit message ends with '\\n'\"\n  echo 2 >foo && git commit -a --fixup HEAD\n  git rebase -i --autosquash --root\n\nNow the editor opens with garbage in line 3 which has to be\nremoved or the rebase fails.\n---\n git-rebase--interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex a1adae8..3ffe14c 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -749,7 +749,7 @@ rearrange_squash () {\n \t\t\t\t\t;;\n \t\t\t\tesac\n \t\t\tdone\n-\t\t\techo \"$sha1 $action $prefix $rest\"\n+\t\t\tprintf \"%s %s %s %s\\n\" \"$sha1\" \"$action\" \"$prefix\" \"$rest\"\n \t\t\t# if it's a single word, try to resolve to a full sha1 and\n \t\t\t# emit a second copy. This allows us to match on both message\n \t\t\t# and on sha1 prefix\n-- \n1.9.0\n\n"},{"id":"236701","messageId":"20140314003701.GF15625@google.com","threadId":"36153","inReplyTo":"20140314000213.GA3739@ibr.ch","subject":"Re: Corner case bug caused by shell dependent behavior","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-03-14T00:37:01Z","receivedAt":"2014-03-14T00:37:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nUwe Storbeck wrote:\n\n> To reproduce the behavior (with dash as /bin/sh):\n>\n>   mkdir test && cd test && git init\n>   echo 1 >foo && git add foo\n>   git commit -m\"this commit message ends with '\\n'\"\n>   echo 2 >foo && git commit -a --fixup HEAD\n>   git rebase -i --autosquash --root\n>\n> Now the editor opens with garbage in line 3 which has to be\n> removed or the rebase fails.\n\nWould it make sense to add this as a test to e.g.\nt/t3404-rebase-interactive.sh?\n\n> The attached one-line patch fixes the bug.\n\nMay we have your sign-off?  (See Documentation/SubmittingPatches\nsection \"Sign your work\" for what this means.\n\nLooks obviously correct, so for what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks,\nJonathan\n"},{"id":"236707","messageId":"20140314022845.GA19757@sigill.intra.peff.net","threadId":"36153","inReplyTo":"20140314000213.GA3739@ibr.ch","subject":"Re: Corner case bug caused by shell dependent behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-14T02:28:46Z","receivedAt":"2014-03-14T02:28:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 14, 2014 at 01:02:13AM +0100, Uwe Storbeck wrote:\n\n> When your system shell (/bin/sh) is a dash control sequences in\n> strings get interpreted by the echo command. A commit message\n> which ends with the string '\\n' may result in a garbage line in\n> the todo list of an interactive rebase which causes the rebase\n> to fail.\n> \n> To reproduce the behavior (with dash as /bin/sh):\n> \n>   mkdir test && cd test && git init\n>   echo 1 >foo && git add foo\n>   git commit -m\"this commit message ends with '\\n'\"\n>   echo 2 >foo && git commit -a --fixup HEAD\n>   git rebase -i --autosquash --root\n\nHmph. We ran into this before and fixed all of the sites (e.g., d1c3b10\nand 938791c). This one appears to have been added a few months later\n(by 68d5d03).\n\n> Maybe there are more places where it would be more robust to use\n> printf instead of echo.\n\nFWIW, I just looked through the other uses of \"echo\" in git-rebase*.sh,\nand I think this is the only problematic case.\n\n> -\t\t\techo \"$sha1 $action $prefix $rest\"\n> +\t\t\tprintf \"%s %s %s %s\\n\" \"$sha1\" \"$action\" \"$prefix\" \"$rest\"\n\nLooks obviously correct. The echo just below here does not need the same\ntreatment, as \"$rest\" is the problematic bit (\"$prefix\" is always\n\"fixup\" or \"squash\").\n\n-Peff\n"},{"id":"236719","messageId":"87d2hpgvmj.fsf@fencepost.gnu.org","threadId":"36153","inReplyTo":"20140314022845.GA19757@sigill.intra.peff.net","subject":"Re: Corner case bug caused by shell dependent behavior","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-14T06:08:36Z","receivedAt":"2014-03-14T06:08:36Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmph. We ran into this before and fixed all of the sites (e.g., d1c3b10\n> and 938791c). This one appears to have been added a few months later\n> (by 68d5d03).\n>\n>> Maybe there are more places where it would be more robust to use\n>> printf instead of echo.\n>\n> FWIW, I just looked through the other uses of \"echo\" in git-rebase*.sh,\n> and I think this is the only problematic case.\n>\n>> -\t\t\techo \"$sha1 $action $prefix $rest\"\n>> +\t\t\tprintf \"%s %s %s %s\\n\" \"$sha1\" \"$action\" \"$prefix\" \"$rest\"\n>\n> Looks obviously correct. The echo just below here does not need the same\n> treatment, as \"$rest\" is the problematic bit (\"$prefix\" is always\n> \"fixup\" or \"squash\").\n\nI'd not rationalize this away by deep analysis.  Copy&paste is a thing,\nso to just use printf whenever _any_ seriously variable strings (source\nnot immediately the shell script itself, perhaps even _any_ nonconstant\nstrings) are involved keeps people from introducing bugs by following\napparent practice.\n\n-- \nDavid Kastrup\n"},{"id":"236760","messageId":"20140314235300.GA25079@ibr.ch","threadId":"36153","inReplyTo":"20140314003701.GF15625@google.com","subject":"Re: Corner case bug caused by shell dependent behavior","fromName":"Uwe Storbeck","fromEmail":"uwe@ibr.ch","sentAt":"2014-03-14T23:53:00Z","receivedAt":"2014-03-14T23:53:00Z","isPatch":false,"sender":{"key":"uwe@ibr.ch","avatar":null},"body":"On Mar 13, Jonathan Nieder wrote:\n> May we have your sign-off?  (See Documentation/SubmittingPatches\n> section \"Sign your work\" for what this means.\n\nI could have found that myself .. thanks! I'll try to follow it\nnow. :)\nI'll resend the patch. Hopefully I'll do it right.\n\n> Would it make sense to add this as a test to e.g.\n> t/t3404-rebase-interactive.sh?\n\nIt's a rather special case, so I'm not sure if it's worth it.\nI'll send a patch which adds a test for it. The test works for\nme, but as I don't understand the test mechanisms already good\nenough a few questions:\n\n- Is it correct to call the fake editor with an empty variable\n  FAKE_LINES when you want it to not change the todo list of a\n  rebase -i and use it as it is (the work is already done by the\n  autosquash option)? I can achieve the same with EDITOR=true.\n  What's the preferred way? Is there an advantage to use the fake\n  editor also in this case?\n- The tests in t3404-rebase-interactive.sh use their variables\n  a bit differently, some just set the variables, some export\n  the variables and some use a subshell to encapsulate them.\n  Also some of the tests reset their rebase state so that\n  subsequent tests, which also use rebase, do not fail when the\n  rebase fails. Other tests don't do that.\n  What's the expected resp. recommended behavior?\n\nWhile trying to understand the test mechanisms I stumbled over\ntwo other problematic uses of echo. These although only affect\nthe test output, not git itself.\n\nUwe\n"}]}