From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,T_DKIMWL_WL_MED shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 5A4271F597 for ; Mon, 30 Jul 2018 09:35:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726808AbeG3LJq (ORCPT ); Mon, 30 Jul 2018 07:09:46 -0400 Received: from smtp-out-6.talktalk.net ([62.24.135.70]:26086 "EHLO smtp-out-6.talktalk.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726723AbeG3LJq (ORCPT ); Mon, 30 Jul 2018 07:09:46 -0400 Received: from [192.168.2.201] ([92.22.43.37]) by smtp.talktalk.net with SMTP id k4aTfpG0wpXFjk4aUfVsOY; Mon, 30 Jul 2018 10:35:35 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=talktalk.net; s=cmr1711; t=1532943335; bh=5yLcvAWvMT8/q45fCBoYnFQIYoUOxVVUvXp/qJ6pGWk=; h=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To; b=hpZpS+DI1vQipdJi6iKQklFrJMlyEfvI1oP2YFYMfhQijPt2eFXwoY4XNMcwtV2w7 WHskND0gLW38QXMmesS0twJZ6SH2TaIA9nTilxC7zE5tFHDQb9T2wNWWxYcG4a9ydX KFJSZWwSvvj2BZ/FVIqwbDzViWtimvDGaPTTcOcc= X-Originating-IP: [92.22.43.37] X-Spam: 0 X-OAuthority: v=2.3 cv=Ob228CbY c=1 sm=1 tr=0 a=PnXNWDbCrl8jSOaLTf+CAA==:117 a=PnXNWDbCrl8jSOaLTf+CAA==:17 a=IkcTkHD0fZMA:10 a=NEAV23lmAAAA:8 a=A1X0JdhQAAAA:8 a=RFM36bF_8kfDygzpX1AA:9 a=k7bOELaBuMCodngi:21 a=qhasBIOpZ6lITn9a:21 a=QEXdDO2ut3YA:10 a=Df3jFdWbhGDLdZNm0fyq:22 Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [RFC PATCH] sequencer: fix quoting in write_author_script To: Johannes Schindelin , Phillip Wood Cc: Git Mailing List , Junio C Hamano , Akinori MUSHA References: <20180718155518.1025-1-phillip.wood@talktalk.net> From: Phillip Wood Message-ID: <7882622e-de96-c875-e11d-677e73b74216@talktalk.net> Date: Mon, 30 Jul 2018 10:35:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfEu0YkS6s4NEZ/Spiij3M0NSoM8AqNA8dxTUaAArFvX29khrriraEKlKldYPaGoaJ+mJlAp+oVovjDJrPjf0p+zul97WS5WxjFah2oAaE5Wy2/MUD2Hg whYaeUihsr5P2P9zTpz9um5aA/c8PjDg8q67tnlw4Svu+9xzv7XZuxARheypAR1D8I/a9a8D01JXzBpKywgjhvUlQqasvN+flns0l36fWwg1zyAdiem7TUkQ LGDgqjHO2lObXJTHXgLZ7kEQIkq0IkBBDB0dTIsyU9CLok27bklhy/Z9wQkV4e+V0s/0Dao/2MwEW9tRmAsDNg== Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 27/07/18 13:37, Johannes Schindelin wrote: > Hi Phillip, Junio and Akinori, > > I just noticed that t3404 is broken without my patches (but with Junio's > fixup), on Windows, macOS and Linux. (See log at the end.) > > On Fri, 27 Jul 2018, Phillip Wood wrote: > >> On 26/07/18 13:33, Johannes Schindelin wrote: >>> >>> On Wed, 18 Jul 2018, Phillip Wood wrote: >>> >>>> Single quotes should be escaped as \' not \\'. Note that this only >>>> affects authors that contain a single quote and then only external >>>> scripts that read the author script and users whose git is upgraded from >>>> the shell version of rebase -i while rebase was stopped. This is because >>>> the parsing in read_env_script() expected the broken version and for >>>> some reason sq_dequote() called by read_author_ident() seems to handle >>>> the broken quoting correctly. >>>> >>>> Ideally write_author_script() would be rewritten to use >>>> split_ident_line() and sq_quote_buf() but this commit just fixes the >>>> immediate bug. >>> >>>> This is untested, unfortuantely I don't have really have time to write a test or >>>> follow this up at the moment, if someone else want to run with it then please >>>> do. >>> >>> I modified the test that was added by Akinori. As it was added very early, >>> and as there is still a test case *after* Akinori's that compares a >>> hard-coded SHA-1, I refrained from using `test_commit` (which would change >>> that SHA-1). See below. >> >> Thanks for adding a test, that sounds like sensible approach, however >> having thought about it I wonder if we should just be writing a plain >> text file (e.g rebase-merge/author-data) and fixing the reader to read >> that if it exists and only then fall back to reading the legacy >> rebase-merge/author-script with a fix to correctly handle the script >> written by the shell version - what do you think? The author-script >> really should be just an implementation detail. If anyone really wants >> to read it they can still do 'read -r l' and split the lines with >> ${l%%=*} and ${l#*=} > > In contrast to `git am`, there *is* a use case where power users might > have come to rely on the presence of the .git/rebase-merge/author-script > file *and* its nature as a shell script snippet: we purposefully allow > scripting `rebase -i`. > > So I don't think that we can declare the file and its format as > implementation detail, even if the idea is very, very tempting. > >>>> diff --git a/sequencer.c b/sequencer.c >>>> index 5354d4d51e..0b78d1f100 100644 >>>> --- a/sequencer.c >>>> +++ b/sequencer.c >>>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message) >>>> else if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='"); >>>> while (*message && *message != '\n' && *message != '\r') >>>> if (skip_prefix(message, "> ", &message)) >>>> break; >>>> else if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@"); >>>> while (*message && *message != '\n' && *message != '\r') >>>> if (*message != '\'') >>>> strbuf_addch(&buf, *(message++)); >>>> else >>>> - strbuf_addf(&buf, "'\\\\%c'", *(message++)); >>>> + strbuf_addf(&buf, "'\\%c'", *(message++)); >>>> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1); >>> >>> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of >>> this, including the fixup to Junio's fixup to the >>> `fix-t3404-author-script-test` branch at https://github.com/dscho/git. >>> >>>> strbuf_release(&buf); >>>> return res; >>>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env) >>>> { >>>> struct strbuf script = STRBUF_INIT; >>>> int i, count = 0; >>>> - char *p, *p2; >>>> + const char *p2; >>>> + char *p; >>>> >>>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) >>>> return -1; >>>> >>>> for (p = script.buf; *p; p++) >>>> - if (skip_prefix(p, "'\\\\''", (const char **)&p2)) >>>> + /* >>>> + * write_author_script() used to escape "'" incorrectly as >>>> + * "'\\\\''" rather than "'\\''" so we check for the correct >>>> + * version the incorrect version in case git was upgraded while >>>> + * rebase was stopped. >>>> + */ >>>> + if (skip_prefix(p, "'\\''", &p2) || >>>> + skip_prefix(p, "'\\\\''", &p2)) >>> >>> I think in this form, it is possibly unsafe because it assumes that the >>> new code cannot generate output that would trigger that same code path. >>> Although I have to admit that I did not give this a great deal of thought. >> >> Hm, I not sure that it can. If the Author begins \\' then this will be >> written as the C string "'\\\\'\\''...". If \\' comes at the end then I >> think this will be written as "\\\\'\\'''", in the middle of the name it >> will be "\\\\'\\''..." > > Yes, that matches my gut feeling... but... > >>> In any case, if you have to think long and hard about some fix, it might >>> be better to go with something that is easier to reason about. So how >>> about this: we already know that the code is buggy, Akinori fixed the bug, >>> where the author-script missed its trailing single-quote. We can use this >>> as a tell-tale for *this* bug. Assuming that Junio will advance both your >>> and Akinori's fix in close proximity. >> >> That sounds like a good approach > > I am glad that you agree to this. > > I am a big fan of this age-old wisdom > that goes somewhat like this: some code is so simple that there is no > space for obvious bugs, and some code is so complicated that there is no > space for obvious bugs. In this context, I would use the modified > version: some code is so easy to reason about that there is no obvious > flaw, and some other code is so difficult to reason about that there is > no obvious flaw. > > Besides, using the sq_bug version can serve as a reminder to "pay down > the technical debt" in the future. > >>> Again, this is pushed to the `fix-t3404-author-script-test` branch at >>> https://github.com/dscho/git; My fixup on top of your patch looks like >>> this (feel free to drop the sq_bug part and only keep the test part): >>> >>> -- snipsnap -- >>> diff --git a/sequencer.c b/sequencer.c >>> index 46c0b3e720f..7abe78dc78e 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -573,13 +573,14 @@ static int write_author_script(const char *message) >>> static int read_env_script(struct argv_array *env) >>> { >>> struct strbuf script = STRBUF_INIT; >>> - int i, count = 0; >>> + int i, count = 0, sq_bug; >>> const char *p2; >>> char *p; >>> >>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) >>> return -1; >>> >>> + sq_bug = script.len && script.buf[script.len - 1] != '\''; >>> for (p = script.buf; *p; p++) >>> /* >>> * write_author_script() used to escape "'" incorrectly as >>> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env) >>> * version the incorrect version in case git was upgraded while >>> * rebase was stopped. >>> */ >> >> We probably want the change the comment slightly to explain sq_bug > > True. Can you give it a shot? If not, I will try to remember some time mid > next week. Sure, I'll send something out tomorrow or Wednesday. I think we should alter the test to check that there is no empty line at the end of the author-script file as that will break the test for the missing "'". > > Here the promised log of t3404 with -i -v -x (on macOS, but Linux and > Windows shows equivalent failures, and the way I read it, the problem is > simply that the test was introduced in the middle of t3404 and > subsequent test cases' assumptions are no longer met): > > -- snipsnap -- > 2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty > 2018-07-27T11:53:04.2464690Z > 2018-07-27T11:53:04.2489730Z expecting success: > 2018-07-27T11:53:04.2513840Z test_when_finished "git rebase --abort ||:" && > 2018-07-27T11:53:04.2536980Z git checkout master && > 2018-07-27T11:53:04.2563050Z set_fake_editor && > 2018-07-27T11:53:04.2590300Z FAKE_LINES="edit 1" git rebase -i HEAD^ && > 2018-07-27T11:53:04.2614900Z test -f .git/rebase-merge/author-script && > 2018-07-27T11:53:04.2639740Z ( > 2018-07-27T11:53:04.2665320Z sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > 2018-07-27T11:53:04.2688410Z eval "$(cat .git/rebase-merge/author-script)" && > 2018-07-27T11:53:04.2710930Z test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > 2018-07-27T11:53:04.2734740Z test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > 2018-07-27T11:53:04.2756700Z test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" > 2018-07-27T11:53:04.2778060Z ) > 2018-07-27T11:53:04.2788910Z > 2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:' > 2018-07-27T11:53:04.2831430Z ++ test 0 = 0 > 2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||: > 2018-07-27T11:53:04.2874430Z } && (exit "$eval_ret"); eval_ret=$?; :' > 2018-07-27T11:53:04.2892720Z ++ git checkout master > 2018-07-27T11:53:04.2911560Z Switched to branch 'master' > 2018-07-27T11:53:04.2929490Z ++ set_fake_editor > 2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh > 2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh' > 2018-07-27T11:53:04.2983780Z ++ cat > 2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh > 2018-07-27T11:53:04.3019770Z +++ pwd > 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' > 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' > 2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR > 2018-07-27T11:53:04.3094750Z ++ EDITOR='"$FAKE_EDITOR"' > 2018-07-27T11:53:04.3112610Z ++ export EDITOR > 2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1' > 2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^' > 2018-07-27T11:53:04.4674460Z rebase -i script before editing: > 2018-07-27T11:53:04.4698420Z pick 8f99a4f E > 2018-07-27T11:53:04.4710290Z > 2018-07-27T11:53:04.4792780Z rebase -i script after editing: > 2018-07-27T11:53:04.4817630Z edit 8f99a4f E > 2018-07-27T11:53:04.5025190Z Rebasing (1/1) > 2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f... E > 2018-07-27T11:53:04.5065680Z You can amend the commit now, with > 2018-07-27T11:53:04.5074810Z > 2018-07-27T11:53:04.5092850Z git commit --amend > 2018-07-27T11:53:04.5101800Z > 2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run > 2018-07-27T11:53:04.5129030Z > 2018-07-27T11:53:04.5147390Z git rebase --continue > 2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script > 2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE > 2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE > 2018-07-27T11:53:04.5220730Z ++ return 0 > 2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script > 2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\''A U Thor'\'' > 2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\''author@example.com'\'' > 2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\''@1112912233 -0700'\''' > 2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor' > 2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com > 2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700' > 2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an > 2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor' > 2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae > 2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com > 2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad > 2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700' > 2018-07-27T11:53:04.5514390Z ++ git rebase --abort > 2018-07-27T11:53:04.6924920Z ++ exit 0 > 2018-07-27T11:53:04.6950890Z ++ eval_ret=0 > 2018-07-27T11:53:04.6973900Z ++ : > 2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse > 2018-07-27T11:53:04.7009400Z > 2018-07-27T11:53:04.7030290Z expecting success: > 2018-07-27T11:53:04.7050880Z set_fake_editor && > 2018-07-27T11:53:04.7072520Z test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && > 2018-07-27T11:53:04.7092910Z test_i18ncmp expect actual > 2018-07-27T11:53:04.7102960Z > 2018-07-27T11:53:04.7123150Z ++ set_fake_editor > 2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh > 2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh' > 2018-07-27T11:53:04.7185360Z ++ cat > 2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh > 2018-07-27T11:53:04.7236720Z +++ pwd > 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' > 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' > 2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR > 2018-07-27T11:53:04.7338290Z ++ EDITOR='"$FAKE_EDITOR"' > 2018-07-27T11:53:04.7362710Z ++ export EDITOR > 2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' > 2018-07-27T11:53:04.7412410Z ++ case "$1" in > 2018-07-27T11:53:04.7436170Z ++ _test_ok= > 2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^' > 2018-07-27T11:53:04.9560670Z ++ exit_code=0 > 2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0 > 2018-07-27T11:53:04.9603200Z ++ list_contains '' success > 2018-07-27T11:53:04.9628730Z ++ case ",$1," in > 2018-07-27T11:53:04.9652120Z ++ return 1 > 2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^' > 2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^ > 2018-07-27T11:53:04.9730750Z ++ return 1 > 2018-07-27T11:53:04.9754580Z error: last command exited with $?=1 > 2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD > 2018-07-27T11:53:04.9802670Z # > 2018-07-27T11:53:04.9826330Z # set_fake_editor && > 2018-07-27T11:53:04.9851320Z # test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 && > 2018-07-27T11:53:04.9875680Z # test_i18ncmp expect actual > 2018-07-27T11:53:04.9899170Z # > 2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1 >