{"thread":{"id":"48989","subject":"[PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","startedAt":"2018-07-31T07:34:08Z","lastAt":"2018-08-27T22:34:56Z","messageCount":57,"participants":["Eric Sunshine","Phillip Wood","Hilco Wijbenga","Junio C Hamano","brian m. carlson","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"353996","messageId":"20180731073331.40007-1-sunshine@sunshineco.com","threadId":"48989","inReplyTo":null,"subject":"[PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T07:33:27Z","receivedAt":"2018-07-31T07:34:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This is a re-roll of [1] which fixes sequencer bugs resulting in commit\nobject corruption when \"rebase -i --root\" swaps in a new commit as root.\nUnfortunately, those bugs made it into v2.18.0 and have already\ncorrupted at least one repository (a local project of mine). Patches 3/4\nand 4/4 are new.\n\nv1 fixed these bugs:\n\n* trailing garbage on the commit's \"author\" header\n\n* extra trailing digit on \"author\" header's timezone (caused by two\n separate bugs)\n\nv2 fixes those same bugs, plus:\n\n* eliminates a bogus \"@\" prepended to the \"author\" header timestamp\n which renders the header corrupt\n\n* takes care to validate information coming from\n \"rebase-merge/author-script\" before incorporating it into the \"author\"\n header since that file may be hand-edited, and bogus hand-edited\n values could corrupt the commit object.\n\nPatch 2/4 of this series conflicts with Akinori MUSHA's\n'am/sequencer-author-script-fix' which takes a stab at fixing one of the\nfour (or so) bugs fixed by this series (namely, adding a missing closing\nquote to GIT_AUTHOR_DATE in \"rebase-merge/author-script\"). That patch\nprobably ought to be dropped (without prejudice) in favor of this series\nfor the following reasons:\n\n* It has the undesirable property of adding an unwanted extra blank line\n to \"rebase-merge/author-script\"; this series doesn't make that\n mistake.\n\n* The fix in this series (patch 2/4) is more complete, also ensuring\n that the return value of sq_dequote() is checked.\n\n* And, most importantly, this series sells the change as a fix for a\n genuine serious bug (commit object corruption), whereas that patch\n talks only about adjusting the file content to make it parseable by\n the shell. It's important for future readers of this change to\n understand that the bug (missing closing quote) had much more dire\n consequences than merely being syntactically invalid as a shell\n script.\n\nThe test added by Akinori MUSHA's patch may still have value, and it may\nmake sense to re-submit it, however, doing so need not hold up this\n(higher priority) series.\n\nPhillip's proposed[2] unification of parsers and other fixes and\ncleanups can easily be built atop this series and, likewise, need not\nprevent these (higher priority) patches from graduating independently.\n\n[1]: https://public-inbox.org/git/20180730092929.71114-1-sunshine@sunshineco.com/\n[2]: https://public-inbox.org/git/1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net/\n\nEric Sunshine (4):\n sequencer: fix \"rebase -i --root\" corrupting author header\n sequencer: fix \"rebase -i --root\" corrupting author header timezone\n sequencer: fix \"rebase -i --root\" corrupting author header timestamp\n sequencer: don't die() on bogus user-edited timestamp\n\n sequencer.c | 39 +++++++++++++++++++++--------------\n t/t3404-rebase-interactive.sh | 10 ++++++++-\n 2 files changed, 33 insertions(+), 16 deletions(-)\n\nRange-diff against v1:\n1: ba10ae4e5d ! 1: 8c47555bcf sequencer: fix \"rebase -i --root\" corrupting author header\n @@ -11,8 +11,8 @@\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 + (Note that the \"@\" preceding the timestamp and the extra \"0\" in the\n + timezone are separate bugs which will be 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 @@ -26,6 +26,7 @@\n inserted as part of the parsing process.\n \n Signed-off-by: Eric Sunshine \n + Acked-by: Johannes Schindelin \n \n diff --git a/sequencer.c b/sequencer.c\n --- a/sequencer.c\n @@ -61,7 +62,7 @@\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]*$\" out\n +'\n +\n test_done\n2: c0400cda85 ! 2: 1d4064147e sequencer: fix \"rebase -i --root\" corrupting author header timezone\n @@ -22,6 +22,9 @@\n a NUL-terminator at the end of the shifted content, which explains the\n duplicated last digit in the timezone.\n \n + (Note that the \"@\" preceding the timestamp is a separate bug which\n + will be fixed subsequently.)\n +\n Signed-off-by: Eric Sunshine \n \n diff --git a/sequencer.c b/sequencer.c\n @@ -56,8 +59,8 @@\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 +-\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-: ---------- > 3: cb65c7dd98 sequencer: fix \"rebase -i --root\" corrupting author header timestamp\n-: ---------- > 4: 3a624da9f4 sequencer: don't die() on bogus user-edited timestamp\n-- \n2.18.0.267.gbc8be36ecb\n\n"},{"id":"353997","messageId":"20180731073331.40007-2-sunshine@sunshineco.com","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v2 1/4] sequencer: fix \"rebase -i --root\" corrupting author header","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T07:33:28Z","receivedAt":"2018-07-31T07:34:08Z","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 \"@\" preceding the timestamp and the extra \"0\" in the\ntimezone are separate bugs which will be fixed 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 \nAcked-by: Johannes Schindelin \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..d6e9b52740 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.267.gbc8be36ecb\n\n"},{"id":"353998","messageId":"20180731073331.40007-3-sunshine@sunshineco.com","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v2 2/4] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T07:33:29Z","receivedAt":"2018-07-31T07:34: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, 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\n(Note that the \"@\" preceding the timestamp is a separate bug which\nwill be fixed subsequently.)\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 d6e9b52740..fd3a18154e 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.267.gbc8be36ecb\n\n"},{"id":"353999","messageId":"20180731073331.40007-5-sunshine@sunshineco.com","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v2 4/4] sequencer: don't die() on bogus user-edited timestamp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T07:33:31Z","receivedAt":"2018-07-31T07:34:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"read_author_ident() is careful to handle errors \"gently\" when parsing\n\"rebase-merge/author-script\" by printing a suitable warning and\nreturning NULL; it never die()'s. One possible reason that parsing might\nfail is that \"rebase-merge/author-script\" has been hand-edited in such a\nway which corrupts it or the information it contains.\n\nHowever, read_author_ident() invokes fmt_ident() which is not so careful\nabout failing \"gently\". It will die() if it encounters a malformed\ntimestamp. Since read_author_ident() doesn't want to die() and since\nit's dealing with possibly hand-edited data, take care to avoid passing\na bogus timestamp to fmt_ident().\n\nA more \"correctly engineered\" fix would be to add a \"gentle\" version of\nfmt_ident(), however, such a change it outside the scope of the bug-fix\nseries. If fmt_ident() ever does grow a \"gentle\" cousin, then the manual\ntimestamp check added here can be retired.\n\nSigned-off-by: Eric Sunshine \n---\n sequencer.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 15a66a334c..9b6cdb6ff8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -742,6 +742,15 @@ static const char *read_author_ident(struct strbuf *buf)\n \t\treturn NULL;\n \t}\n \n+\t/* validate date since fmt_ident() will die() on bad value */\n+\tif (parse_date(val[2], &out)){\n+\t\twarning(_(\"invalid date format '%s' in '%s'\"),\n+\t\t\tval[2], rebase_path_author_script());\n+\t\tstrbuf_release(&out);\n+\t\treturn NULL;\n+\t}\n+\n+\tstrbuf_reset(&out);\n \tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n \tstrbuf_swap(buf, &out);\n \tstrbuf_release(&out);\n-- \n2.18.0.267.gbc8be36ecb\n\n"},{"id":"354000","messageId":"20180731073331.40007-4-sunshine@sunshineco.com","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v2 3/4] sequencer: fix \"rebase -i --root\" corrupting author header timestamp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T07:33:30Z","receivedAt":"2018-07-31T07:34: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 timestamp by prepending a \"@\":\n\n author A U Thor @1112912773 -0700\n\nThe commit parser is very strict about the format of the \"author\"\nheader, and does not allow a \"@\" in that position.\n\nThe \"@\" comes from GIT_AUTHOR_DATE in \"rebase-merge/author-script\",\nsignifying a Unix epoch-based timestamp, however, read_author_ident()\nincorrectly allows it to slip into the commit's \"author\" header, thus\ncorrupting it.\n\nOne possible fix would be simply to filter out the \"@\" when constructing\nthe \"author\" header timestamp, however, a more correct fix is to parse\nthe GIT_AUTHOR_DATE date (via parse_date()) and format the parsed result\ninto the \"author\" header. Since \"rebase-merge/author-script\" may be\nedited by the user, this approach has the extra benefit of catching\nother potential timestamp corruption due to hand-editing.\n\nWe can do better than calling parse_date() ourselves and constructing\nthe \"author\" header manually, however, by instead taking advantage of\nfmt_ident() which does this work for us.\n\nThe benefits of using fmt_ident() are twofold. First, it simplifies the\nlogic considerably by allowing us to avoid the complexity of building\nthe \"author\" header in parallel with and in the same buffer from which\n\"rebase-merge/author-script\" is being parsed. Instead, fmt_ident() is\ninvoked to compose the header after parsing is complete.\n\nSecond, fmt_ident() is careful to prevent \"crud\" from polluting the\ncomposed ident. As with validating GIT_AUTHOR_DATE, this \"crud\"\navoidance prevents other (possibly hand-edited) bogus author information\nfrom \"rebase-merge/author-script\" from corrupting the commit object.\n\nSigned-off-by: Eric Sunshine \n---\n sequencer.c | 23 +++++++++--------------\n t/t3404-rebase-interactive.sh | 2 +-\n 2 files changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1008f6d71a..15a66a334c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -709,14 +709,16 @@ static const char *read_author_ident(struct strbuf *buf)\n \tconst char *keys[] = {\n \t\t\"GIT_AUTHOR_NAME=\", \"GIT_AUTHOR_EMAIL=\", \"GIT_AUTHOR_DATE=\"\n \t};\n-\tchar *in, *out, *eol;\n-\tint i = 0, len;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tchar *in, *eol;\n+\tconst char *val[3];\n+\tint i = 0;\n \n \tif (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n \t\treturn NULL;\n \n \t/* dequote values and construct ident line in-place */\n-\tfor (in = out = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n+\tfor (in = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n \t\tif (!skip_prefix(in, keys[i], (const char **)&in)) {\n \t\t\twarning(\"could not parse '%s' (looking for '%s'\",\n \t\t\t\trebase_path_author_script(), keys[i]);\n@@ -730,16 +732,7 @@ static const char *read_author_ident(struct strbuf *buf)\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 */\n-\t\t\t*(out++) = ' ';\n-\t\tif (i == 1) /* email needs to be surrounded by <...> */\n-\t\t\t*(out++) = '<';\n-\t\tmemmove(out, in, len);\n-\t\tout += len;\n-\t\tif (i == 1) /* email needs to be surrounded by <...> */\n-\t\t\t*(out++) = '>';\n+\t\tval[i] = in;\n \t\tin = eol + 1;\n \t}\n \n@@ -749,7 +742,9 @@ static const char *read_author_ident(struct strbuf *buf)\n \t\treturn NULL;\n \t}\n \n-\tstrbuf_setlen(buf, out - buf->buf);\n+\tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n+\tstrbuf_swap(buf, &out);\n+\tstrbuf_release(&out);\n \treturn buf->buf;\n }\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex fd3a18154e..d340018781 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][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.267.gbc8be36ecb\n\n"},{"id":"354003","messageId":"c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-3-sunshine@sunshineco.com","subject":"Re: [PATCH v2 2/4] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T09:50:44Z","receivedAt":"2018-07-31T09:50:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/18 08:33, 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> (Note that the \"@\" preceding the timestamp is a separate bug which\n> will be fixed subsequently.)\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\nUnfortunately the caller does not treat NULL as an error, so this will \nchange the date and potentially the author of the commit. While that \nisn't corruption in the sense that it creates a sane commit, it does \ncorrupt the author data compared to its expected value. I think it would \nbe better to die in the short term, or fix the caller.\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 d6e9b52740..fd3a18154e 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":"354004","messageId":"31870bca-329f-2451-750a-56d917153844@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-4-sunshine@sunshineco.com","subject":"Re: [PATCH v2 3/4] sequencer: fix \"rebase -i --root\" corrupting author header timestamp","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T10:00:56Z","receivedAt":"2018-07-31T10:01:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/18 08:33, Eric Sunshine wrote:\n> When \"git rebase -i --root\" creates a new root commit, it corrupts the\n> \"author\" header's timestamp by prepending a \"@\":\n> \n> author A U Thor @1112912773 -0700\n> \n> The commit parser is very strict about the format of the \"author\"\n> header, and does not allow a \"@\" in that position.\n> \n> The \"@\" comes from GIT_AUTHOR_DATE in \"rebase-merge/author-script\",\n> signifying a Unix epoch-based timestamp, however, read_author_ident()\n> incorrectly allows it to slip into the commit's \"author\" header, thus\n> corrupting it.\n> \n> One possible fix would be simply to filter out the \"@\" when constructing\n> the \"author\" header timestamp, however, a more correct fix is to parse\n> the GIT_AUTHOR_DATE date (via parse_date()) and format the parsed result\n> into the \"author\" header. Since \"rebase-merge/author-script\" may be\n> edited by the user, this approach has the extra benefit of catching\n> other potential timestamp corruption due to hand-editing.\n> \n> We can do better than calling parse_date() ourselves and constructing\n> the \"author\" header manually, however, by instead taking advantage of\n> fmt_ident() which does this work for us.\n> \n> The benefits of using fmt_ident() are twofold. First, it simplifies the\n> logic considerably by allowing us to avoid the complexity of building\n> the \"author\" header in parallel with and in the same buffer from which\n> \"rebase-merge/author-script\" is being parsed. Instead, fmt_ident() is\n> invoked to compose the header after parsing is complete.\n> \n> Second, fmt_ident() is careful to prevent \"crud\" from polluting the\n> composed ident. As with validating GIT_AUTHOR_DATE, this \"crud\"\n> avoidance prevents other (possibly hand-edited) bogus author information\n> from \"rebase-merge/author-script\" from corrupting the commit object.\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> sequencer.c | 23 +++++++++--------------\n> t/t3404-rebase-interactive.sh | 2 +-\n> 2 files changed, 10 insertions(+), 15 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 1008f6d71a..15a66a334c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -709,14 +709,16 @@ static const char *read_author_ident(struct strbuf *buf)\n> \tconst char *keys[] = {\n> \t\t\"GIT_AUTHOR_NAME=\", \"GIT_AUTHOR_EMAIL=\", \"GIT_AUTHOR_DATE=\"\n> \t};\n> -\tchar *in, *out, *eol;\n> -\tint i = 0, len;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tchar *in, *eol;\n> +\tconst char *val[3];\n> +\tint i = 0;\n> \n> \tif (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n> \t\treturn NULL;\n> \n> \t/* dequote values and construct ident line in-place */\n> -\tfor (in = out = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n> +\tfor (in = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n> \t\tif (!skip_prefix(in, keys[i], (const char **)&in)) {\n> \t\t\twarning(\"could not parse '%s' (looking for '%s'\",\n> \t\t\t\trebase_path_author_script(), keys[i]);\n> @@ -730,16 +732,7 @@ static const char *read_author_ident(struct strbuf *buf)\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 */\n> -\t\t\t*(out++) = ' ';\n> -\t\tif (i == 1) /* email needs to be surrounded by <...> */\n> -\t\t\t*(out++) = '<';\n> -\t\tmemmove(out, in, len);\n> -\t\tout += len;\n> -\t\tif (i == 1) /* email needs to be surrounded by <...> */\n> -\t\t\t*(out++) = '>';\n> +\t\tval[i] = in;\n> \t\tin = eol + 1;\n> \t}\n> \n> @@ -749,7 +742,9 @@ static const char *read_author_ident(struct strbuf *buf)\n> \t\treturn NULL;\n> \t}\n> \n> -\tstrbuf_setlen(buf, out - buf->buf);\n> +\tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n> +\tstrbuf_swap(buf, &out);\n> +\tstrbuf_release(&out);\n> \treturn buf->buf;\n> }\n\nThis is a welcome simplification\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index fd3a18154e..d340018781 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n\nNow that the author is correct, can we test_cmp() it against its \nexpected value to make sure there are no hidden surprises in the name \nand email in the future. (It would be reassuring to test an author with \n\"'\" in the name as well but that is out of scope for this series.)\n\n> @@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '\n+\tgit cat-file commit HEAD^ |grep ^author >expected &&\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"2 1\" git rebase -i --root &&\n> \tgit cat-file commit HEAD^ >out &&\n-\tgit cat-file commit HEAD^ >out &&\n> -\tgrep \"^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n+\tgit cat-file commit HEAD^ |grep ^author >out &&\n+\ttest_cmp expected out\t\n> +\tgrep \"^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n> ' > test_done\n> \n\n"},{"id":"354005","messageId":"71924f65-1890-0460-55a1-987354807c7d@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-5-sunshine@sunshineco.com","subject":"Re: [PATCH v2 4/4] sequencer: don't die() on bogus user-edited timestamp","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T10:02:25Z","receivedAt":"2018-07-31T10:02:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/18 08:33, Eric Sunshine wrote:\n> read_author_ident() is careful to handle errors \"gently\" when parsing\n> \"rebase-merge/author-script\" by printing a suitable warning and\n> returning NULL; it never die()'s. One possible reason that parsing might\n> fail is that \"rebase-merge/author-script\" has been hand-edited in such a\n> way which corrupts it or the information it contains.\n> \n> However, read_author_ident() invokes fmt_ident() which is not so careful\n> about failing \"gently\". It will die() if it encounters a malformed\n> timestamp. Since read_author_ident() doesn't want to die() and since\n> it's dealing with possibly hand-edited data, take care to avoid passing\n> a bogus timestamp to fmt_ident().\n> \n> A more \"correctly engineered\" fix would be to add a \"gentle\" version of\n> fmt_ident(), however, such a change it outside the scope of the bug-fix\n> series. If fmt_ident() ever does grow a \"gentle\" cousin, then the manual\n> timestamp check added here can be retired.\n> \n> Signed-off-by: Eric Sunshine \n> ---\n> sequencer.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 15a66a334c..9b6cdb6ff8 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -742,6 +742,15 @@ static const char *read_author_ident(struct strbuf *buf)\n> \t\treturn NULL;\n> \t}\n> \n> +\t/* validate date since fmt_ident() will die() on bad value */\n> +\tif (parse_date(val[2], &out)){\n> +\t\twarning(_(\"invalid date format '%s' in '%s'\"),\n> +\t\t\tval[2], rebase_path_author_script());\n> +\t\tstrbuf_release(&out);\n> +\t\treturn NULL;\n> +\t}\n\nI think if you're going to do this then the caller needs to be changed \nto treat NULL as an error\n\n> +\tstrbuf_reset(&out);\n> \tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n> \tstrbuf_swap(buf, &out);\n> \tstrbuf_release(&out);\n> \n\n"},{"id":"354006","messageId":"f9a7e77d-3c67-082e-ed8d-701f4d0d1759@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T10:05:51Z","receivedAt":"2018-07-31T10:06:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 31/07/18 08:33, Eric Sunshine wrote:\n> This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n> object corruption when \"rebase -i --root\" swaps in a new commit as root.\n> Unfortunately, those bugs made it into v2.18.0 and have already\n> corrupted at least one repository (a local project of mine). Patches 3/4\n> and 4/4 are new.\n> \n> v1 fixed these bugs:\n> \n> * trailing garbage on the commit's \"author\" header\n> \n> * extra trailing digit on \"author\" header's timezone (caused by two\n> separate bugs)\n> \n> v2 fixes those same bugs, plus:\n> \n> * eliminates a bogus \"@\" prepended to the \"author\" header timestamp\n> which renders the header corrupt\n> \n> * takes care to validate information coming from\n> \"rebase-merge/author-script\" before incorporating it into the \"author\"\n> header since that file may be hand-edited, and bogus hand-edited\n> values could corrupt the commit object.\n> \n> Patch 2/4 of this series conflicts with Akinori MUSHA's\n> 'am/sequencer-author-script-fix' which takes a stab at fixing one of the\n> four (or so) bugs fixed by this series (namely, adding a missing closing\n> quote to GIT_AUTHOR_DATE in \"rebase-merge/author-script\"). That patch\n> probably ought to be dropped (without prejudice) in favor of this series\n> for the following reasons:\n> \n> * It has the undesirable property of adding an unwanted extra blank line\n> to \"rebase-merge/author-script\"; this series doesn't make that\n> mistake.\n> \n> * The fix in this series (patch 2/4) is more complete, also ensuring\n> that the return value of sq_dequote() is checked.\n> \n> * And, most importantly, this series sells the change as a fix for a\n> genuine serious bug (commit object corruption), whereas that patch\n> talks only about adjusting the file content to make it parseable by\n> the shell. It's important for future readers of this change to\n> understand that the bug (missing closing quote) had much more dire\n> consequences than merely being syntactically invalid as a shell\n> script.\n> \n> The test added by Akinori MUSHA's patch may still have value, and it may\n> make sense to re-submit it, however, doing so need not hold up this\n> (higher priority) series.\n\nYes I think it does, also the patch that Johannes and I have on top of \nit to fix the quoting of \"'\" in write_author_script() relies on fixing \nthe missing trailing quote and handling of \"'\" at the same time, \nhopefully we can get that rebased on top of these asap.\n\nBest Wishes\n\nPhillip\n\n> Phillip's proposed[2] unification of parsers and other fixes and\n> cleanups can easily be built atop this series and, likewise, need not\n> prevent these (higher priority) patches from graduating independently.\n> \n> [1]: https://public-inbox.org/git/20180730092929.71114-1-sunshine@sunshineco.com/\n> [2]: https://public-inbox.org/git/1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net/\n> \n> Eric Sunshine (4):\n> sequencer: fix \"rebase -i --root\" corrupting author header\n> sequencer: fix \"rebase -i --root\" corrupting author header timezone\n> sequencer: fix \"rebase -i --root\" corrupting author header timestamp\n> sequencer: don't die() on bogus user-edited timestamp\n> \n> sequencer.c | 39 +++++++++++++++++++++--------------\n> t/t3404-rebase-interactive.sh | 10 ++++++++-\n> 2 files changed, 33 insertions(+), 16 deletions(-)\n> \n> Range-diff against v1:\n> 1: ba10ae4e5d ! 1: 8c47555bcf sequencer: fix \"rebase -i --root\" corrupting author header\n> @@ -11,8 +11,8 @@\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> + (Note that the \"@\" preceding the timestamp and the extra \"0\" in the\n> + timezone are separate bugs which will be 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> @@ -26,6 +26,7 @@\n> inserted as part of the parsing process.\n> \n> Signed-off-by: Eric Sunshine \n> + Acked-by: Johannes Schindelin \n> \n> diff --git a/sequencer.c b/sequencer.c\n> --- a/sequencer.c\n> @@ -61,7 +62,7 @@\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]*$\" out\n> +'\n> +\n> test_done\n> 2: c0400cda85 ! 2: 1d4064147e sequencer: fix \"rebase -i --root\" corrupting author header timezone\n> @@ -22,6 +22,9 @@\n> a NUL-terminator at the end of the shifted content, which explains the\n> duplicated last digit in the timezone.\n> \n> + (Note that the \"@\" preceding the timestamp is a separate bug which\n> + will be fixed subsequently.)\n> +\n> Signed-off-by: Eric Sunshine \n> \n> diff --git a/sequencer.c b/sequencer.c\n> @@ -56,8 +59,8 @@\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> +-\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> -: ---------- > 3: cb65c7dd98 sequencer: fix \"rebase -i --root\" corrupting author header timestamp\n> -: ---------- > 4: 3a624da9f4 sequencer: don't die() on bogus user-edited timestamp\n> \n\n"},{"id":"354008","messageId":"CAPig+cRu6fi_SG7LeTBjAWG8aoCT7LmSipDq2a9bPR0_Ae1pFg@mail.gmail.com","threadId":"48989","inReplyTo":"c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net","subject":"Re: [PATCH v2 2/4] sequencer: fix \"rebase -i --root\" corrupting author header timezone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T10:15:17Z","receivedAt":"2018-07-31T10:15:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 5:50 AM Phillip Wood wrote:\n> On 31/07/18 08:33, Eric Sunshine wrote:\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> Unfortunately the caller does not treat NULL as an error, so this will\n> change the date and potentially the author of the commit. While that\n> isn't corruption in the sense that it creates a sane commit, it does\n> corrupt the author data compared to its expected value. I think it would\n> be better to die in the short term, or fix the caller.\n\nAlthough the caller may not treat NULL as an _error_, it is\nnevertheless prepared to handle NULL. Moreover, this new code mirrors\nexisting behavior in read_author_ident() in which it already returns\nNULL for other errors encountered during the parse, so I think\nreturning NULL is the correct thing to do within the scope of this\npatch series, especially considering that this series is about fixing\ngenuine commit object corruption from which a typical will be unable\nto recover (and may not even notice until some time in the future).\n\nWhile the unexpected change in value you describe is unfortunate, this\npatch neither helps nor hurts that since the problem already exists in\nthat other parts of this function already return NULL, and since the\njunk value used by read_author_ident() before this patch was already\nan \"unexpected change\". More importantly, the commit object itself is\nnot corrupted by this issue, and this sort of \"error\" is something\nfrom which a typical user is likely to be able to recover, so IMHO is\noutside the scope of the critical bug fixes of this series. Fixing\nthat issue can easily be done on top of this series.\n\nThanks for thinking critically about this.\n"},{"id":"354009","messageId":"CAPig+cQQvZgFFWfn=OaJ969YKhGx5PQ3r6iSMBpqpjDZGBuViA@mail.gmail.com","threadId":"48989","inReplyTo":"31870bca-329f-2451-750a-56d917153844@talktalk.net","subject":"Re: [PATCH v2 3/4] sequencer: fix \"rebase -i --root\" corrupting author header timestamp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T10:30:15Z","receivedAt":"2018-07-31T10:30:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 6:01 AM Phillip Wood wrote:\n> Now that the author is correct, can we test_cmp() it against its\n> expected value to make sure there are no hidden surprises in the name\n> and email in the future. (It would be reassuring to test an author with\n> \"'\" in the name as well but that is out of scope for this series.)\n>\n> + git cat-file commit HEAD^ |grep ^author >expected &&\n> > set_fake_editor &&\n> > FAKE_LINES=\"2 1\" git rebase -i --root &&\n> > git cat-file commit HEAD^ >out &&\n> - git cat-file commit HEAD^ >out &&\n> > - grep \"^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n> + git cat-file commit HEAD^ |grep ^author >out &&\n> + test_cmp expected out\n> > + grep \"^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n> > ' > test_done\n\nI deliberately avoided test_cmp() in favor of 'grep' specifically\nbecause I didn't want to hard-code the timestamp into the 'expect'\nfile since future changes to tests preceding this one could\npotentially outdate the hard-coded value, and setting up my own commit\nto ensure a consistent timestamp would have made this test longer and\nless \"obvious\".\n\nHowever, your approach sidesteps those concerns nicely. That said, I\nthink such a simplification could be done on top of this patch since\nthe current change to the test in this patch makes it very clear (to\nthe reader) that the \"@\" problem has been corrected, whereas it would\nnot be at all obvious if this patch incorporated your simplification.\n\nWhile your idea is nice, I'd rather not re-roll this series just for\nthat. (I'd really like to see these fixes for this critical commit\nobject corruption land as soon as possible without re-rolling\nrepeatedly for \"optional\" or less important changes.) Perhaps such a\nsimplification can be done in the series you're working on(?).\n\nThanks for the review.\n"},{"id":"354010","messageId":"CAPig+cShUNYTZ_fcqdNFAGtRipt8kwaufVuD5rXmgU6P73epeQ@mail.gmail.com","threadId":"48989","inReplyTo":"71924f65-1890-0460-55a1-987354807c7d@talktalk.net","subject":"Re: [PATCH v2 4/4] sequencer: don't die() on bogus user-edited timestamp","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T10:38:41Z","receivedAt":"2018-07-31T10:38:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 6:02 AM Phillip Wood wrote:\n> On 31/07/18 08:33, Eric Sunshine wrote:\n> > + /* validate date since fmt_ident() will die() on bad value */\n> > + if (parse_date(val[2], &out)){\n> > + warning(_(\"invalid date format '%s' in '%s'\"),\n> > + val[2], rebase_path_author_script());\n> > + strbuf_release(&out);\n> > + return NULL;\n> > + }\n>\n> I think if you're going to do this then the caller needs to be changed\n> to treat NULL as an error\n\nI humbly disagree for the reasons outlined in my response[1] to your\nreview of 2/4. Summarized, this function already returns NULL in\nseveral cases upon error, so the change made by this patch is\nconsistent with existing behavior (and certainly doesn't make the\nsituation worse). This patch series is about fixing the critical bugs\nresulting in genuine commit object corruption; it's not about\nre-engineering a case which was perhaps not thought through thoroughly\n(changing commit data unexpectedly) but which does not genuinely\ncorrupt the commit object itself.\n\n[1]: https://public-inbox.org/git/CAPig+cRu6fi_SG7LeTBjAWG8aoCT7LmSipDq2a9bPR0_Ae1pFg@mail.gmail.com/\n"},{"id":"354012","messageId":"CAPig+cRkdAoV7gsEeKMcvhckifRxU32+ec2zxY7MgNGmJp5Sug@mail.gmail.com","threadId":"48989","inReplyTo":"f9a7e77d-3c67-082e-ed8d-701f4d0d1759@talktalk.net","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T10:46:20Z","receivedAt":"2018-07-31T10:46:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 6:06 AM Phillip Wood wrote:\n> On 31/07/18 08:33, Eric Sunshine wrote:\n> > Patch 2/4 of this series conflicts with Akinori MUSHA's\n> > 'am/sequencer-author-script-fix' which takes a stab at fixing one of the\n> > four (or so) bugs fixed by this series (namely, adding a missing closing\n> > quote to GIT_AUTHOR_DATE in \"rebase-merge/author-script\"). That patch\n> > probably ought to be dropped (without prejudice) in favor of this series\n> > for the following reasons:\n> > [...]\n> > The test added by Akinori MUSHA's patch may still have value, and it may\n> > make sense to re-submit it, however, doing so need not hold up this\n> > (higher priority) series.\n>\n> Yes I think it does, also the patch that Johannes and I have on top of\n> it to fix the quoting of \"'\" in write_author_script() relies on fixing\n> the missing trailing quote and handling of \"'\" at the same time,\n> hopefully we can get that rebased on top of these asap.\n\nI'm not sure if \"Yes I think it does\" means that Akinori's test has\nvalue or if it means that this series should be held back waiting for\nother changes built atop it.\n\nAnyhow, thanks for reading over the series. I appreciate it even if\nour \"sense of priority\" doesn't always align (as evidenced by your\nreview comments and my responses).\n"},{"id":"354014","messageId":"20180731111532.9358-1-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v2 0/2] Fix author script quoting","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T11:15:30Z","receivedAt":"2018-07-31T11:15:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThese build on Eric's patches. The first patch would be better if it\nwas integrated into Eric's patches. The second is a rebased version of\nmy previous patch for fixing quoting in the author script with some\nadditions by Johannes and rebased on top of Eric's fixes. The special\ncasing for backwards compatibility in the second patch relies on the\nquoting and the missing trailing \"'\" from GIT_AUTHOR_DATE being fixed\nconcurrently.\n\n\nPhillip Wood (2):\n sequencer: handle errors in read_author_ident()\n sequencer: fix quoting in write_author_script\n\n sequencer.c | 72 +++++++++++++++++++++--------------\n t/t3404-rebase-interactive.sh | 17 ++++++++-\n 2 files changed, 59 insertions(+), 30 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"354015","messageId":"20180731111532.9358-2-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180731111532.9358-1-phillip.wood@talktalk.net","subject":"[PATCH v2 1/2] sequencer: handle errors in read_author_ident()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T11:15:31Z","receivedAt":"2018-07-31T11:15:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe calling code treated NULL as a valid return value, so fix this by\nreturning and integer and passing in a parameter to receive the author.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 49 ++++++++++++++++++++++++++-----------------------\n 1 file changed, 26 insertions(+), 23 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 944dea6997..212b912420 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -701,57 +701,58 @@ static char *get_author(const char *message)\n }\n \n /* Read author-script and return an ident line (author timestamp) */\n-static const char *read_author_ident(struct strbuf *buf)\n+static int read_author_ident(char **author)\n {\n \tconst char *keys[] = {\n \t\t\"GIT_AUTHOR_NAME=\", \"GIT_AUTHOR_EMAIL=\", \"GIT_AUTHOR_DATE=\"\n \t};\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf out = STRBUF_INIT;\n \tchar *in, *eol;\n \tconst char *val[3];\n \tint i = 0;\n \n-\tif (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n-\t\treturn NULL;\n+\tif (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0)\n+\t\treturn -1;\n \n \t/* dequote values and construct ident line in-place */\n-\tfor (in = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n+\tfor (in = buf.buf; i < 3 && in - buf.buf < buf.len; i++) {\n \t\tif (!skip_prefix(in, keys[i], (const char **)&in)) {\n-\t\t\twarning(\"could not parse '%s' (looking for '%s'\",\n-\t\t\t\trebase_path_author_script(), keys[i]);\n-\t\t\treturn NULL;\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn error(\"could not parse '%s' (looking for '%s'\",\n+\t\t\t\t rebase_path_author_script(), keys[i]);\n \t\t}\n-\n \t\teol = strchrnul(in, '\\n');\n \t\t*eol = '\\0';\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\tstrbuf_release(&buf);\n+\t\t\treturn error(_(\"bad quoting on %s value in '%s'\"),\n+\t\t\t\t keys[i], rebase_path_author_script());\n \t\t}\n \t\tval[i] = in;\n \t\tin = eol + 1;\n \t}\n \n \tif (i < 3) {\n-\t\twarning(\"could not parse '%s' (looking for '%s')\",\n-\t\t\trebase_path_author_script(), keys[i]);\n-\t\treturn NULL;\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(\"could not parse '%s' (looking for '%s')\",\n+\t\t\t rebase_path_author_script(), keys[i]);\n \t}\n \n \t/* validate date since fmt_ident() will die() on bad value */\n \tif (parse_date(val[2], &out)){\n-\t\twarning(_(\"invalid date format '%s' in '%s'\"),\n+\t\terror(_(\"invalid date format '%s' in '%s'\"),\n \t\t\tval[2], rebase_path_author_script());\n \t\tstrbuf_release(&out);\n-\t\treturn NULL;\n+\t\tstrbuf_release(&buf);\n+\t\treturn -1;\n \t}\n \n \tstrbuf_reset(&out);\n \tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n-\tstrbuf_swap(buf, &out);\n-\tstrbuf_release(&out);\n-\treturn buf->buf;\n+\t*author = strbuf_detach(&out, NULL);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static const char staged_changes_advice[] =\n@@ -794,12 +795,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tconst char *value;\n \n \tif ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {\n-\t\tstruct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n-\t\tconst char *author = is_rebase_i(opts) ?\n-\t\t\tread_author_ident(&script) : NULL;\n+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tchar *author = NULL;\n \t\tstruct object_id root_commit, *cache_tree_oid;\n \t\tint res = 0;\n \n+\t\tif (is_rebase_i(opts) && read_author_ident(&author))\n+\t\t\treturn -1;\n+\n \t\tif (!defmsg)\n \t\t\tBUG(\"root commit without message\");\n \n@@ -817,7 +820,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\t\t\t\t opts->gpg_sign);\n \n \t\tstrbuf_release(&msg);\n-\t\tstrbuf_release(&script);\n+\t\tfree(author);\n \t\tif (!res) {\n \t\t\tupdate_ref(NULL, \"CHERRY_PICK_HEAD\", &root_commit, NULL,\n \t\t\t\t REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);\n-- \n2.18.0\n\n"},{"id":"354016","messageId":"20180731111532.9358-3-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180731111532.9358-1-phillip.wood@talktalk.net","subject":"[PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T11:15:32Z","receivedAt":"2018-07-31T11:15:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSingle quotes should be escaped as \\' not \\\\'. Note that this only\naffects authors that contain a single quote and then only external\nscripts that read the author script and users whose git is upgraded from\nthe shell version of rebase -i while rebase was stopped. This is because\nthe parsing in read_env_script() expected the broken version and for\nsome reason sq_dequote() called by read_author_ident() seems to handle\nthe broken quoting correctly.\n\nIdeally rebase and am would share the same code for reading and\nwriting the author script, but this commit just fixes the immediate\nbug.\n\nThe test is based on an original by Akinori MUSHA which was designed to\ncheck that the author script was parsable by posix shell.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 23 +++++++++++++++++------\n t/t3404-rebase-interactive.sh | 17 ++++++++++++++++-\n 2 files changed, 33 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 212b912420..b6e17c1c32 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -636,21 +636,21 @@ static int write_author_script(const char *message)\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (skip_prefix(message, \"> \", &message))\n \t\t\tbreak;\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\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@@ -664,14 +664,25 @@ static int write_author_script(const char *message)\n static int read_env_script(struct argv_array *env)\n {\n \tstruct strbuf script = STRBUF_INIT;\n-\tint i, count = 0;\n-\tchar *p, *p2;\n+\tint i, count = 0, sq_bug;\n+\tconst char *p2;\n+\tchar *p;\n \n \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n \t\treturn -1;\n \n+\t/*\n+\t * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE\n+\t * line with a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather\n+\t * than \"'\\\\''\". We check for the terminating \"'\" on the last line to\n+\t * see how \"'\" has been escaped in case git was upgraded while rebase\n+\t * was stopped.\n+\t */\n+\tsq_bug = script.len && script.buf[script.len - 2] != '\\'';\n \tfor (p = script.buf; *p; p++)\n-\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n+\t\tif (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n+\t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n+\t\telse if (skip_prefix(p, \"'\\\\''\", &p2))\n \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n \t\telse if (*p == '\\'')\n \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b72167ecd5..ae9036d4d9 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n+SQ=\"'\"\n+test_expect_success 'rebase -i writes correct author-script' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit checkout -b author-with-sq master &&\n+\tGIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n+\ttail -n1 .git/rebase-merge/author-script | grep \"$SQ\\$\" &&\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n+\t\t. .git/rebase-merge/author-script &&\n+\t\ttest \"$(git show -s --date=raw --format=%an,%ae,@%ad)\" = \\\n+\t\t\t\"Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE\"\n+\t)\n+'\n+\n test_expect_success 'rebase -i with the exec command' '\n \tgit checkout master &&\n \t(\n@@ -1361,7 +1377,6 @@ test_expect_success 'editor saves as CR/LF' '\n \t)\n '\n \n-SQ=\"'\"\n test_expect_success 'rebase -i --gpg-sign=' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tset_fake_editor &&\n-- \n2.18.0\n\n"},{"id":"354017","messageId":"9c327584-f5da-bd40-e395-9273a4011a66@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cRkdAoV7gsEeKMcvhckifRxU32+ec2zxY7MgNGmJp5Sug@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-07-31T11:19:52Z","receivedAt":"2018-07-31T11:19:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 31/07/18 11:46, Eric Sunshine wrote:\n> On Tue, Jul 31, 2018 at 6:06 AM Phillip Wood wrote:\n>> On 31/07/18 08:33, Eric Sunshine wrote:\n>>> Patch 2/4 of this series conflicts with Akinori MUSHA's\n>>> 'am/sequencer-author-script-fix' which takes a stab at fixing one of the\n>>> four (or so) bugs fixed by this series (namely, adding a missing closing\n>>> quote to GIT_AUTHOR_DATE in \"rebase-merge/author-script\"). That patch\n>>> probably ought to be dropped (without prejudice) in favor of this series\n>>> for the following reasons:\n>>> [...]\n>>> The test added by Akinori MUSHA's patch may still have value, and it may\n>>> make sense to re-submit it, however, doing so need not hold up this\n>>> (higher priority) series.\n>>\n>> Yes I think it does, also the patch that Johannes and I have on top of\n>> it to fix the quoting of \"'\" in write_author_script() relies on fixing\n>> the missing trailing quote and handling of \"'\" at the same time,\n>> hopefully we can get that rebased on top of these asap.\n> \n> I'm not sure if \"Yes I think it does\" means that Akinori's test has\n> value or if it means that this series should be held back waiting for\n> other changes built atop it.\n\nIt means we should use a test based on that with the quoting fixes on\ntop of this series and progress them together if possible. I think the\nquoting patch I just sent is good now so hopefully there wont be any\nissue with it holding up your fixes.\n\n> Anyhow, thanks for reading over the series. I appreciate it even if\n> our \"sense of priority\" doesn't always align (as evidenced by your\n> review comments and my responses).\n\nMy \"sense of priority\" was informed by the hope that the quoting patch\nwouldn't hold things up (let's hope I was right about that!).\n\nBest Wishes\n\nPhillip\n\n"},{"id":"354018","messageId":"CAPig+cRERb+87YoQK_p_vVwPoHDQ0K+t519NuJK4xHzewYC0GA@mail.gmail.com","threadId":"48989","inReplyTo":"CAPig+cRkdAoV7gsEeKMcvhckifRxU32+ec2zxY7MgNGmJp5Sug@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T11:27:30Z","receivedAt":"2018-07-31T11:27:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 6:46 AM Eric Sunshine wrote:\n> Anyhow, thanks for reading over the series. I appreciate it even if\n> our \"sense of priority\" doesn't always align (as evidenced by your\n> review comments and my responses).\n\nTo be clear, the changes you suggest all make sense, and would be\nwelcome (especially the bug fixes), but I consider them lower priority\nthan the fixes in this series, and here's why:\n\nThe commit object corruption caused by the bugs fixed by this series\nare unavoidable. Anyone using \"rebase -i --root\" to swap in a new\ncommit as root is going to end up with a corrupt repository no matter\nwhat. There's no way to side step it. And, most people won't know how\nto fix the corruption, assuming they even notice it.\n\nIf I understand correctly, the issues you describe are unlikely to\ncome up in practice. The only way they can arise is if someone hand\nedits the script (something only power users will do) _and_ botches\nthe edit in the process, or if the person's name contains an\napostrophe (possible, though perhaps uncommon?). Also, (if again I\nunderstand correctly) they are only data \"corruptions\", not genuine\nbroken-repository corruptions, thus are more likely to be fixable by a\ntypical user.\n\nSo it's not that I think your proposed fixes and suggestions are\nunimportant, I just don't think they belong in this series, and would\nbe happy to see them atop it.\n\nThanks.\n"},{"id":"354122","messageId":"CAPig+cT15a-QTTAjC61td4h_YhHz0WSa8iT_3XqUUc6LZHWC=A@mail.gmail.com","threadId":"48989","inReplyTo":"20180731111532.9358-2-phillip.wood@talktalk.net","subject":"Re: [PATCH v2 1/2] sequencer: handle errors in read_author_ident()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T20:47:52Z","receivedAt":"2018-07-31T20:48:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n> The calling code treated NULL as a valid return value, so fix this by\n> returning and integer and passing in a parameter to receive the author.\n\nIt might be difficult for future readers (those who didn't follow the\ndiscussion) to understand how/why NULL is not sufficient to signal an\nerror. Perhaps incorporating the explanation from your email[1] which\ndiscussed that the author name, email, and/or date might change\nunexpectedly would be sufficient. This excerpt from [1] might be a\ngood starting point:\n\n ... the caller does not treat NULL as an error, so this will\n change the date and potentially the author of the commit\n ... [which] does corrupt the author data compared to its\n expected value.\n\n[1]: https://public-inbox.org/git/c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net/\n\n> Signed-off-by: Phillip Wood \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -701,57 +701,58 @@ static char *get_author(const char *message)\n> -static const char *read_author_ident(struct strbuf *buf)\n> +static int read_author_ident(char **author)\n\nSo, the caller is now responsible for freeing the string placed in\n'author'. Okay.\n\n> {\n> - if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n> - return NULL;\n> + if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0)\n> + return -1;\n\nI think you need to strbuf_release(&buf) in this error path since\nstrbuf_read_file() doesn't guarantee that the strbuf hasn't been\npartially populated when it returns an error. (That is, this is\nleaking.)\n\n> /* dequote values and construct ident line in-place */\n\nUgh, this comment should have been adjusted in my series. A minor\nmatter, though, which can be tweaked later.\n\n> /* validate date since fmt_ident() will die() on bad value */\n> if (parse_date(val[2], &out)){\n> - warning(_(\"invalid date format '%s' in '%s'\"),\n> + error(_(\"invalid date format '%s' in '%s'\"),\n> val[2], rebase_path_author_script());\n> strbuf_release(&out);\n> - return NULL;\n> + strbuf_release(&buf);\n> + return -1;\n\nYou were careful to print the error, which references a value from\n'buf', before destroying 'buf'. Good.\n\n(A simplifying alternative would have been to not print the actual\nvalue and instead say generally that \"the date\" was bad. Not a big\ndeal.)\n\n> }\n> - strbuf_swap(buf, &out);\n> - strbuf_release(&out);\n> - return buf->buf;\n> + *author = strbuf_detach(&out, NULL);\n\nAnd, 'author' is only assigned when 0 is returned, so the caller only\nhas to free(author) upon success. Fine.\n\n> + strbuf_release(&buf);\n> + return 0;\n> }\n>\n> static const char staged_changes_advice[] =\n> @@ -794,12 +795,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> - struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n> - const char *author = is_rebase_i(opts) ?\n> - read_author_ident(&script) : NULL;\n> + struct strbuf msg = STRBUF_INIT;\n> + char *author = NULL;\n> struct object_id root_commit, *cache_tree_oid;\n> int res = 0;\n>\n> + if (is_rebase_i(opts) && read_author_ident(&author))\n> + return -1;\n\nLogic looks correct, and it's nice to see that you went with 'return\n-1' rather than die(), especially since the caller of run_git_commit()\nis already able to handle -1.\n"},{"id":"354129","messageId":"CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com","threadId":"48989","inReplyTo":"20180731111532.9358-3-phillip.wood@talktalk.net","subject":"Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-31T21:39:25Z","receivedAt":"2018-07-31T21:39:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n> Single quotes should be escaped as \\' not \\\\'. Note that this only\n> affects authors that contain a single quote and then only external\n> scripts that read the author script and users whose git is upgraded from\n> the shell version of rebase -i while rebase was stopped. This is because\n> the parsing in read_env_script() expected the broken version and for\n> some reason sq_dequote() called by read_author_ident() seems to handle\n> the broken quoting correctly.\n\nIs the:\n\n ...for some reason sq_dequote() called by read_author_ident()\n seems to handle the broken quoting correctly.\n\nbit outdated? We know now from patch 2/4 of my series[1] that\nread_author_ident() wasn't handling it correctly at all. It was merely\nignoring the return value from sq_dequote() and using whatever broken\nvalue came back from it.\n\n[1]: https://public-inbox.org/git/20180731073331.40007-3-sunshine@sunshineco.com/\n\n> Helped-by: Johannes Schindelin \n> Signed-off-by: Phillip Wood \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -664,14 +664,25 @@ static int write_author_script(const char *message)\n> static int read_env_script(struct argv_array *env)\n> {\n> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> return -1;\n\nThis is not a problem introduced by this patch, but since\nstrbuf_read_file() doesn't guarantee that memory hasn't been allocated\nwhen it returns an error, this is leaking.\n\n> + /*\n> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE\n> + * line with a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather\n> + * than \"'\\\\''\". We check for the terminating \"'\" on the last line to\n> + * see how \"'\" has been escaped in case git was upgraded while rebase\n> + * was stopped.\n> + */\n> + sq_bug = script.len && script.buf[script.len - 2] != '\\'';\n\nI think you need to be checking 'script.len > 1', not just\n'script.len', otherwise you might access memory outside the allocated\nbuffer.\n\nThis is a very \"delicate\" check, assuming that a hand-edited file\nwon't end with, say, an extra newline. I wonder if this level of\nbackward-compatibility is overkill for such an unlikely case.\n\n> for (p = script.buf; *p; p++)\n> - if (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n> + if (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> + strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> + else if (skip_prefix(p, \"'\\\\''\", &p2))\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '\n> +test_expect_success 'rebase -i writes correct author-script' '\n> + test_when_finished \"test_might_fail git rebase --abort\" &&\n> + git checkout -b author-with-sq master &&\n> + GIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n> + set_fake_editor &&\n> + FAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n\nHmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?\n"},{"id":"354144","messageId":"CAE1pOi3oeBEyD8skwfLAd7JviGYOUZaZwj3u9pznM4eLAR7fDQ@mail.gmail.com","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Hilco Wijbenga","fromEmail":"hilco.wijbenga@gmail.com","sentAt":"2018-08-01T01:30:30Z","receivedAt":"2018-08-01T01:30:54Z","isPatch":true,"sender":{"key":"hilco.wijbenga@gmail.com","avatar":null},"body":"Hi Eric,\n\nOn Tue, Jul 31, 2018 at 12:33 AM, Eric Sunshine wrote:\n> This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n> object corruption when \"rebase -i --root\" swaps in a new commit as root.\n> Unfortunately, those bugs made it into v2.18.0 and have already\n> corrupted at least one repository (a local project of mine). Patches 3/4\n> and 4/4 are new.\n>\n> v1 fixed these bugs:\n>\n> * trailing garbage on the commit's \"author\" header\n>\n> * extra trailing digit on \"author\" header's timezone (caused by two\n> separate bugs)\n>\n> v2 fixes those same bugs, plus:\n>\n> * eliminates a bogus \"@\" prepended to the \"author\" header timestamp\n> which renders the header corrupt\n>\n> * takes care to validate information coming from\n> \"rebase-merge/author-script\" before incorporating it into the \"author\"\n> header since that file may be hand-edited, and bogus hand-edited\n> values could corrupt the commit object.\n>\n\nDoes this also fix losing the initial commit if it is empty?\n\nGiven\n\ngit init ; git commit -m 'Initial commit' --allow-empty ; touch\nfile.txt ; git add file.txt ; git commit -m 'Add file.txt' ; git\nrebase --root\n\nI would expect there to be 2 commits but the first one has\ndisappeared. (This usually happens with \"git rebase -i --root\" early\non in a new project.)\n\nCheers,\nHilco\n"},{"id":"354147","messageId":"CAPig+cRe1R1YWpLaXVtTmwhaJiz2dONU83qtn_+XhaRevjn2fQ@mail.gmail.com","threadId":"48989","inReplyTo":"CAE1pOi3oeBEyD8skwfLAd7JviGYOUZaZwj3u9pznM4eLAR7fDQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-01T06:22:11Z","receivedAt":"2018-08-01T06:22:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 31, 2018 at 9:30 PM Hilco Wijbenga wrote:\n> On Tue, Jul 31, 2018 at 12:33 AM, Eric Sunshine wrote:\n> > This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n> > object corruption when \"rebase -i --root\" swaps in a new commit as root.\n> > Unfortunately, those bugs made it into v2.18.0 and have already\n> > corrupted at least one repository (a local project of mine). Patches 3/4\n> > and 4/4 are new.\n>\n> Does this also fix losing the initial commit if it is empty?\n>\n> git init ; git commit -m 'Initial commit' --allow-empty ; touch\n> file.txt ; git add file.txt ; git commit -m 'Add file.txt' ; git\n> rebase --root\n>\n> I would expect there to be 2 commits but the first one has\n> disappeared. (This usually happens with \"git rebase -i --root\" early\n> on in a new project.)\n\nThis series should have no impact on that issue; it is fixing root\ncommit object corruption, and does not touch or change how the commit\ngraph is maintained or how the rebasing machinery itself works (and\nthe --allow-empty stuff is part of that machinery).\n\nAs Dscho is cc:'d already, perhaps he can chime in as a resident expert.\n\nBy the way, what happens if you use --keep-empty while rebasing?\n"},{"id":"354151","messageId":"04f5f533-c971-6adc-9bcd-229e24ab2752@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cT15a-QTTAjC61td4h_YhHz0WSa8iT_3XqUUc6LZHWC=A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: handle errors in read_author_ident()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-01T09:28:54Z","receivedAt":"2018-08-01T09:28:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nThanks for taking a look at this\n\nOn 31/07/18 21:47, Eric Sunshine wrote:\n> On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n>> The calling code treated NULL as a valid return value, so fix this by\n>> returning and integer and passing in a parameter to receive the author.\n> \n> It might be difficult for future readers (those who didn't follow the\n> discussion) to understand how/why NULL is not sufficient to signal an\n> error. Perhaps incorporating the explanation from your email[1] which\n> discussed that the author name, email, and/or date might change\n> unexpectedly would be sufficient. This excerpt from [1] might be a\n> good starting point:\n> \n> ... the caller does not treat NULL as an error, so this will\n> change the date and potentially the author of the commit\n> ... [which] does corrupt the author data compared to its\n> expected value.\n> \n> [1]: https://public-inbox.org/git/c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net/\n> \n>> Signed-off-by: Phillip Wood \n>> ---\n>> diff --git a/sequencer.c b/sequencer.c\n>> @@ -701,57 +701,58 @@ static char *get_author(const char *message)\n>> -static const char *read_author_ident(struct strbuf *buf)\n>> +static int read_author_ident(char **author)\n> \n> So, the caller is now responsible for freeing the string placed in\n> 'author'. Okay.\n> \n>> {\n>> - if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n>> - return NULL;\n>> + if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0)\n>> + return -1;\n> \n> I think you need to strbuf_release(&buf) in this error path since\n> strbuf_read_file() doesn't guarantee that the strbuf hasn't been\n> partially populated when it returns an error. (That is, this is\n> leaking.)\n\nGood point, I'll fix it\n\n>> /* dequote values and construct ident line in-place */\n> \n> Ugh, this comment should have been adjusted in my series. A minor\n> matter, though, which can be tweaked later.\n> \n>> /* validate date since fmt_ident() will die() on bad value */\n>> if (parse_date(val[2], &out)){\n>> - warning(_(\"invalid date format '%s' in '%s'\"),\n>> + error(_(\"invalid date format '%s' in '%s'\"),\n>> val[2], rebase_path_author_script());\n>> strbuf_release(&out);\n>> - return NULL;\n>> + strbuf_release(&buf);\n>> + return -1;\n> \n> You were careful to print the error, which references a value from\n> 'buf', before destroying 'buf'. Good.\n> \n> (A simplifying alternative would have been to not print the actual\n> value and instead say generally that \"the date\" was bad. Not a big\n> deal.)\n> \n>> }\n>> - strbuf_swap(buf, &out);\n>> - strbuf_release(&out);\n>> - return buf->buf;\n>> + *author = strbuf_detach(&out, NULL);\n> \n> And, 'author' is only assigned when 0 is returned, so the caller only\n> has to free(author) upon success. Fine.\n> \n>> + strbuf_release(&buf);\n>> + return 0;\n>> }\n>>\n>> static const char staged_changes_advice[] =\n>> @@ -794,12 +795,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n>> - struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n>> - const char *author = is_rebase_i(opts) ?\n>> - read_author_ident(&script) : NULL;\n>> + struct strbuf msg = STRBUF_INIT;\n>> + char *author = NULL;\n>> struct object_id root_commit, *cache_tree_oid;\n>> int res = 0;\n>>\n>> + if (is_rebase_i(opts) && read_author_ident(&author))\n>> + return -1;\n> \n> Logic looks correct, and it's nice to see that you went with 'return\n> -1' rather than die(), especially since the caller of run_git_commit()\n> is already able to handle -1.\n\nYes, it reschedules the pick so the user has a chance to fix the \nauthor-script and then run 'git rebase --continue'\n\nBest Wishes\n\nPhillip\n\n"},{"id":"354153","messageId":"22f98273-f7f4-adf3-1a8b-b0e4ec6029b3@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com","subject":"Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-01T10:24:14Z","receivedAt":"2018-08-01T10:24:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 31/07/18 22:39, Eric Sunshine wrote:\n> On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n>> Single quotes should be escaped as \\' not \\\\'. Note that this only\n>> affects authors that contain a single quote and then only external\n>> scripts that read the author script and users whose git is upgraded from\n>> the shell version of rebase -i while rebase was stopped. This is because\n>> the parsing in read_env_script() expected the broken version and for\n>> some reason sq_dequote() called by read_author_ident() seems to handle\n>> the broken quoting correctly.\n> \n> Is the:\n> \n> ...for some reason sq_dequote() called by read_author_ident()\n> seems to handle the broken quoting correctly.\n> \n> bit outdated? We know now from patch 2/4 of my series[1] that\n> read_author_ident() wasn't handling it correctly at all. It was merely\n> ignoring the return value from sq_dequote() and using whatever broken\n> value came back from it.\n\nYes you're right, when I tested it before I must of had GIT_AUTHOR_NAME \nset to the name with the \"'\" in it when I ran the rebase because it \nappeared to work, but actually sj_dequote() was returning NULL and so \ncommit_tree() just picked up the default author. I've just changed the \ntest you added to\n\ntest_expect_success 'valid author header after --root swap' '\n\trebase_setup_and_clean author-header no-conflict-branch &&\n\tset_fake_editor &&\n\tgit commit --amend --author=\"Au ${SQ}thor \" \n--no-edit &&\n\tgit cat-file commit HEAD | grep ^author >expected &&\n\tFAKE_LINES=\"5 1\" git rebase -i --root &&\n\tgit cat-file commit HEAD^ | grep ^author >actual &&\n\ttest_cmp expected actual\n'\n\nand it fails without the fixes to write_author_script().\n\n> \n> [1]: https://public-inbox.org/git/20180731073331.40007-3-sunshine@sunshineco.com/\n> \n>> Helped-by: Johannes Schindelin \n>> Signed-off-by: Phillip Wood \n>> ---\n>> diff --git a/sequencer.c b/sequencer.c\n>> @@ -664,14 +664,25 @@ static int write_author_script(const char *message)\n>> static int read_env_script(struct argv_array *env)\n>> {\n>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>> return -1;\n> \n> This is not a problem introduced by this patch, but since\n> strbuf_read_file() doesn't guarantee that memory hasn't been allocated\n> when it returns an error, this is leaking.\n\nI can fix that\n\n>> + /*\n>> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE\n>> + * line with a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather\n>> + * than \"'\\\\''\". We check for the terminating \"'\" on the last line to\n>> + * see how \"'\" has been escaped in case git was upgraded while rebase\n>> + * was stopped.\n>> + */\n>> + sq_bug = script.len && script.buf[script.len - 2] != '\\'';\n> \n> I think you need to be checking 'script.len > 1', not just\n> 'script.len', otherwise you might access memory outside the allocated\n> buffer.\n\nGood catch, Johannes's original was checking script.buf[script.len - 1] \nwhich I corrected but forget to adjust the previous check.\n\n> This is a very \"delicate\" check, assuming that a hand-edited file\n> won't end with, say, an extra newline. I wonder if this level of\n> backward-compatibility is overkill for such an unlikely case.\n\nYes, it is a bit fragile. Originally the patch just unquoted the correct \nand incorrect quoting but Johannes was worried that might lead to errors \nand suggested this check. The check is aimed at people whose git gets \nupgraded while rebase is stopped for a conflict resolution or edit and \nso have the bad quoting in the author-script from the old version of git \nwhich started the rebase. Authors with \"'\" in the name are uncommon but \nnot unheard of, I think when I checked there were about half a dozen in \ngit's history. I'm not sure what to do for the best.\n\n>> for (p = script.buf; *p; p++)\n>> - if (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n>> + if (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n>> + strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n>> + else if (skip_prefix(p, \"'\\\\''\", &p2))\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '\n>> +test_expect_success 'rebase -i writes correct author-script' '\n>> + test_when_finished \"test_might_fail git rebase --abort\" &&\n>> + git checkout -b author-with-sq master &&\n>> + GIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n>> + set_fake_editor &&\n>> + FAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n> \n> Hmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?\n\n-k is short for --keep-empty which is needed as the test creates an \nempty commit to check the author (I think that is to avoid changing the \ntree - Johannes wrote that bit).\n\nThanks for your comments, I'll do a reroll\n\nPhillip\n"},{"id":"354162","messageId":"xmqqin4u863h.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"22f98273-f7f4-adf3-1a8b-b0e4ec6029b3@talktalk.net","subject":"Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T15:22:10Z","receivedAt":"2018-08-01T15:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> Is the:\n>>\n>> ...for some reason sq_dequote() called by read_author_ident()\n>> seems to handle the broken quoting correctly.\n>>\n>> bit outdated? We know now from patch 2/4 of my series[1] that\n>> read_author_ident() wasn't handling it correctly at all. It was merely\n>> ignoring the return value from sq_dequote() and using whatever broken\n>> value came back from it.\n>\n> Yes you're right, when I tested it...\n>\n> Thanks for your comments, I'll do a reroll\n\nThanks, both. Sounds like we are quickly converging to the\nresolution ;-)\n"},{"id":"354165","messageId":"1f9a9062-445d-0c37-9b21-941d8fef659b@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com","subject":"Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-01T15:50:40Z","receivedAt":"2018-08-01T15:50:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/18 22:39, Eric Sunshine wrote:\n> On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n>> Single quotes should be escaped as \\' not \\\\'. Note that this only\n>> affects authors that contain a single quote and then only external\n>> scripts that read the author script and users whose git is upgraded from\n>> the shell version of rebase -i while rebase was stopped. This is because\n>> the parsing in read_env_script() expected the broken version and for\n>> some reason sq_dequote() called by read_author_ident() seems to handle\n>> the broken quoting correctly.\n> \n> Is the:\n> \n> ...for some reason sq_dequote() called by read_author_ident()\n> seems to handle the broken quoting correctly.\n> \n> bit outdated? We know now from patch 2/4 of my series[1] that\n> read_author_ident() wasn't handling it correctly at all. It was merely\n> ignoring the return value from sq_dequote() and using whatever broken\n> value came back from it.\n> \n> [1]: https://public-inbox.org/git/20180731073331.40007-3-sunshine@sunshineco.com/\n> \n>> Helped-by: Johannes Schindelin \n>> Signed-off-by: Phillip Wood \n>> ---\n>> diff --git a/sequencer.c b/sequencer.c\n>> @@ -664,14 +664,25 @@ static int write_author_script(const char *message)\n>> static int read_env_script(struct argv_array *env)\n>> {\n>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>> return -1;\n> \n> This is not a problem introduced by this patch, but since\n> strbuf_read_file() doesn't guarantee that memory hasn't been allocated\n> when it returns an error, this is leaking.\n> \n>> + /*\n>> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE\n>> + * line with a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather\n>> + * than \"'\\\\''\". We check for the terminating \"'\" on the last line to\n>> + * see how \"'\" has been escaped in case git was upgraded while rebase\n>> + * was stopped.\n>> + */\n>> + sq_bug = script.len && script.buf[script.len - 2] != '\\'';\n> \n> I think you need to be checking 'script.len > 1', not just\n> 'script.len', otherwise you might access memory outside the allocated\n> buffer.\n> \n> This is a very \"delicate\" check, assuming that a hand-edited file\n> won't end with, say, an extra newline. I wonder if this level of\n> backward-compatibility is overkill for such an unlikely case.\n\nI think I'll get rid of the check and instead use a version number \nwritten to .git/rebase-merge/interactive to indicate if we need to fix \nthe quoting (if there's no number then it needs fixing). We can \nincrement the version number in the future if we ever need to implement \nother fallbacks to handle the case where git got upgraded while rebase \nwas stopped. I'll send a patch tomorrow\n\nBest Wishes\n\nPhillip\n\n> \n>> for (p = script.buf; *p; p++)\n>> - if (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n>> + if (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n>> + strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n>> + else if (skip_prefix(p, \"'\\\\''\", &p2))\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> @@ -75,6 +75,22 @@ test_expect_success 'rebase --keep-empty' '\n>> +test_expect_success 'rebase -i writes correct author-script' '\n>> + test_when_finished \"test_might_fail git rebase --abort\" &&\n>> + git checkout -b author-with-sq master &&\n>> + GIT_AUTHOR_NAME=\"Auth O$SQ R\" git commit --allow-empty -m with-sq &&\n>> + set_fake_editor &&\n>> + FAKE_LINES=\"edit 1\" git rebase -ki HEAD^ &&\n> \n> Hmph, -k doesn't seem to be documented in git-rebase.txt. Is it needed here?\n> \n\n"},{"id":"354180","messageId":"CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com","threadId":"48989","inReplyTo":"1f9a9062-445d-0c37-9b21-941d8fef659b@talktalk.net","subject":"Re: [PATCH v2 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-01T19:19:41Z","receivedAt":"2018-08-01T19:19:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 1, 2018 at 11:50 AM Phillip Wood wrote:\n> On 31/07/18 22:39, Eric Sunshine wrote:\n> > On Tue, Jul 31, 2018 at 7:15 AM Phillip Wood wrote:\n> >> + /*\n> >> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE\n> >> + * line with a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather\n> >> + * than \"'\\\\''\". We check for the terminating \"'\" on the last line to\n> >> + * see how \"'\" has been escaped in case git was upgraded while rebase\n> >> + * was stopped.\n> >> + */\n> >> + sq_bug = script.len && script.buf[script.len - 2] != '\\'';\n> >\n> > This is a very \"delicate\" check, assuming that a hand-edited file\n> > won't end with, say, an extra newline. I wonder if this level of\n> > backward-compatibility is overkill for such an unlikely case.\n>\n> I think I'll get rid of the check and instead use a version number\n> written to .git/rebase-merge/interactive to indicate if we need to fix\n> the quoting (if there's no number then it needs fixing). We can\n> increment the version number in the future if we ever need to implement\n> other fallbacks to handle the case where git got upgraded while rebase\n> was stopped. I'll send a patch tomorrow\n\nHmm, that approach is pretty heavyweight and would add a fair bit of\nnew code and complexity which itself could harbor bugs. When I\ncommented that the check was \"delicate\", I was (especially) referring\nto the rigid \"script[len-2]\", not necessarily to the basic idea of the\ncheck. So, you could keep the check (in spirit) but make it more\nrobust. Like this, for instance:\n\n/* big comment explaining old buggy stuff */\nstatic int broken_quoting(const char *s, size_t n)\n{\n const char *t = s + n;\n while (t > s && *--t == '\\n')\n /* empty */;\n if (t > s && *--t != '\\'')\n return 1;\n return 0;\n}\n\nstatic int read_env_script(...)\n{\n ...\n sq_bug = broken_quoting(script.buf, script.len);\n ...\n}\n\nI would feel much more comfortable with a simple solution like this\nthan with one introducing new complexity associated with adding a\nversion number.\n"},{"id":"354208","messageId":"20180801232517.GB106068@genre.crustytoothpaste.net","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-01T23:25:17Z","receivedAt":"2018-08-01T23:25:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 31, 2018 at 03:33:27AM -0400, Eric Sunshine wrote:\n> This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n> object corruption when \"rebase -i --root\" swaps in a new commit as root.\n> Unfortunately, those bugs made it into v2.18.0 and have already\n> corrupted at least one repository (a local project of mine). Patches 3/4\n> and 4/4 are new.\n> \n> v1 fixed these bugs:\n> \n> * trailing garbage on the commit's \"author\" header\n> \n> * extra trailing digit on \"author\" header's timezone (caused by two\n> separate bugs)\n> \n> v2 fixes those same bugs, plus:\n> \n> * eliminates a bogus \"@\" prepended to the \"author\" header timestamp\n> which renders the header corrupt\n> \n> * takes care to validate information coming from\n> \"rebase-merge/author-script\" before incorporating it into the \"author\"\n> header since that file may be hand-edited, and bogus hand-edited\n> values could corrupt the commit object.\n\nI looked at this series and it seems sane and logical to me. Thanks for\nacting quickly to fix the corruption.\n\nReviewed-by: brian m. carlson \n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"354224","messageId":"CAPig+cSZa2a0raGr6z7WOi24gg4m=sc7k19=t2AYzPbeYNVfjQ@mail.gmail.com","threadId":"48989","inReplyTo":"20180801232517.GB106068@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-02T08:09:51Z","receivedAt":"2018-08-02T08:10:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 1, 2018 at 7:25 PM brian m. carlson\n wrote:\n> On Tue, Jul 31, 2018 at 03:33:27AM -0400, Eric Sunshine wrote:\n> > This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n> > object corruption when \"rebase -i --root\" swaps in a new commit as root.\n> > Unfortunately, those bugs made it into v2.18.0 and have already\n> > corrupted at least one repository (a local project of mine). Patches 3/4\n> > and 4/4 are new.\n>\n> I looked at this series and it seems sane and logical to me. Thanks for\n> acting quickly to fix the corruption.\n>\n> Reviewed-by: brian m. carlson \n\nThanks for the review.\n"},{"id":"354228","messageId":"20180802112002.720-2-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180802112002.720-1-phillip.wood@talktalk.net","subject":"[PATCH v3 1/2] sequencer: handle errors in read_author_ident()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-02T11:20:01Z","receivedAt":"2018-08-02T11:20:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe calling code did not treat NULL as an error. Instead NULL caused\nit to fallback to using the default author when creating the new\ncommit. This changed the date and potentially the author of the\ncommit which corrupted the author data compared to its expected\nvalue. Fix this by returning and integer and passing in a parameter to\nreceive the author.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n changes since v2:\n \n - Improved commit message\n - Fixed memory leak\n - Translated a couple of error messages\n\n sequencer.c | 52 ++++++++++++++++++++++++++++------------------------\n 1 file changed, 28 insertions(+), 24 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 944dea6997..1bf8b0c431 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -701,57 +701,59 @@ static char *get_author(const char *message)\n }\n \n /* Read author-script and return an ident line (author timestamp) */\n-static const char *read_author_ident(struct strbuf *buf)\n+static int read_author_ident(char **author)\n {\n \tconst char *keys[] = {\n \t\t\"GIT_AUTHOR_NAME=\", \"GIT_AUTHOR_EMAIL=\", \"GIT_AUTHOR_DATE=\"\n \t};\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf out = STRBUF_INIT;\n \tchar *in, *eol;\n \tconst char *val[3];\n \tint i = 0;\n \n-\tif (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n-\t\treturn NULL;\n+\tif (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn -1;\n+\t}\n \n-\t/* dequote values and construct ident line in-place */\n-\tfor (in = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {\n+\tfor (in = buf.buf; i < 3 && in - buf.buf < buf.len; i++) {\n \t\tif (!skip_prefix(in, keys[i], (const char **)&in)) {\n-\t\t\twarning(\"could not parse '%s' (looking for '%s'\",\n-\t\t\t\trebase_path_author_script(), keys[i]);\n-\t\t\treturn NULL;\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn error(_(\"could not parse '%s' (looking for '%s')\"),\n+\t\t\t\t rebase_path_author_script(), keys[i]);\n \t\t}\n-\n \t\teol = strchrnul(in, '\\n');\n \t\t*eol = '\\0';\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\tstrbuf_release(&buf);\n+\t\t\treturn error(_(\"bad quoting on %s value in '%s'\"),\n+\t\t\t\t keys[i], rebase_path_author_script());\n \t\t}\n \t\tval[i] = in;\n \t\tin = eol + 1;\n \t}\n \n \tif (i < 3) {\n-\t\twarning(\"could not parse '%s' (looking for '%s')\",\n-\t\t\trebase_path_author_script(), keys[i]);\n-\t\treturn NULL;\n+\t\tstrbuf_release(&buf);\n+\t\treturn error(_(\"could not parse '%s' (looking for '%s')\"),\n+\t\t\t rebase_path_author_script(), keys[i]);\n \t}\n \n \t/* validate date since fmt_ident() will die() on bad value */\n \tif (parse_date(val[2], &out)){\n-\t\twarning(_(\"invalid date format '%s' in '%s'\"),\n+\t\terror(_(\"invalid date format '%s' in '%s'\"),\n \t\t\tval[2], rebase_path_author_script());\n \t\tstrbuf_release(&out);\n-\t\treturn NULL;\n+\t\tstrbuf_release(&buf);\n+\t\treturn -1;\n \t}\n \n \tstrbuf_reset(&out);\n \tstrbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));\n-\tstrbuf_swap(buf, &out);\n-\tstrbuf_release(&out);\n-\treturn buf->buf;\n+\t*author = strbuf_detach(&out, NULL);\n+\tstrbuf_release(&buf);\n+\treturn 0;\n }\n \n static const char staged_changes_advice[] =\n@@ -794,12 +796,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tconst char *value;\n \n \tif ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {\n-\t\tstruct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n-\t\tconst char *author = is_rebase_i(opts) ?\n-\t\t\tread_author_ident(&script) : NULL;\n+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tchar *author = NULL;\n \t\tstruct object_id root_commit, *cache_tree_oid;\n \t\tint res = 0;\n \n+\t\tif (is_rebase_i(opts) && read_author_ident(&author))\n+\t\t\treturn -1;\n+\n \t\tif (!defmsg)\n \t\t\tBUG(\"root commit without message\");\n \n@@ -817,7 +821,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\t\t\t\t opts->gpg_sign);\n \n \t\tstrbuf_release(&msg);\n-\t\tstrbuf_release(&script);\n+\t\tfree(author);\n \t\tif (!res) {\n \t\t\tupdate_ref(NULL, \"CHERRY_PICK_HEAD\", &root_commit, NULL,\n \t\t\t\t REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);\n-- \n2.18.0\n\n"},{"id":"354230","messageId":"20180802112002.720-1-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v3 0/2] Fix author script quoting","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-02T11:20:00Z","receivedAt":"2018-08-02T11:20:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThanks to Eric for his comments on v2. The first patch hasn't changed\nmuch, the second one quite a bit. See the individual patches for a\nlist of changes\n\nPhillip Wood (2):\n sequencer: handle errors in read_author_ident()\n sequencer: fix quoting in write_author_script\n\n git-rebase--interactive.sh | 2 +-\n sequencer.c | 137 ++++++++++++++++++++++++++--------\n sequencer.h | 1 +\n t/t3404-rebase-interactive.sh | 20 ++++-\n 4 files changed, 123 insertions(+), 37 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"354229","messageId":"20180802112002.720-3-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180802112002.720-1-phillip.wood@talktalk.net","subject":"[PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-02T11:20:02Z","receivedAt":"2018-08-02T11:20:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSingle quotes should be escaped as \\' not \\\\'. The bad quoting breaks\nthe interactive version of 'rebase --root' (which is used when there is\nno '--onto' even if the user does not specify --interactive) for authors\nthat contain \"'\" as sq_dequote() called read_author_ident() errors out\non the bad quoting.\n\nFor other interactive rebases this only affects external scripts that\nread the author script and users whose git is upgraded from the shell\nversion of rebase -i while rebase was stopped when the author contains\n\"'\". This is because the parsing in read_env_script() expected the\nbroken quoting.\n\nThis patch includes code to gracefully handle the broken quoting when\ngit has been upgraded while rebase was stopped. It does this by\nrecording a version number (currently 1) in\n$GIT_DIR/rebase-merge/interactive to indicate that the author-script\nwas created with correct quoting. Previously this file was always\nempty. The fallback path also fixes any missing \"'\" at the end of the\nGIT_AUTHOR_DATE line.\n\nThe fallback code has been manually tested by reverting the quoting\nfixes in write_author_script() with and without reverting the previous\nfix for the missing \"'\" at the end of the GIT_AUTHOR_DATE line and\nrunning t3404-rebase-interactive.sh.\n\nIdeally rebase and am would share the same code for reading and\nwriting the author script, but this commit just fixes the immediate\nbug.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Phillip Wood \n---\n\nNotes:\n Changes since v2:\n \n - Changed the way it detects and handles broken quoting when git\n is upgraded while rebase is stopped. This means that the fix for\n quoting is no longer tied to the fix for the trailing \"'\" on the\n GIT_AUTHOR_DATE line. It now also fixes broken quoting in\n read_author_ident() which would error out with the last version.\n - Changed the tests for authors that contain \"'\". If sq_dequote() and\n parse the author script then the shell should be able to and the\n most important thing is to check that the author in the new commit\n is correct.\n - Fixed an existing memory leak\n \n Note Eric expressed concerns about using a version number, but I had\n already written this code so I thought I send it and see how people\n felt about the additionally complexity. I like the fact that it\n decouple the quoting fix from Eric's patches. In the end the extra\n code is just reading an integer from a text file. The test for adding\n the trailing quote to the author script is quite strict can be\n defeated by a badly edited author script but hopefully that is\n unlikely as it is only invoked if git got upgraded while rebase was\n stopped.\n\n git-rebase--interactive.sh | 2 +-\n sequencer.c | 89 +++++++++++++++++++++++++++++++----\n sequencer.h | 1 +\n t/t3404-rebase-interactive.sh | 20 ++++++--\n 4 files changed, 97 insertions(+), 15 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 06a7b79307..c1e3f947a5 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -880,7 +880,7 @@ init_basic_state () {\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n \trm -f \"$(git rev-parse --git-path REBASE_HEAD)\"\n \n-\t: > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n+\techo 1 > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n \twrite_basic_state\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 1bf8b0c431..2599f9d80e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -44,6 +44,12 @@ static GIT_PATH_FUNC(git_path_head_file, \"sequencer/head\")\n static GIT_PATH_FUNC(git_path_abort_safety_file, \"sequencer/abort-safety\")\n \n static GIT_PATH_FUNC(rebase_path, \"rebase-merge\")\n+/*\n+ * This file indicates that an interactive rebase is in progress, if it contians\n+ * an integer then that is a version indicator for the contents of files in\n+ * .git/rebase-merge\n+ */\n+static GIT_PATH_FUNC(rebase_path_interactive, \"rebase-merge/interactive\")\n /*\n * The file containing rebase commands, comments, and empty lines.\n * This file is created by \"git rebase -i\" then edited by the user. As\n@@ -636,42 +642,79 @@ static int write_author_script(const char *message)\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (skip_prefix(message, \"> \", &message))\n \t\t\tbreak;\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\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 }\n \n+/*\n+ * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE line with\n+ * a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather than \"'\\\\''\". Fix\n+ * these problems before dequoting in when git was upgraded while rebase was\n+ * stopped.\n+ */\n+static int fix_bad_author_script(struct strbuf *script)\n+{\n+\tconst char *next;\n+\tsize_t off = 0;\n+\n+\twhile ((next = strstr(script->buf + off, \"'\\\\\\\\''\"))) {\n+\t\toff = next - script->buf + 4;\n+\t\tstrbuf_splice(script, next - script->buf, 5,\"'\\\\''\", 4);\n+\t}\n+\n+\tif ((next = strstr(script->buf, \"\\nGIT_AUTHOR_DATE='\")) &&\n+\t (next = strchr(++next, '\\n')) &&\n+\t ++next - script->buf == script->len) {\n+\t\tif (script->buf[script->len - 2] != '\\'')\n+\t\t\tstrbuf_insert(script, script->len - 1, \"'\", 1);\n+\t} else {\n+\t\treturn error(_(\"unable to parse '%s'\"),\n+\t\t\t rebase_path_author_script());\n+\t}\n+\n+\treturn 0;\n+}\n+\n /*\n * Read a list of environment variable assignments (such as the author-script\n * file) into an environment block. Returns -1 on error, 0 otherwise.\n */\n-static int read_env_script(struct argv_array *env)\n+static int read_env_script(struct replay_opts *opts, struct argv_array *env)\n {\n \tstruct strbuf script = STRBUF_INIT;\n \tint i, count = 0;\n-\tchar *p, *p2;\n+\tconst char *p2;\n+\tchar *p;\n \n-\tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n+\tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0) {\n+\t\tstrbuf_release(&script);\n \t\treturn -1;\n+\t}\n+\n+\tif (!opts->version && fix_bad_author_script(&script)) {\n+\t\tstrbuf_release(&script);\n+\t\treturn -1;\n+\t}\n \n \tfor (p = script.buf; *p; p++)\n-\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n+\t\tif (skip_prefix(p, \"'\\\\''\", &p2))\n \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n \t\telse if (*p == '\\'')\n \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\n@@ -701,7 +744,7 @@ static char *get_author(const char *message)\n }\n \n /* Read author-script and return an ident line (author timestamp) */\n-static int read_author_ident(char **author)\n+static int read_author_ident(struct replay_opts *opts, char **author)\n {\n \tconst char *keys[] = {\n \t\t\"GIT_AUTHOR_NAME=\", \"GIT_AUTHOR_EMAIL=\", \"GIT_AUTHOR_DATE=\"\n@@ -717,6 +760,11 @@ static int read_author_ident(char **author)\n \t\treturn -1;\n \t}\n \n+\tif (!opts->version && fix_bad_author_script(&buf)) {\n+\t\tstrbuf_release(&buf);\n+\t\treturn -1;\n+\t}\n+\n \tfor (in = buf.buf; i < 3 && in - buf.buf < buf.len; i++) {\n \t\tif (!skip_prefix(in, keys[i], (const char **)&in)) {\n \t\t\tstrbuf_release(&buf);\n@@ -801,7 +849,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\tstruct object_id root_commit, *cache_tree_oid;\n \t\tint res = 0;\n \n-\t\tif (is_rebase_i(opts) && read_author_ident(&author))\n+\t\tif (is_rebase_i(opts) && read_author_ident(opts, &author))\n \t\t\treturn -1;\n \n \t\tif (!defmsg)\n@@ -839,7 +887,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \t\t\tcmd.err = -1;\n \t\t}\n \n-\t\tif (read_env_script(&cmd.env_array)) {\n+\t\tif (read_env_script(opts, &cmd.env_array)) {\n \t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n \t\t\treturn error(_(staged_changes_advice),\n@@ -2238,6 +2286,27 @@ static int read_populate_opts(struct replay_opts *opts)\n \tif (is_rebase_i(opts)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (read_oneliner(&buf, rebase_path_interactive(), 0)) {\n+\t\t\tif (buf.len) {\n+\t\t\t\tchar *end;\n+\t\t\t\tlong version = strtol(buf.buf, &end, 10);\n+\t\t\t\tif (version < 1 ||version > INT_MAX ||\n+\t\t\t\t *end != '\\0') {\n+\t\t\t\t\tstrbuf_release(&buf);\n+\t\t\t\t\treturn error(_(\"unable to parse '%s'\"),\n+\t\t\t\t\t\t rebase_path_interactive());\n+\t\t\t\t}\n+\t\t\t\topts->version = (int)version;\n+\t\t\t} else {\n+\t\t\t\topts->version = 0;\n+\t\t\t}\n+\t\t\tstrbuf_reset(&buf);\n+\t\t} else {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn error(_(\"unable to read '%s'\"),\n+\t\t\t\t rebase_path_interactive());\n+\t\t}\n+\n \t\tif (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {\n \t\t\tif (!starts_with(buf.buf, \"-S\"))\n \t\t\t\tstrbuf_reset(&buf);\ndiff --git a/sequencer.h b/sequencer.h\nindex c5787c6b56..24d69e7ff7 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -34,6 +34,7 @@ struct replay_opts {\n \tint keep_redundant_commits;\n \tint verbose;\n \n+\tint version;\n \tint mainline;\n \n \tchar *gpg_sign;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b72167ecd5..4e2e787f26 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -75,6 +75,7 @@ test_expect_success 'rebase --keep-empty' '\n \ttest_line_count = 6 actual\n '\n \n+SQ=\"'\"\n test_expect_success 'rebase -i with the exec command' '\n \tgit checkout master &&\n \t(\n@@ -1361,7 +1362,6 @@ test_expect_success 'editor saves as CR/LF' '\n \t)\n '\n \n-SQ=\"'\"\n test_expect_success 'rebase -i --gpg-sign=' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tset_fake_editor &&\n@@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\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][0-9][0-9]$\" out\n+\tgit commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n+\tgit cat-file commit HEAD | grep ^author >expected &&\n+\tFAKE_LINES=\"5 1\" git rebase -i --root &&\n+\tgit cat-file commit HEAD^ | grep ^author >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'valid author header when author contains single quote' '\n+\trebase_setup_and_clean author-header no-conflict-branch &&\n+\tset_fake_editor &&\n+\tgit commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n+\tgit cat-file commit HEAD | grep ^author >expected &&\n+\tFAKE_LINES=\"1 5\" git rebase -i --root &&\n+\tgit cat-file commit HEAD | grep ^author >actual &&\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.18.0\n\n"},{"id":"354278","messageId":"xmqqk1p83cig.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"20180802112002.720-3-phillip.wood@talktalk.net","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-02T17:27:03Z","receivedAt":"2018-08-02T17:27:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Single quotes should be escaped as \\' not \\\\'. The bad quoting breaks\n> the interactive version of 'rebase --root' (which is used when there is\n> no '--onto' even if the user does not specify --interactive) for authors\n> that contain \"'\" as sq_dequote() called read_author_ident() errors out\n> on the bad quoting.\n>\n> For other interactive rebases this only affects external scripts that\n> read the author script and users whose git is upgraded from the shell\n> version of rebase -i while rebase was stopped when the author contains\n> \"'\". This is because the parsing in read_env_script() expected the\n> broken quoting.\n\nI wasn't following the discussion, but is it the general consensus\nthat reading the broken a-i file is a requirement for the new code?\nNot an objection phrased as a question.\n\nI do not think it is worth worrying about the \"upgrade while rebase\nwas in progress\" case, if it involves much more code than necessary\nwithout its support, especially if the only thing the user needs to\ndo recover from such a situation is to say \"rebase --abort\" and then\nto retry the same rebase with the fixed version that was installed\nin the meantime. Let's see how much we need to bend over backwards\nto do this \"transition\" thing.\n\n> Ideally rebase and am would share the same code for reading and\n> writing the author script, but this commit just fixes the immediate\n> bug.\n\nOK.\n\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 06a7b79307..c1e3f947a5 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -880,7 +880,7 @@ init_basic_state () {\n> \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n> \trm -f \"$(git rev-parse --git-path REBASE_HEAD)\"\n> \n> -\t: > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n> +\techo 1 > \"$state_dir\"/interactive || die \"$(gettext \"Could not mark as interactive\")\"\n\nThis impacts the work Alban is doing, which at the end removes this\nscript altogether.\n\n> +/*\n> + * write_author_script() used to fail to terminate the GIT_AUTHOR_DATE line with\n> + * a \"'\" and also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather than \"'\\\\''\". Fix\n\nI think the comment here (for both the wrong and the right versions)\nis easier to read if you wrote the string as literal without C, i.e.\nThe string is \"'\\\\''\" but as a string literal in C it is expressed\nas \"'\\\\\\\\''\".\n\n> +static int fix_bad_author_script(struct strbuf *script)\n> +{\n> +\tconst char *next;\n> +\tsize_t off = 0;\n> +\n> +\twhile ((next = strstr(script->buf + off, \"'\\\\\\\\''\"))) {\n\nThis looks brittle.\n\nWe need assurance that the first \"'\\\\''\" we see on the line came\nfrom the attempt by the broken writer to write out a single \"'\", and\nnot from anything else. The broken writer places its own \"'\"\nimmediately after GIT_AUTHOR_NAME= (just like the corrected one\ndoes) before moving on to the end-user payload. Can the single\nquote at the beginning of the substring you are looking for be that\none? If the end user's payload began with two backslashes, that\nwould have produced a result that matches the first three bytes of\nthe substring you are looking for. But there is no way for the\nend-user payload to make the next two bytes \"''\"---any byte other\nthan a sq would result in a sq added to the result, and a byte that\nis a sq would give one sq followed by a bs.\n\nOK, so this is probably doing the right thing, as long as we know we\nare reading from the old and broken writer. It still does look\nunnecessarily ugly and over-engineered to have this (and the\n\"version\" reading code), though, at least to me, but perhaps it is\njust me.\n\nThanks.\n\n\n"},{"id":"354360","messageId":"CAPig+cSZ3Zm=qFcvGjyj_uStn=JMAYuskMa0O_2yxkKjaRWTSg@mail.gmail.com","threadId":"48989","inReplyTo":"20180802112002.720-2-phillip.wood@talktalk.net","subject":"Re: [PATCH v3 1/2] sequencer: handle errors in read_author_ident()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T07:09:30Z","receivedAt":"2018-08-03T07:09:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 7:20 AM Phillip Wood wrote:\n> The calling code did not treat NULL as an error. Instead NULL caused\n> it to fallback to using the default author when creating the new\n> commit. This changed the date and potentially the author of the\n> commit which corrupted the author data compared to its expected\n> value. Fix this by returning and integer and passing in a parameter to\n> receive the author.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -701,57 +701,59 @@ static char *get_author(const char *message)\n> -static const char *read_author_ident(struct strbuf *buf)\n> +static int read_author_ident(char **author)\n> {\n> + struct strbuf buf = STRBUF_INIT;\n> - if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)\n> - return NULL;\n> + if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0) {\n> + strbuf_release(&buf);\n> + return -1;\n> + }\n\n[...much noisiness snipped...]\n\n> @@ -794,12 +796,14 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {\n> - struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n> - const char *author = is_rebase_i(opts) ?\n> - read_author_ident(&script) : NULL;\n> + struct strbuf msg = STRBUF_INIT;\n> + char *author = NULL;\n>\n> + if (is_rebase_i(opts) && read_author_ident(&author))\n> + return -1;\n> +\n\nI think this patch can be simplified considerably by shifting one's\nperspective. If we admit that read_author_ident() is already correctly\nreporting an error by returning NULL (which is exactly what it is\ndoing), then the bug is is purely on the calling side; namely, the\ncaller is ignoring the error. (In fact, your commit message already\nstates this.)\n\nSo, if it's the caller which is buggy, then read_author_ident() does\nnot require _any_ changes (meaning all the noisiness in this patch can\nbe dropped), and only the caller needs a fix, and that change can be\nquite tiny. To wit, instead of initializing 'author' like this (as\ndone in the current \"buggy\" code):\n\n const char *author = is_rebase_i(opts) ?\n read_author_ident(&script) : NULL;\n\nDo this instead:\n\n const char *author = NULL;\n ...\n if (is_rebase_i(opts)) {\n author = read_author_ident(&script);\n if (!author) {\n strbuf_release(&script);\n return -1;\n }\n }\n\nThat's it, a minimal fix giving the same result, without all the code\nchurn, thus safer (less opportunity, for instance, to introduce a leak\nas in v1). Of course, you can collapse the above even further to:\n\n if (is_rebase_i(opts) &&\n !(author = read_author_ident(&script)) {\n strbuf_release(&script);\n return -1;\n }\n\nThough, I think that is less readable.\n"},{"id":"354362","messageId":"CAPig+cQaaHfkR8jrXb-+PB4bSZbKLQzRkZNo+DNfz92hKO=04Q@mail.gmail.com","threadId":"48989","inReplyTo":"xmqqk1p83cig.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T07:59:39Z","receivedAt":"2018-08-03T07:59:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano wrote:\n> Phillip Wood writes:\n> > For other interactive rebases this only affects external scripts that\n> > read the author script and users whose git is upgraded from the shell\n> > version of rebase -i while rebase was stopped when the author contains\n> > \"'\". This is because the parsing in read_env_script() expected the\n> > broken quoting.\n>\n> I wasn't following the discussion, but is it the general consensus\n> that reading the broken a-i file is a requirement for the new code?\n> Not an objection phrased as a question.\n>\n> I do not think it is worth worrying about the \"upgrade while rebase\n> was in progress\" case, if it involves much more code than necessary\n> without its support, especially if the only thing the user needs to\n> do recover from such a situation is to say \"rebase --abort\" and then\n> to retry the same rebase with the fixed version that was installed\n> in the meantime. [...]\n>\n> [...] It still does look\n> unnecessarily ugly and over-engineered to have this (and the\n> \"version\" reading code), though, at least to me, but perhaps it is\n> just me.\n\nIt's not just you. I also questioned[1] if such backward compatibility\nwas needed, and had concerns[2] about a version file being heavyweight\nand over-engineered.\n\nThis is a lot of new code (possibly harboring its own bugs) for a\nsituation unlikely to arise, and which becomes ever more unlikely as\ntime passes. Also, unlike long-lived (years or decades) resources,\nsuch as a repository or pack file, for instance, for which a version\nnumber makes sense, this file is very short-lived, which makes it even\nmore difficult to justify adding this much machinery for something so\nunlikely to arise in practice.\n\nThe overall aim of this series to fix these bugs is laudable, but I\nwould be happy to see this one reduced to just a \"bug fix\" patch\nwithout all the backward-compatibility machinery (and wouldn't mind\nseeing patch 1/2 simplified[3], as well).\n\nThanks.\n\n[1]: https://public-inbox.org/git/CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com/\n[2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/\n[3]: https://public-inbox.org/git/CAPig+cSZ3Zm=qFcvGjyj_uStn=JMAYuskMa0O_2yxkKjaRWTSg@mail.gmail.com/\n"},{"id":"354366","messageId":"c7b8629d-7b93-2fbf-6793-0d566e86a229@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cQaaHfkR8jrXb-+PB4bSZbKLQzRkZNo+DNfz92hKO=04Q@mail.gmail.com","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-03T09:33:05Z","receivedAt":"2018-08-03T09:33:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Dear Eric and Junio\nOn 03/08/18 08:59, Eric Sunshine wrote:\n> On Thu, Aug 2, 2018 at 1:27 PM Junio C Hamano wrote:\n>> Phillip Wood writes:\n>>> For other interactive rebases this only affects external scripts that\n>>> read the author script and users whose git is upgraded from the shell\n>>> version of rebase -i while rebase was stopped when the author contains\n>>> \"'\". This is because the parsing in read_env_script() expected the\n>>> broken quoting.\n>>\n>> I wasn't following the discussion, but is it the general consensus\n>> that reading the broken a-i file is a requirement for the new code?\n>> Not an objection phrased as a question.\n>>\n>> I do not think it is worth worrying about the \"upgrade while rebase\n>> was in progress\" case, if it involves much more code than necessary\n>> without its support, especially if the only thing the user needs to\n>> do recover from such a situation is to say \"rebase --abort\" and then\n>> to retry the same rebase with the fixed version that was installed\n>> in the meantime. [...]\n>>\n>> [...] It still does look\n>> unnecessarily ugly and over-engineered to have this (and the\n>> \"version\" reading code), though, at least to me, but perhaps it is\n>> just me.\n> \n> It's not just you. I also questioned[1] if such backward compatibility\n> was needed, and had concerns[2] about a version file being heavyweight\n> and over-engineered.\n\nIf there isn't some backward compatibility then if git gets upgraded\nwhile rebase is stopped then the author data will be silently corrupted\nif it contains \"'\". read_author_ident() will error out but that is only\nused for the root commit. read_env_script() which is used for normal\npicks will not dequote the badly quoted value correctly and will not\nreturn an error. It is unlikely but possible, I'll leave it to Junio to\ndecide if it is worth it\n\n> \n> This is a lot of new code (possibly harboring its own bugs) for a\n> situation unlikely to arise, and which becomes ever more unlikely as\n> time passes. Also, unlike long-lived (years or decades) resources,\n> such as a repository or pack file, for instance, for which a version\n> number makes sense, this file is very short-lived, which makes it even\n> more difficult to justify adding this much machinery for something so\n> unlikely to arise in practice.\n\nThere is a precedent for adding backwards compatibility 84df4560ed\n(\"rebase: extract code for writing basic state\", 2011-02-06) though it\nis much simpler. Part of the commit message reads\n\n Note that non-interactive rebase stores the sha1 of the\n original head in a file called orig-head, while interactive\n rebase stores it in a file called head. Change this by\n writing to orig-head in both cases. When reading, try to read\n from orig-head. If that fails, read from head instead. This\n protects users who upgraded git while they had an ongoing\n interactive rebase, while still making it possible to remove\n the code that reads from head at some point in the future.\n\nBest Wishes\n\nPhillip\n\n> The overall aim of this series to fix these bugs is laudable, but I\n> would be happy to see this one reduced to just a \"bug fix\" patch\n> without all the backward-compatibility machinery (and wouldn't mind\n> seeing patch 1/2 simplified[3], as well).\n> \n> Thanks.\n> \n> [1]: https://public-inbox.org/git/CAPig+cR5VHP8muo5_A_9t7OPZam8O_uPb0nd73B15Ye92n+p7Q@mail.gmail.com/\n> [2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/\n> [3]: https://public-inbox.org/git/CAPig+cSZ3Zm=qFcvGjyj_uStn=JMAYuskMa0O_2yxkKjaRWTSg@mail.gmail.com/\n> \n\n"},{"id":"354369","messageId":"CAPig+cTyTHxwFvk3ZtOq3L7KEtEjKLKu6-RnLC-_NuL1Xzhqzw@mail.gmail.com","threadId":"48989","inReplyTo":"c7b8629d-7b93-2fbf-6793-0d566e86a229@talktalk.net","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T10:02:11Z","receivedAt":"2018-08-03T10:02:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 5:33 AM Phillip Wood wrote:\n> If there isn't some backward compatibility then if git gets upgraded\n> while rebase is stopped then the author data will be silently corrupted\n> if it contains \"'\". read_author_ident() will error out but that is only\n> used for the root commit. read_env_script() which is used for normal\n> picks will not dequote the badly quoted value correctly and will not\n> return an error. It is unlikely but possible, I'll leave it to Junio to\n> decide if it is worth it\n\nIf I understand correctly, the approach you implemented earlier[1]\n(perhaps coupled with the more robust detection suggested here[2])\nwould be sufficient to handle this backward compatibility concern.\nWhile it may not be as pretty or generalized as the current patch, it\ninvolves far less machinery, thus is less likely to harbor its own\nbugs. The earlier version is also much more self-contained, which\nmakes it easier to drop at some point when backward compatibility is\nno longer a concern (if ever).\n\n> There is a precedent for adding backwards compatibility 84df4560ed\n> (\"rebase: extract code for writing basic state\", 2011-02-06) though it\n> is much simpler.\n\nIndeed, it is much simpler, adding a one-liner 'else' case to an\n'if-then' for backward compatibility. Your earlier implementation[1]\nwas pretty much the equivalent, just adding an extra one-liner arm to\nan 'if-then' statement.\n\nThe bug fix itself is important, and, while I do favor the cleaner\napproach of not worrying about backward compatibility for this fairly\nunlikely case, your earlier version seems a better compromise between\nhaving no backward compatibility and the much more heavyweight version\nimplemented here.\n\nAnyhow, I'm fine with whatever Junio decides.\n\n[1]: https://public-inbox.org/git/20180731111532.9358-3-phillip.wood@talktalk.net/\n[2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/\n"},{"id":"354390","messageId":"455fafb5-3c92-4348-0c2c-0a4ab62cf2ce@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cTyTHxwFvk3ZtOq3L7KEtEjKLKu6-RnLC-_NuL1Xzhqzw@mail.gmail.com","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-03T14:12:49Z","receivedAt":"2018-08-03T14:12:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\nOn 03/08/18 11:02, Eric Sunshine wrote:\n> On Fri, Aug 3, 2018 at 5:33 AM Phillip Wood wrote:\n>> If there isn't some backward compatibility then if git gets upgraded\n>> while rebase is stopped then the author data will be silently corrupted\n>> if it contains \"'\". read_author_ident() will error out but that is only\n>> used for the root commit. read_env_script() which is used for normal\n>> picks will not dequote the badly quoted value correctly and will not\n>> return an error. It is unlikely but possible, I'll leave it to Junio to\n>> decide if it is worth it\n> \n> If I understand correctly, the approach you implemented earlier[1]\n> (perhaps coupled with the more robust detection suggested here[2])\n> would be sufficient to handle this backward compatibility concern.\n> While it may not be as pretty or generalized as the current patch, it\n> involves far less machinery, thus is less likely to harbor its own\n> bugs. The earlier version is also much more self-contained, which\n> makes it easier to drop at some point when backward compatibility is\n> no longer a concern (if ever).\n\nYes I think the earlier approach with the more robust detection you \nsuggested is probably a good compromise. Junio does that sound good to you?\n\nBest Wishes\n\nPhillip\n\n>> There is a precedent for adding backwards compatibility 84df4560ed\n>> (\"rebase: extract code for writing basic state\", 2011-02-06) though it\n>> is much simpler.\n> \n> Indeed, it is much simpler, adding a one-liner 'else' case to an\n> 'if-then' for backward compatibility. Your earlier implementation[1]\n> was pretty much the equivalent, just adding an extra one-liner arm to\n> an 'if-then' statement.\n> \n> The bug fix itself is important, and, while I do favor the cleaner\n> approach of not worrying about backward compatibility for this fairly\n> unlikely case, your earlier version seems a better compromise between\n> having no backward compatibility and the much more heavyweight version\n> implemented here.\n> \n> Anyhow, I'm fine with whatever Junio decides.\n> \n> [1]: https://public-inbox.org/git/20180731111532.9358-3-phillip.wood@talktalk.net/\n> [2]: https://public-inbox.org/git/CAPig+cTttbV2FjnoS_SZtwh2J4wwzsbK+48BAbt1cV0utynYzw@mail.gmail.com/\n> \n\n"},{"id":"354397","messageId":"xmqqtvobzbsv.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"CAPig+cSZ3Zm=qFcvGjyj_uStn=JMAYuskMa0O_2yxkKjaRWTSg@mail.gmail.com","subject":"Re: [PATCH v3 1/2] sequencer: handle errors in read_author_ident()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T15:53:36Z","receivedAt":"2018-08-03T15:53:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I think this patch can be simplified considerably by shifting one's\n> perspective. If we admit that read_author_ident() is already correctly\n> reporting an error by returning NULL (which is exactly what it is\n> doing), then the bug is is purely on the calling side; namely, the\n> caller is ignoring the error. (In fact, your commit message already\n> states this.)\n\nThis approach looks quite sensible.\n"},{"id":"354698","messageId":"CAE1pOi3+FW8itk4AHhG+r02TGyGTeoZpq7S6HKXkQtOVqigWGQ@mail.gmail.com","threadId":"48989","inReplyTo":"CAPig+cRe1R1YWpLaXVtTmwhaJiz2dONU83qtn_+XhaRevjn2fQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Hilco Wijbenga","fromEmail":"hilco.wijbenga@gmail.com","sentAt":"2018-08-07T01:19:38Z","receivedAt":"2018-08-07T01:20:03Z","isPatch":true,"sender":{"key":"hilco.wijbenga@gmail.com","avatar":null},"body":"On Tue, Jul 31, 2018 at 11:22 PM, Eric Sunshine wrote:\n> On Tue, Jul 31, 2018 at 9:30 PM Hilco Wijbenga wrote:\n>> On Tue, Jul 31, 2018 at 12:33 AM, Eric Sunshine wrote:\n>> > This is a re-roll of [1] which fixes sequencer bugs resulting in commit\n>> > object corruption when \"rebase -i --root\" swaps in a new commit as root.\n>> > Unfortunately, those bugs made it into v2.18.0 and have already\n>> > corrupted at least one repository (a local project of mine). Patches 3/4\n>> > and 4/4 are new.\n>>\n>> Does this also fix losing the initial commit if it is empty?\n>>\n>> git init ; git commit -m 'Initial commit' --allow-empty ; touch\n>> file.txt ; git add file.txt ; git commit -m 'Add file.txt' ; git\n>> rebase --root\n>>\n>> I would expect there to be 2 commits but the first one has\n>> disappeared. (This usually happens with \"git rebase -i --root\" early\n>> on in a new project.)\n>\n> This series should have no impact on that issue; it is fixing root\n> commit object corruption, and does not touch or change how the commit\n> graph is maintained or how the rebasing machinery itself works (and\n> the --allow-empty stuff is part of that machinery).\n>\n> As Dscho is cc:'d already, perhaps he can chime in as a resident expert.\n>\n> By the way, what happens if you use --keep-empty while rebasing?\n\nI was not aware of that flag. But, although I was expecting it to, if\nI use it with the rebase, I don't see any different behaviour. I can't\nreally make out what its purpose is from \"Keep the commits that do not\nchange anything from its parents in the result.\".\n\nBut your suggestion did make me think about what behaviour I would\nlike to see, exactly. I like that Git removes commits that no longer\nserve any purpose (because I've included their changes in earlier\ncommits). So I would not want to keep commits that become empty during\nthe rebase. What I would like to see is that commits that _start out_\nas empty, are retained. Would such behaviour make sense? Or would that\nbe considered surprising behaviour?\n"},{"id":"354702","messageId":"CAPig+cT0ATgRMY-mhttViaburFW37G352BX=mj-Cnr2_wZruaQ@mail.gmail.com","threadId":"48989","inReplyTo":"CAE1pOi3+FW8itk4AHhG+r02TGyGTeoZpq7S6HKXkQtOVqigWGQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-07T03:31:41Z","receivedAt":"2018-08-07T03:31:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 6, 2018 at 9:20 PM Hilco Wijbenga wrote:\n> But your suggestion did make me think about what behaviour I would\n> like to see, exactly. I like that Git removes commits that no longer\n> serve any purpose (because I've included their changes in earlier\n> commits). So I would not want to keep commits that become empty during\n> the rebase. What I would like to see is that commits that _start out_\n> as empty, are retained. Would such behaviour make sense? Or would that\n> be considered surprising behaviour?\n\nI, personally, have no opinion since I don't use empty commits.\nPerhaps someone more experienced and more long-sighted will chime in.\n"},{"id":"354720","messageId":"20180807093452.22524-1-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180731073331.40007-1-sunshine@sunshineco.com","subject":"[PATCH v4 0/2] fix author-script quoting","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-07T09:34:50Z","receivedAt":"2018-08-07T09:35:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nI've updated these based on Eric's suggestions, hopefully they're good\nto go now. Thanks Eric for you help.\n\nPhillip Wood (2):\n sequencer: handle errors from read_author_ident()\n sequencer: fix quoting in write_author_script\n\n sequencer.c | 47 ++++++++++++++++++++++++++++-------\n t/t3404-rebase-interactive.sh | 18 +++++++++++---\n 2 files changed, 53 insertions(+), 12 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"354721","messageId":"20180807093452.22524-2-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180807093452.22524-1-phillip.wood@talktalk.net","subject":"[PATCH v4 1/2] sequencer: handle errors from read_author_ident()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-07T09:34:51Z","receivedAt":"2018-08-07T09:35:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCheck for a NULL return value from read_author_ident() that indicates\nan error. Previously the NULL author was passed to commit_tree() which\nwould then fallback to using the default author when creating the new\ncommit. This changed the date and potentially the author of the commit\nwhich corrupted the author data compared to its expected value.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Phillip Wood \n---\n\nNotes:\n changes since v3:\n \n - Implemented the simpler scheme suggested by Eric\n\n sequencer.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 944dea6997..c4e4418559 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -795,11 +795,18 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tif ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {\n \t\tstruct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n-\t\tconst char *author = is_rebase_i(opts) ?\n-\t\t\tread_author_ident(&script) : NULL;\n+\t\tconst char *author = NULL;\n \t\tstruct object_id root_commit, *cache_tree_oid;\n \t\tint res = 0;\n \n+\t\tif (is_rebase_i(opts)) {\n+\t\t\tauthor = read_author_ident(&script);\n+\t\t\tif (!author) {\n+\t\t\t\tstrbuf_release(&script);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (!defmsg)\n \t\t\tBUG(\"root commit without message\");\n \n-- \n2.18.0\n\n"},{"id":"354722","messageId":"20180807093452.22524-3-phillip.wood@talktalk.net","threadId":"48989","inReplyTo":"20180807093452.22524-1-phillip.wood@talktalk.net","subject":"[PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-07T09:34:52Z","receivedAt":"2018-08-07T09:35:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSingle quotes should be escaped as \\' not \\\\'. The bad quoting breaks\nthe interactive version of 'rebase --root' (which is used when there\nis no '--onto' even if the user does not specify --interactive) for\nauthors that contain \"'\" as sq_dequote() called by read_author_ident()\nerrors out on the bad quoting.\n\nFor other interactive rebases this only affects external scripts that\nread the author script and users whose git is upgraded from the shell\nversion of rebase -i while rebase was stopped when the author contains\n\"'\". This is because the parsing in read_env_script() expected the\nbroken quoting.\n\nThis patch includes code to handle the broken quoting when\ngit has been upgraded while rebase was stopped. It does this by\ndetecting the missing \"'\" at the end of the GIT_AUTHOR_DATE line to see\nif it should dequote \\\\' as \"'\". Note this is only implemented for\nnormal picks, not for creating a new root commit (rebase will stop with\nan error complaining out bad quoting in that case).\n\nThe fallback code has been manually tested by reverting both the quoting\nfixes in write_author_script() and the previous fix for the missing \"'\"\nat the end of the GIT_AUTHOR_DATE line and running\nt3404-rebase-interactive.sh.\n\nIdeally rebase and am would share the same code for reading and\nwriting the author script, but this commit just fixes the immediate\nbug.\n\nHelped-by: Eric Sunshine \nHelped-by: Johannes Schindelin \nSigned-off-by: Phillip Wood \n---\n\nNotes:\n changes since v3:\n \n - Reverted the implementation to v2 with more robust detection of the\n missing \"'\" on the last line of the author script based on a\n suggestion by Eric. This means that this series needs to progress\n closely with Eric's series of fixes or the fallback code will never\n be called.\n \n - Tweaked the last test\n\n sequencer.c | 36 ++++++++++++++++++++++++++++-------\n t/t3404-rebase-interactive.sh | 18 +++++++++++++++---\n 2 files changed, 44 insertions(+), 10 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c4e4418559..ba11fe5bca 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -636,42 +636,64 @@ static int write_author_script(const char *message)\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_EMAIL='\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (skip_prefix(message, \"> \", &message))\n \t\t\tbreak;\n \t\telse if (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\n+\t\t\tstrbuf_addf(&buf, \"'\\\\%c'\", *(message++));\n \tstrbuf_addstr(&buf, \"'\\nGIT_AUTHOR_DATE='@\");\n \twhile (*message && *message != '\\n' && *message != '\\r')\n \t\tif (*message != '\\'')\n \t\t\tstrbuf_addch(&buf, *(message++));\n \t\telse\n-\t\t\tstrbuf_addf(&buf, \"'\\\\\\\\%c'\", *(message++));\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 }\n \n+\n+/*\n+ * write_author_script() used to fail to terminate the last line with a \"'\" and\n+ * also escaped \"'\" incorrectly as \"'\\\\\\\\''\" rather than \"'\\\\''\". We check for\n+ * the terminating \"'\" on the last line to see how \"'\" has been escaped in case\n+ * git was upgraded while rebase was stopped.\n+ */\n+static int quoting_is_broken(const char *s, size_t n)\n+{\n+\t/* Skip any empty lines in case the file was hand edited */\n+\twhile (n > 0 && s[--n] == '\\n')\n+\t\t; /* empty */\n+\tif (n > 0 && s[n] != '\\'')\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n /*\n * Read a list of environment variable assignments (such as the author-script\n * file) into an environment block. Returns -1 on error, 0 otherwise.\n */\n static int read_env_script(struct argv_array *env)\n {\n \tstruct strbuf script = STRBUF_INIT;\n-\tint i, count = 0;\n-\tchar *p, *p2;\n+\tint i, count = 0, sq_bug;\n+\tconst char *p2;\n+\tchar *p;\n \n \tif (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n \t\treturn -1;\n-\n+\t/* write_author_script() used to quote incorrectly */\n+\tsq_bug = quoting_is_broken(script.buf, script.len);\n \tfor (p = script.buf; *p; p++)\n-\t\tif (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n+\t\tif (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n+\t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n+\t\telse if (skip_prefix(p, \"'\\\\''\", &p2))\n \t\t\tstrbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n \t\telse if (*p == '\\'')\n \t\t\tstrbuf_splice(&script, p-- - script.buf, 1, \"\", 0);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b72167ecd5..834a124d60 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\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][0-9][0-9]$\" out\n+\tgit commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n+\tgit cat-file commit HEAD | grep ^author >expected &&\n+\tFAKE_LINES=\"5 1\" git rebase -i --root &&\n+\tgit cat-file commit HEAD^ | grep ^author >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'valid author header when author contains single quote' '\n+\trebase_setup_and_clean author-header no-conflict-branch &&\n+\tset_fake_editor &&\n+\tgit commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n+\tgit cat-file commit HEAD | grep ^author >expected &&\n+\tFAKE_LINES=\"2\" git rebase -i HEAD~2 &&\n+\tgit cat-file commit HEAD | grep ^author >actual &&\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.18.0\n\n"},{"id":"354723","messageId":"CAPig+cQkY1_qDu=fdbA0gVPH88B47bM5BpgbYY8ivsj1Y0QHTw@mail.gmail.com","threadId":"48989","inReplyTo":"20180807093452.22524-3-phillip.wood@talktalk.net","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-07T10:23:35Z","receivedAt":"2018-08-07T10:23:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n> - Reverted the implementation to v2 with more robust detection of the\n> missing \"'\" on the last line of the author script based on a\n> suggestion by Eric. This means that this series needs to progress\n> closely with Eric's series of fixes or the fallback code will never\n> be called.\n\nThanks for working on this. I haven't read the patch closely yet, but\none thing caught my attention as I ran my eye over it...\n\n> +static int quoting_is_broken(const char *s, size_t n)\n> +{\n> + /* Skip any empty lines in case the file was hand edited */\n> + while (n > 0 && s[--n] == '\\n')\n> + ; /* empty */\n> + if (n > 0 && s[n] != '\\'')\n> + return 1;\n\nTo be \"technically correct\", I think the condition in the 'if'\nstatement should be \">=\". It should never happen in practice that the\nentire content of the file is a single character followed by zero or\nmore newlines, but using the proper condition \">=\" would save future\nreaders of this code a \"huh?\" moment.\n"},{"id":"354733","messageId":"753b860e-c922-fd70-9d2d-0581976a6f37@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cQkY1_qDu=fdbA0gVPH88B47bM5BpgbYY8ivsj1Y0QHTw@mail.gmail.com","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-07T13:54:25Z","receivedAt":"2018-08-07T13:54:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 07/08/18 11:23, Eric Sunshine wrote:\n> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n>> - Reverted the implementation to v2 with more robust detection of the\n>> missing \"'\" on the last line of the author script based on a\n>> suggestion by Eric. This means that this series needs to progress\n>> closely with Eric's series of fixes or the fallback code will never\n>> be called.\n> \n> Thanks for working on this. I haven't read the patch closely yet, but\n> one thing caught my attention as I ran my eye over it...\n> \n>> +static int quoting_is_broken(const char *s, size_t n)\n>> +{\n>> + /* Skip any empty lines in case the file was hand edited */\n>> + while (n > 0 && s[--n] == '\\n')\n>> + ; /* empty */\n>> + if (n > 0 && s[n] != '\\'')\n>> + return 1;\n> \n> To be \"technically correct\", I think the condition in the 'if'\n> statement should be \">=\". It should never happen in practice that the\n> entire content of the file is a single character followed by zero or\n> more newlines, but using the proper condition \">=\" would save future\n> readers of this code a \"huh?\" moment.\n> \n\nI'm not sure it is that simple. If the script consists solely of a\nsingle quote then we should return 1, if it is a single character that\nis not \"'\" then it should return 0. Currently it returns 0 in both those\ncases so is technically broken when the script is \"'\". If it used \">=\"\ninstead then I think it would return 0 when it should return 1 and vice\nversa. As you say this shouldn't happen in practice.\n\nBest Wishes\n\nPhillip\n"},{"id":"354755","messageId":"xmqqsh3qks9o.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"455fafb5-3c92-4348-0c2c-0a4ab62cf2ce@talktalk.net","subject":"Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T17:20:35Z","receivedAt":"2018-08-07T17:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Yes I think the earlier approach with the more robust detection you\n> suggested is probably a good compromise. Junio does that sound good to\n> you?\n\nSurely, and thanks.\n"},{"id":"354772","messageId":"xmqqr2j9khnv.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"CAPig+cT0ATgRMY-mhttViaburFW37G352BX=mj-Cnr2_wZruaQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T21:09:40Z","receivedAt":"2018-08-07T21:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Aug 6, 2018 at 9:20 PM Hilco Wijbenga wrote:\n>> But your suggestion did make me think about what behaviour I would\n>> like to see, exactly. I like that Git removes commits that no longer\n>> serve any purpose (because I've included their changes in earlier\n>> commits). So I would not want to keep commits that become empty during\n>> the rebase. What I would like to see is that commits that _start out_\n>> as empty, are retained. Would such behaviour make sense? Or would that\n>> be considered surprising behaviour?\n>\n> I, personally, have no opinion since I don't use empty commits.\n> Perhaps someone more experienced and more long-sighted will chime in.\n\n0661e49a (\"git-rebase.txt: document behavioral differences between\nmodes\", 2018-06-27) added the following. In short, \"rebase -i\"\nshould already behave that way.\n\n+ * empty commits:\n+\n+ am-based rebase will drop any \"empty\" commits, whether the\n+ commit started empty (had no changes relative to its parent to\n+ start with) or ended empty (all changes were already applied\n+ upstream in other commits).\n+\n+ merge-based rebase does the same.\n+\n+ interactive-based rebase will by default drop commits that\n+ started empty and halt if it hits a commit that ended up empty.\n+ The `--keep-empty` option exists for interactive rebases to allow\n+ it to keep commits that started empty.\n"},{"id":"354790","messageId":"CAPig+cQdH+Xiunej1E6EnA6btFzZJTPo27cFY_UdaFZF7J7t-w@mail.gmail.com","threadId":"48989","inReplyTo":"753b860e-c922-fd70-9d2d-0581976a6f37@talktalk.net","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-08T08:43:43Z","receivedAt":"2018-08-08T08:43:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 7, 2018 at 9:54 AM Phillip Wood wrote:\n> On 07/08/18 11:23, Eric Sunshine wrote:\n> > On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n> >> + if (n > 0 && s[n] != '\\'')\n> >> + return 1;\n> >\n> > To be \"technically correct\", I think the condition in the 'if'\n> > statement should be \">=\". It should never happen in practice that the\n> > entire content of the file is a single character followed by zero or\n> > more newlines, but using the proper condition \">=\" would save future\n> > readers of this code a \"huh?\" moment.\n>\n> I'm not sure it is that simple. If the script consists solely of a\n> single quote then we should return 1, if it is a single character that\n> is not \"'\" then it should return 0. Currently it returns 0 in both those\n> cases so is technically broken when the script is \"'\". If it used \">=\"\n> instead then I think it would return 0 when it should return 1 and vice\n> versa. As you say this shouldn't happen in practice.\n\nIt shouldn't happen in practice, but if a human (power user) edits\nthis file, we shouldn't discount the possibility. However, I'm not so\nconcerned about quoting_is_broken() returning a meaningful answer in\nthis case since we have much bigger problems if we get such a file.\n(Indeed, what answer could quoting_is_broken() return which would be\nuseful or meaningful for such a malformed file?)\n\nWhat does concern me is that read_env_script() doesn't seem to care\nabout such a malformed file; it doesn't do any validation at all.\nContrast that with read_author_ident() which is pretty strict about\nthe content it expects to find in the file. So, it might make sense to\nupgrade read_env_script() to do some sort of validation on each line\n(though that shouldn't be in this patch, and doesn't even need to be\nin this series). For instance, it could check that each line looks\nsomething like what would be matched by this regex: /[A-Z0-9_]+='.+'/\n\n(And, no, I'm not saying that regex should be used for validation; I'm\njust using it as an example.)\n\nAs for '>' vs. '>=', it caused more than a slight hiccup when I was\nscanning the code, and I worry that future readers could be similarly\nimpacted.\n"},{"id":"354793","messageId":"CAPig+cR-O07v98SmjhKkXodN=SoSK3ahp2t2zNJ54uo3-_Fq3Q@mail.gmail.com","threadId":"48989","inReplyTo":"20180807093452.22524-3-phillip.wood@talktalk.net","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-08T09:39:37Z","receivedAt":"2018-08-08T09:39:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n> Single quotes should be escaped as \\' not \\\\'. The bad quoting breaks\n> the interactive version of 'rebase --root' (which is used when there\n> is no '--onto' even if the user does not specify --interactive) for\n> authors that contain \"'\" as sq_dequote() called by read_author_ident()\n> errors out on the bad quoting.\n> [...]\n> Signed-off-by: Phillip Wood \n> ---\n> diff --git a/sequencer.c b/sequencer.c\n> @@ -636,42 +636,64 @@ static int write_author_script(const char *message)\n> static int read_env_script(struct argv_array *env)\n> {\n> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n> return -1;\n\nErm, again, not introduced by this patch, but this is leaking 'script'\nin the error path. You had plugged this leak in the previous round but\nthat fix got lost when you reverted to this simpler approach. Not\ncritical, though; the leak probably ought to be fixed by a separate\npatch anyhow (which doesn't necessarily need to be part of this\nseries).\n\n> + /* write_author_script() used to quote incorrectly */\n> + sq_bug = quoting_is_broken(script.buf, script.len);\n> for (p = script.buf; *p; p++)\n> - if (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n> + if (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n> + strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> + else if (skip_prefix(p, \"'\\\\''\", &p2))\n> strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n\nThe two strbuf_splice() invocations are identical, so an alternate way\nof expressing this would be:\n\n if ((sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2)) ||\n skip_prefix(p, \"'\\\\''\", &p2))\n strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n\nNot necessarily clearer, and certainly not worth a re-roll.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> @@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\n> test_expect_success 'valid author header after --root swap' '\n> rebase_setup_and_clean author-header no-conflict-branch &&\n> set_fake_editor &&\n> - FAKE_LINES=\"2 1\" git rebase -i --root &&\n> - git cat-file commit HEAD^ >out &&\n> - grep \"^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n> + git commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n> + git cat-file commit HEAD | grep ^author >expected &&\n> + FAKE_LINES=\"5 1\" git rebase -i --root &&\n> + git cat-file commit HEAD^ | grep ^author >actual &&\n> + test_cmp expected actual\n> +'\n\nIt probably would have been clearer to change to this test to use\ntest_cmp() instead of 'grep' in a separate patch since it's not\ndirectly related to the fixes in this patch, and then to do the\n\"commit --amend\" in this patch. However, probably not worth a re-roll.\n\n> +test_expect_success 'valid author header when author contains single quote' '\n> + rebase_setup_and_clean author-header no-conflict-branch &&\n> + set_fake_editor &&\n> + git commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n> + git cat-file commit HEAD | grep ^author >expected &&\n> + FAKE_LINES=\"2\" git rebase -i HEAD~2 &&\n> + git cat-file commit HEAD | grep ^author >actual &&\n> + test_cmp expected actual\n> '\n\nThis test is so similar to the one above that it is tempting to try to\nrefactor the code so the tests can share implementation, however, the\nend result would probably be less readable, so they're probably fine\nas-is.\n"},{"id":"354794","messageId":"CAPig+cStNhC=+naumnpw0+XzDKZ1Un+pziZ7sa8J0cu6O8pa4Q@mail.gmail.com","threadId":"48989","inReplyTo":"20180807093452.22524-2-phillip.wood@talktalk.net","subject":"Re: [PATCH v4 1/2] sequencer: handle errors from read_author_ident()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-08T09:43:06Z","receivedAt":"2018-08-08T09:43:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n> Check for a NULL return value from read_author_ident() that indicates\n> an error. Previously the NULL author was passed to commit_tree() which\n> would then fallback to using the default author when creating the new\n> commit. This changed the date and potentially the author of the commit\n> which corrupted the author data compared to its expected value.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Phillip Wood \n> ---\n> changes since v3:\n> - Implemented the simpler scheme suggested by Eric\n\nThis iteration is much nicer, much simpler, and much easier to review. Thanks.\n"},{"id":"354795","messageId":"CAPig+cSyaxXv=a1s3nM2nGYj9D09iqgsfPD02o9cjEeWXFhR-A@mail.gmail.com","threadId":"48989","inReplyTo":"20180807093452.22524-1-phillip.wood@talktalk.net","subject":"Re: [PATCH v4 0/2] fix author-script quoting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-08T09:51:13Z","receivedAt":"2018-08-08T09:51:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n> I've updated these based on Eric's suggestions, hopefully they're good\n> to go now. Thanks Eric for you help.\n\nThanks, I left a few comments on patch 2/2. Aside from the '>' vs.\n'>=' issue (over which I lost more than a few minutes cogitating), I\ndon't think there are any blockers this time around.\n\nThere are 2 or 3 things which can be done as follow-ups (indeed, even\nthe '>' vs. '>=' could be so), but I don't think any of them are so\ncritical that they need to be part of this series (for instance, the\nstrbuf leak in the error path has been around for some time, and can\nbe fixed later).\n"},{"id":"354855","messageId":"xmqqr2j8j194.fsf@gitster-ct.c.googlers.com","threadId":"48989","inReplyTo":"CAPig+cQdH+Xiunej1E6EnA6btFzZJTPo27cFY_UdaFZF7J7t-w@mail.gmail.com","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-08T16:01:43Z","receivedAt":"2018-08-08T16:01:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> What does concern me is that read_env_script() doesn't seem to care\n> about such a malformed file; it doesn't do any validation at all.\n> Contrast that with read_author_ident() which is pretty strict about\n> the content it expects to find in the file. So, it might make sense to\n> upgrade read_env_script() to do some sort of validation on each line\n> (though that shouldn't be in this patch, and doesn't even need to be\n> in this series).\n\nI do not think it is within the scope of these bugfix patches, but I\ntend to agree that in the longer term it would be a good idea to\nunify these two helpers that read exactly the same file stored at\nrebase_path_author_script(), and make the result stricter, rather\nthan tightening two helpers independently.\n\n"},{"id":"354980","messageId":"8d2da302-cf48-cc03-6300-a80bd3d67bab@talktalk.net","threadId":"48989","inReplyTo":"xmqqr2j8j194.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-09T10:06:15Z","receivedAt":"2018-08-09T10:06:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\nOn 08/08/18 17:01, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>> What does concern me is that read_env_script() doesn't seem to care\n>> about such a malformed file; it doesn't do any validation at all.\n>> Contrast that with read_author_ident() which is pretty strict about\n>> the content it expects to find in the file. So, it might make sense to\n>> upgrade read_env_script() to do some sort of validation on each line\n>> (though that shouldn't be in this patch, and doesn't even need to be\n>> in this series).\n> \n> I do not think it is within the scope of these bugfix patches, but I\n> tend to agree that in the longer term it would be a good idea to\n> unify these two helpers that read exactly the same file stored at\n> rebase_path_author_script(), and make the result stricter, rather\n> than tightening two helpers independently.\n\nThat's my longer term goal, ideally sharing code with am.\n\nBest Wishes\n\nPhillip\n"},{"id":"354981","messageId":"da635ab4-9175-18aa-ffae-91c4ee8229ec@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cQdH+Xiunej1E6EnA6btFzZJTPo27cFY_UdaFZF7J7t-w@mail.gmail.com","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-09T10:08:40Z","receivedAt":"2018-08-09T10:08:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 08/08/18 09:43, Eric Sunshine wrote:\n> On Tue, Aug 7, 2018 at 9:54 AM Phillip Wood wrote:\n>> On 07/08/18 11:23, Eric Sunshine wrote:\n>>> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n>>>> + if (n > 0 && s[n] != '\\'')\n>>>> + return 1;\n>>>\n>>> To be \"technically correct\", I think the condition in the 'if'\n>>> statement should be \">=\". It should never happen in practice that the\n>>> entire content of the file is a single character followed by zero or\n>>> more newlines, but using the proper condition \">=\" would save future\n>>> readers of this code a \"huh?\" moment.\n>>\n>> I'm not sure it is that simple. If the script consists solely of a\n>> single quote then we should return 1, if it is a single character that\n>> is not \"'\" then it should return 0. Currently it returns 0 in both those\n>> cases so is technically broken when the script is \"'\". If it used \">=\"\n>> instead then I think it would return 0 when it should return 1 and vice\n>> versa. As you say this shouldn't happen in practice.\n> \n> It shouldn't happen in practice, but if a human (power user) edits\n> this file, we shouldn't discount the possibility. However, I'm not so\n> concerned about quoting_is_broken() returning a meaningful answer in\n> this case since we have much bigger problems if we get such a file.\n> (Indeed, what answer could quoting_is_broken() return which would be\n> useful or meaningful for such a malformed file?)\n> \n> What does concern me is that read_env_script() doesn't seem to care\n> about such a malformed file; it doesn't do any validation at all.\n> Contrast that with read_author_ident() which is pretty strict about\n> the content it expects to find in the file. So, it might make sense to\n> upgrade read_env_script() to do some sort of validation on each line\n> (though that shouldn't be in this patch, and doesn't even need to be\n> in this series). For instance, it could check that each line looks\n> something like what would be matched by this regex: /[A-Z0-9_]+='.+'/\n\nI think it should just share some code with get_author_ident() that uses\nsq_dequote(), that's for another day though.\n\n> (And, no, I'm not saying that regex should be used for validation; I'm\n> just using it as an example.)\n> \n> As for '>' vs. '>=', it caused more than a slight hiccup when I was\n> scanning the code, and I worry that future readers could be similarly\n> impacted.\n\nI don't have a strong opinion either way, if Junio wants to fix it up\nI'd be happy with that, but I'm not keen on another iteration just for that.\n\nBest Wishes\n\nPhillip\n\n\n"},{"id":"354982","messageId":"0d74e405-fa8f-ae5c-ed1d-be3220f2b2da@talktalk.net","threadId":"48989","inReplyTo":"CAPig+cR-O07v98SmjhKkXodN=SoSK3ahp2t2zNJ54uo3-_Fq3Q@mail.gmail.com","subject":"Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-09T10:11:48Z","receivedAt":"2018-08-09T10:11:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/08/18 10:39, Eric Sunshine wrote:\n> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood wrote:\n>> Single quotes should be escaped as \\' not \\\\'. The bad quoting breaks\n>> the interactive version of 'rebase --root' (which is used when there\n>> is no '--onto' even if the user does not specify --interactive) for\n>> authors that contain \"'\" as sq_dequote() called by read_author_ident()\n>> errors out on the bad quoting.\n>> [...]\n>> Signed-off-by: Phillip Wood \n>> ---\n>> diff --git a/sequencer.c b/sequencer.c\n>> @@ -636,42 +636,64 @@ static int write_author_script(const char *message)\n>> static int read_env_script(struct argv_array *env)\n>> {\n>> if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)\n>> return -1;\n> \n> Erm, again, not introduced by this patch, but this is leaking 'script'\n> in the error path. You had plugged this leak in the previous round but\n> that fix got lost when you reverted to this simpler approach. Not\n> critical, though; the leak probably ought to be fixed by a separate\n> patch anyhow (which doesn't necessarily need to be part of this\n> series).\n\nI'm hoping this will go away when I work on unifying the code to read\nthe author-script with am.\n\n>> + /* write_author_script() used to quote incorrectly */\n>> + sq_bug = quoting_is_broken(script.buf, script.len);\n>> for (p = script.buf; *p; p++)\n>> - if (skip_prefix(p, \"'\\\\\\\\''\", (const char **)&p2))\n>> + if (sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2))\n>> + strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n>> + else if (skip_prefix(p, \"'\\\\''\", &p2))\n>> strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> \n> The two strbuf_splice() invocations are identical, so an alternate way\n> of expressing this would be:\n> \n> if ((sq_bug && skip_prefix(p, \"'\\\\\\\\''\", &p2)) ||\n> skip_prefix(p, \"'\\\\''\", &p2))\n> strbuf_splice(&script, p - script.buf, p2 - p, \"'\", 1);\n> \n> Not necessarily clearer, and certainly not worth a re-roll.\n> \n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> @@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\n>> test_expect_success 'valid author header after --root swap' '\n>> rebase_setup_and_clean author-header no-conflict-branch &&\n>> set_fake_editor &&\n>> - FAKE_LINES=\"2 1\" git rebase -i --root &&\n>> - git cat-file commit HEAD^ >out &&\n>> - grep \"^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$\" out\n>> + git commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n>> + git cat-file commit HEAD | grep ^author >expected &&\n>> + FAKE_LINES=\"5 1\" git rebase -i --root &&\n>> + git cat-file commit HEAD^ | grep ^author >actual &&\n>> + test_cmp expected actual\n>> +'\n> \n> It probably would have been clearer to change to this test to use\n> test_cmp() instead of 'grep' in a separate patch since it's not\n> directly related to the fixes in this patch, and then to do the\n> \"commit --amend\" in this patch. However, probably not worth a re-roll.\n\nIn hindsight that would have been clearer, but hopefully this version\nisn't too confusing\n\n>> +test_expect_success 'valid author header when author contains single quote' '\n>> + rebase_setup_and_clean author-header no-conflict-branch &&\n>> + set_fake_editor &&\n>> + git commit --amend --author=\"Au ${SQ}thor \" --no-edit &&\n>> + git cat-file commit HEAD | grep ^author >expected &&\n>> + FAKE_LINES=\"2\" git rebase -i HEAD~2 &&\n>> + git cat-file commit HEAD | grep ^author >actual &&\n>> + test_cmp expected actual\n>> '\n> \n> This test is so similar to the one above that it is tempting to try to\n> refactor the code so the tests can share implementation, however, the\n> end result would probably be less readable, so they're probably fine\n> as-is.\n\nYes, I think it could look messy\n\nThank you very much for all your help and comments on this series\n\nBest Wishes\n\nPhillip\n\n"},{"id":"356634","messageId":"nycvar.QRO.7.76.6.1808280033130.73@tvgsbejvaqbjf.bet","threadId":"48989","inReplyTo":"CAPig+cT0ATgRMY-mhttViaburFW37G352BX=mj-Cnr2_wZruaQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] fix \"rebase -i --root\" corrupting root commit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-27T22:34:40Z","receivedAt":"2018-08-27T22:34:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 6 Aug 2018, Eric Sunshine wrote:\n\n> On Mon, Aug 6, 2018 at 9:20 PM Hilco Wijbenga wrote:\n> > But your suggestion did make me think about what behaviour I would\n> > like to see, exactly. I like that Git removes commits that no longer\n> > serve any purpose (because I've included their changes in earlier\n> > commits). So I would not want to keep commits that become empty during\n> > the rebase. What I would like to see is that commits that _start out_\n> > as empty, are retained. Would such behaviour make sense? Or would that\n> > be considered surprising behaviour?\n> \n> I, personally, have no opinion since I don't use empty commits.\n> Perhaps someone more experienced and more long-sighted will chime in.\n\nI got aware about two weeks ago that my mail provider fails to deliver all\nthe mails that are addressed to me. Two days ago, I managed to get this\nmail (and the thread) via public-inbox (thanks, Eric!!!). Hence my late\nreply.\n\nHilco: if you provide a patch to extend the test suite to demonstrate the\nbreakage (via `test_expect_failure`), I promise to try my best to provide\na fix on top.\n\nCiao,\nJohannes\n"}]}