On 08/08/18 10:39, Eric Sunshine wrote:
> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood <phillip.wood@xxxxxxxxxxxx> wrote:
>> 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 by read_author_ident()
>> errors out on the bad quoting.
>> [...]
>> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>> ---
>> diff --git a/sequencer.c b/sequencer.c
>> @@ -636,42 +636,64 @@ 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;
>
> Erm, again, not introduced by this patch, but this is leaking 'script'
> in the error path. You had plugged this leak in the previous round but
> that fix got lost when you reverted to this simpler approach. Not
> critical, though; the leak probably ought to be fixed by a separate
> patch anyhow (which doesn't necessarily need to be part of this
> series).
I'm hoping this will go away when I work on unifying the code to read
the author-script with am.
>> + /* write_author_script() used to quote incorrectly */
>> + sq_bug = quoting_is_broken(script.buf, script.len);
>> 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))
>> strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>
> The two strbuf_splice() invocations are identical, so an alternate way
> of expressing this would be:
>
> if ((sq_bug && skip_prefix(p, "'\\\\''", &p2)) ||
> skip_prefix(p, "'\\''", &p2))
> strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>
> Not necessarily clearer, and certainly not worth a re-roll.
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> @@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
>> test_expect_success 'valid author header after --root swap' '
>> rebase_setup_and_clean author-header no-conflict-branch &&
>> set_fake_editor &&
>> - FAKE_LINES="2 1" git rebase -i --root &&
>> - git cat-file commit HEAD^ >out &&
>> - grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
>> + git commit --amend --author="Au ${SQ}thor <author@xxxxxxxxxxx>" --no-edit &&
>> + git cat-file commit HEAD | grep ^author >expected &&
>> + FAKE_LINES="5 1" git rebase -i --root &&
>> + git cat-file commit HEAD^ | grep ^author >actual &&
>> + test_cmp expected actual
>> +'
>
> It probably would have been clearer to change to this test to use
> test_cmp() instead of 'grep' in a separate patch since it's not
> directly related to the fixes in this patch, and then to do the
> "commit --amend" in this patch. However, probably not worth a re-roll.
In hindsight that would have been clearer, but hopefully this version
isn't too confusing
>> +test_expect_success 'valid author header when author contains single quote' '
>> + rebase_setup_and_clean author-header no-conflict-branch &&
>> + set_fake_editor &&
>> + git commit --amend --author="Au ${SQ}thor <author@xxxxxxxxxxx>" --no-edit &&
>> + git cat-file commit HEAD | grep ^author >expected &&
>> + FAKE_LINES="2" git rebase -i HEAD~2 &&
>> + git cat-file commit HEAD | grep ^author >actual &&
>> + test_cmp expected actual
>> '
>
> This test is so similar to the one above that it is tempting to try to
> refactor the code so the tests can share implementation, however, the
> end result would probably be less readable, so they're probably fine
> as-is.
Yes, I think it could look messy
Thank you very much for all your help and comments on this series
Best Wishes
Phillip
