Phillip Wood <phillip.wood@xxxxxxxxxxxx> writes:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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 read_author_ident() errors out
> on the bad quoting.
>
> For other interactive rebases this only affects external scripts that
> read the author script and users whose git is upgraded from the shell
> version of rebase -i while rebase was stopped when the author contains
> "'". This is because the parsing in read_env_script() expected the
> broken quoting.
I wasn't following the discussion, but is it the general consensus
that reading the broken a-i file is a requirement for the new code?
Not an objection phrased as a question.
I do not think it is worth worrying about the "upgrade while rebase
was in progress" case, if it involves much more code than necessary
without its support, especially if the only thing the user needs to
do recover from such a situation is to say "rebase --abort" and then
to retry the same rebase with the fixed version that was installed
in the meantime. Let's see how much we need to bend over backwards
to do this "transition" thing.
> Ideally rebase and am would share the same code for reading and
> writing the author script, but this commit just fixes the immediate
> bug.
OK.
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 06a7b79307..c1e3f947a5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -880,7 +880,7 @@ init_basic_state () {
> mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
> rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>
> - : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
> + echo 1 > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
This impacts the work Alban is doing, which at the end removes this
script altogether.
> +/*
> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE line with
> + * a "'" and also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". Fix
I think the comment here (for both the wrong and the right versions)
is easier to read if you wrote the string as literal without C, i.e.
The string is "'\\''" but as a string literal in C it is expressed
as "'\\\\''".
> +static int fix_bad_author_script(struct strbuf *script)
> +{
> + const char *next;
> + size_t off = 0;
> +
> + while ((next = strstr(script->buf + off, "'\\\\''"))) {
This looks brittle.
We need assurance that the first "'\\''" we see on the line came
from the attempt by the broken writer to write out a single "'", and
not from anything else. The broken writer places its own "'"
immediately after GIT_AUTHOR_NAME= (just like the corrected one
does) before moving on to the end-user payload. Can the single
quote at the beginning of the substring you are looking for be that
one? If the end user's payload began with two backslashes, that
would have produced a result that matches the first three bytes of
the substring you are looking for. But there is no way for the
end-user payload to make the next two bytes "''"---any byte other
than a sq would result in a sq added to the result, and a byte that
is a sq would give one sq followed by a bs.
OK, so this is probably doing the right thing, as long as we know we
are reading from the old and broken writer. It still does look
unnecessarily ugly and over-engineered to have this (and the
"version" reading code), though, at least to me, but perhaps it is
just me.
Thanks.
