Hi Johannes
On 26/07/18 13:33, Johannes Schindelin wrote:
> Hi Phillip,
>
> On Wed, 18 Jul 2018, Phillip Wood wrote:
>
>> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>>
>> 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.
>>
>> Ideally write_author_script() would be rewritten to use
>> split_ident_line() and sq_quote_buf() but this commit just fixes the
>> immediate bug.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>> ---
>
> Good catch.
>
>> This is untested, unfortuantely I don't have really have time to write a test or
>> follow this up at the moment, if someone else want to run with it then please
>> do.
>
> I modified the test that was added by Akinori. As it was added very early,
> and as there is still a test case *after* Akinori's that compares a
> hard-coded SHA-1, I refrained from using `test_commit` (which would change
> that SHA-1). See below.
Thanks for adding a test, that sounds like sensible approach, however
having thought about it I wonder if we should just be writing a plain
text file (e.g rebase-merge/author-data) and fixing the reader to read
that if it exists and only then fall back to reading the legacy
rebase-merge/author-script with a fix to correctly handle the script
written by the shell version - what do you think? The author-script
really should be just an implementation detail. If anyone really wants
to read it they can still do 'read -r l' and split the lines with
${l%%=*} and ${l#*=}
>> diff --git a/sequencer.c b/sequencer.c
>> index 5354d4d51e..0b78d1f100 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
>> else if (*message != '\'')
>> strbuf_addch(&buf, *(message++));
>> else
>> - strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> + strbuf_addf(&buf, "'\\%c'", *(message++));
>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
>> while (*message && *message != '\n' && *message != '\r')
>> if (skip_prefix(message, "> ", &message))
>> break;
>> else if (*message != '\'')
>> strbuf_addch(&buf, *(message++));
>> else
>> - strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> + strbuf_addf(&buf, "'\\%c'", *(message++));
>> strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
>> while (*message && *message != '\n' && *message != '\r')
>> if (*message != '\'')
>> strbuf_addch(&buf, *(message++));
>> else
>> - strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> + strbuf_addf(&buf, "'\\%c'", *(message++));
>> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>
> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
> this, including the fixup to Junio's fixup to the
> `fix-t3404-author-script-test` branch at https://github.com/dscho/git.
>
>> strbuf_release(&buf);
>> return res;
>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
>> {
>> struct strbuf script = STRBUF_INIT;
>> int i, count = 0;
>> - char *p, *p2;
>> + const char *p2;
>> + char *p;
>>
>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>> return -1;
>>
>> for (p = script.buf; *p; p++)
>> - if (skip_prefix(p, "'\\\\''", (const char **)&p2))
>> + /*
>> + * write_author_script() used to escape "'" incorrectly as
>> + * "'\\\\''" rather than "'\\''" so we check for the correct
>> + * version the incorrect version in case git was upgraded while
>> + * rebase was stopped.
>> + */
>> + if (skip_prefix(p, "'\\''", &p2) ||
>> + skip_prefix(p, "'\\\\''", &p2))
>
> I think in this form, it is possibly unsafe because it assumes that the
> new code cannot generate output that would trigger that same code path.
> Although I have to admit that I did not give this a great deal of thought.
Hm, I not sure that it can. If the Author begins \\' then this will be
written as the C string "'\\\\'\\''...". If \\' comes at the end then I
think this will be written as "\\\\'\\'''", in the middle of the name it
will be "\\\\'\\''..."
> In any case, if you have to think long and hard about some fix, it might
> be better to go with something that is easier to reason about. So how
> about this: we already know that the code is buggy, Akinori fixed the bug,
> where the author-script missed its trailing single-quote. We can use this
> as a tell-tale for *this* bug. Assuming that Junio will advance both your
> and Akinori's fix in close proximity.
That sounds like a good approach
> Again, this is pushed to the `fix-t3404-author-script-test` branch at
> https://github.com/dscho/git; My fixup on top of your patch looks like
> this (feel free to drop the sq_bug part and only keep the test part):
>
> -- snipsnap --
> diff --git a/sequencer.c b/sequencer.c
> index 46c0b3e720f..7abe78dc78e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -573,13 +573,14 @@ static int write_author_script(const char *message)
> static int read_env_script(struct argv_array *env)
> {
> struct strbuf script = STRBUF_INIT;
> - int i, count = 0;
> + int i, count = 0, sq_bug;
> const char *p2;
> char *p;
>
> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> return -1;
>
> + sq_bug = script.len && script.buf[script.len - 1] != '\'';
> for (p = script.buf; *p; p++)
> /*
> * write_author_script() used to escape "'" incorrectly as
> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)
> * version the incorrect version in case git was upgraded while
> * rebase was stopped.
> */
We probably want the change the comment slightly to explain sq_bug
> - if (skip_prefix(p, "'\\''", &p2) ||
> - skip_prefix(p, "'\\\\''", &p2))
> + if (sq_bug && skip_prefix(p, "'\\\\''", &p2))
> + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> + else if (skip_prefix(p, "'\\''", &p2))
> strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> else if (*p == '\'')
> strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 97f0b4bf881..dd726ff4dc4 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' '
> test_line_count = 6 actual
> '
>
> +SQ="'"
> test_expect_success 'rebase -i writes correct author-script' '
> test_when_finished "test_might_fail git rebase --abort" &&
> - git checkout master &&
> + 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 -i HEAD^ &&
> + FAKE_LINES="edit 1" git rebase -ki HEAD^ &&
> (
> sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> . .git/rebase-merge/author-script &&
> test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \
> - "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"
> + "Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"
> )
> '
Testing all three variables together is nice.
Best Wishes
Phillip
> @@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' '
> )
> '
>
> -SQ="'"
> test_expect_success 'rebase -i --gpg-sign=<key-id>' '
> set_fake_editor &&
> FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>
