@matheusbsbs / matheus-bagliano / commits / 4aa5ff9409

sequencer: fix quoting in write_author_script

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. 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. This patch includes code to handle the broken quoting when git has been upgraded while rebase was stopped. It does this by detecting the missing "'" at the end of the GIT_AUTHOR_DATE line to see if it should dequote \\' as "'". Note this is only implemented for normal picks, not for creating a new root commit (rebase will stop with an error complaining out bad quoting in that case). The fallback code has been manually tested by reverting both the quoting fixes in write_author_script() and the previous fix for the missing "'" at the end of the GIT_AUTHOR_DATE line and running t3404-rebase-interactive.sh. Ideally rebase and am would share the same code for reading and writing the author script, but this commit just fixes the immediate bug. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Aug 7, 2018 at 10:34 UTC 4aa5ff9409aece369bf443bcb967db3832b0222c

2 files changed +44 -10

sequencer.c

+29 -7

@@ -636,7 +636,7 @@ missing_author:

636 else if (*message != '\'')

637 strbuf_addch(&buf, *(message++));

638 else

639 - strbuf_addf(&buf, "'\\\\%c'", *(message++));

639 + strbuf_addf(&buf, "'\\%c'", *(message++));

640 strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");

641 while (*message && *message != '\n' && *message != '\r')

642 if (skip_prefix(message, "> ", &message))

@@ -644,19 +644,37 @@ missing_author:

644 else if (*message != '\'')

645 strbuf_addch(&buf, *(message++));

646 else

647 - strbuf_addf(&buf, "'\\\\%c'", *(message++));

647 + strbuf_addf(&buf, "'\\%c'", *(message++));

648 strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");

649 while (*message && *message != '\n' && *message != '\r')

650 if (*message != '\'')

651 strbuf_addch(&buf, *(message++));

652 else

653 - strbuf_addf(&buf, "'\\\\%c'", *(message++));

653 + strbuf_addf(&buf, "'\\%c'", *(message++));

654 strbuf_addch(&buf, '\'');

655 res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);

656 strbuf_release(&buf);

657 return res;

658 }

659

660 +

661 +/*

662 + * write_author_script() used to fail to terminate the last line with a "'" and

663 + * also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for

664 + * the terminating "'" on the last line to see how "'" has been escaped in case

665 + * git was upgraded while rebase was stopped.

666 + */

667 +static int quoting_is_broken(const char *s, size_t n)

668 +{

669 + /* Skip any empty lines in case the file was hand edited */

670 + while (n > 0 && s[--n] == '\n')

671 + ; /* empty */

672 + if (n > 0 && s[n] != '\'')

673 + return 1;

674 +

675 + return 0;

676 +}

677 +

678 /*

679 * Read a list of environment variable assignments (such as the author-script

680 * file) into an environment block. Returns -1 on error, 0 otherwise.

@@ -664,14 +682,18 @@ missing_author:

682 static int read_env_script(struct argv_array *env)

683 {

684 struct strbuf script = STRBUF_INIT;

667 - int i, count = 0;

668 - char *p, *p2;

685 + int i, count = 0, sq_bug;

686 + const char *p2;

687 + char *p;

688

689 if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)

690 return -1;

672 -

691 + /* write_author_script() used to quote incorrectly */

692 + sq_bug = quoting_is_broken(script.buf, script.len);

693 for (p = script.buf; *p; p++)

674 - if (skip_prefix(p, "'\\\\''", (const char **)&p2))

694 + if (sq_bug && skip_prefix(p, "'\\\\''", &p2))

695 + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);

696 + else if (skip_prefix(p, "'\\''", &p2))

697 strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);

698 else if (*p == '\'')

699 strbuf_splice(&script, p-- - script.buf, 1, "", 0);

t/t3404-rebase-interactive.sh

+15 -3

@@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '

1382 test_expect_success 'valid author header after --root swap' '

1383 rebase_setup_and_clean author-header no-conflict-branch &&

1384 set_fake_editor &&

1385 - FAKE_LINES="2 1" git rebase -i --root &&

1386 - git cat-file commit HEAD^ >out &&

1387 - grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out

1385 + git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&

1386 + git cat-file commit HEAD | grep ^author >expected &&

1387 + FAKE_LINES="5 1" git rebase -i --root &&

1388 + git cat-file commit HEAD^ | grep ^author >actual &&

1389 + test_cmp expected actual

1390 +'

1391 +

1392 +test_expect_success 'valid author header when author contains single quote' '

1393 + rebase_setup_and_clean author-header no-conflict-branch &&

1394 + set_fake_editor &&

1395 + git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&

1396 + git cat-file commit HEAD | grep ^author >expected &&

1397 + FAKE_LINES="2" git rebase -i HEAD~2 &&

1398 + git cat-file commit HEAD | grep ^author >actual &&

1399 + test_cmp expected actual

1400 '

1401

1402 test_done