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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 1, 2018, 19:19 UTC
Message-ID: <CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com>
In-Reply-To: <1f9a9062-445d-0c37-9b21-941d8fef659b@talktalk.net>

On Wed, Aug 1, 2018 at 11:50 AM Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 21 quoted lines

> On 31/07/18 22:39, Eric Sunshine wrote:
> > On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> >> +       /*
> >> +        * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE
> >> +        * line with a "'" and also escaped "'" incorrectly as "'\\\\''" rather
> >> +        * than "'\\''". We check for the terminating "'" on the last line to
> >> +        * see how "'" has been escaped in case git was upgraded while rebase
> >> +        * was stopped.
> >> +        */
> >> +       sq_bug = script.len && script.buf[script.len - 2] != '\'';
> >
> > This is a very "delicate" check, assuming that a hand-edited file
> > won't end with, say, an extra newline. I wonder if this level of
> > backward-compatibility is overkill for such an unlikely case.
>
> I think I'll get rid of the check and instead use a version number
> written to .git/rebase-merge/interactive to indicate if we need to fix
> the quoting (if there's no number then it needs fixing). We can
> increment the version number in the future if we ever need to implement
> other fallbacks to handle the case where git got upgraded while rebase
> was stopped. I'll send a patch tomorrow

Hmm, that approach is pretty heavyweight and would add a fair bit of new code and complexity which itself could harbor bugs. When I commented that the check was "delicate", I was (especially) referring to the rigid "script[len-2]", not necessarily to the basic idea of the check. So, you could keep the check (in spirit) but make it more robust. Like this, for instance:

/* big comment explaining old buggy stuff */
static int broken_quoting(const char *s, size_t n)
{
    const char *t = s + n;
    while (t > s && *--t == '\n')
        /* empty */;
    if (t > s && *--t != '\'')
        return 1;
    return 0;
}

static int read_env_script(...)
{
    ...
    sq_bug = broken_quoting(script.buf, script.len);
    ...
}

I would feel much more comfortable with a simple solution like this than with one introducing new complexity associated with adding a version number.