{"thread":{"id":"48867","subject":"[PATCH] sequencer.c: terminate the last line of author-script properly","startedAt":"2018-07-12T11:59:44Z","lastAt":"2018-07-30T09:35:40Z","messageCount":27,"participants":["Akinori MUSHA","Junio C Hamano","Eric Sunshine","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352324","messageId":"86a7qwpt9g.knu@iDaemons.org","threadId":"48867","inReplyTo":null,"subject":"[PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Akinori MUSHA","fromEmail":"knu@idaemons.org","sentAt":"2018-07-12T11:18:52Z","receivedAt":"2018-07-12T11:59:44Z","isPatch":true,"sender":{"key":"knu@idaemons.org","avatar":null},"body":"It looks like write_author_script() intends to write out a file in\nBourne shell syntax, but it doesn't put a closing single quote on the\nlast line.\n\nThis patch makes .git/rebase-merge/author-script actually parsable by\nsh(1) by adding a single quote and a linefeed to terminate the line\nproperly.\n\nSigned-off-by: Akinori MUSHA \n---\n sequencer.c | 1 +\n t/t3404-rebase-interactive.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461..5f32b6df1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\tstrbuf_addstr(&buf, \"'\\n\");\n \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n \tstrbuf_release(&buf);\n \treturn res;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 352a52e59..345b103eb 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n+test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n+\ttest_when_finished \"git rebase --abort ||:\" &&\n+\tgit checkout master &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n+\ttest -f .git/rebase-merge/author-script &&\n+\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n+\teval \"$(cat .git/rebase-merge/author-script)\" &&\n+\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n+\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n+\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+'\n+\n test_expect_success 'rebase -i with the exec command' '\n \tgit checkout master &&\n \t(\n-- \n2.18.0\n\n\n-- \nAkinori MUSHA / https://akinori.org/\n"},{"id":"352370","messageId":"xmqqwou0xtar.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"86a7qwpt9g.knu@iDaemons.org","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T17:22:52Z","receivedAt":"2018-07-12T17:22:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Akinori MUSHA\" writes:\n\n> It looks like write_author_script() intends to write out a file in\n> Bourne shell syntax, but it doesn't put a closing single quote on the\n> last line.\n\ns/closing single quote/& and the terminating newline/?\n\n>\n> This patch makes .git/rebase-merge/author-script actually parsable by\n> sh(1) by adding a single quote and a linefeed to terminate the line\n> properly.\n\nSounds good.\n\nI wonder why this breakage was left unnoticed for a long time,\nthough. It's not like writing and reading the author-script from C\ncode was done first in the \"rebase -i\" and friends that are users of\nthe sequencer machinery (I think we had code to do so in \"git am\"\nthat was rewritten in C first). Do we have a similar issue over\nthere as well? If not, perhaps if we reused the existing code that\nwas not broken, we wouldn't have seen this breakage on the sequencer\nside?\n\nThanks.\n\n>\n> Signed-off-by: Akinori MUSHA \n> ---\n> sequencer.c | 1 +\n> t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 4034c0461..5f32b6df1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\tstrbuf_addstr(&buf, \"'\\n\");\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> \tstrbuf_release(&buf);\n> \treturn res;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 352a52e59..345b103eb 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout master &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> +\ttest -f .git/rebase-merge/author-script &&\n> +\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> +\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +'\n> +\n> test_expect_success 'rebase -i with the exec command' '\n> \tgit checkout master &&\n> \t(\n> -- \n> 2.18.0\n"},{"id":"352428","messageId":"xmqqa7qww6uk.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"86a7qwpt9g.knu@iDaemons.org","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:13:07Z","receivedAt":"2018-07-12T20:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Akinori MUSHA\" writes:\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 352a52e59..345b103eb 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout master &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> +\ttest -f .git/rebase-merge/author-script &&\n> +\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n\nIs this \"unset\" safe? Some POSIX compliant shells barf if you unset\na variable that is not set, so the answer to my question is yes only\nif we know these three variables are always set.\n\n> +\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n\nOh, actually it is even worse than that. What if author-script is\nbogus, like in the version before your patch fixes the code? We do\nnot restore the AUTHOR_NAME/EMAIL/DATE after this test_expect_success\nfails. How does that, i.e. missing some variable, affect execution\nof later steps in this same test script?\n\nI _think_ the right and safe way to fix taht is to do something like\nthis:\n\n\ttest -f .git/rebase-merge/author-script &&\n\t(\n\t\tsafe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&\n\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n\t\ttest ... &&\n\t\ttest ... &&\n\t\ttest ...\n\t)\n\nThat way, we won't have to worry about GIT_AUTHOR_* variables\ngetting modified and affecting the tests that come later in the\nscript.\n\n> +'\n> +\n> test_expect_success 'rebase -i with the exec command' '\n> \tgit checkout master &&\n> \t(\n> -- \n> 2.18.0\n"},{"id":"352430","messageId":"CAPig+cQWA+sE3iH89kQGWgS+0HQDK-V5++S0+F6_dpfOCfXxDg@mail.gmail.com","threadId":"48867","inReplyTo":"xmqqa7qww6uk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-12T20:16:50Z","receivedAt":"2018-07-12T20:17:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano wrote:\n> I _think_ the right and safe way to fix taht is to do something like\n> this:\n>\n> test -f .git/rebase-merge/author-script &&\n> (\n> safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&\n\ns/safe_unset/sane_unset/\n\n> eval \"$(cat .git/rebase-merge/author-script)\" &&\n> test ... &&\n> test ... &&\n> test ...\n> )\n"},{"id":"352432","messageId":"CAPc5daXRCtSSQTNxUz3DOL-sp=pgNKb13htmQaHR8KAjuUm3tg@mail.gmail.com","threadId":"48867","inReplyTo":"CAPig+cQWA+sE3iH89kQGWgS+0HQDK-V5++S0+F6_dpfOCfXxDg@mail.gmail.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:23:27Z","receivedAt":"2018-07-12T20:23:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yup ;-)\n\nOn Thu, Jul 12, 2018 at 1:16 PM, Eric Sunshine wrote:\n> On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano wrote:\n>> I _think_ the right and safe way to fix taht is to do something like\n>> this:\n>>\n>> test -f .git/rebase-merge/author-script &&\n>> (\n>> safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&\n>\n> s/safe_unset/sane_unset/\n>\n>> eval \"$(cat .git/rebase-merge/author-script)\" &&\n>> test ... &&\n>> test ... &&\n>> test ...\n>> )\n"},{"id":"352435","messageId":"xmqqr2k8uqlw.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"xmqqa7qww6uk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:49:15Z","receivedAt":"2018-07-12T20:49:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I _think_ the right and safe way to fix taht is to do something like\n> this:\n>\n> \ttest -f .git/rebase-merge/author-script &&\n> \t(\n> \t\tsafe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&\n> \t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> \t\ttest ... &&\n> \t\ttest ... &&\n> \t\ttest ...\n> \t)\n>\n> That way, we won't have to worry about GIT_AUTHOR_* variables\n> getting modified and affecting the tests that come later in the\n> script.\n\nIt turns out that the use of subshell is *essential* for this test,\nas GIT_AUTHOR_* variables are exported and must remain so. unsetting\nand reading back may allows us to ensure that shell variables have\nthe expected value, but then they are no longer exported, which will\nmean later tests will use whatever random author ident the person or\nthe 'bot who is running the tests, not the one expected to be used\nby the test author(s).\n\nFor tonight's pushout, I'll queue this on top.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Thu, 12 Jul 2018 13:23:02 -0700\nSubject: [PATCH] SQUASH???\n\n---\n t/t3404-rebase-interactive.sh | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 2d189da2f1..b0cef509ab 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n \tset_fake_editor &&\n \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n \ttest -f .git/rebase-merge/author-script &&\n-\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n-\teval \"$(cat .git/rebase-merge/author-script)\" &&\n-\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n-\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n-\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n+\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n+\t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n+\t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n+\t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+\t)\n '\n \n test_expect_success 'rebase -i with the exec command' '\n-- \n2.18.0-129-ge3331758f1\n\n"},{"id":"352844","messageId":"xmqqpnzlla1p.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"CAPig+cQWA+sE3iH89kQGWgS+0HQDK-V5++S0+F6_dpfOCfXxDg@mail.gmail.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T23:25:22Z","receivedAt":"2018-07-17T23:25:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll squash the following in (which I have been carrying in 'pu' for\nthe past few days) unless I hear otherwise soonish to correct the\nissues raised during the review.\n\nThanks.\n\n t/t3404-rebase-interactive.sh | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 2d189da2f1..b0cef509ab 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n \tset_fake_editor &&\n \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n \ttest -f .git/rebase-merge/author-script &&\n-\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n-\teval \"$(cat .git/rebase-merge/author-script)\" &&\n-\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n-\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n-\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n+\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n+\t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n+\t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n+\t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+\t)\n '\n \n test_expect_success 'rebase -i with the exec command' '\n-- \n2.18.0-129-ge3331758f1\n\n"},{"id":"352847","messageId":"868t69oye0.knu@iDaemons.org","threadId":"48867","inReplyTo":"xmqqpnzlla1p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Akinori MUSHA","fromEmail":"knu@idaemons.org","sentAt":"2018-07-18T06:23:35Z","receivedAt":"2018-07-18T06:23:49Z","isPatch":true,"sender":{"key":"knu@idaemons.org","avatar":null},"body":"That's perfectly fine with me. I just thought each test case would\nrun in a separate shell process and that's why I chose not to use a\nsubshell for the last lines. I've learned a lot from feedback from\nyou all. Thanks!\n\nOn Wed, 18 Jul 2018 08:25:22 +0900,\nJunio C Hamano wrote:\n> \n> I'll squash the following in (which I have been carrying in 'pu' for\n> the past few days) unless I hear otherwise soonish to correct the\n> issues raised during the review.\n> \n> Thanks.\n> \n> t/t3404-rebase-interactive.sh | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 2d189da2f1..b0cef509ab 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> \ttest -f .git/rebase-merge/author-script &&\n> -\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> -\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> -\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> -\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> -\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +\t(\n> +\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> +\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> +\t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> +\t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +\t)\n> '\n> \n> test_expect_success 'rebase -i with the exec command' '\n> -- \n> 2.18.0-129-ge3331758f1\n> \n"},{"id":"352849","messageId":"f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net","threadId":"48867","inReplyTo":"86a7qwpt9g.knu@iDaemons.org","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T09:25:54Z","receivedAt":"2018-07-18T09:26:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Akinori\n\nOn 12/07/18 12:18, Akinori MUSHA wrote:\n> \n> It looks like write_author_script() intends to write out a file in\n> Bourne shell syntax, but it doesn't put a closing single quote on the\n> last line.\n> \n> This patch makes .git/rebase-merge/author-script actually parsable by\n> sh(1) by adding a single quote and a linefeed to terminate the line\n> properly.\n> \n> Signed-off-by: Akinori MUSHA \n> ---\n> sequencer.c | 1 +\n> t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 4034c0461..5f32b6df1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\tstrbuf_addstr(&buf, \"'\\n\");\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n\nThe third parameter here means that write_message() will append a new \nline (you can check this by looking at the file that's created) so \nstrictly speaking we only need to add \"'\" to the end of the message, \nalternatively it would be more obvious to keep adding \"'\\n\" and change 1 \nto 0 in the call to write_message()\n\nBest Wishes\n\nPhillip\n\n> \tstrbuf_release(&buf);\n> \treturn res;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 352a52e59..345b103eb 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout master &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> +\ttest -f .git/rebase-merge/author-script &&\n> +\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> +\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +'\n> +\n> test_expect_success 'rebase -i with the exec command' '\n> \tgit checkout master &&\n> \t(\n> \n\n"},{"id":"352852","messageId":"eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net","threadId":"48867","inReplyTo":"xmqqwou0xtar.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T09:45:26Z","receivedAt":"2018-07-18T09:45:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 12/07/18 18:22, Junio C Hamano wrote:\n> \n> \"Akinori MUSHA\" writes:\n> \n>> It looks like write_author_script() intends to write out a file in\n>> Bourne shell syntax, but it doesn't put a closing single quote on the\n>> last line.\n> \n> s/closing single quote/& and the terminating newline/?\n> \n>>\n>> This patch makes .git/rebase-merge/author-script actually parsable by\n>> sh(1) by adding a single quote and a linefeed to terminate the line\n>> properly.\n> \n> Sounds good.\n> \n> I wonder why this breakage was left unnoticed for a long time,\n> though. It's not like writing and reading the author-script from C\n> code was done first in the \"rebase -i\" and friends that are users of\n> the sequencer machinery \n\nThe only consumer of a faulty author script written by the sequencer is \nread_env_script() in sequencer.c which doesn't worry about checking that \nquotes are paired. The shell versions of rebase use the code in \ngit-sh-setup.sh to create the author script which does write valid posix \nshell.\n\n> (I think we had code to do so in \"git am\"\n> that was rewritten in C first).\n\nThe code in builtin/am.c doesn't try to write valid posix shell (if one \nassumes it is the only consumer of the author script then it doesn't \nneed to) which results in simpler code, but external scripts cannot \nsafely eval it anymore.\n\n> Do we have a similar issue over\n> there as well? If not, perhaps if we reused the existing code that\n> was not broken, we wouldn't have seen this breakage on the sequencer\n> side?\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n>>\n>> Signed-off-by: Akinori MUSHA \n>> ---\n>> sequencer.c | 1 +\n>> t/t3404-rebase-interactive.sh | 13 +++++++++++++\n>> 2 files changed, 14 insertions(+)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 4034c0461..5f32b6df1 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n>> \t\t\tstrbuf_addch(&buf, *(message++));\n>> \t\telse\n>> \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>> +\tstrbuf_addstr(&buf, \"'\\n\");\n>> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n>> \tstrbuf_release(&buf);\n>> \treturn res;\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 352a52e59..345b103eb 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n>> \ttest_line_count = 6 actual\n>> '\n>> \n>> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n>> +\ttest_when_finished \"git rebase --abort ||:\" &&\n>> +\tgit checkout master &&\n>> +\tset_fake_editor &&\n>> +\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n>> +\ttest -f .git/rebase-merge/author-script &&\n>> +\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n>> +\teval \"$(cat .git/rebase-merge/author-script)\" &&\n>> +\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n>> +\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n>> +\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n>> +'\n>> +\n>> test_expect_success 'rebase -i with the exec command' '\n>> \tgit checkout master &&\n>> \t(\n>> -- \n>> 2.18.0\n\n"},{"id":"352866","messageId":"eb295aea-dae5-5e1c-bacf-2c77d3ce0195@talktalk.net","threadId":"48867","inReplyTo":"eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T13:46:17Z","receivedAt":"2018-07-18T13:46:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/07/18 10:45, Phillip Wood wrote:\n> \n> Hi Junio\n> \n> On 12/07/18 18:22, Junio C Hamano wrote:\n>>\n>> \"Akinori MUSHA\" writes:\n>>\n>>> It looks like write_author_script() intends to write out a file in\n>>> Bourne shell syntax, but it doesn't put a closing single quote on the\n>>> last line.\n>>\n>> s/closing single quote/& and the terminating newline/?\n>>\n>>>\n>>> This patch makes .git/rebase-merge/author-script actually parsable by\n>>> sh(1) by adding a single quote and a linefeed to terminate the line\n>>> properly.\n>>\n>> Sounds good.\n>>\n>> I wonder why this breakage was left unnoticed for a long time,\n>> though.  It's not like writing and reading the author-script from C\n>> code was done first in the \"rebase -i\" and friends that are users of\n>> the sequencer machinery \n> \n> The only consumer of a faulty author script written by the sequencer is \n> read_env_script() in sequencer.c which doesn't worry about checking that \n> quotes are paired. \n\nThat's not quite true anymore, recently another consumer \nread_author_ident() was added which uses sq_dequote() instead of custom \ncode. Looking more closely at write_author_script() the quoting of \nsingle quotes is buggy they are escaped as \\\\' instead of \\'. The code \nin read_env_script() expects the buggy quoting so it works. I just tried \nthe code path that uses sq_dequote() by doing rebase --root with an \nauthor containing a single quote and that worked but I'm not sure why.\n\nThe shell versions of rebase use the code in\n> git-sh-setup.sh to create the author script which does write valid posix \n> shell.\n> \n>> (I think we had code to do so in \"git am\"\n>> that was rewritten in C first).\n> \n> The code in builtin/am.c doesn't try to write valid posix shell (if one \n> assumes it is the only consumer of the author script then it doesn't \n> need to) \n\nThough it will break if git gets upgraded while am is stopped for a \nconflict resolution. Given that the code has been around for a while \nthat may not be much of a concern now\n\nwhich results in simpler code, but external scripts cannot\n> safely eval it anymore.\n> \n>>  Do we have a similar issue over\n>> there as well?  If not, perhaps if we reused the existing code that\n>> was not broken, we wouldn't have seen this breakage on the sequencer\n>> side?\n> \n> Best Wishes\n> \n> Phillip\n> \n>> Thanks.\n>>\n>>>\n>>> Signed-off-by: Akinori MUSHA \n>>> ---\n>>>   sequencer.c                   |  1 +\n>>>   t/t3404-rebase-interactive.sh | 13 +++++++++++++\n>>>   2 files changed, 14 insertions(+)\n>>>\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 4034c0461..5f32b6df1 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n>>>               strbuf_addch(&buf, *(message++));\n>>>           else\n>>>               strbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++)) >>> +    strbuf_addstr(&buf, \"'\\n\");\n>>>       res = write_message(buf.buf, buf.len, \n>>> rebase_path_author_script(), 1);\n>>>       strbuf_release(&buf);\n>>>       return res;\n>>> diff --git a/t/t3404-rebase-interactive.sh \n>>> b/t/t3404-rebase-interactive.sh\n>>> index 352a52e59..345b103eb 100755\n>>> --- a/t/t3404-rebase-interactive.sh\n>>> +++ b/t/t3404-rebase-interactive.sh\n>>> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n>>>       test_line_count = 6 actual\n>>>   '\n>>> +test_expect_success 'rebase -i writes out \n>>> .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n>>> +    test_when_finished \"git rebase --abort ||:\" &&\n>>> +    git checkout master &&\n>>> +    set_fake_editor &&\n>>> +    FAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n>>> +    test -f .git/rebase-merge/author-script &&\n>>> +    unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n>>> +    eval \"$(cat .git/rebase-merge/author-script)\" &&\n>>> +    test \"$(git show --quiet --pretty=format:%an)\" = \n>>> \"$GIT_AUTHOR_NAME\" &&\n>>> +    test \"$(git show --quiet --pretty=format:%ae)\" = \n>>> \"$GIT_AUTHOR_EMAIL\" &&\n>>> +    test \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \n>>> \"$GIT_AUTHOR_DATE\"\n>>> +'\n>>> +\n>>>   test_expect_success 'rebase -i with the exec command' '\n>>>       git checkout master &&\n>>>       (\n>>> -- \n>>> 2.18.0\n> \n\n"},{"id":"352867","messageId":"aad05e61-db99-429e-bbe6-a8f91ef18e3f@talktalk.net","threadId":"48867","inReplyTo":"86a7qwpt9g.knu@iDaemons.org","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T13:50:23Z","receivedAt":"2018-07-18T13:50:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Akinori\nOn 12/07/18 12:18, Akinori MUSHA wrote:\n> \n> It looks like write_author_script() intends to write out a file in\n> Bourne shell syntax, but it doesn't put a closing single quote on the\n> last line.\n> \n> This patch makes .git/rebase-merge/author-script actually parsable by\n> sh(1) by adding a single quote and a linefeed to terminate the line\n> properly.\n> \n> Signed-off-by: Akinori MUSHA \n> ---\n> sequencer.c | 1 +\n> t/t3404-rebase-interactive.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 4034c0461..5f32b6df1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\tstrbuf_addstr(&buf, \"'\\n\");\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> \tstrbuf_release(&buf);\n> \treturn res;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 352a52e59..345b103eb 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n> +\ttest_when_finished \"git rebase --abort ||:\" &&\n> +\tgit checkout master &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> +\ttest -f .git/rebase-merge/author-script &&\n> +\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> +\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> +\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +'\n\nHave you checked that this test fails without your fix being applied? I \njust ran\n\nsh -c 'eval \"$(cat .git/rebase-merge/author-script)\"; echo \n\"$GIT_AUTHOR_NAME\"'\n\nwhile a rebase was stopped for an edit and it worked despite the fact \nthat there is a missing quote at the end of the GIT_AUTHOR_DATE in the \nauthor script file.\n\nBest Wishes\n\nPhillip\n\n> test_expect_success 'rebase -i with the exec command' '\n> \tgit checkout master &&\n> \t(\n> \n\n"},{"id":"352868","messageId":"a39862e1-ad27-6989-6a32-1898448525f0@talktalk.net","threadId":"48867","inReplyTo":"aad05e61-db99-429e-bbe6-a8f91ef18e3f@talktalk.net","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T13:58:08Z","receivedAt":"2018-07-18T13:58:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/07/18 14:50, Phillip Wood wrote:\n> \n> Hi Akinori\n> On 12/07/18 12:18, Akinori MUSHA wrote:\n>>\n>> It looks like write_author_script() intends to write out a file in\n>> Bourne shell syntax, but it doesn't put a closing single quote on the\n>> last line.\n>>\n>> This patch makes .git/rebase-merge/author-script actually parsable by\n>> sh(1) by adding a single quote and a linefeed to terminate the line\n>> properly.\n>>\n>> Signed-off-by: Akinori MUSHA \n>> ---\n>>   sequencer.c                   |  1 +\n>>   t/t3404-rebase-interactive.sh | 13 +++++++++++++\n>>   2 files changed, 14 insertions(+)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 4034c0461..5f32b6df1 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)\n>>               strbuf_addch(&buf, *(message++));\n>>           else\n>>               strbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>> +    strbuf_addstr(&buf, \"'\\n\");\n>>       res = write_message(buf.buf, buf.len, \n>> rebase_path_author_script(), 1);\n>>       strbuf_release(&buf);\n>>       return res;\n>> diff --git a/t/t3404-rebase-interactive.sh \n>> b/t/t3404-rebase-interactive.sh\n>> index 352a52e59..345b103eb 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -75,6 +75,19 @@ test_expect_success 'rebase --keep-empty' '\n>>       test_line_count = 6 actual\n>>   '\n>> +test_expect_success 'rebase -i writes out \n>> .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n>> +    test_when_finished \"git rebase --abort ||:\" &&\n>> +    git checkout master &&\n>> +    set_fake_editor &&\n>> +    FAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n>> +    test -f .git/rebase-merge/author-script &&\n>> +    unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n>> +    eval \"$(cat .git/rebase-merge/author-script)\" &&\n>> +    test \"$(git show --quiet --pretty=format:%an)\" = \n>> \"$GIT_AUTHOR_NAME\" &&\n>> +    test \"$(git show --quiet --pretty=format:%ae)\" = \n>> \"$GIT_AUTHOR_EMAIL\" &&\n>> +    test \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \n>> \"$GIT_AUTHOR_DATE\"\n>> +'\n> \n> Have you checked that this test fails without your fix being applied? I \n> just ran\n> \n> sh -c 'eval \"$(cat .git/rebase-merge/author-script)\"; echo \n> \"$GIT_AUTHOR_NAME\"'\n> \n> while a rebase was stopped for an edit and it worked despite the fact \n> that there is a missing quote at the end of the GIT_AUTHOR_DATE in the \n> author script file.\n> \n> Best Wishes\n> \n> Phillip\n\nPlease ignore that, I messed up, there was a closing \"'\" as I was using \nthe shell version by a mistake.\n\nSorry for the noise\n\nPhillip\n\n>>   test_expect_success 'rebase -i with the exec command' '\n>>       git checkout master &&\n>>       (\n>>\n> \n\n"},{"id":"352882","messageId":"20180718155518.1025-1-phillip.wood@talktalk.net","threadId":"48867","inReplyTo":"eb295aea-dae5-5e1c-bacf-2c77d3ce0195@talktalk.net","subject":"[RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-18T15:55:18Z","receivedAt":"2018-07-18T15:55:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSingle quotes should be escaped as \\' not \\\\'. Note that this only\naffects authors that contain a single quote and then only external\nscripts that read the author script and users whose git is upgraded from\nthe shell version of rebase -i while rebase was stopped. This is because\nthe parsing in read_env_script() expected the broken version and for\nsome reason sq_dequote() called by read_author_ident() seems to handle\nthe broken quoting correctly.\n\nIdeally write_author_script() would be rewritten to use\nsplit_ident_line() and sq_quote_buf() but this commit just fixes the\nimmediate bug.\n\nSigned-off-by: Phillip Wood \n---\n\nThis is untested, unfortuantely I don't have really have time to write a test or\nfollow this up at the moment, if someone else want to run with it then please\ndo.\n\nsequencer.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5354d4d51e..0b78d1f100 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (skip_prefix(message, \"> \", &message))\n \t\t\tbreak;\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n \tstrbuf_release(&buf);\n \treturn res;\n@@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n {\n \tstruct strbuf script = STRBUF_INIT;\n \tint i, count = 0;\n-\tchar *p, *p2;\n+\tconst char *p2;\n+\tchar *p;\n \n \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n \t\treturn -1;\n \n \tfor (p = script.buf; *p; p++)\n-\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n+\t\t/*\n+\t\t * write_author_script() used to escape \"'\" incorrectly as\n+\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n+\t\t * version the incorrect version in case git was upgraded while\n+\t\t * rebase was stopped.\n+\t\t */\n+\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n+\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n \t\telse if (*p == '\\'')\n \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\n-- \n2.18.0\n\n"},{"id":"352920","messageId":"xmqq7elslayl.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T17:17:54Z","receivedAt":"2018-07-18T17:17:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> (I think we had code to do so in \"git am\"\n>> that was rewritten in C first).\n>\n> The code in builtin/am.c doesn't try to write valid posix shell (if\n> one assumes it is the only consumer of the author script then it\n> doesn't need to) which results in simpler code, but external scripts\n> cannot safely eval it anymore.\n\nAre you sure about that? If so we probably should see if we can fix\nthe writer, and better yet, if we can share code with the writer\ndiscussed here, as presumably we are fixing it in this thread.\n\nBut I do not see how builtin/am.c::write_author_script() would\nproduce something that would not eval correctly. sq_quote_buf() was\nintroduced specifically to write correct string for shell's\nconsumption.\n"},{"id":"352923","messageId":"xmqq36wglamv.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"eb295aea-dae5-5e1c-bacf-2c77d3ce0195@talktalk.net","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T17:24:56Z","receivedAt":"2018-07-18T17:25:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> The only consumer of a faulty author script written by the sequencer\n>> is read_env_script() in sequencer.c which doesn't worry about\n>> checking that quotes are paired. \n>\n> That's not quite true anymore, recently another consumer\n> read_author_ident() was added which uses sq_dequote() instead of\n> custom code. Looking more closely at write_author_script() the quoting\n> of single quotes is buggy they are escaped as \\\\' instead of \\'.\n\nThat's embarrassing re-invention (instead of reuse) with additional\nbug X-<. It seems that all of that blame to d87d48b2 (\"sequencer:\nlearn about the special \"fake root commit\" handling\", 2018-05-04).\n\nWe should fix both broken writer and readers that compensate for\nbreakage in the writer, I guess. Sigh...\n\n"},{"id":"353017","messageId":"9ecdf160-bc8b-b91d-a70e-ca6fb9d2a756@talktalk.net","threadId":"48867","inReplyTo":"xmqq7elslayl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-19T09:20:53Z","receivedAt":"2018-07-19T09:20:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 18/07/18 18:17, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>> (I think we had code to do so in \"git am\"\n>>> that was rewritten in C first).\n>>\n>> The code in builtin/am.c doesn't try to write valid posix shell (if\n>> one assumes it is the only consumer of the author script then it\n>> doesn't need to) which results in simpler code, but external scripts\n>> cannot safely eval it anymore.\n> \n> Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer\n> discussed here, as presumably we are fixing it in this thread.\n> \n> But I do not see how builtin/am.c::write_author_script() would\n> produce something that would not eval correctly. sq_quote_buf() was\n> introduced specifically to write correct string for shell's\n> consumption.\n\nYou're right, I'm not sure how I missed the calls to sq_quote_buf()\nyesterday, sharing the am code with the sequencer would clean things up\nnicely.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"353467","messageId":"xmqq36w81wh1.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"20180718155518.1025-1-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-24T15:31:38Z","receivedAt":"2018-07-24T15:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Single quotes should be escaped as \\' not \\\\'. Note that this only\n> affects authors that contain a single quote and then only external\n> scripts that read the author script and users whose git is upgraded from\n> the shell version of rebase -i while rebase was stopped. This is because\n> the parsing in read_env_script() expected the broken version and for\n> some reason sq_dequote() called by read_author_ident() seems to handle\n> the broken quoting correctly.\n>\n> Ideally write_author_script() would be rewritten to use\n> split_ident_line() and sq_quote_buf() but this commit just fixes the\n> immediate bug.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> This is untested, unfortuantely I don't have really have time to write a test or\n> follow this up at the moment, if someone else want to run with it then please\n> do.\n\nAny follow-up on this?\n\nDscho, I do agree with Phillip that the author-script this code\nproduces does not quote single quotes correctly (if we consider that\nthe author-script is meant to be compatible with shell script\nversion and what \"git am\" uses, that is), and Phillip's fix looks\nlike a reasonable first step going forward (the next step would\nremove the transitional band-aid from the reading code, also added\nby this patch).\n\nThanks.\n\n>\n> sequencer.c | 18 +++++++++++++-----\n> 1 file changed, 13 insertions(+), 5 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 5354d4d51e..0b78d1f100 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n> \t\telse if (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n> \twhile (*message && *message != '\\n' && *message != '\\r')\n> \t\tif (skip_prefix(message, \"> \", &message))\n> \t\t\tbreak;\n> \t\telse if (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n> \twhile (*message && *message != '\\n' && *message != '\\r')\n> \t\tif (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> \tstrbuf_release(&buf);\n> \treturn res;\n> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n> {\n> \tstruct strbuf script = STRBUF_INIT;\n> \tint i, count = 0;\n> -\tchar *p, *p2;\n> +\tconst char *p2;\n> +\tchar *p;\n> \n> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> \t\treturn -1;\n> \n> \tfor (p = script.buf; *p; p++)\n> -\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n> +\t\t/*\n> +\t\t * write_author_script() used to escape \"'\" incorrectly as\n> +\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n> +\t\t * version the incorrect version in case git was upgraded while\n> +\t\t * rebase was stopped.\n> +\t\t */\n> +\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n> +\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> \t\telse if (*p == '\\'')\n> \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\n"},{"id":"353636","messageId":"nycvar.QRO.7.76.6.1807261347040.71@tvgsbejvaqbjf.bet","threadId":"48867","inReplyTo":"xmqqpnzlla1p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T12:07:07Z","receivedAt":"2018-07-26T12:07:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 17 Jul 2018, Junio C Hamano wrote:\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 2d189da2f1..b0cef509ab 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n\nYou missed a very long line here.\n\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> \ttest -f .git/rebase-merge/author-script &&\n\nWhy do we need this, if we already have an `eval` later on?\n\n> -\tunset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> -\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> -\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> -\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> -\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> +\t(\n> +\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> +\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n\nWhy not\n\n\t. .git/rebase-merge/author-script\n\ninstead? Less roundabout, easier to read, I think.\n\n> +\t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n\nHow is this even working without `-s`?\n\n*clicketyclick*\n\nAh, --quiet does this. Wait. `git show --quiet` is not even documented.\n\nAll of those lines are too long, though. I am surprised you did not catch\nthat.\n\nBesides, this would be more compact, less repetitive, *and* more readable\nas\n\n\ttest \"$(git show -s --date=raw --format=%an,%ae,@%ad)\" = \\\n\t\t\"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n\nt3404-rebase-interactive.sh already takes 8 minutes (last I checked,\nanyway) to run on a *fast* machine. There is absolutely no need to\nintroduce even more spawning, not when it is so easily avoided.\n\n> +\t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> +\t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n\nIt is a shame that we cannot use %at directly here.\n\n> +\t)\n> '\n> \n> test_expect_success 'rebase -i with the exec command' '\n\nNote: this is not a criticism of the original patch. It is a criticism of\nthe review which could really have been better.\n\nI also saw that the test_when_finished uses a shell construct that shell\nscript aficionados might like, but these days it is a lot better to use\n`test_might_fail` instead. Let's do this, then.\n\nSo here goes, the clean-up patch on top of your 843654e435e (why does it\nhave to be so darned tedious to get from a mail to the corresponding\ncommit in `pu`), in all its glory:\n\n-- snipsnap --\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b0cef509ab7..97f0b4bf881 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,18 +75,16 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n-test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse' '\n-\ttest_when_finished \"git rebase --abort ||:\" &&\n+test_expect_success 'rebase -i writes correct author-script' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout master &&\n \tset_fake_editor &&\n \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n-\ttest -f .git/rebase-merge/author-script &&\n \t(\n \t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n-\t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n-\t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n-\t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n-\t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n+\t\t. .git/rebase-merge/author-script &&\n+\t\ttest \"$(git show -s --date=raw --format=%an,%ae,@%ad)\" = \\\n+\t\t\t\"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n \t)\n '\n \n"},{"id":"353639","messageId":"nycvar.QRO.7.76.6.1807261332130.71@tvgsbejvaqbjf.bet","threadId":"48867","inReplyTo":"20180718155518.1025-1-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T12:33:46Z","receivedAt":"2018-07-26T12:34:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 18 Jul 2018, Phillip Wood wrote:\n\n> From: Phillip Wood \n> \n> Single quotes should be escaped as \\' not \\\\'. Note that this only\n> affects authors that contain a single quote and then only external\n> scripts that read the author script and users whose git is upgraded from\n> the shell version of rebase -i while rebase was stopped. This is because\n> the parsing in read_env_script() expected the broken version and for\n> some reason sq_dequote() called by read_author_ident() seems to handle\n> the broken quoting correctly.\n> \n> Ideally write_author_script() would be rewritten to use\n> split_ident_line() and sq_quote_buf() but this commit just fixes the\n> immediate bug.\n> \n> Signed-off-by: Phillip Wood \n> ---\n\nGood catch.\n\n> This is untested, unfortuantely I don't have really have time to write a test or\n> follow this up at the moment, if someone else want to run with it then please\n> do.\n\nI modified the test that was added by Akinori. As it was added very early,\nand as there is still a test case *after* Akinori's that compares a\nhard-coded SHA-1, I refrained from using `test_commit` (which would change\nthat SHA-1). See below.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 5354d4d51e..0b78d1f100 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n> \t\telse if (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n> \twhile (*message && *message != '\\n' && *message != '\\r')\n> \t\tif (skip_prefix(message, \"> \", &message))\n> \t\t\tbreak;\n> \t\telse if (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n> \twhile (*message && *message != '\\n' && *message != '\\r')\n> \t\tif (*message != '\\'')\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n\nI resolved the merge conflict with Akinori's patch. FWIW I pushed all of\nthis, including the fixup to Junio's fixup to the\n`fix-t3404-author-script-test` branch at https://github.com/dscho/git.\n\n> \tstrbuf_release(&buf);\n> \treturn res;\n> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n> {\n> \tstruct strbuf script = STRBUF_INIT;\n> \tint i, count = 0;\n> -\tchar *p, *p2;\n> +\tconst char *p2;\n> +\tchar *p;\n> \n> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> \t\treturn -1;\n> \n> \tfor (p = script.buf; *p; p++)\n> -\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n> +\t\t/*\n> +\t\t * write_author_script() used to escape \"'\" incorrectly as\n> +\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n> +\t\t * version the incorrect version in case git was upgraded while\n> +\t\t * rebase was stopped.\n> +\t\t */\n> +\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n> +\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n\nI think in this form, it is possibly unsafe because it assumes that the\nnew code cannot generate output that would trigger that same code path.\nAlthough I have to admit that I did not give this a great deal of thought.\n\nIn any case, if you have to think long and hard about some fix, it might\nbe better to go with something that is easier to reason about. So how\nabout this: we already know that the code is buggy, Akinori fixed the bug,\nwhere the author-script missed its trailing single-quote. We can use this\nas a tell-tale for *this* bug. Assuming that Junio will advance both your\nand Akinori's fix in close proximity.\n\nAgain, this is pushed to the `fix-t3404-author-script-test` branch at\nhttps://github.com/dscho/git; My fixup on top of your patch looks like\nthis (feel free to drop the sq_bug part and only keep the test part):\n\n-- snipsnap --\ndiff --git a/sequencer.c b/sequencer.c\nindex 46c0b3e720f..7abe78dc78e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -573,13 +573,14 @@ static int write_author_script(const char *message)\n static int read_env_script(struct argv_array *env)\n {\n \tstruct strbuf script = STRBUF_INIT;\n-\tint i, count = 0;\n+\tint i, count = 0, sq_bug;\n \tconst char *p2;\n \tchar *p;\n \n \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n \t\treturn -1;\n \n+\tsq_bug = script.len && script.buf[script.len - 1] != '\\'';\n \tfor (p = script.buf; *p; p++)\n \t\t/*\n \t\t * write_author_script() used to escape \"'\" incorrectly as\n@@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)\n \t\t * version the incorrect version in case git was upgraded while\n \t\t * rebase was stopped.\n \t\t */\n-\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n-\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n+\t\tif (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n+\t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n+\t\telse if (skip_prefix(p, \"'\\\\''\", &p2))\n \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n \t\telse if (*p == '\\'')\n \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 97f0b4bf881..dd726ff4dc4 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n+SQ=\"'\"\n test_expect_success 'rebase -i writes correct author-script' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n-\tgit checkout master &&\n+\tgit checkout -b author-with-sq master &&\n+\tGIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n \tset_fake_editor &&\n-\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n+\tFAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n \t(\n \t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n \t\t. .git/rebase-merge/author-script &&\n \t\ttest \"$(git show -s --date=raw --format=%an,%ae,@%ad)\" = \\\n-\t\t\t\"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n+\t\t\t\"Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n \t)\n '\n \n@@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' '\n \t)\n '\n \n-SQ=\"'\"\n test_expect_success 'rebase -i --gpg-sign=' '\n \tset_fake_editor &&\n \tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n"},{"id":"353640","messageId":"nycvar.QRO.7.76.6.1807261434550.71@tvgsbejvaqbjf.bet","threadId":"48867","inReplyTo":"9ecdf160-bc8b-b91d-a70e-ca6fb9d2a756@talktalk.net","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-26T12:39:18Z","receivedAt":"2018-07-26T12:39:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 19 Jul 2018, Phillip Wood wrote:\n\n> On 18/07/18 18:17, Junio C Hamano wrote:\n> > Phillip Wood writes:\n> > \n> >>> (I think we had code to do so in \"git am\"\n> >>> that was rewritten in C first).\n> >>\n> >> The code in builtin/am.c doesn't try to write valid posix shell (if\n> >> one assumes it is the only consumer of the author script then it\n> >> doesn't need to) which results in simpler code, but external scripts\n> >> cannot safely eval it anymore.\n> > \n> > Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer\n> > discussed here, as presumably we are fixing it in this thread.\n> > \n> > But I do not see how builtin/am.c::write_author_script() would\n> > produce something that would not eval correctly. sq_quote_buf() was\n> > introduced specifically to write correct string for shell's\n> > consumption.\n> \n> You're right, I'm not sure how I missed the calls to sq_quote_buf()\n> yesterday, sharing the am code with the sequencer would clean things up\n> nicely.\n\nNo, actually Phillip was right. The `author-script` file written by\n`git-am` was always an implementation detail, and as there was no\n(intended) way to call shell scripts while running `git-am`, the only\nshell script to intentionally use `author-script` was `git-am` itself.\n\nEver since `git-am` is a builtin, the `author-script` file format could be\nchanged, because it is an implementation detail, no more nor less, and I\nthink it *should* be changed, too. We're spending useless cycles on\nquoting and dequoting, when writing a NUL-separated list of var=value\npairs would be totally sufficient to our ends.\n\nCiao,\nDscho\n"},{"id":"353664","messageId":"xmqqbmatrixc.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"nycvar.QRO.7.76.6.1807261347040.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T17:44:15Z","receivedAt":"2018-07-26T17:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Tue, 17 Jul 2018, Junio C Hamano wrote:\n>\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 2d189da2f1..b0cef509ab 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n>\n> You missed a very long line here.\n>\n>> \tset_fake_editor &&\n>> \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n>> \ttest -f .git/rebase-merge/author-script &&\n>\n> Why do we need this, if we already have an `eval` later on?\n\nYou are commenting on a wrong version. Comment on the original.\n"},{"id":"353666","messageId":"xmqq7elhrihw.fsf@gitster-ct.c.googlers.com","threadId":"48867","inReplyTo":"nycvar.QRO.7.76.6.1807261434550.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T17:53:31Z","receivedAt":"2018-07-26T17:53:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> You're right, I'm not sure how I missed the calls to sq_quote_buf()\n>> yesterday, sharing the am code with the sequencer would clean things up\n>> nicely.\n>\n> No, actually Phillip was right. The `author-script` file written by\n> `git-am` was always an implementation detail, and as there was no\n> (intended) way to call shell scripts while running `git-am`, the only\n> shell script to intentionally use `author-script` was `git-am` itself.\n\nWell the thing is that you did not write \"am\". When I wrote \"am\", I\ndid so with a deliberate design decision to keep the author-script\nin the same format so that it can be read by shell.\n\nYou are behaving as if you made a silent decision to improve the\nauthor-script by designing a better micro-format that deviates from\nwhat shells read by doubling the backslash quoting and losing the\nsingle quote at the end of the line for only the last one, and your\njustification is that it does not matter how broken the new\nmicro-format is because it is an implementation detail nobody should\ncare. And worse yet, you did that improvement without telling\nanybody else why the new format is better.\n\nThat's just silly.\n\nJust like everybody else, you are sometimes wrong and you sometimes\nmake mistakes. The rest of time you are not wrong and your design\ndecisions are not mistaken, but trying to defend an obvious mistake\nlike this one with silly excuses is an easy way to lose credibility.\n\n"},{"id":"353696","messageId":"dae25f9d-5fde-a9c5-2178-9e06f3150885@talktalk.net","threadId":"48867","inReplyTo":"nycvar.QRO.7.76.6.1807261332130.71@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-27T10:36:51Z","receivedAt":"2018-07-27T10:36:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\nOn 26/07/18 13:33, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Wed, 18 Jul 2018, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> Single quotes should be escaped as \\' not \\\\'. Note that this only\n>> affects authors that contain a single quote and then only external\n>> scripts that read the author script and users whose git is upgraded from\n>> the shell version of rebase -i while rebase was stopped. This is because\n>> the parsing in read_env_script() expected the broken version and for\n>> some reason sq_dequote() called by read_author_ident() seems to handle\n>> the broken quoting correctly.\n>>\n>> Ideally write_author_script() would be rewritten to use\n>> split_ident_line() and sq_quote_buf() but this commit just fixes the\n>> immediate bug.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n> \n> Good catch.\n> \n>> This is untested, unfortuantely I don't have really have time to write a test or\n>> follow this up at the moment, if someone else want to run with it then please\n>> do.\n> \n> I modified the test that was added by Akinori. As it was added very early,\n> and as there is still a test case *after* Akinori's that compares a\n> hard-coded SHA-1, I refrained from using `test_commit` (which would change\n> that SHA-1). See below.\n\nThanks for adding a test, that sounds like sensible approach, however\nhaving thought about it I wonder if we should just be writing a plain\ntext file (e.g rebase-merge/author-data) and fixing the reader to read\nthat if it exists and only then fall back to reading the legacy\nrebase-merge/author-script with a fix to correctly handle the script\nwritten by the shell version - what do you think? The author-script\nreally should be just an implementation detail. If anyone really wants\nto read it they can still do 'read -r l' and split the lines with\n${l%%=*} and ${l#*=}\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 5354d4d51e..0b78d1f100 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n>> \t\telse if (*message != '\\'')\n>> \t\t\tstrbuf_addch(&buf, *(message++));\n>> \t\telse\n>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n>> \twhile (*message && *message != '\\n' && *message != '\\r')\n>> \t\tif (skip_prefix(message, \"> \", &message))\n>> \t\t\tbreak;\n>> \t\telse if (*message != '\\'')\n>> \t\t\tstrbuf_addch(&buf, *(message++));\n>> \t\telse\n>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n>> \twhile (*message && *message != '\\n' && *message != '\\r')\n>> \t\tif (*message != '\\'')\n>> \t\t\tstrbuf_addch(&buf, *(message++));\n>> \t\telse\n>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> \n> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of\n> this, including the fixup to Junio's fixup to the\n> `fix-t3404-author-script-test` branch at https://github.com/dscho/git.\n> \n>> \tstrbuf_release(&buf);\n>> \treturn res;\n>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n>> {\n>> \tstruct strbuf script = STRBUF_INIT;\n>> \tint i, count = 0;\n>> -\tchar *p, *p2;\n>> +\tconst char *p2;\n>> +\tchar *p;\n>> \n>> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>> \t\treturn -1;\n>> \n>> \tfor (p = script.buf; *p; p++)\n>> -\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n>> +\t\t/*\n>> +\t\t * write_author_script() used to escape \"'\" incorrectly as\n>> +\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n>> +\t\t * version the incorrect version in case git was upgraded while\n>> +\t\t * rebase was stopped.\n>> +\t\t */\n>> +\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n>> +\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> \n> I think in this form, it is possibly unsafe because it assumes that the\n> new code cannot generate output that would trigger that same code path.\n> Although I have to admit that I did not give this a great deal of thought.\n\nHm, I not sure that it can. If the Author begins \\\\' then this will be\nwritten as the C string \"'\\\\\\\\'\\\\''...\". If \\\\' comes at the end then I\nthink this will be written as \"\\\\\\\\'\\\\'''\", in the middle of the name it\nwill be \"\\\\\\\\'\\\\''...\"\n\n> In any case, if you have to think long and hard about some fix, it might\n> be better to go with something that is easier to reason about. So how\n> about this: we already know that the code is buggy, Akinori fixed the bug,\n> where the author-script missed its trailing single-quote. We can use this\n> as a tell-tale for *this* bug. Assuming that Junio will advance both your\n> and Akinori's fix in close proximity.\n\nThat sounds like a good approach\n> Again, this is pushed to the `fix-t3404-author-script-test` branch at\n> https://github.com/dscho/git; My fixup on top of your patch looks like\n> this (feel free to drop the sq_bug part and only keep the test part):\n> \n> -- snipsnap --\n> diff --git a/sequencer.c b/sequencer.c\n> index 46c0b3e720f..7abe78dc78e 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -573,13 +573,14 @@ static int write_author_script(const char *message)\n> static int read_env_script(struct argv_array *env)\n> {\n> \tstruct strbuf script = STRBUF_INIT;\n> -\tint i, count = 0;\n> +\tint i, count = 0, sq_bug;\n> \tconst char *p2;\n> \tchar *p;\n> \n> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> \t\treturn -1;\n> \n> +\tsq_bug = script.len && script.buf[script.len - 1] != '\\'';\n> \tfor (p = script.buf; *p; p++)\n> \t\t/*\n> \t\t * write_author_script() used to escape \"'\" incorrectly as\n> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)\n> \t\t * version the incorrect version in case git was upgraded while\n> \t\t * rebase was stopped.\n> \t\t */\n\nWe probably want the change the comment slightly to explain sq_bug\n\n> -\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n> -\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> +\t\tif (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> +\t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> +\t\telse if (skip_prefix(p, \"'\\\\''\", &p2))\n> \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> \t\telse if (*p == '\\'')\n> \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 97f0b4bf881..dd726ff4dc4 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' '\n> \ttest_line_count = 6 actual\n> '\n> \n> +SQ=\"'\"\n> test_expect_success 'rebase -i writes correct author-script' '\n> \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> -\tgit checkout master &&\n> +\tgit checkout -b author-with-sq master &&\n> +\tGIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n> \tset_fake_editor &&\n> -\tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> +\tFAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n> \t(\n> \t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> \t\t. .git/rebase-merge/author-script &&\n> \t\ttest \"$(git show -s --date=raw --format=%an,%ae,@%ad)\" = \\\n> -\t\t\t\"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n> +\t\t\t\"Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n> \t)\n> '\n\nTesting all three variables together is nice.\n\nBest Wishes\n\nPhillip\n\n> @@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' '\n> \t)\n> '\n> \n> -SQ=\"'\"\n> test_expect_success 'rebase -i --gpg-sign=' '\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n> \n\n"},{"id":"353699","messageId":"nycvar.QRO.7.76.6.1807271415410.10478@tvgsbejvaqbjf.bet","threadId":"48867","inReplyTo":"dae25f9d-5fde-a9c5-2178-9e06f3150885@talktalk.net","subject":"Re: [RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-27T12:37:10Z","receivedAt":"2018-07-27T12:37:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip, Junio and Akinori,\n\nI just noticed that t3404 is broken without my patches (but with Junio's\nfixup), on Windows, macOS and Linux. (See log at the end.)\n\nOn Fri, 27 Jul 2018, Phillip Wood wrote:\n\n> On 26/07/18 13:33, Johannes Schindelin wrote:\n> > \n> > On Wed, 18 Jul 2018, Phillip Wood wrote:\n> > \n> >> Single quotes should be escaped as \\' not \\\\'. Note that this only\n> >> affects authors that contain a single quote and then only external\n> >> scripts that read the author script and users whose git is upgraded from\n> >> the shell version of rebase -i while rebase was stopped. This is because\n> >> the parsing in read_env_script() expected the broken version and for\n> >> some reason sq_dequote() called by read_author_ident() seems to handle\n> >> the broken quoting correctly.\n> >>\n> >> Ideally write_author_script() would be rewritten to use\n> >> split_ident_line() and sq_quote_buf() but this commit just fixes the\n> >> immediate bug.\n> > \n> >> This is untested, unfortuantely I don't have really have time to write a test or\n> >> follow this up at the moment, if someone else want to run with it then please\n> >> do.\n> > \n> > I modified the test that was added by Akinori. As it was added very early,\n> > and as there is still a test case *after* Akinori's that compares a\n> > hard-coded SHA-1, I refrained from using `test_commit` (which would change\n> > that SHA-1). See below.\n> \n> Thanks for adding a test, that sounds like sensible approach, however\n> having thought about it I wonder if we should just be writing a plain\n> text file (e.g rebase-merge/author-data) and fixing the reader to read\n> that if it exists and only then fall back to reading the legacy\n> rebase-merge/author-script with a fix to correctly handle the script\n> written by the shell version - what do you think? The author-script\n> really should be just an implementation detail. If anyone really wants\n> to read it they can still do 'read -r l' and split the lines with\n> ${l%%=*} and ${l#*=}\n\nIn contrast to `git am`, there *is* a use case where power users might\nhave come to rely on the presence of the .git/rebase-merge/author-script\nfile *and* its nature as a shell script snippet: we purposefully allow\nscripting `rebase -i`.\n\nSo I don't think that we can declare the file and its format as\nimplementation detail, even if the idea is very, very tempting.\n\n> >> diff --git a/sequencer.c b/sequencer.c\n> >> index 5354d4d51e..0b78d1f100 100644\n> >> --- a/sequencer.c\n> >> +++ b/sequencer.c\n> >> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n> >> \t\telse if (*message != '\\'')\n> >> \t\t\tstrbuf_addch(&buf, *(message++));\n> >> \t\telse\n> >> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> >> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> >> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n> >> \twhile (*message && *message != '\\n' && *message != '\\r')\n> >> \t\tif (skip_prefix(message, \"> \", &message))\n> >> \t\t\tbreak;\n> >> \t\telse if (*message != '\\'')\n> >> \t\t\tstrbuf_addch(&buf, *(message++));\n> >> \t\telse\n> >> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> >> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> >> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n> >> \twhile (*message && *message != '\\n' && *message != '\\r')\n> >> \t\tif (*message != '\\'')\n> >> \t\t\tstrbuf_addch(&buf, *(message++));\n> >> \t\telse\n> >> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> >> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n> >> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> > \n> > I resolved the merge conflict with Akinori's patch. FWIW I pushed all of\n> > this, including the fixup to Junio's fixup to the\n> > `fix-t3404-author-script-test` branch at https://github.com/dscho/git.\n> > \n> >> \tstrbuf_release(&buf);\n> >> \treturn res;\n> >> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n> >> {\n> >> \tstruct strbuf script = STRBUF_INIT;\n> >> \tint i, count = 0;\n> >> -\tchar *p, *p2;\n> >> +\tconst char *p2;\n> >> +\tchar *p;\n> >> \n> >> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> >> \t\treturn -1;\n> >> \n> >> \tfor (p = script.buf; *p; p++)\n> >> -\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n> >> +\t\t/*\n> >> +\t\t * write_author_script() used to escape \"'\" incorrectly as\n> >> +\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n> >> +\t\t * version the incorrect version in case git was upgraded while\n> >> +\t\t * rebase was stopped.\n> >> +\t\t */\n> >> +\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n> >> +\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> > \n> > I think in this form, it is possibly unsafe because it assumes that the\n> > new code cannot generate output that would trigger that same code path.\n> > Although I have to admit that I did not give this a great deal of thought.\n> \n> Hm, I not sure that it can. If the Author begins \\\\' then this will be\n> written as the C string \"'\\\\\\\\'\\\\''...\". If \\\\' comes at the end then I\n> think this will be written as \"\\\\\\\\'\\\\'''\", in the middle of the name it\n> will be \"\\\\\\\\'\\\\''...\"\n\nYes, that matches my gut feeling... but...\n\n> > In any case, if you have to think long and hard about some fix, it might\n> > be better to go with something that is easier to reason about. So how\n> > about this: we already know that the code is buggy, Akinori fixed the bug,\n> > where the author-script missed its trailing single-quote. We can use this\n> > as a tell-tale for *this* bug. Assuming that Junio will advance both your\n> > and Akinori's fix in close proximity.\n> \n> That sounds like a good approach\n\nI am glad that you agree to this.\n\nI am a big fan of this age-old wisdom\nthat goes somewhat like this: some code is so simple that there is no\nspace for obvious bugs, and some code is so complicated that there is no\nspace for obvious bugs. In this context, I would use the modified\nversion: some code is so easy to reason about that there is no obvious\nflaw, and some other code is so difficult to reason about that there is\nno obvious flaw.\n\nBesides, using the sq_bug version can serve as a reminder to \"pay down\nthe technical debt\" in the future.\n\n> > Again, this is pushed to the `fix-t3404-author-script-test` branch at\n> > https://github.com/dscho/git; My fixup on top of your patch looks like\n> > this (feel free to drop the sq_bug part and only keep the test part):\n> > \n> > -- snipsnap --\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 46c0b3e720f..7abe78dc78e 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -573,13 +573,14 @@ static int write_author_script(const char *message)\n> > static int read_env_script(struct argv_array *env)\n> > {\n> > \tstruct strbuf script = STRBUF_INIT;\n> > -\tint i, count = 0;\n> > +\tint i, count = 0, sq_bug;\n> > \tconst char *p2;\n> > \tchar *p;\n> > \n> > \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> > \t\treturn -1;\n> > \n> > +\tsq_bug = script.len && script.buf[script.len - 1] != '\\'';\n> > \tfor (p = script.buf; *p; p++)\n> > \t\t/*\n> > \t\t * write_author_script() used to escape \"'\" incorrectly as\n> > @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)\n> > \t\t * version the incorrect version in case git was upgraded while\n> > \t\t * rebase was stopped.\n> > \t\t */\n> \n> We probably want the change the comment slightly to explain sq_bug\n\nTrue. Can you give it a shot? If not, I will try to remember some time mid\nnext week.\n\nHere the promised log of t3404 with -i -v -x (on macOS, but Linux and\nWindows shows equivalent failures, and the way I read it, the problem is\nsimply that the test was introduced in the middle of t3404 and\nsubsequent test cases' assumptions are no longer met):\n\n-- snipsnap --\n2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty\n2018-07-27T11:53:04.2464690Z \n2018-07-27T11:53:04.2489730Z expecting success: \n2018-07-27T11:53:04.2513840Z \ttest_when_finished \"git rebase --abort ||:\" &&\n2018-07-27T11:53:04.2536980Z \tgit checkout master &&\n2018-07-27T11:53:04.2563050Z \tset_fake_editor &&\n2018-07-27T11:53:04.2590300Z \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n2018-07-27T11:53:04.2614900Z \ttest -f .git/rebase-merge/author-script &&\n2018-07-27T11:53:04.2639740Z \t(\n2018-07-27T11:53:04.2665320Z \t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n2018-07-27T11:53:04.2688410Z \t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n2018-07-27T11:53:04.2710930Z \t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n2018-07-27T11:53:04.2734740Z \t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n2018-07-27T11:53:04.2756700Z \t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n2018-07-27T11:53:04.2778060Z \t)\n2018-07-27T11:53:04.2788910Z \n2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:'\n2018-07-27T11:53:04.2831430Z ++ test 0 = 0\n2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||:\n2018-07-27T11:53:04.2874430Z \t\t} && (exit \"$eval_ret\"); eval_ret=$?; :'\n2018-07-27T11:53:04.2892720Z ++ git checkout master\n2018-07-27T11:53:04.2911560Z Switched to branch 'master'\n2018-07-27T11:53:04.2929490Z ++ set_fake_editor\n2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh\n2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh'\n2018-07-27T11:53:04.2983780Z ++ cat\n2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh\n2018-07-27T11:53:04.3019770Z +++ pwd\n2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR\n2018-07-27T11:53:04.3094750Z ++ EDITOR='\"$FAKE_EDITOR\"'\n2018-07-27T11:53:04.3112610Z ++ export EDITOR\n2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1'\n2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^'\n2018-07-27T11:53:04.4674460Z rebase -i script before editing:\n2018-07-27T11:53:04.4698420Z pick 8f99a4f E\n2018-07-27T11:53:04.4710290Z \n2018-07-27T11:53:04.4792780Z rebase -i script after editing:\n2018-07-27T11:53:04.4817630Z edit 8f99a4f E\n2018-07-27T11:53:04.5025190Z Rebasing (1/1)\n2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f... E\n2018-07-27T11:53:04.5065680Z You can amend the commit now, with\n2018-07-27T11:53:04.5074810Z \n2018-07-27T11:53:04.5092850Z git commit --amend \n2018-07-27T11:53:04.5101800Z \n2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run\n2018-07-27T11:53:04.5129030Z \n2018-07-27T11:53:04.5147390Z git rebase --continue\n2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script\n2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE\n2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE\n2018-07-27T11:53:04.5220730Z ++ return 0\n2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script\n2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\\''A U Thor'\\''\n2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\\''author@example.com'\\''\n2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\\''@1112912233 -0700'\\'''\n2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor'\n2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com\n2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700'\n2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an\n2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor'\n2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae\n2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com\n2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad\n2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700'\n2018-07-27T11:53:04.5514390Z ++ git rebase --abort\n2018-07-27T11:53:04.6924920Z ++ exit 0\n2018-07-27T11:53:04.6950890Z ++ eval_ret=0\n2018-07-27T11:53:04.6973900Z ++ :\n2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse\n2018-07-27T11:53:04.7009400Z \n2018-07-27T11:53:04.7030290Z expecting success: \n2018-07-27T11:53:04.7050880Z \tset_fake_editor &&\n2018-07-27T11:53:04.7072520Z \ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n2018-07-27T11:53:04.7092910Z \ttest_i18ncmp expect actual\n2018-07-27T11:53:04.7102960Z \n2018-07-27T11:53:04.7123150Z ++ set_fake_editor\n2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh\n2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh'\n2018-07-27T11:53:04.7185360Z ++ cat\n2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh\n2018-07-27T11:53:04.7236720Z +++ pwd\n2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR\n2018-07-27T11:53:04.7338290Z ++ EDITOR='\"$FAKE_EDITOR\"'\n2018-07-27T11:53:04.7362710Z ++ export EDITOR\n2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'\n2018-07-27T11:53:04.7412410Z ++ case \"$1\" in\n2018-07-27T11:53:04.7436170Z ++ _test_ok=\n2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'\n2018-07-27T11:53:04.9560670Z ++ exit_code=0\n2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0\n2018-07-27T11:53:04.9603200Z ++ list_contains '' success\n2018-07-27T11:53:04.9628730Z ++ case \",$1,\" in\n2018-07-27T11:53:04.9652120Z ++ return 1\n2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^'\n2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^\n2018-07-27T11:53:04.9730750Z ++ return 1\n2018-07-27T11:53:04.9754580Z error: last command exited with $?=1\n2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD\n2018-07-27T11:53:04.9802670Z #\t\n2018-07-27T11:53:04.9826330Z #\t\tset_fake_editor &&\n2018-07-27T11:53:04.9851320Z #\t\ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n2018-07-27T11:53:04.9875680Z #\t\ttest_i18ncmp expect actual\n2018-07-27T11:53:04.9899170Z #\t\n2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1\n"},{"id":"353722","messageId":"nycvar.QRO.7.76.6.1807271749290.10478@tvgsbejvaqbjf.bet","threadId":"48867","inReplyTo":"xmqqbmatrixc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sequencer.c: terminate the last line of author-script properly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-27T15:49:43Z","receivedAt":"2018-07-27T15:50:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 Jul 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Hi Junio,\n> >\n> > On Tue, 17 Jul 2018, Junio C Hamano wrote:\n> >\n> >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> >> index 2d189da2f1..b0cef509ab 100755\n> >> --- a/t/t3404-rebase-interactive.sh\n> >> +++ b/t/t3404-rebase-interactive.sh\n> >> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in \"ed\n> >\n> > You missed a very long line here.\n> >\n> >> \tset_fake_editor &&\n> >> \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> >> \ttest -f .git/rebase-merge/author-script &&\n> >\n> > Why do we need this, if we already have an `eval` later on?\n> \n> You are commenting on a wrong version. Comment on the original.\n\nSorry, no time. Take this review, or leave it.\n\nCiao,\nDscho\n"},{"id":"353859","messageId":"7882622e-de96-c875-e11d-677e73b74216@talktalk.net","threadId":"48867","inReplyTo":"nycvar.QRO.7.76.6.1807271415410.10478@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-30T09:35:33Z","receivedAt":"2018-07-30T09:35:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/07/18 13:37, Johannes Schindelin wrote:\n> Hi Phillip, Junio and Akinori,\n> \n> I just noticed that t3404 is broken without my patches (but with Junio's\n> fixup), on Windows, macOS and Linux. (See log at the end.)\n> \n> On Fri, 27 Jul 2018, Phillip Wood wrote:\n> \n>> On 26/07/18 13:33, Johannes Schindelin wrote:\n>>>\n>>> On Wed, 18 Jul 2018, Phillip Wood wrote:\n>>>\n>>>> Single quotes should be escaped as \\' not \\\\'. Note that this only\n>>>> affects authors that contain a single quote and then only external\n>>>> scripts that read the author script and users whose git is upgraded from\n>>>> the shell version of rebase -i while rebase was stopped. This is because\n>>>> the parsing in read_env_script() expected the broken version and for\n>>>> some reason sq_dequote() called by read_author_ident() seems to handle\n>>>> the broken quoting correctly.\n>>>>\n>>>> Ideally write_author_script() would be rewritten to use\n>>>> split_ident_line() and sq_quote_buf() but this commit just fixes the\n>>>> immediate bug.\n>>>\n>>>> This is untested, unfortuantely I don't have really have time to write a test or\n>>>> follow this up at the moment, if someone else want to run with it then please\n>>>> do.\n>>>\n>>> I modified the test that was added by Akinori. As it was added very early,\n>>> and as there is still a test case *after* Akinori's that compares a\n>>> hard-coded SHA-1, I refrained from using `test_commit` (which would change\n>>> that SHA-1). See below.\n>>\n>> Thanks for adding a test, that sounds like sensible approach, however\n>> having thought about it I wonder if we should just be writing a plain\n>> text file (e.g rebase-merge/author-data) and fixing the reader to read\n>> that if it exists and only then fall back to reading the legacy\n>> rebase-merge/author-script with a fix to correctly handle the script\n>> written by the shell version - what do you think? The author-script\n>> really should be just an implementation detail. If anyone really wants\n>> to read it they can still do 'read -r l' and split the lines with\n>> ${l%%=*} and ${l#*=}\n> \n> In contrast to `git am`, there *is* a use case where power users might\n> have come to rely on the presence of the .git/rebase-merge/author-script\n> file *and* its nature as a shell script snippet: we purposefully allow\n> scripting `rebase -i`.\n> \n> So I don't think that we can declare the file and its format as\n> implementation detail, even if the idea is very, very tempting.\n> \n>>>> diff --git a/sequencer.c b/sequencer.c\n>>>> index 5354d4d51e..0b78d1f100 100644\n>>>> --- a/sequencer.c\n>>>> +++ b/sequencer.c\n>>>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)\n>>>> \t\telse if (*message != '\\'')\n>>>> \t\t\tstrbuf_addch(&buf, *(message++));\n>>>> \t\telse\n>>>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>>>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>>>> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n>>>> \twhile (*message && *message != '\\n' && *message != '\\r')\n>>>> \t\tif (skip_prefix(message, \"> \", &message))\n>>>> \t\t\tbreak;\n>>>> \t\telse if (*message != '\\'')\n>>>> \t\t\tstrbuf_addch(&buf, *(message++));\n>>>> \t\telse\n>>>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>>>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>>>> \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n>>>> \twhile (*message && *message != '\\n' && *message != '\\r')\n>>>> \t\tif (*message != '\\'')\n>>>> \t\t\tstrbuf_addch(&buf, *(message++));\n>>>> \t\telse\n>>>> -\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n>>>> +\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n>>>> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n>>>\n>>> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of\n>>> this, including the fixup to Junio's fixup to the\n>>> `fix-t3404-author-script-test` branch at https://github.com/dscho/git.\n>>>\n>>>> \tstrbuf_release(&buf);\n>>>> \treturn res;\n>>>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)\n>>>> {\n>>>> \tstruct strbuf script = STRBUF_INIT;\n>>>> \tint i, count = 0;\n>>>> -\tchar *p, *p2;\n>>>> +\tconst char *p2;\n>>>> +\tchar *p;\n>>>> \n>>>> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>>>> \t\treturn -1;\n>>>> \n>>>> \tfor (p = script.buf; *p; p++)\n>>>> -\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n>>>> +\t\t/*\n>>>> +\t\t * write_author_script() used to escape \"'\" incorrectly as\n>>>> +\t\t * \"'\\\\\\\\''\" rather than \"'\\\\''\" so we check for the correct\n>>>> +\t\t * version the incorrect version in case git was upgraded while\n>>>> +\t\t * rebase was stopped.\n>>>> +\t\t */\n>>>> +\t\tif (skip_prefix(p, \"'\\\\''\", &p2) ||\n>>>> +\t\t skip_prefix(p, \"'\\\\\\\\''\", &p2))\n>>>\n>>> I think in this form, it is possibly unsafe because it assumes that the\n>>> new code cannot generate output that would trigger that same code path.\n>>> Although I have to admit that I did not give this a great deal of thought.\n>>\n>> Hm, I not sure that it can. If the Author begins \\\\' then this will be\n>> written as the C string \"'\\\\\\\\'\\\\''...\". If \\\\' comes at the end then I\n>> think this will be written as \"\\\\\\\\'\\\\'''\", in the middle of the name it\n>> will be \"\\\\\\\\'\\\\''...\"\n> \n> Yes, that matches my gut feeling... but...\n> \n>>> In any case, if you have to think long and hard about some fix, it might\n>>> be better to go with something that is easier to reason about. So how\n>>> about this: we already know that the code is buggy, Akinori fixed the bug,\n>>> where the author-script missed its trailing single-quote. We can use this\n>>> as a tell-tale for *this* bug. Assuming that Junio will advance both your\n>>> and Akinori's fix in close proximity.\n>>\n>> That sounds like a good approach\n> \n> I am glad that you agree to this.\n> \n> I am a big fan of this age-old wisdom\n> that goes somewhat like this: some code is so simple that there is no\n> space for obvious bugs, and some code is so complicated that there is no\n> space for obvious bugs. In this context, I would use the modified\n> version: some code is so easy to reason about that there is no obvious\n> flaw, and some other code is so difficult to reason about that there is\n> no obvious flaw.\n> \n> Besides, using the sq_bug version can serve as a reminder to \"pay down\n> the technical debt\" in the future.\n> \n>>> Again, this is pushed to the `fix-t3404-author-script-test` branch at\n>>> https://github.com/dscho/git; My fixup on top of your patch looks like\n>>> this (feel free to drop the sq_bug part and only keep the test part):\n>>>\n>>> -- snipsnap --\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 46c0b3e720f..7abe78dc78e 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -573,13 +573,14 @@ static int write_author_script(const char *message)\n>>> static int read_env_script(struct argv_array *env)\n>>> {\n>>> \tstruct strbuf script = STRBUF_INIT;\n>>> -\tint i, count = 0;\n>>> +\tint i, count = 0, sq_bug;\n>>> \tconst char *p2;\n>>> \tchar *p;\n>>> \n>>> \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>>> \t\treturn -1;\n>>> \n>>> +\tsq_bug = script.len && script.buf[script.len - 1] != '\\'';\n>>> \tfor (p = script.buf; *p; p++)\n>>> \t\t/*\n>>> \t\t * write_author_script() used to escape \"'\" incorrectly as\n>>> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)\n>>> \t\t * version the incorrect version in case git was upgraded while\n>>> \t\t * rebase was stopped.\n>>> \t\t */\n>>\n>> We probably want the change the comment slightly to explain sq_bug\n> \n> True. Can you give it a shot? If not, I will try to remember some time mid\n> next week.\n\nSure, I'll send something out tomorrow or Wednesday. I think we should\nalter the test to check that there is no empty line at the end of the\nauthor-script file as that will break the test for the missing \"'\".\n\n> \n> Here the promised log of t3404 with -i -v -x (on macOS, but Linux and\n> Windows shows equivalent failures, and the way I read it, the problem is\n> simply that the test was introduced in the middle of t3404 and\n> subsequent test cases' assumptions are no longer met):\n> \n> -- snipsnap --\n> 2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty\n> 2018-07-27T11:53:04.2464690Z \n> 2018-07-27T11:53:04.2489730Z expecting success: \n> 2018-07-27T11:53:04.2513840Z \ttest_when_finished \"git rebase --abort ||:\" &&\n> 2018-07-27T11:53:04.2536980Z \tgit checkout master &&\n> 2018-07-27T11:53:04.2563050Z \tset_fake_editor &&\n> 2018-07-27T11:53:04.2590300Z \tFAKE_LINES=\"edit 1\" git rebase -i HEAD^ &&\n> 2018-07-27T11:53:04.2614900Z \ttest -f .git/rebase-merge/author-script &&\n> 2018-07-27T11:53:04.2639740Z \t(\n> 2018-07-27T11:53:04.2665320Z \t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n> 2018-07-27T11:53:04.2688410Z \t\teval \"$(cat .git/rebase-merge/author-script)\" &&\n> 2018-07-27T11:53:04.2710930Z \t\ttest \"$(git show --quiet --pretty=format:%an)\" = \"$GIT_AUTHOR_NAME\" &&\n> 2018-07-27T11:53:04.2734740Z \t\ttest \"$(git show --quiet --pretty=format:%ae)\" = \"$GIT_AUTHOR_EMAIL\" &&\n> 2018-07-27T11:53:04.2756700Z \t\ttest \"$(git show --quiet --date=raw --pretty=format:@%ad)\" = \"$GIT_AUTHOR_DATE\"\n> 2018-07-27T11:53:04.2778060Z \t)\n> 2018-07-27T11:53:04.2788910Z \n> 2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:'\n> 2018-07-27T11:53:04.2831430Z ++ test 0 = 0\n> 2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||:\n> 2018-07-27T11:53:04.2874430Z \t\t} && (exit \"$eval_ret\"); eval_ret=$?; :'\n> 2018-07-27T11:53:04.2892720Z ++ git checkout master\n> 2018-07-27T11:53:04.2911560Z Switched to branch 'master'\n> 2018-07-27T11:53:04.2929490Z ++ set_fake_editor\n> 2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh\n> 2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh'\n> 2018-07-27T11:53:04.2983780Z ++ cat\n> 2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh\n> 2018-07-27T11:53:04.3019770Z +++ pwd\n> 2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n> 2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n> 2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR\n> 2018-07-27T11:53:04.3094750Z ++ EDITOR='\"$FAKE_EDITOR\"'\n> 2018-07-27T11:53:04.3112610Z ++ export EDITOR\n> 2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1'\n> 2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^'\n> 2018-07-27T11:53:04.4674460Z rebase -i script before editing:\n> 2018-07-27T11:53:04.4698420Z pick 8f99a4f E\n> 2018-07-27T11:53:04.4710290Z \n> 2018-07-27T11:53:04.4792780Z rebase -i script after editing:\n> 2018-07-27T11:53:04.4817630Z edit 8f99a4f E\n> 2018-07-27T11:53:04.5025190Z Rebasing (1/1)\n> 2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f... E\n> 2018-07-27T11:53:04.5065680Z You can amend the commit now, with\n> 2018-07-27T11:53:04.5074810Z \n> 2018-07-27T11:53:04.5092850Z git commit --amend \n> 2018-07-27T11:53:04.5101800Z \n> 2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run\n> 2018-07-27T11:53:04.5129030Z \n> 2018-07-27T11:53:04.5147390Z git rebase --continue\n> 2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script\n> 2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE\n> 2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE\n> 2018-07-27T11:53:04.5220730Z ++ return 0\n> 2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script\n> 2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\\''A U Thor'\\''\n> 2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\\''author@example.com'\\''\n> 2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\\''@1112912233 -0700'\\'''\n> 2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor'\n> 2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com\n> 2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700'\n> 2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an\n> 2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor'\n> 2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae\n> 2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com\n> 2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad\n> 2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700'\n> 2018-07-27T11:53:04.5514390Z ++ git rebase --abort\n> 2018-07-27T11:53:04.6924920Z ++ exit 0\n> 2018-07-27T11:53:04.6950890Z ++ eval_ret=0\n> 2018-07-27T11:53:04.6973900Z ++ :\n> 2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in \"edit\" that sh(1) can parse\n> 2018-07-27T11:53:04.7009400Z \n> 2018-07-27T11:53:04.7030290Z expecting success: \n> 2018-07-27T11:53:04.7050880Z \tset_fake_editor &&\n> 2018-07-27T11:53:04.7072520Z \ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n> 2018-07-27T11:53:04.7092910Z \ttest_i18ncmp expect actual\n> 2018-07-27T11:53:04.7102960Z \n> 2018-07-27T11:53:04.7123150Z ++ set_fake_editor\n> 2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh\n> 2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh'\n> 2018-07-27T11:53:04.7185360Z ++ cat\n> 2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh\n> 2018-07-27T11:53:04.7236720Z +++ pwd\n> 2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n> 2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'\n> 2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR\n> 2018-07-27T11:53:04.7338290Z ++ EDITOR='\"$FAKE_EDITOR\"'\n> 2018-07-27T11:53:04.7362710Z ++ export EDITOR\n> 2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'\n> 2018-07-27T11:53:04.7412410Z ++ case \"$1\" in\n> 2018-07-27T11:53:04.7436170Z ++ _test_ok=\n> 2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'\n> 2018-07-27T11:53:04.9560670Z ++ exit_code=0\n> 2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0\n> 2018-07-27T11:53:04.9603200Z ++ list_contains '' success\n> 2018-07-27T11:53:04.9628730Z ++ case \",$1,\" in\n> 2018-07-27T11:53:04.9652120Z ++ return 1\n> 2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^'\n> 2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^\n> 2018-07-27T11:53:04.9730750Z ++ return 1\n> 2018-07-27T11:53:04.9754580Z error: last command exited with $?=1\n> 2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD\n> 2018-07-27T11:53:04.9802670Z #\t\n> 2018-07-27T11:53:04.9826330Z #\t\tset_fake_editor &&\n> 2018-07-27T11:53:04.9851320Z #\t\ttest_must_fail env FAKE_LINES=\"1 exec_true\" git rebase -i HEAD^ >actual 2>&1 &&\n> 2018-07-27T11:53:04.9875680Z #\t\ttest_i18ncmp expect actual\n> 2018-07-27T11:53:04.9899170Z #\t\n> 2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1\n> \n\n"}]}