On Wed, Aug 1, 2018 at 11:50 AM Phillip Wood <phillip.w...@talktalk.net> wrote:
> On 31/07/18 22:39, Eric Sunshine wrote:
> > On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood <phillip.w...@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.
