On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood <phillip.wood@xxxxxxxxxxxx> wrote:
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.
Is the:
...for some reason sq_dequote() called by read_author_ident()
seems to handle the broken quoting correctly.
bit outdated? We know now from patch 2/4 of my series[1] that
read_author_ident() wasn't handling it correctly at all. It was merely
ignoring the return value from sq_dequote() and using whatever broken
value came back from it.
[1]: https://public-inbox.org/git/20180731073331.40007-3-sunshine@xxxxxxxxxxxxxx/
> Helped-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -664,14 +664,25 @@ 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;
This is not a problem introduced by this patch, but since
strbuf_read_file() doesn't guarantee that memory hasn't been allocated
when it returns an error, this is leaking.
> + /*
> + * 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] != '\'';
I think you need to be checking 'script.len > 1', not just
'script.len', otherwise you might access memory outside the allocated
buffer.
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.
> 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))
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '
> +test_expect_success 'rebase -i writes correct author-script' '
> + test_when_finished "test_might_fail git rebase --abort" &&
> + git checkout -b author-with-sq master &&
> + GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq &&
> + set_fake_editor &&
> + FAKE_LINES="edit 1" git rebase -ki HEAD^ &&
Hmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?
