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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Aug 1, 2018 at 11:50 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> 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.