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

[Eric Sunshine <sunshine@sunshineco.com>]

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).

> +       /* 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.

> +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.