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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2018, 17:27 UTC
Message-ID: <xmqqk1p83cig.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180802112002.720-3-phillip.wood@talktalk.net>

Phillip Wood <phillip.wood@talktalk.net> writes:

Show 13 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.

Show 10 quoted lines

> 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 "'\\\\''".

Show 6 quoted lines

> +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.