Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script

[Phillip Wood <phillip.wood@talktalk.net>]

On 08/08/18 10:39, Eric Sunshine wrote:
> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
>> Single quotes should be escaped as \' not \\'. The bad quoting breaks
>> the interactive version of 'rebase --root' (which is used when there
>> is no '--onto' even if the user does not specify --interactive) for
>> authors that contain "'" as sq_dequote() called by read_author_ident()
>> errors out on the bad quoting.
>> [...]
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> diff --git a/sequencer.c b/sequencer.c
>> @@ -636,42 +636,64 @@ static int write_author_script(const char *message)
>>  static int read_env_script(struct argv_array *env)
>>  {
>>         if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>>                 return -1;
> 
> Erm, again, not introduced by this patch, but this is leaking 'script'
> in the error path. You had plugged this leak in the previous round but
> that fix got lost when you reverted to this simpler approach. Not
> critical, though; the leak probably ought to be fixed by a separate
> patch anyhow (which doesn't necessarily need to be part of this
> series).

I'm hoping this will go away when I work on unifying the code to read
the author-script with am.

>> +       /* write_author_script() used to quote incorrectly */
>> +       sq_bug = quoting_is_broken(script.buf, script.len);
>>         for (p = script.buf; *p; p++)
>> -               if (skip_prefix(p, "'\\\\''", (const char **)&p2))
>> +               if (sq_bug && skip_prefix(p, "'\\\\''", &p2))
>> +                       strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>> +               else if (skip_prefix(p, "'\\''", &p2))
>>                         strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> 
> The two strbuf_splice() invocations are identical, so an alternate way
> of expressing this would be:
> 
>      if ((sq_bug && skip_prefix(p, "'\\\\''", &p2)) ||
>         skip_prefix(p, "'\\''", &p2))
>         strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> 
> Not necessarily clearer, and certainly not worth a re-roll.
> 
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> @@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
>>  test_expect_success 'valid author header after --root swap' '
>>         rebase_setup_and_clean author-header no-conflict-branch &&
>>         set_fake_editor &&
>> -       FAKE_LINES="2 1" git rebase -i --root &&
>> -       git cat-file commit HEAD^ >out &&
>> -       grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
>> +       git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&
>> +       git cat-file commit HEAD | grep ^author >expected &&
>> +       FAKE_LINES="5 1" git rebase -i --root &&
>> +       git cat-file commit HEAD^ | grep ^author >actual &&
>> +       test_cmp expected actual
>> +'
> 
> It probably would have been clearer to change to this test to use
> test_cmp() instead of 'grep' in a separate patch since it's not
> directly related to the fixes in this patch, and then to do the
> "commit --amend" in this patch. However, probably not worth a re-roll.

In hindsight that would have been clearer, but hopefully this version
isn't too confusing

>> +test_expect_success 'valid author header when author contains single quote' '
>> +       rebase_setup_and_clean author-header no-conflict-branch &&
>> +       set_fake_editor &&
>> +       git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&
>> +       git cat-file commit HEAD | grep ^author >expected &&
>> +       FAKE_LINES="2" git rebase -i HEAD~2 &&
>> +       git cat-file commit HEAD | grep ^author >actual &&
>> +       test_cmp expected actual
>>  '
> 
> This test is so similar to the one above that it is tempting to try to
> refactor the code so the tests can share implementation, however, the
> end result would probably be less readable, so they're probably fine
> as-is.

Yes, I think it could look messy

Thank you very much for all your help and comments on this series

Best Wishes

Phillip