{"thread":{"id":"48977","subject":"[PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","startedAt":"2018-07-30T09:30:10Z","lastAt":"2018-07-30T19:19:14Z","messageCount":13,"participants":["Eric Sunshine","Phillip Wood","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"353856","messageId":"20180730092929.71114-1-sunshine@sunshineco.com","threadId":"48977","inReplyTo":null,"subject":"[PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T09:29:27Z","receivedAt":"2018-07-30T09:30:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series fixes bugs causing corruption of the root commit when\n\"rebase -i --root\" is used to swap in a new root commit. In particular,\nthe \"author\" header has trailing garbage. Some tools handle the\ncorruption somewhat gracefully by showing a bogus date, but others barf\non it (gitk, for instance). git-fsck correctly identifies the\ncorruption. I discovered this after git-rebase corrupted one of my own\nprojects.\n\nUnfortunately, these bugs (from js/sequencer-and-root-commits) made it\ninto the v2.18.0 release. It's worrying that a released Git can be\ncreating corrupt commits, but fortunately \"rebase -i --root\" is not\nlikely used often (especially on well-established projects).\nNevertheless, it may be 'maint' worthy and applies cleanly there.\n\nIt was only after I diagnosed and fixed these bugs that I thought to\ncheck 'pu' and discovered that Akinori MUSHA already made a stab[1] at\nfixing one of the three bugs which this series fixes. Akinori's fix has\nthe somewhat undesirable property that it adds an extra blank line to\nthe end of the script, as Phillip correctly pointed out in review[2].\nPatch 2/2 of this series has the more \"correct\" fix, in addition to\nfixing another bug.\n\nMoreover, patch 2/2 of this series provides a more thorough fix overall\nthan Akinori, so it may make sense to replace his patch with this\nseries, though perhaps keep the test his patch adds to augment the\nstrict test of the \"author\" header added by this series.\n\n[1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/\n[2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/\n\nEric Sunshine (2):\n sequencer: fix \"rebase -i --root\" corrupting author header\n sequencer: fix \"rebase -i --root\" corrupting author header timezone\n\n sequencer.c | 9 +++++++--\n t/t3404-rebase-interactive.sh | 10 +++++++++-\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad\n\n"},{"id":"353857","messageId":"20180730092929.71114-2-sunshine@sunshineco.com","threadId":"48977","inReplyTo":"20180730092929.71114-1-sunshine@sunshineco.com","subject":"[PATCH 1/2] sequencer: fix \"rebase -i --root\" corrupting author header","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T09:29:28Z","receivedAt":"2018-07-30T09:30:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When \"git rebase -i --root\" creates a new root commit (say, by swapping\nin a different commit for the root), it corrupts the commit's \"author\"\nheader with trailing garbage:\n\n author A U Thor @1112912773 -07000or@example.com\n\nThis is a result of read_author_ident() neglecting to NUL-terminate the\nbuffer into which it composes the \"author\" header.\n\n(Note that the extra \"0\" in the timezone is a separate bug which will be\nfixed subsequently.)\n\nSecurity considerations: Construction of the \"author\" header by\nread_author_ident() happens in-place and in parallel with parsing the\ncontent of \"rebase-merge/author-script\" which occupies the same buffer.\nThis is possible because the constructed \"author\" header is always\nsmaller than the content of \"rebase-merge/author-script\". Despite\nneglecting to NUL-terminate the constructed \"author\" header, memory is\nnever accessed (either by read_author_ident() or its caller) beyond the\nallocated buffer since a NUL-terminator is present at the end of the\nloaded \"rebase-merge/author-script\" content, and additional NUL's are\ninserted as part of the parsing process.\n\nSigned-off-by: Eric Sunshine \n---\n sequencer.c | 2 +-\n t/t3404-rebase-interactive.sh | 10 +++++++++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 16c1411054..78864d9072 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -744,7 +744,7 @@ static const char *read_author_ident(struct strbuf *buf)\n \t\treturn NULL;\n \t}\n \n-\tbuf->len = out - buf->buf;\n+\tstrbuf_setlen(buf, out - buf->buf);\n \treturn buf->buf;\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 01616901bd..8509c89a26 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1238,7 +1238,7 @@ rebase_setup_and_clean () {\n \t\ttest_might_fail git branch -D $1 &&\n \t\ttest_might_fail git rebase --abort\n \t\" &&\n-\tgit checkout -b $1 master\n+\tgit checkout -b $1 ${2:-master}\n }\n \n test_expect_success 'drop' '\n@@ -1415,4 +1415,12 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\n \ttest_i18ngrep \"$SQ-S\\\"S I Gner\\\"$SQ\" err\n '\n \n+test_expect_success 'valid author header after --root swap' '\n+\trebase_setup_and_clean author-header no-conflict-branch &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"2 1\" git rebase -i --root &&\n+\tgit cat-file commit HEAD^ >out &&\n+\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$\" out\n+'\n+\n test_done\n-- \n2.18.0.597.ga71716f1ad\n\n"},{"id":"353858","messageId":"20180730092929.71114-3-sunshine@sunshineco.com","threadId":"48977","inReplyTo":"20180730092929.71114-1-sunshine@sunshineco.com","subject":"[PATCH 2/2] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T09:29:29Z","receivedAt":"2018-07-30T09:30:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"When \"git rebase -i --root\" creates a new root commit, it corrupts the\n\"author\" header's timezone by repeating the last digit:\n\n author A U Thor @1112912773 -07000\n\nThis is due to two bugs.\n\nFirst, write_author_script() neglects to add the closing quote to the\nvalue of GIT_AUTHOR_DATE when generating \"rebase-merge/author-script\".\n\nSecond, although sq_dequote() correctly diagnoses the missing closing\nquote, read_author_ident() ignores sq_dequote()'s return value and\nblindly uses the result of the aborted dequote.\n\nsq_dequote() performs dequoting in-place by removing quoting and\nshifting content downward. When it detects misquoting (lack of closing\nquote, in this case), it gives up and returns an error without inserting\na NUL-terminator at the end of the shifted content, which explains the\nduplicated last digit in the timezone.\n\nSigned-off-by: Eric Sunshine \n---\n sequencer.c | 7 ++++++-\n t/t3404-rebase-interactive.sh | 2 +-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 78864d9072..1008f6d71a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -654,6 +654,7 @@ static int write_author_script(const char *message)\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\tstrbuf_addch(&buf, '\\'');\n \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n \tstrbuf_release(&buf);\n \treturn res;\n@@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)\n \n \t\teol = strchrnul(in, '\\n');\n \t\t*eol = '\\0';\n-\t\tsq_dequote(in);\n+\t\tif (!sq_dequote(in)) {\n+\t\t\twarning(_(\"bad quoting on %s value in '%s'\"),\n+\t\t\t\tkeys[i], rebase_path_author_script());\n+\t\t\treturn NULL;\n+\t\t}\n \t\tlen = strlen(in);\n \n \t\tif (i > 0) /* separate values by spaces */\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 8509c89a26..37796bb4c1 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '\n \tset_fake_editor &&\n \tFAKE_LINES=\"2 1\" git rebase -i --root &&\n \tgit cat-file commit HEAD^ >out &&\n-\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$\" out\n+\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n '\n \n test_done\n-- \n2.18.0.597.ga71716f1ad\n\n"},{"id":"353861","messageId":"CAPig+cSYPEqapdQ=7zjW-RsM6XxCw2vPzZzV32uh_SgO4cQSog@mail.gmail.com","threadId":"48977","inReplyTo":"20180730092929.71114-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T10:06:11Z","receivedAt":"2018-07-30T10:06:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 30, 2018 at 5:30 AM Eric Sunshine wrote:\n> This series fixes bugs causing corruption of the root commit when\n> \"rebase -i --root\" is used to swap in a new root commit. In particular,\n> the \"author\" header has trailing garbage. Some tools handle the\n> corruption somewhat gracefully by showing a bogus date, but others barf\n> on it (gitk, for instance). git-fsck correctly identifies the\n> corruption. I discovered this after git-rebase corrupted one of my own\n> projects.\n\nUnfortunately, after sending this series, I see that there is\nadditional corruption that needs to be addressed. In particular, the\n\"author\" header time is incorrectly prefixed with \"@\", so this series\nis going to need a re-roll to fix that, as well.\n"},{"id":"353862","messageId":"1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net","threadId":"48977","inReplyTo":"20180730092929.71114-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-30T12:14:27Z","receivedAt":"2018-07-30T12:14:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 30/07/18 10:29, Eric Sunshine wrote:\n> This series fixes bugs causing corruption of the root commit when\n> \"rebase -i --root\" is used to swap in a new root commit. In particular,\n> the \"author\" header has trailing garbage. Some tools handle the\n> corruption somewhat gracefully by showing a bogus date, but others barf\n> on it (gitk, for instance). git-fsck correctly identifies the\n> corruption. I discovered this after git-rebase corrupted one of my own\n> projects.\n> \n> Unfortunately, these bugs (from js/sequencer-and-root-commits) made it\n> into the v2.18.0 release. It's worrying that a released Git can be\n> creating corrupt commits, but fortunately \"rebase -i --root\" is not\n> likely used often (especially on well-established projects).\n> Nevertheless, it may be 'maint' worthy and applies cleanly there.\n> \n> It was only after I diagnosed and fixed these bugs that I thought to\n> check 'pu' and discovered that Akinori MUSHA already made a stab[1] at\n> fixing one of the three bugs which this series fixes. Akinori's fix has\n> the somewhat undesirable property that it adds an extra blank line to\n> the end of the script, as Phillip correctly pointed out in review[2].\n> Patch 2/2 of this series has the more \"correct\" fix, in addition to\n> fixing another bug.\n> \n> Moreover, patch 2/2 of this series provides a more thorough fix overall\n> than Akinori, so it may make sense to replace his patch with this\n> series, though perhaps keep the test his patch adds to augment the\n> strict test of the \"author\" header added by this series.\n\nJohannes and I have some fixups for Akinori's patch on the branch\nfix-t3403-author-script-test at https://github.com/phillipwood/git\n\nThat branch also contains a fix for the bad quoting of names with \"'\" in\nthem. I think it would be good to somehow try and combine this series\nwith those patches.\n\nI'd really like to see a single function to read and another to write\nthe author script that is shared by 'git am' and 'git rebase -i', rather\nthan the two writers and three readers we have at the moment. I was\nthinking of doing that in the longer term, but given the extra bug\nyou've found in read_author_script() maybe we should do that sooner\nrather than later.\n\n> [1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/\n> [2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/\n> \n> Eric Sunshine (2):\n> sequencer: fix \"rebase -i --root\" corrupting author header\n> sequencer: fix \"rebase -i --root\" corrupting author header timezone\n> \n> sequencer.c | 9 +++++++--\n> t/t3404-rebase-interactive.sh | 10 +++++++++-\n> 2 files changed, 16 insertions(+), 3 deletions(-)\n> \n\n"},{"id":"353865","messageId":"f079bf7b-d627-20dc-cd93-dfa3496f6877@talktalk.net","threadId":"48977","inReplyTo":"20180730092929.71114-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-30T12:20:38Z","receivedAt":"2018-07-30T12:20:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\nOn 30/07/18 10:29, Eric Sunshine wrote:\n> When \"git rebase -i --root\" creates a new root commit, it corrupts the\n> \"author\" header's timezone by repeating the last digit:\n> \n> author A U Thor @1112912773 -07000\n> \n> This is due to two bugs.\n> \n> First, write_author_script() neglects to add the closing quote to the\n> value of GIT_AUTHOR_DATE when generating \"rebase-merge/author-script\".\n> \n> Second, although sq_dequote() correctly diagnoses the missing closing\n> quote, read_author_ident() ignores sq_dequote()'s return value and\n> blindly uses the result of the aborted dequote.\n> \n> sq_dequote() performs dequoting in-place by removing quoting and\n> shifting content downward. When it detects misquoting (lack of closing\n> quote, in this case), it gives up and returns an error without inserting\n> a NUL-terminator at the end of the shifted content, which explains the\n> duplicated last digit in the timezone.\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> sequencer.c | 7 ++++++-\n> t/t3404-rebase-interactive.sh | 2 +-\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 78864d9072..1008f6d71a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -654,6 +654,7 @@ static int write_author_script(const char *message)\n> \t\t\tstrbuf_addch(&buf, *(message++));\n> \t\telse\n> \t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n> +\tstrbuf_addch(&buf, '\\'');\n> \tres = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);\n> \tstrbuf_release(&buf);\n> \treturn res;\n> @@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)\n> \n> \t\teol = strchrnul(in, '\\n');\n> \t\t*eol = '\\0';\n> -\t\tsq_dequote(in);\n> +\t\tif (!sq_dequote(in)) {\n> +\t\t\twarning(_(\"bad quoting on %s value in '%s'\"),\n> +\t\t\t\tkeys[i], rebase_path_author_script());\n> +\t\t\treturn NULL;\n\nI think we want to handle the broken author script properly rather than\nreturning NULL. If we had a single function\nint read_author_script(const char **name, const char **author, const\nchar **date)\nto read the author script that tried sq_dequote() and then fell back to\ncode based on read_env_script() that handled the missing \"'\" at the end\nand also the bad quoting of \"'\" if sq_dequote() failed it would make it\neasier to fix the existing bugs, rather than having to fix\nread_author_ident() and read_env_script() separately. What do you think?\n\nBest Wishes\n\nPhillip\n\n> +\t\t}\n> \t\tlen = strlen(in);\n> \n> \t\tif (i > 0) /* separate values by spaces */\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 8509c89a26..37796bb4c1 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"2 1\" git rebase -i --root &&\n> \tgit cat-file commit HEAD^ >out &&\n> -\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$\" out\n> +\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n> '\n> \n> test_done\n> \n\n"},{"id":"353886","messageId":"xmqq1sbkkaim.fsf@gitster-ct.c.googlers.com","threadId":"48977","inReplyTo":"1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-30T15:29:05Z","receivedAt":"2018-07-30T15:29:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> Moreover, patch 2/2 of this series provides a more thorough fix overall\n>> than Akinori, so it may make sense to replace his patch with this\n>> series, though perhaps keep the test his patch adds to augment the\n>> strict test of the \"author\" header added by this series.\n>\n> Johannes and I have some fixups for Akinori's patch on the branch\n> fix-t3403-author-script-test at https://github.com/phillipwood/git\n>\n> That branch also contains a fix for the bad quoting of names with \"'\" in\n> them. I think it would be good to somehow try and combine this series\n> with those patches.\n\nThanks for working well together. Always nice to see contributors\nthinking beyond immediate band-aid and for longer term ;-)\n\n> I'd really like to see a single function to read and another to write\n> the author script that is shared by 'git am' and 'git rebase -i', rather\n> than the two writers and three readers we have at the moment. I was\n> thinking of doing that in the longer term, but given the extra bug\n> you've found in read_author_script() maybe we should do that sooner\n> rather than later.\n\n"},{"id":"353899","messageId":"nycvar.QRO.7.76.6.1807301744090.10478@tvgsbejvaqbjf.bet","threadId":"48977","inReplyTo":"20180730092929.71114-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/2] sequencer: fix \"rebase -i --root\" corrupting author header","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-30T15:44:23Z","receivedAt":"2018-07-30T15:44:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 30 Jul 2018, Eric Sunshine wrote:\n\n> When \"git rebase -i --root\" creates a new root commit (say, by swapping\n> in a different commit for the root), it corrupts the commit's \"author\"\n> header with trailing garbage:\n> \n> author A U Thor @1112912773 -07000or@example.com\n> \n> This is a result of read_author_ident() neglecting to NUL-terminate the\n> buffer into which it composes the \"author\" header.\n> \n> (Note that the extra \"0\" in the timezone is a separate bug which will be\n> fixed subsequently.)\n> \n> Security considerations: Construction of the \"author\" header by\n> read_author_ident() happens in-place and in parallel with parsing the\n> content of \"rebase-merge/author-script\" which occupies the same buffer.\n> This is possible because the constructed \"author\" header is always\n> smaller than the content of \"rebase-merge/author-script\". Despite\n> neglecting to NUL-terminate the constructed \"author\" header, memory is\n> never accessed (either by read_author_ident() or its caller) beyond the\n> allocated buffer since a NUL-terminator is present at the end of the\n> loaded \"rebase-merge/author-script\" content, and additional NUL's are\n> inserted as part of the parsing process.\n> \n> Signed-off-by: Eric Sunshine \n\nACK!\n\nThanks,\nDscho\n\n> ---\n> sequencer.c | 2 +-\n> t/t3404-rebase-interactive.sh | 10 +++++++++-\n> 2 files changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 16c1411054..78864d9072 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -744,7 +744,7 @@ static const char *read_author_ident(struct strbuf *buf)\n> \t\treturn NULL;\n> \t}\n> \n> -\tbuf->len = out - buf->buf;\n> +\tstrbuf_setlen(buf, out - buf->buf);\n> \treturn buf->buf;\n> }\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 01616901bd..8509c89a26 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1238,7 +1238,7 @@ rebase_setup_and_clean () {\n> \t\ttest_might_fail git branch -D $1 &&\n> \t\ttest_might_fail git rebase --abort\n> \t\" &&\n> -\tgit checkout -b $1 master\n> +\tgit checkout -b $1 ${2:-master}\n> }\n> \n> test_expect_success 'drop' '\n> @@ -1415,4 +1415,12 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\n> \ttest_i18ngrep \"$SQ-S\\\"S I Gner\\\"$SQ\" err\n> '\n> \n> +test_expect_success 'valid author header after --root swap' '\n> +\trebase_setup_and_clean author-header no-conflict-branch &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"2 1\" git rebase -i --root &&\n> +\tgit cat-file commit HEAD^ >out &&\n> +\tgrep \"^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$\" out\n> +'\n> +\n> test_done\n> -- \n> 2.18.0.597.ga71716f1ad\n> \n> \n"},{"id":"353900","messageId":"nycvar.QRO.7.76.6.1807301746190.10478@tvgsbejvaqbjf.bet","threadId":"48977","inReplyTo":"CAPig+cSYPEqapdQ=7zjW-RsM6XxCw2vPzZzV32uh_SgO4cQSog@mail.gmail.com","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-30T15:47:01Z","receivedAt":"2018-07-30T15:47:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 30 Jul 2018, Eric Sunshine wrote:\n\n> On Mon, Jul 30, 2018 at 5:30 AM Eric Sunshine wrote:\n> > This series fixes bugs causing corruption of the root commit when\n> > \"rebase -i --root\" is used to swap in a new root commit. In particular,\n> > the \"author\" header has trailing garbage. Some tools handle the\n> > corruption somewhat gracefully by showing a bogus date, but others barf\n> > on it (gitk, for instance). git-fsck correctly identifies the\n> > corruption. I discovered this after git-rebase corrupted one of my own\n> > projects.\n> \n> Unfortunately, after sending this series, I see that there is\n> additional corruption that needs to be addressed. In particular, the\n> \"author\" header time is incorrectly prefixed with \"@\", so this series\n> is going to need a re-roll to fix that, as well.\n\nAFAIR the `@` was not an oversight, but required so that we could pass in\nthe Unix epoch.\n\nCiao,\nDscho\n"},{"id":"353901","messageId":"nycvar.QRO.7.76.6.1807301747330.10478@tvgsbejvaqbjf.bet","threadId":"48977","inReplyTo":"1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-30T15:49:36Z","receivedAt":"2018-07-30T15:49:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Mon, 30 Jul 2018, Phillip Wood wrote:\n\n> On 30/07/18 10:29, Eric Sunshine wrote:\n> > This series fixes bugs causing corruption of the root commit when\n> > \"rebase -i --root\" is used to swap in a new root commit. In particular,\n> > the \"author\" header has trailing garbage. Some tools handle the\n> > corruption somewhat gracefully by showing a bogus date, but others barf\n> > on it (gitk, for instance). git-fsck correctly identifies the\n> > corruption. I discovered this after git-rebase corrupted one of my own\n> > projects.\n> > \n> > Unfortunately, these bugs (from js/sequencer-and-root-commits) made it\n> > into the v2.18.0 release. It's worrying that a released Git can be\n> > creating corrupt commits, but fortunately \"rebase -i --root\" is not\n> > likely used often (especially on well-established projects).\n> > Nevertheless, it may be 'maint' worthy and applies cleanly there.\n> > \n> > It was only after I diagnosed and fixed these bugs that I thought to\n> > check 'pu' and discovered that Akinori MUSHA already made a stab[1] at\n> > fixing one of the three bugs which this series fixes. Akinori's fix has\n> > the somewhat undesirable property that it adds an extra blank line to\n> > the end of the script, as Phillip correctly pointed out in review[2].\n> > Patch 2/2 of this series has the more \"correct\" fix, in addition to\n> > fixing another bug.\n> > \n> > Moreover, patch 2/2 of this series provides a more thorough fix overall\n> > than Akinori, so it may make sense to replace his patch with this\n> > series, though perhaps keep the test his patch adds to augment the\n> > strict test of the \"author\" header added by this series.\n> \n> Johannes and I have some fixups for Akinori's patch on the branch\n> fix-t3403-author-script-test at https://github.com/phillipwood/git\n> \n> That branch also contains a fix for the bad quoting of names with \"'\" in\n> them. I think it would be good to somehow try and combine this series\n> with those patches.\n\nI would like that, too.\n\n> I'd really like to see a single function to read and another to write\n> the author script that is shared by 'git am' and 'git rebase -i', rather\n> than the two writers and three readers we have at the moment. I was\n> thinking of doing that in the longer term, but given the extra bug\n> you've found in read_author_script() maybe we should do that sooner\n> rather than later.\n\nYou are at least the second person (after Junio) with that wish.\n\nPlease note, however, that the purpose of author-script reading/writing is\nvery different in git-am vs rebase -i, or at least it used to be:\nread_env_script() in sequencer.c very specifically wants to construct an\nenv parameter for use in run_command().\n\nDon't let me stop you from trying to consolidate the two different code\npaths, though.\n\nCiao,\nDscho\n\n> \n> > [1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/\n> > [2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/\n> > \n> > Eric Sunshine (2):\n> > sequencer: fix \"rebase -i --root\" corrupting author header\n> > sequencer: fix \"rebase -i --root\" corrupting author header timezone\n> > \n> > sequencer.c | 9 +++++++--\n> > t/t3404-rebase-interactive.sh | 10 +++++++++-\n> > 2 files changed, 16 insertions(+), 3 deletions(-)\n> > \n> \n> \n"},{"id":"353931","messageId":"CAPig+cQyWHSmww59qP8DTyCFpbB8HdL+cnANX8uSXVsuOviBCw@mail.gmail.com","threadId":"48977","inReplyTo":"f079bf7b-d627-20dc-cd93-dfa3496f6877@talktalk.net","subject":"Re: [PATCH 2/2] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T18:45:28Z","receivedAt":"2018-07-30T18:45:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood wrote:\n> On 30/07/18 10:29, Eric Sunshine wrote:\n> > When \"git rebase -i --root\" creates a new root commit, it corrupts the\n> > \"author\" header's timezone by repeating the last digit:\n> > [...]\n> > Signed-off-by: Eric Sunshine \n> > ---\n> > diff --git a/sequencer.c b/sequencer.c\n> > @@ -654,6 +654,7 @@ static int write_author_script(const char *message)\n> > + strbuf_addch(&buf, '\\'');\n> > @@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)\n> > - sq_dequote(in);\n> > + if (!sq_dequote(in)) {\n> > + warning(_(\"bad quoting on %s value in '%s'\"),\n> > + keys[i], rebase_path_author_script());\n> > + return NULL;\n>\n> I think we want to handle the broken author script properly rather than\n> returning NULL. If we had a single function\n> int read_author_script(const char **name, const char **author, const\n> char **date)\n> to read the author script that tried sq_dequote() and then fell back to\n> code based on read_env_script() that handled the missing \"'\" at the end\n> and also the bad quoting of \"'\" if sq_dequote() failed it would make it\n> easier to fix the existing bugs, rather than having to fix\n> read_author_ident() and read_env_script() separately. What do you think?\n\nThat makes sense as a long-term plan, however, I'm concerned with the\nimmediate problem that a released version of Git can (and did, in my\ncase) corrupt commit objects. So, in the short term, I think it makes\nsense to get this minimal fix landed, and build the more \"correctly\nengineered\" solution on top of it, without the pressure of worrying\nabout corruption spreading.\n"},{"id":"353937","messageId":"CAPig+cRaxgNVkvjumgiMY_ssmvKWRh6USBdu27WbBrT7y7SP3g@mail.gmail.com","threadId":"48977","inReplyTo":"1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T19:15:03Z","receivedAt":"2018-07-30T19:15:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 30, 2018 at 8:14 AM Phillip Wood wrote:\n> On 30/07/18 10:29, Eric Sunshine wrote:\n> > It was only after I diagnosed and fixed these bugs that I thought to\n> > check 'pu' and discovered that Akinori MUSHA already made a stab[1] at\n> > fixing one of the three bugs which this series fixes. Akinori's fix has\n> > the somewhat undesirable property that it adds an extra blank line to\n> > the end of the script, as Phillip correctly pointed out in review[2].\n> > Patch 2/2 of this series has the more \"correct\" fix, in addition to\n> > fixing another bug.\n> >\n> > Moreover, patch 2/2 of this series provides a more thorough fix overall\n> > than Akinori, so it may make sense to replace his patch with this\n> > series, though perhaps keep the test his patch adds to augment the\n> > strict test of the \"author\" header added by this series.\n>\n> Johannes and I have some fixups for Akinori's patch on the branch\n> fix-t3403-author-script-test at https://github.com/phillipwood/git\n\nI don't see a branch with that name there. There are a couple \"wip\"\nbranches, however, named wip/fix-t3403-author-script-test and\nwip/fix-t3404-author-script-test. I'm guessing you wanted me to look\nat the former.\n\n> That branch also contains a fix for the bad quoting of names with \"'\" in\n> them. I think it would be good to somehow try and combine this series\n> with those patches.\n\nIt appears that your patches are fixing issues and a test outside the\nissues fixed by my series (aside from the one line inserting the\nmissing closing quote). As such, I think your patches can be built\natop this series without worrying about conflicts. That would allow\nthis commit-corruption-bug-fixing series to land without being tied to\nthose \"wip\" patches which address lower-priority problems.\n\n> I'd really like to see a single function to read and another to write\n> the author script that is shared by 'git am' and 'git rebase -i', rather\n> than the two writers and three readers we have at the moment. I was\n> thinking of doing that in the longer term, but given the extra bug\n> you've found in read_author_script() maybe we should do that sooner\n> rather than later.\n\nAgreed. That seems a reasonable long-term goal but needn't hold up\nthis series which addresses very real bugs leading to object\ncorruption.\n"},{"id":"353938","messageId":"CAPig+cT78x-2er4mMMwPAjGYpf=MSzYNUti_T6XGoO9Ue4EMog@mail.gmail.com","threadId":"48977","inReplyTo":"nycvar.QRO.7.76.6.1807301746190.10478@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/2] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-30T19:19:01Z","receivedAt":"2018-07-30T19:19:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 30, 2018 at 11:47 AM Johannes Schindelin\n wrote:\n> On Mon, 30 Jul 2018, Eric Sunshine wrote:\n> > Unfortunately, after sending this series, I see that there is\n> > additional corruption that needs to be addressed. In particular, the\n> > \"author\" header time is incorrectly prefixed with \"@\", so this series\n> > is going to need a re-roll to fix that, as well.\n>\n> AFAIR the `@` was not an oversight, but required so that we could pass in\n> the Unix epoch.\n\nI don't think it was an oversight either, but it is nevertheless\nincorrect to use the \"@\" in the commit object's \"author\" header.\n\nAs I understand it, you do \"need\" the \"@\" to distinguish a Unix epoch\nvalue assigned to GIT_AUTHOR_DATE, but the commit object format is\nvery exacting in the datestamp format it accepts, and it does not\nallow \"@\". So, the date from GIT_AUTHOR_DATE needs to be converted to\na format acceptable to the commit object, otherwise the commit is\nconsidered corrupt.\n"}]}