This is a re-roll of [1] which fixes sequencer bugs resulting in commit object corruption when "rebase -i --root" swaps in a new commit as root. Unfortunately, those bugs made it into v2.18.0 and have already corrupted at least one repository (a local project of mine). Patches 3/4 and 4/4 are new. v1 fixed these bugs: * trailing garbage on the commit's "author" header * extra trailing digit on "author" header's timezone (caused by two separate bugs) v2 fixes those same bugs, plus: * eliminates a bogus "@" prepended to the "author" header timestamp which renders the header corrupt * takes care to validate information coming from "rebase-merge/author-script" before incorporating it into the "author" header since that file may be hand-edited, and bogus hand-edited values could corrupt the commit object. Patch 2/4 of this series conflicts with Akinori MUSHA's 'am/sequencer-author-script-fix' which takes a stab at fixing one of the four (or so) bugs fixed by this series (namely, adding a missing closing quote to GIT_AUTHOR_DATE in "rebase-merge/author-script"). That patch probably ought to be dropped (without prejudice) in favor of this series for the following reasons: * It has the undesirable property of adding an unwanted extra blank line to "rebase-merge/author-script"; this series doesn't make that mistake. * The fix in this series (patch 2/4) is more complete, also ensuring that the return value of sq_dequote() is checked. * And, most importantly, this series sells the change as a fix for a genuine serious bug (commit object corruption), whereas that patch talks only about adjusting the file content to make it parseable by the shell. It's important for future readers of this change to understand that the bug (missing closing quote) had much more dire consequences than merely being syntactically invalid as a shell script. The test added by Akinori MUSHA's patch may still have value, and it may make sense to re-submit it, however, doing so need not hold up this (higher priority) series. Phillip's proposed[2] unification of parsers and other fixes and cleanups can easily be built atop this series and, likewise, need not prevent these (higher priority) patches from graduating independently. [1]: https://public-inbox.org/git/20180730092929.71114-1-sunshine@xxxxxxxxxxxxxx/ [2]: https://public-inbox.org/git/1f172fc1-4b51-cdf7-e841-5ca41e209be4@xxxxxxxxxxxx/ Eric Sunshine (4): sequencer: fix "rebase -i --root" corrupting author header sequencer: fix "rebase -i --root" corrupting author header timezone sequencer: fix "rebase -i --root" corrupting author header timestamp sequencer: don't die() on bogus user-edited timestamp sequencer.c | 39 +++++++++++++++++++++-------------- t/t3404-rebase-interactive.sh | 10 ++++++++- 2 files changed, 33 insertions(+), 16 deletions(-) Range-diff against v1: 1: ba10ae4e5d ! 1: 8c47555bcf sequencer: fix "rebase -i --root" corrupting author header @@ -11,8 +11,8 @@ This is a result of read_author_ident() neglecting to NUL-terminate the buffer into which it composes the "author" header. - (Note that the extra "0" in the timezone is a separate bug which will be - fixed subsequently.) + (Note that the "@" preceding the timestamp and the extra "0" in the + timezone are separate bugs which will be fixed subsequently.) Security considerations: Construction of the "author" header by read_author_ident() happens in-place and in parallel with parsing the @@ -26,6 +26,7 @@ inserted as part of the parsing process. Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> + Acked-by: Johannes Schindelin <johannes.schindelin@xxxxxx> diff --git a/sequencer.c b/sequencer.c --- a/sequencer.c @@ -61,7 +62,7 @@ + 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]*$" out ++ grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9]*$" out +' + test_done 2: c0400cda85 ! 2: 1d4064147e sequencer: fix "rebase -i --root" corrupting author header timezone @@ -22,6 +22,9 @@ a NUL-terminator at the end of the shifted content, which explains the duplicated last digit in the timezone. + (Note that the "@" preceding the timestamp is a separate bug which + will be fixed subsequently.) + Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> diff --git a/sequencer.c b/sequencer.c @@ -56,8 +59,8 @@ 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]*$" out -+ grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out +- grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9]*$" out ++ grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out ' test_done -: ---------- > 3: cb65c7dd98 sequencer: fix "rebase -i --root" corrupting author header timestamp -: ---------- > 4: 3a624da9f4 sequencer: don't die() on bogus user-edited timestamp -- 2.18.0.267.gbc8be36ecb
