{"thread":{"id":"50546","subject":"[RFC PATCH 5/5] fast-import: fix erroneous handling of get-mark with empty orphan commits","startedAt":"2019-02-20T22:59:02Z","lastAt":"2019-04-01T16:02:58Z","messageCount":15,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"369746","messageId":"20190220225846.10658-6-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[RFC PATCH 5/5] fast-import: fix erroneous handling of get-mark with empty orphan commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:46Z","receivedAt":"2019-02-20T22:59:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When get-mark was introduced in commit 28c7b1f7b7b7 (\"fast-import: add a\nget-mark command\", 2015-07-01), it followed the precedent of the\ncat-blob command to be allowed on any line other than in the middle of a\ndata directive; see commit 777f80d7429b (\"fast-import: Allow cat-blob\nrequests at arbitrary points in stream\", 2010-11-28). It was useful to\nallow cat-blob directives in the middle of a commit to get more data\nthat would be used in writing the current commit object. get-mark is\nnot similarly useful since fast-import can already use either object id\nor mark. Further, trying to allow this command anywhere caused parsing\nbugs. Fix the parsing problems by only allowing get-mark commands to\nappear when other commands have completed.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 4 ----\n fast-import.c | 8 ++------\n t/t9300-fast-import.sh | 4 ++--\n 3 files changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 982f82b0b3..33cce1e150 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -971,10 +971,6 @@ might want to refer to in their commit messages.\n \t'get-mark' SP ':' LF\n ....\n \n-This command can be used anywhere in the stream that comments are\n-accepted. In particular, the `get-mark` command can be used in the\n-middle of a commit but not in the middle of a `data` command.\n-\n See ``Responses To Commands'' below for details about how to read\n this output safely.\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 338db61e6e..064c55e8be 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1748,8 +1748,6 @@ static int read_next_command(void)\n \t}\n \n \tfor (;;) {\n-\t\tconst char *p;\n-\n \t\tif (unread_command_buf) {\n \t\t\tunread_command_buf = 0;\n \t\t} else {\n@@ -1782,10 +1780,6 @@ static int read_next_command(void)\n \t\t\trc->prev->next = rc;\n \t\t\tcmd_tail = rc;\n \t\t}\n-\t\tif (skip_prefix(command_buf.buf, \"get-mark \", &p)) {\n-\t\t\tparse_get_mark(p);\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (command_buf.buf[0] == '#')\n \t\t\tcontinue;\n \t\treturn 0;\n@@ -3318,6 +3312,8 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_ls(v, NULL);\n \t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n \t\t\tparse_cat_blob(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"get-mark \", &v))\n+\t\t\tparse_get_mark(v);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex c304c8c47c..3668263c40 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3276,11 +3276,11 @@ cat >>W-input <<-W_INPUT_END\n \tLFsget-mark :1\n \tW_INPUT_END\n \n-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n \tsed -e s/LFs// W-input | tr L \"\\n\" | git fast-import\n '\n \n-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with one newline' '\n \tsed -e s/LFs/L/ W-input | tr L \"\\n\" | git fast-import\n '\n \n-- \n2.21.0.rc2.5.g8f70af2367\n\n"},{"id":"369747","messageId":"20190220225846.10658-1-newren@gmail.com","threadId":"50546","inReplyTo":null,"subject":"[RFC PATCH 0/5] Fix some fast-import parsing issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:41Z","receivedAt":"2019-02-20T22:59:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"I found a few issues with parsing in fast-import (dating back to\ngit-2.6.0). I was thrown off track for a while by the fact that the\n.git/fast_import_crash_$PID file erroneously claimed that the\npenultimate command it read was somehow truncated; e.g. for input of\nthe form:\n\n commit refs/heads/master\n mark :1\n author Full Name 1000000000 +0100\n committer Full Name 1000000000 +0100\n data 53\n Commit message for intentionally empty orphan commit\n get-mark :1\n \nthe crash file was reporting\n\n Most Recent Commands Before Crash\n ---------------------------------\n commit refs/heads/master\n mark :1\n author Full Name 1000000000 +0100\n committer Full Name 1000000000 +0100\n data 53\n * get-mark :1\n \nIn other words, it dropped the commit message line entirely, which\nmade me wonder if I was having weird buffering or flushing issues\nuntil I was able to narrow it down to the simple testcase above and\ncould duplicate with\n cat seven-line-test-file | git fast-import\nI don't know if there are other ways to get fast-import to give a\ncorrupted \"Most Recent Commands Before Crash\" report, but this one\ndoesn't trigger after my other fixes below.\n\nSwitching gears from the bad reporting to the original parsing bug\nthough, it is clear fast-import somehow mishandled get-mark directives\nafter empty orphan commits. Digging into it, I think it was caused by\nfollowing a precedent set by a somewhat unsafe implementation of\ntrying to allow cat-blob directives to appear in the middle of a\ncommit. My spelunking showed the following commits seemed to be the\nmost relevant as to how we got to the current state:\n\n 777f80d7429b (\"fast-import: Allow cat-blob requests at arbitrary\n points in stream\", 2010-11-28)\n 8dc6a373d201 (\"fast-import: add 'ls' command\", 2010-12-02)\n 97313bef2a16 (\"fast-import: use skip_prefix for parsing input\",\n 2014-06-18)\n 28c7b1f7b7b7 (\"fast-import: add a get-mark command\", 2015-07-01)\n\nI've cc'ed the relevant folks, and have a few patches that fix the\nissue and I think make the parser more robust against future issues in\na way that I think is safe enough for backward compatibility, but\n\"backward compatible enough\" might concern some folks; if so, please\ntake a look at patches 4 and 5.\n\nElijah Newren (5):\n t9300: demonstrate bug with get-mark and empty orphan commits\n git-fast-import.txt: fix wording about where ls command can appear\n fast-import: check most prominent commands first\n fast-import: only allow cat-blob requests where it makes sense\n fast-import: fix erroneous handling of get-mark with empty orphan\n commits\n\n Documentation/git-fast-import.txt | 22 +++++++++---------\n fast-import.c | 31 ++++++++++++++------------\n t/t9300-fast-import.sh | 37 +++++++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 24 deletions(-)\n\n-- \n2.21.0.rc2.5.g8f70af2367\n"},{"id":"369748","messageId":"20190220225846.10658-3-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[RFC PATCH 2/5] git-fast-import.txt: fix wording about where ls command can appear","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:43Z","receivedAt":"2019-02-20T22:59:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The docs claimed `ls` commands could appear almost anywhere, but the\ncode told a different story. Modify the docs to match the code.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 339b6e7e98..f7e2d330b1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -1016,8 +1016,8 @@ printing a blob from the active commit (with `cat-blob`) or copying a\n blob or tree from a previous commit for use in the current one (with\n `filemodify`).\n \n-The `ls` command can be used anywhere in the stream that comments are\n-accepted, including the middle of a commit.\n+The `ls` command can also be used where a `filemodify` directive can\n+appear, allowing it to be used in the middle of a commit.\n \n Reading from the active commit::\n \tThis form can only be used in the middle of a `commit`.\n-- \n2.21.0.rc2.5.g8f70af2367\n\n"},{"id":"369749","messageId":"20190220225846.10658-4-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[RFC PATCH 3/5] fast-import: check most prominent commands first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:44Z","receivedAt":"2019-02-20T22:59:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This is not a very important change, and one that I expect to have no\nperformance impact whatsoever, but reading the code bothered me. The\nparsing of command types in cmd_main() mostly runs in order of most\ncommon to least common commands; sure, it's hard to say for sure what\nthe most common are without some type of study, but it seems fairly\nclear to mark the original four (\"blob\", \"commit\", \"tag\", \"reset\") as\nthe most prominent. Indeed, the parsing for most other commands were\nadded to later in the list. However, when \"ls\" was added, it was stuck\nnear the top of the list, with no rationale for that particular\nlocation. Move it down to later to appease my Tourette's-like internal\ntwitching that its former location was causing.\n\nSigned-off-by: Elijah Newren \n---\n fast-import.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex b7ba755c2b..3114ce17f1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3303,14 +3303,14 @@ int cmd_main(int argc, const char **argv)\n \t\tconst char *v;\n \t\tif (!strcmp(\"blob\", command_buf.buf))\n \t\t\tparse_new_blob();\n-\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n-\t\t\tparse_ls(v, NULL);\n \t\telse if (skip_prefix(command_buf.buf, \"commit \", &v))\n \t\t\tparse_new_commit(v);\n \t\telse if (skip_prefix(command_buf.buf, \"tag \", &v))\n \t\t\tparse_new_tag(v);\n \t\telse if (skip_prefix(command_buf.buf, \"reset \", &v))\n \t\t\tparse_reset_branch(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n+\t\t\tparse_ls(v, NULL);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\n-- \n2.21.0.rc2.5.g8f70af2367\n\n"},{"id":"369750","messageId":"20190220225846.10658-5-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[RFC PATCH 4/5] fast-import: only allow cat-blob requests where it makes sense","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:45Z","receivedAt":"2019-02-20T22:59:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 777f80d7429b (\"fast-import: Allow cat-blob requests at\narbitrary points in stream\", 2010-11-28), fast-import started allowing\ncat-blob commands to appear on the start of any line except in the\nmiddle of a \"data\" command. It could be in the middle of various\ndirectives that were part of a tag command, or in the middle of\ncheckpoints or progresses (each of which allow an optional second empty\nnewline), or even immediately after the mark command of a blob before\nthe data directive appeared (raising the question of what if it used the\nmark for the blob that just barely appeared in the stream that we do not\nyet have the data for). None of these locations make any sense as\nplaces to put cat-blob requests.\n\nThe purpose of this change as stated in that commit message was to\n\n [save] frontends from having to loop over everything they want to\n commit in the next commit and cat-ing the necessary objects in\n advance.\n\nHowever, that can be achieved by simply allowing cat-blob requests to\nappear whenever a filemodify directive is allowed. Further, it avoids\nsetting a bad precedent for other commands to follow (e.g. get-mark); a\nprecedent which caused parsing problems in corner cases.\n\nTechnically, inline filemodify directives add a slight wrinkle in that\nfrontends might want to have cat-blob directives appear after the start\nof the filemodify and before the data directive contained within it. I\nthink it would have been better to disallow such a case (it would be\ntrivial to use cat-blob before the filemodify instead), but since there\nis evidence this was used, for backwards compatibility let's support\nthat case too.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 7 ++++---\n fast-import.c | 19 +++++++++++++------\n 2 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex f7e2d330b1..982f82b0b3 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -1001,9 +1001,10 @@ Output uses the same format as `git cat-file --batch`:\n \t LF\n ====\n \n-This command can be used anywhere in the stream that comments are\n-accepted. In particular, the `cat-blob` command can be used in the\n-middle of a commit but not in the middle of a `data` command.\n+This command can be used where a `filemodify` directive can appear,\n+allowing it to be used in the middle of a commit. For a `filemodify`\n+using an inline directive, it can also appear right before the `data`\n+directive.\n \n See ``Responses To Commands'' below for details about how to read\n this output safely.\ndiff --git a/fast-import.c b/fast-import.c\nindex 3114ce17f1..338db61e6e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1786,10 +1786,6 @@ static int read_next_command(void)\n \t\t\tparse_get_mark(p);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (skip_prefix(command_buf.buf, \"cat-blob \", &p)) {\n-\t\t\tparse_cat_blob(p);\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (command_buf.buf[0] == '#')\n \t\t\tcontinue;\n \t\treturn 0;\n@@ -2254,8 +2250,15 @@ static void file_change_m(const char *p, struct branch *b)\n \t\t\tstrbuf_addstr(&uq, p);\n \t\t\tp = uq.buf;\n \t\t}\n-\t\tread_next_command();\n-\t\tparse_and_store_blob(&last_blob, &oid, 0);\n+\t\twhile (read_next_command() != EOF) {\n+\t\t\tconst char *v;\n+\t\t\tif (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\t\tparse_cat_blob(v);\n+\t\t\telse {\n+\t\t\t\tparse_and_store_blob(&last_blob, &oid, 0);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t} else {\n \t\tenum object_type expected = S_ISDIR(mode) ?\n \t\t\t\t\t\tOBJ_TREE: OBJ_BLOB;\n@@ -2627,6 +2630,8 @@ static void parse_new_commit(const char *arg)\n \t\t\tfile_change_deleteall(b);\n \t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n \t\t\tparse_ls(v, b);\n+\t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\tparse_cat_blob(v);\n \t\telse {\n \t\t\tunread_command_buf = 1;\n \t\t\tbreak;\n@@ -3311,6 +3316,8 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_reset_branch(v);\n \t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n \t\t\tparse_ls(v, NULL);\n+\t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\tparse_cat_blob(v);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\n-- \n2.21.0.rc2.5.g8f70af2367\n\n"},{"id":"369751","messageId":"20190220225846.10658-2-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[RFC PATCH 1/5] t9300: demonstrate bug with get-mark and empty orphan commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-20T22:58:42Z","receivedAt":"2019-02-20T22:59:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 7 +++++-\n t/t9300-fast-import.sh | 37 +++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 43ab3b1637..339b6e7e98 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -422,7 +422,12 @@ However it is recommended that a `filedeleteall` command precede\n all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in\n the same commit, as `filedeleteall` wipes the branch clean (see below).\n \n-The `LF` after the command is optional (it used to be required).\n+The `LF` after the command is optional (it used to be required). Note\n+that for reasons of backward compatibility, if the commit ends with a\n+`data` command (i.e. it has has no `from`, `merge`, `filemodify`,\n+`filedelete`, `filecopy`, `filerename`, `filedeleteall` or\n+`notemodify` commands) then two `LF` commands may appear at the end of\n+the command instead of just one.\n \n `author`\n ^^^^^^^^\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 59a13b6a77..c304c8c47c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3262,4 +3262,41 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n \tbackground_import_still_running\n '\n \n+###\n+### series W (get-mark and empty orphan commits)\n+###\n+\n+cat >>W-input <<-W_INPUT_END\n+\tcommit refs/heads/W-branch\n+\tmark :1\n+\tauthor Full Name 1000000000 +0100\n+\tcommitter Full Name 1000000000 +0100\n+\tdata 27\n+\tIntentionally empty commit\n+\tLFsget-mark :1\n+\tW_INPUT_END\n+\n+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n+\tsed -e s/LFs// W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '\n+\tsed -e s/LFs/L/ W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with ugly second newline' '\n+\t# Technically, this should fail as it has too many linefeeds\n+\t# according to the grammar in fast-import.txt. But, for whatever\n+\t# reason, it works. Since using the correct number of newlines\n+\t# does not work with older (pre-2.22) versions of git, allow apps\n+\t# that used this second-newline workaround to keep working by\n+\t# checking it with this test...\n+\tsed -e s/LFs/LL/ W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with erroneous third newline' '\n+\t# ...but do NOT allow more empty lines than that (see previous test).\n+\tsed -e s/LFs/LLL/ W-input | tr L \"\\n\" | test_must_fail git fast-import\n+'\n+\n test_done\n-- \n2.21.0.rc2.5.g8f70af2367\n\n"},{"id":"370999","messageId":"CABPp-BHp83H1qhrd-j1yvdWz56AwDJogFjf_3iaEDVjFvGansg@mail.gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"Re: [RFC PATCH 0/5] Fix some fast-import parsing issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-03-08T16:53:11Z","receivedAt":"2019-03-08T16:53:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Wed, Feb 20, 2019 at 2:58 PM Elijah Newren wrote:\n>\n> I found a few issues with parsing in fast-import (dating back to\n....\n> I've cc'ed the relevant folks, and have a few patches that fix the\n> issue and I think make the parser more robust against future issues in\n> a way that I think is safe enough for backward compatibility, but\n> \"backward compatible enough\" might concern some folks; if so, please\n> take a look at patches 4 and 5.\n\nJust thought I'd ping to see if folks have any concerns with this\nslight tweak to backward compatibility; if not, I'll just repost the\npatches removing the RFC label.\n"},{"id":"372869","messageId":"xmqqwokevwbb.fsf@gitster-ct.c.googlers.com","threadId":"50546","inReplyTo":"CABPp-BHp83H1qhrd-j1yvdWz56AwDJogFjf_3iaEDVjFvGansg@mail.gmail.com","subject":"Re: [RFC PATCH 0/5] Fix some fast-import parsing issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T10:44:56Z","receivedAt":"2019-04-01T10:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren wrote:\n>>\n>> I found a few issues with parsing in fast-import (dating back to\n> ....\n>> I've cc'ed the relevant folks, and have a few patches that fix the\n>> issue and I think make the parser more robust against future issues in\n>> a way that I think is safe enough for backward compatibility, but\n>> \"backward compatible enough\" might concern some folks; if so, please\n>> take a look at patches 4 and 5.\n>\n> Just thought I'd ping to see if folks have any concerns with this\n> slight tweak to backward compatibility; if not, I'll just repost the\n> patches removing the RFC label.\n\nIt's been more than a month and we haven't hard from anybody,\nperhaps?\n"},{"id":"372898","messageId":"CABPp-BEHiYddGzVfuQoiN403na2dOSXBbnJDffp3pPsEqQ2Pgw@mail.gmail.com","threadId":"50546","inReplyTo":"xmqqwokevwbb.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 0/5] Fix some fast-import parsing issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T15:52:48Z","receivedAt":"2019-04-01T15:53:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Apr 1, 2019 at 3:44 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren wrote:\n> >>\n> >> I found a few issues with parsing in fast-import (dating back to\n> > ....\n> >> I've cc'ed the relevant folks, and have a few patches that fix the\n> >> issue and I think make the parser more robust against future issues in\n> >> a way that I think is safe enough for backward compatibility, but\n> >> \"backward compatible enough\" might concern some folks; if so, please\n> >> take a look at patches 4 and 5.\n> >\n> > Just thought I'd ping to see if folks have any concerns with this\n> > slight tweak to backward compatibility; if not, I'll just repost the\n> > patches removing the RFC label.\n>\n> It's been more than a month and we haven't hard from anybody,\n> perhaps?\n\nIndeed; I was going to resend sooner but got wrapped up in\ndirectory-rename-conflict-by-default, switch and restore series\ndiscussions, etc. I'll rip off the RFC label and resend.\n"},{"id":"372899","messageId":"20190401160011.20678-1-newren@gmail.com","threadId":"50546","inReplyTo":"20190220225846.10658-1-newren@gmail.com","subject":"[PATCH v2 0/5] Fix some fast-import parsing issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:06Z","receivedAt":"2019-04-01T16:00:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series fixes a few issues with parsing in fast-import, dating back to\ngit-2.6.0. The only change since v1 is removing the RFC label; sadly, no\none commented on the first round.\n\nText from my original submission:\n\nThe point behind this series is that fast-import somehow mishandled\nget-mark directives after empty orphan commits. Digging into it, I\nthink it was caused by following a precedent set by a somewhat unsafe\nimplementation of trying to allow cat-blob directives to appear in the\nmiddle of a commit. My spelunking showed the following commits seemed\nto be the most relevant as to how we got to the current state:\n\n 777f80d7429b (\"fast-import: Allow cat-blob requests at arbitrary\n points in stream\", 2010-11-28)\n 8dc6a373d201 (\"fast-import: add 'ls' command\", 2010-12-02)\n 97313bef2a16 (\"fast-import: use skip_prefix for parsing input\",\n 2014-06-18)\n 28c7b1f7b7b7 (\"fast-import: add a get-mark command\", 2015-07-01)\n\nI have a few patches that fix the issue and I think make the parser\nmore robust against future issues in a way that I think is safe enough\nfor backward compatibility, but \"backward compatible enough\" might\nconcern some folks; if so, please take a look at patches 4 and 5.\n\nElijah Newren (5):\n t9300: demonstrate bug with get-mark and empty orphan commits\n git-fast-import.txt: fix wording about where ls command can appear\n fast-import: check most prominent commands first\n fast-import: only allow cat-blob requests where it makes sense\n fast-import: fix erroneous handling of get-mark with empty orphan\n commits\n\n Documentation/git-fast-import.txt | 22 +++++++++---------\n fast-import.c | 31 ++++++++++++++------------\n t/t9300-fast-import.sh | 37 +++++++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 24 deletions(-)\n\n-- \n2.21.0.1.g0a561c1dbd\n\n"},{"id":"372900","messageId":"20190401160011.20678-2-newren@gmail.com","threadId":"50546","inReplyTo":"20190401160011.20678-1-newren@gmail.com","subject":"[PATCH v2 1/5] t9300: demonstrate bug with get-mark and empty orphan commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:07Z","receivedAt":"2019-04-01T16:00:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 7 +++++-\n t/t9300-fast-import.sh | 37 +++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 43ab3b1637..339b6e7e98 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -422,7 +422,12 @@ However it is recommended that a `filedeleteall` command precede\n all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in\n the same commit, as `filedeleteall` wipes the branch clean (see below).\n \n-The `LF` after the command is optional (it used to be required).\n+The `LF` after the command is optional (it used to be required). Note\n+that for reasons of backward compatibility, if the commit ends with a\n+`data` command (i.e. it has has no `from`, `merge`, `filemodify`,\n+`filedelete`, `filecopy`, `filerename`, `filedeleteall` or\n+`notemodify` commands) then two `LF` commands may appear at the end of\n+the command instead of just one.\n \n `author`\n ^^^^^^^^\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 59a13b6a77..c304c8c47c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3262,4 +3262,41 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n \tbackground_import_still_running\n '\n \n+###\n+### series W (get-mark and empty orphan commits)\n+###\n+\n+cat >>W-input <<-W_INPUT_END\n+\tcommit refs/heads/W-branch\n+\tmark :1\n+\tauthor Full Name 1000000000 +0100\n+\tcommitter Full Name 1000000000 +0100\n+\tdata 27\n+\tIntentionally empty commit\n+\tLFsget-mark :1\n+\tW_INPUT_END\n+\n+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n+\tsed -e s/LFs// W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '\n+\tsed -e s/LFs/L/ W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with ugly second newline' '\n+\t# Technically, this should fail as it has too many linefeeds\n+\t# according to the grammar in fast-import.txt. But, for whatever\n+\t# reason, it works. Since using the correct number of newlines\n+\t# does not work with older (pre-2.22) versions of git, allow apps\n+\t# that used this second-newline workaround to keep working by\n+\t# checking it with this test...\n+\tsed -e s/LFs/LL/ W-input | tr L \"\\n\" | git fast-import\n+'\n+\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with erroneous third newline' '\n+\t# ...but do NOT allow more empty lines than that (see previous test).\n+\tsed -e s/LFs/LLL/ W-input | tr L \"\\n\" | test_must_fail git fast-import\n+'\n+\n test_done\n-- \n2.21.0.1.g0a561c1dbd\n\n"},{"id":"372901","messageId":"20190401160011.20678-6-newren@gmail.com","threadId":"50546","inReplyTo":"20190401160011.20678-1-newren@gmail.com","subject":"[PATCH v2 5/5] fast-import: fix erroneous handling of get-mark with empty orphan commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:11Z","receivedAt":"2019-04-01T16:00:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When get-mark was introduced in commit 28c7b1f7b7b7 (\"fast-import: add a\nget-mark command\", 2015-07-01), it followed the precedent of the\ncat-blob command to be allowed on any line other than in the middle of a\ndata directive; see commit 777f80d7429b (\"fast-import: Allow cat-blob\nrequests at arbitrary points in stream\", 2010-11-28). It was useful to\nallow cat-blob directives in the middle of a commit to get more data\nthat would be used in writing the current commit object. get-mark is\nnot similarly useful since fast-import can already use either object id\nor mark. Further, trying to allow this command anywhere caused parsing\nbugs. Fix the parsing problems by only allowing get-mark commands to\nappear when other commands have completed.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 4 ----\n fast-import.c | 8 ++------\n t/t9300-fast-import.sh | 4 ++--\n 3 files changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 982f82b0b3..33cce1e150 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -971,10 +971,6 @@ might want to refer to in their commit messages.\n \t'get-mark' SP ':' LF\n ....\n \n-This command can be used anywhere in the stream that comments are\n-accepted. In particular, the `get-mark` command can be used in the\n-middle of a commit but not in the middle of a `data` command.\n-\n See ``Responses To Commands'' below for details about how to read\n this output safely.\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 338db61e6e..064c55e8be 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1748,8 +1748,6 @@ static int read_next_command(void)\n \t}\n \n \tfor (;;) {\n-\t\tconst char *p;\n-\n \t\tif (unread_command_buf) {\n \t\t\tunread_command_buf = 0;\n \t\t} else {\n@@ -1782,10 +1780,6 @@ static int read_next_command(void)\n \t\t\trc->prev->next = rc;\n \t\t\tcmd_tail = rc;\n \t\t}\n-\t\tif (skip_prefix(command_buf.buf, \"get-mark \", &p)) {\n-\t\t\tparse_get_mark(p);\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (command_buf.buf[0] == '#')\n \t\t\tcontinue;\n \t\treturn 0;\n@@ -3318,6 +3312,8 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_ls(v, NULL);\n \t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n \t\t\tparse_cat_blob(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"get-mark \", &v))\n+\t\t\tparse_get_mark(v);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex c304c8c47c..3668263c40 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3276,11 +3276,11 @@ cat >>W-input <<-W_INPUT_END\n \tLFsget-mark :1\n \tW_INPUT_END\n \n-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with no newlines' '\n \tsed -e s/LFs// W-input | tr L \"\\n\" | git fast-import\n '\n \n-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '\n+test_expect_success !MINGW 'W: get-mark & empty orphan commit with one newline' '\n \tsed -e s/LFs/L/ W-input | tr L \"\\n\" | git fast-import\n '\n \n-- \n2.21.0.1.g0a561c1dbd\n\n"},{"id":"372902","messageId":"20190401160011.20678-4-newren@gmail.com","threadId":"50546","inReplyTo":"20190401160011.20678-1-newren@gmail.com","subject":"[PATCH v2 3/5] fast-import: check most prominent commands first","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:09Z","receivedAt":"2019-04-01T16:00:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This is not a very important change, and one that I expect to have no\nperformance impact whatsoever, but reading the code bothered me. The\nparsing of command types in cmd_main() mostly runs in order of most\ncommon to least common commands; sure, it's hard to say for sure what\nthe most common are without some type of study, but it seems fairly\nclear to mark the original four (\"blob\", \"commit\", \"tag\", \"reset\") as\nthe most prominent. Indeed, the parsing for most other commands were\nadded to later in the list. However, when \"ls\" was added, it was stuck\nnear the top of the list, with no rationale for that particular\nlocation. Move it down to later to appease my Tourette's-like internal\ntwitching that its former location was causing.\n\nSigned-off-by: Elijah Newren \n---\n fast-import.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex b7ba755c2b..3114ce17f1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3303,14 +3303,14 @@ int cmd_main(int argc, const char **argv)\n \t\tconst char *v;\n \t\tif (!strcmp(\"blob\", command_buf.buf))\n \t\t\tparse_new_blob();\n-\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n-\t\t\tparse_ls(v, NULL);\n \t\telse if (skip_prefix(command_buf.buf, \"commit \", &v))\n \t\t\tparse_new_commit(v);\n \t\telse if (skip_prefix(command_buf.buf, \"tag \", &v))\n \t\t\tparse_new_tag(v);\n \t\telse if (skip_prefix(command_buf.buf, \"reset \", &v))\n \t\t\tparse_reset_branch(v);\n+\t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n+\t\t\tparse_ls(v, NULL);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\n-- \n2.21.0.1.g0a561c1dbd\n\n"},{"id":"372903","messageId":"20190401160011.20678-5-newren@gmail.com","threadId":"50546","inReplyTo":"20190401160011.20678-1-newren@gmail.com","subject":"[PATCH v2 4/5] fast-import: only allow cat-blob requests where it makes sense","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:10Z","receivedAt":"2019-04-01T16:01:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 777f80d7429b (\"fast-import: Allow cat-blob requests at\narbitrary points in stream\", 2010-11-28), fast-import started allowing\ncat-blob commands to appear on the start of any line except in the\nmiddle of a \"data\" command. It could be in the middle of various\ndirectives that were part of a tag command, or in the middle of\ncheckpoints or progresses (each of which allow an optional second empty\nnewline), or even immediately after the mark command of a blob before\nthe data directive appeared (raising the question of what if it used the\nmark for the blob that just barely appeared in the stream that we do not\nyet have the data for). None of these locations make any sense as\nplaces to put cat-blob requests.\n\nThe purpose of this change as stated in that commit message was to\n\n [save] frontends from having to loop over everything they want to\n commit in the next commit and cat-ing the necessary objects in\n advance.\n\nHowever, that can be achieved by simply allowing cat-blob requests to\nappear whenever a filemodify directive is allowed. Further, it avoids\nsetting a bad precedent for other commands to follow (e.g. get-mark); a\nprecedent which caused parsing problems in corner cases.\n\nTechnically, inline filemodify directives add a slight wrinkle in that\nfrontends might want to have cat-blob directives appear after the start\nof the filemodify and before the data directive contained within it. I\nthink it would have been better to disallow such a case (it would be\ntrivial to use cat-blob before the filemodify instead), but since there\nis evidence this was used, for backwards compatibility let's support\nthat case too.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 7 ++++---\n fast-import.c | 19 +++++++++++++------\n 2 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex f7e2d330b1..982f82b0b3 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -1001,9 +1001,10 @@ Output uses the same format as `git cat-file --batch`:\n \t LF\n ====\n \n-This command can be used anywhere in the stream that comments are\n-accepted. In particular, the `cat-blob` command can be used in the\n-middle of a commit but not in the middle of a `data` command.\n+This command can be used where a `filemodify` directive can appear,\n+allowing it to be used in the middle of a commit. For a `filemodify`\n+using an inline directive, it can also appear right before the `data`\n+directive.\n \n See ``Responses To Commands'' below for details about how to read\n this output safely.\ndiff --git a/fast-import.c b/fast-import.c\nindex 3114ce17f1..338db61e6e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1786,10 +1786,6 @@ static int read_next_command(void)\n \t\t\tparse_get_mark(p);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (skip_prefix(command_buf.buf, \"cat-blob \", &p)) {\n-\t\t\tparse_cat_blob(p);\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (command_buf.buf[0] == '#')\n \t\t\tcontinue;\n \t\treturn 0;\n@@ -2254,8 +2250,15 @@ static void file_change_m(const char *p, struct branch *b)\n \t\t\tstrbuf_addstr(&uq, p);\n \t\t\tp = uq.buf;\n \t\t}\n-\t\tread_next_command();\n-\t\tparse_and_store_blob(&last_blob, &oid, 0);\n+\t\twhile (read_next_command() != EOF) {\n+\t\t\tconst char *v;\n+\t\t\tif (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\t\tparse_cat_blob(v);\n+\t\t\telse {\n+\t\t\t\tparse_and_store_blob(&last_blob, &oid, 0);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t} else {\n \t\tenum object_type expected = S_ISDIR(mode) ?\n \t\t\t\t\t\tOBJ_TREE: OBJ_BLOB;\n@@ -2627,6 +2630,8 @@ static void parse_new_commit(const char *arg)\n \t\t\tfile_change_deleteall(b);\n \t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n \t\t\tparse_ls(v, b);\n+\t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\tparse_cat_blob(v);\n \t\telse {\n \t\t\tunread_command_buf = 1;\n \t\t\tbreak;\n@@ -3311,6 +3316,8 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_reset_branch(v);\n \t\telse if (skip_prefix(command_buf.buf, \"ls \", &v))\n \t\t\tparse_ls(v, NULL);\n+\t\telse if (skip_prefix(command_buf.buf, \"cat-blob \", &v))\n+\t\t\tparse_cat_blob(v);\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n \t\telse if (!strcmp(\"done\", command_buf.buf))\n-- \n2.21.0.1.g0a561c1dbd\n\n"},{"id":"372904","messageId":"20190401160011.20678-3-newren@gmail.com","threadId":"50546","inReplyTo":"20190401160011.20678-1-newren@gmail.com","subject":"[PATCH v2 2/5] git-fast-import.txt: fix wording about where ls command can appear","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-04-01T16:00:08Z","receivedAt":"2019-04-01T16:02:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The docs claimed `ls` commands could appear almost anywhere, but the\ncode told a different story. Modify the docs to match the code.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-fast-import.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 339b6e7e98..f7e2d330b1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -1016,8 +1016,8 @@ printing a blob from the active commit (with `cat-blob`) or copying a\n blob or tree from a previous commit for use in the current one (with\n `filemodify`).\n \n-The `ls` command can be used anywhere in the stream that comments are\n-accepted, including the middle of a commit.\n+The `ls` command can also be used where a `filemodify` directive can\n+appear, allowing it to be used in the middle of a commit.\n \n Reading from the active commit::\n \tThis form can only be used in the middle of a `commit`.\n-- \n2.21.0.1.g0a561c1dbd\n\n"}]}