{"thread":{"id":"30123","subject":"[PATCH] fast-import: catch garbage after marks in from/merge","startedAt":"2012-04-01T22:54:07Z","lastAt":"2012-04-10T21:40:49Z","messageCount":22,"participants":["Pete Wyckoff","Jonathan Nieder","Dmitry Ivankov","Junio C Hamano","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188288","messageId":"20120401225407.GA12127@padd.com","threadId":"30123","inReplyTo":null,"subject":"[PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-01T22:54:07Z","receivedAt":"2012-04-01T22:54:07Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"A forgotten LF can lead to a confusing bug. The last\nline in this commit command is wrong:\n\n commit refs/heads/S2\n committer Name 1112912893 -0400\n data <\n---\nI spent too long tracking down the bug described in the\ncommit message. It might help future users if fast-import\nwere to complain in this case.\n\n fast-import.c | 24 +++++++++--\n t/t9300-fast-import.sh | 104 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 124 insertions(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex a85275d..13001bb 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2537,8 +2537,16 @@ static int parse_from(struct branch *b)\n \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n \t} else if (*from == ':') {\n-\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n-\t\tstruct object_entry *oe = find_mark(idnum);\n+\t\tchar *eptr;\n+\t\tuintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n+\t\tstruct object_entry *oe;\n+\t\tif (eptr) {\n+\t\t\tfor (; *eptr && isspace(*eptr); eptr++) ;\n+\t\t\tif (*eptr)\n+\t\t\t\tdie(\"Garbage after mark: %s\",\n+\t\t\t\t command_buf.buf);\n+\t\t}\n+\t\toe = find_mark(idnum);\n \t\tif (oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n \t\thashcpy(b->sha1, oe->idx.sha1);\n@@ -2572,8 +2580,16 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\tif (s)\n \t\t\thashcpy(n->sha1, s->sha1);\n \t\telse if (*from == ':') {\n-\t\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n-\t\t\tstruct object_entry *oe = find_mark(idnum);\n+\t\t\tchar *eptr;\n+\t\t\tuintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n+\t\t\tstruct object_entry *oe;\n+\t\t\tif (eptr) {\n+\t\t\t\tfor (; *eptr && isspace(*eptr); eptr++) ;\n+\t\t\t\tif (*eptr)\n+\t\t\t\t\tdie(\"Garbage after mark: %s\",\n+\t\t\t\t\t command_buf.buf);\n+\t\t\t}\n+\t\t\toe = find_mark(idnum);\n \t\t\tif (oe->type != OBJ_COMMIT)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n \t\t\thashcpy(n->sha1, oe->idx.sha1);\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 0f5b5e5..46346cd 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2635,4 +2635,108 @@ test_expect_success \\\n \t'n=$(grep $a verify | wc -l) &&\n \t test 1 = $n'\n \n+###\n+### series S\n+###\n+#\n+# Set up 1, 2, 3 of this:\n+#\n+# 1--2--4\n+# \\ /\n+# -3-\n+#\n+# Then typo the merge to create #4, make sure it complains.\n+#\n+test_tick\n+cat >input < $GIT_COMMITTER_DATE\n+data < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data < from :1M 100644 :103 hello.c\n>\n> It is missing a newline and should be:\n>\n> from :1\n> M 100644 :103 hello.c\n\nGood idea; thanks.\n\nI agree that this at least deserves a warning and probably should\nerror out.\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2537,8 +2537,16 @@ static int parse_from(struct branch *b)\n> \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n> \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n> \t} else if (*from == ':') {\n> -\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n> -\t\tstruct object_entry *oe = find_mark(idnum);\n> +\t\tchar *eptr;\n> +\t\tuintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n> +\t\tstruct object_entry *oe;\n> +\t\tif (eptr) {\n> +\t\t\tfor (; *eptr && isspace(*eptr); eptr++) ;\n> +\t\t\tif (*eptr)\n> +\t\t\t\tdie(\"Garbage after mark: %s\",\n\nThe implementation seems more complicated than it needs to be. Why\nallow whitespace after the mark number?\n\nCurious,\nJonathan\n"},{"id":"188293","messageId":"20120402001354.GA12651@padd.com","threadId":"30123","inReplyTo":"20120401231259.GE20883@burratino","subject":"Re: [PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-02T00:13:54Z","receivedAt":"2012-04-02T00:13:54Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"jrnieder@gmail.com wrote on Sun, 01 Apr 2012 18:12 -0500:\n> Hi Pete,\n> \n> Pete Wyckoff wrote:\n> \n> > from :1M 100644 :103 hello.c\n> >\n> > It is missing a newline and should be:\n> >\n> > from :1\n> > M 100644 :103 hello.c\n> \n> Good idea; thanks.\n> \n> I agree that this at least deserves a warning and probably should\n> error out.\n> \n> [...]\n> > --- a/fast-import.c\n> > +++ b/fast-import.c\n> > @@ -2537,8 +2537,16 @@ static int parse_from(struct branch *b)\n> > \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n> > \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n> > \t} else if (*from == ':') {\n> > -\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n> > -\t\tstruct object_entry *oe = find_mark(idnum);\n> > +\t\tchar *eptr;\n> > +\t\tuintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n> > +\t\tstruct object_entry *oe;\n> > +\t\tif (eptr) {\n> > +\t\t\tfor (; *eptr && isspace(*eptr); eptr++) ;\n> > +\t\t\tif (*eptr)\n> > +\t\t\t\tdie(\"Garbage after mark: %s\",\n> \n> The implementation seems more complicated than it needs to be. Why\n> allow whitespace after the mark number?\n\nFear of breaking existing fast-import users that might happen\nto have stray whitespace, or \\r\\n terminators.\n\nOther similar fast-import are less forgiving, such as\nparse_cat_blob. Maybe we should generalize and enforce its\napproach to parsing marks.\n\n\t\t-- Pete\n"},{"id":"188296","messageId":"CA+gfSn_8J-HzNjLMi2fXn1XQNA9wx3EVuiseq3pjy0nP-odb5A@mail.gmail.com","threadId":"30123","inReplyTo":"20120402001354.GA12651@padd.com","subject":"Re: [PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-04-02T06:56:40Z","receivedAt":"2012-04-02T06:56:40Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Mon, Apr 2, 2012 at 6:13 AM, Pete Wyckoff wrote:\n> jrnieder@gmail.com wrote on Sun, 01 Apr 2012 18:12 -0500:\n>> Hi Pete,\n>>\n>> Pete Wyckoff wrote:\n>>\n>> >     from :1M 100644 :103 hello.c\n>> >\n>> > It is missing a newline and should be:\n>> >\n>> >     from :1\n>> >     M 100644 :103 hello.c\n>>\n>> Good idea; thanks.\n>>\n>> I agree that this at least deserves a warning and probably should\n>> error out.\n>>\n>> [...]\n>> > --- a/fast-import.c\n>> > +++ b/fast-import.c\n>> > @@ -2537,8 +2537,16 @@ static int parse_from(struct branch *b)\n>> >             hashcpy(b->branch_tree.versions[0].sha1, t);\n>> >             hashcpy(b->branch_tree.versions[1].sha1, t);\n>> >     } else if (*from == ':') {\n>> > -           uintmax_t idnum = strtoumax(from + 1, NULL, 10);\n>> > -           struct object_entry *oe = find_mark(idnum);\n>> > +           char *eptr;\n>> > +           uintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n>> > +           struct object_entry *oe;\n>> > +           if (eptr) {\n>> > +                   for (; *eptr && isspace(*eptr); eptr++) ;\n>> > +                   if (*eptr)\n>> > +                           die(\"Garbage after mark: %s\",\n>>\n>> The implementation seems more complicated than it needs to be.  Why\n>> allow whitespace after the mark number?\n>\n> Fear of breaking existing fast-import users that might happen\n> to have stray whitespace, or \\r\\n terminators.\n>\n> Other similar fast-import are less forgiving, such as\n> parse_cat_blob.  Maybe we should generalize and enforce its\n> approach to parsing marks.\n\nDocs say that \"fast-import is very strict about its input\", so\nprobably it is ok to both deny trailing spaces and fix all other\nstrtoumax()-es.\n\n>\n>                -- Pete\n"},{"id":"188314","messageId":"20120402154356.GD3365@burratino","threadId":"30123","inReplyTo":"20120402001354.GA12651@padd.com","subject":"Re: [PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-02T15:43:56Z","receivedAt":"2012-04-02T15:43:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Pete Wyckoff wrote:\n> jrnieder@gmail.com wrote on Sun, 01 Apr 2012 18:12 -0500:\n\n>> Why allow whitespace after the mark number?\n>\n> Fear of breaking existing fast-import users that might happen\n> to have stray whitespace, or \\r\\n terminators.\n\nOh. I think worrying about that sort of thing is a very good thing.\nIn particular, the possibility of \\r\\n terminators from an importer\nthat writes its output in text mode on Windows is somewhat scary.\n\nLuckily such a problem would be caught as soon as the importer uses\na filemodify (M) command:\n\n - if it quotes the path:\n\n\tfatal: Garbage after path in: M ...\n\n - if it doesn't quote the path, the resulting import would use\n filenames with trailing CRs.\n\nSo we narrowly escape that problem.\n\nOther types of stray whitespace at eol seem less likely but it's\nprobably worth letting a patch like this incubate in the \"next\" branch\nfor a little while to see if anyone complains.\n\nThe reason to care instead of just being permissive is that git\nfast-import is not the only fast-import backend. The tighter we can\nmake the parsing without breaking existing frontends, the better the\nchance that a frontend that was only tested against git fast-import\nwill be usable against all other backends, too.\n\nIn other words, there are two requirements in tension here\n(compatibility with sloppy frontends, compatibility with fussy\nbackends). Some day fast-import might want to learn a --permissive\noption to reconcile them. In this particular case being strict\nunconditionally seems safe enough.\n\nMy only other hint is that as Dmitry mentioned it would be nice to\nnice to use the same logic for other places where fast-import parses a\nnumber at the end of a line, so people don't keep on making the same\nprint vs println mistake and sending new patches to detect it in each\nplace one at a time.\n\nSane?\n\nThanks,\nJonathan\n"},{"id":"188319","messageId":"7v398mhzyz.fsf@alter.siamese.dyndns.org","threadId":"30123","inReplyTo":"20120401225407.GA12127@padd.com","subject":"Re: [PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T16:15:32Z","receivedAt":"2012-04-02T16:15:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pete Wyckoff writes:\n\n> A forgotten LF can lead to a confusing bug. The last\n> line in this commit command is wrong:\n> ...\n> It is missing a newline and should be:\n>\n> from :1\n> M 100644 :103 hello.c\n\nDuring my first reading of this, this introductory part made me think \"oh,\nso this is a patch to fix somebody who produces a wrong data that is fed\nto fast-import\". But it does not seem to be the case.\n\nPlease rephrase the first sentence. Is it a confusing _bug_, or the\nprogram produces a garbage output when fed a garbage output?\n\n> Make fast-import complain about the buggy input, for both\n> from and merge lines that use marks.\n\nPerhaps these two lines, negated to state the current behaviour e.g. \"git\nfast-import does not complain when a mark that is used in 'from' or\n'merge' command to name a commit is not followed by the mandatory LF.\" can\nbe used to replace the first sentence, followed by \"It does X\" to describe\nthe user-observable breakage for a bonus point.\n\n> Signed-off-by: Pete Wyckoff \n> ---\n> I spent too long tracking down the bug described in the\n> commit message. It might help future users if fast-import\n> were to complain in this case.\n\nIt would help future users if the commit log actually described the\nsymptom caused by the bug; otherwise, future users would not notice when\nhitting the same issue.\n\n> diff --git a/fast-import.c b/fast-import.c\n> index a85275d..13001bb 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2537,8 +2537,16 @@ static int parse_from(struct branch *b)\n> \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n> \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n> \t} else if (*from == ':') {\n> -\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n> -\t\tstruct object_entry *oe = find_mark(idnum);\n> +\t\tchar *eptr;\n> +\t\tuintmax_t idnum = strtoumax(from + 1, &eptr, 10);\n> +\t\tstruct object_entry *oe;\n> +\t\tif (eptr) {\n> +\t\t\tfor (; *eptr && isspace(*eptr); eptr++) ;\n\nPut the empty body on a separate line, i.e.\n\n\t\t\tfor ( ; isspace(*eptr); eptr++)\n\t\t\t\t; /* nothing */\n\n> +\t\t\tif (*eptr)\n> +\t\t\t\tdie(\"Garbage after mark: %s\",\n> +\t\t\t\t command_buf.buf);\n\nGood.\n\n> +\t\t}\n> +\t\toe = find_mark(idnum);\n> \t\tif (oe->type != OBJ_COMMIT)\n> \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n\nWould it help future callers if you made this small part that parses a\nmark into a separate small helper function that returns an oe and\nincrement the pointer so that the caller can peek at the terminating\ncharacter to enforce the syntax? E.g.\n\n\t} else if (*from == ':') {\n\t\tchar *cp = from + 1;\n\t\tstruct object_entry *oe = parse_mark(&cp);\n\t\tif (*cp)\n\t\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n if (!oe || oe->type != OBJ_COMMIT)\n \tdie(\"No such commit: %s\", command_buf.buf);\n\t}\n"},{"id":"188320","messageId":"7vy5qegld8.fsf@alter.siamese.dyndns.org","threadId":"30123","inReplyTo":"CA+gfSn_8J-HzNjLMi2fXn1XQNA9wx3EVuiseq3pjy0nP-odb5A@mail.gmail.com","subject":"Re: [PATCH] fast-import: catch garbage after marks in from/merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T16:16:19Z","receivedAt":"2012-04-02T16:16:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Ivankov writes:\n\n>> Other similar fast-import are less forgiving, such as\n>> parse_cat_blob. Maybe we should generalize and enforce its\n>> approach to parsing marks.\n>\n> Docs say that \"fast-import is very strict about its input\", so\n> probably it is ok to both deny trailing spaces and fix all other\n> strtoumax()-es.\n\nConcurred.\n"},{"id":"188374","messageId":"1333417910-17955-1-git-send-email-pw@padd.com","threadId":"30123","inReplyTo":"20120401225407.GA12127@padd.com","subject":"[PATCHv2 0/2] fast-import: tighten parsing of mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-03T01:51:48Z","receivedAt":"2012-04-03T01:51:48Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"Thanks Dmitry, Jonathan and Junio for the comments. I'm happy to\nhave fast-import be strict about its format, and have added code\nand tests that demand exactly one space, or an end-of-line, as\nnecessary. I also made sure the other error messages involved\nwith parsing datarefs are correct.\n\nJonathan, good observation on CRLF users. If we did want to\ncater to them, doing it centrally in read_next_command() would be\nthe way to go. But why bother.\n\nRegarding fixing up all end-of-line number parsing, I think the\nonly other one is dates. Both \"raw\" and \"now\" check for garbage\nat end-of-line, but \"rfc2822\" uses a generic function that\naccepts junk. I'm not motivated to add a lot of code to fix that\ncorner case.\n\nJunio, I made the commit message more clear. For the idea of\ncombining find_mark + parse_mark, that isn't general enough for\nall users. This is the construct used in many places:\n\n oe = find_mark(parse_mark_ref_space(p, &x));\n\nAlso, I did the unit tests first, to make sure things were broken\nas I expected. You can squash it all together if you prefer.\n\n\t\t-- Pete\n\nPete Wyckoff (2):\n fast-import: test behavior of garbage after mark references\n fast-import: tighten parsing of mark references\n\n fast-import.c | 97 ++++++++++++++----\n t/t9300-fast-import.sh | 267 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 342 insertions(+), 22 deletions(-)\n\n-- \n1.7.10.rc2.2.g38670\n"},{"id":"188375","messageId":"1333417910-17955-2-git-send-email-pw@padd.com","threadId":"30123","inReplyTo":"1333417910-17955-1-git-send-email-pw@padd.com","subject":"[PATCHv2 1/2] fast-import: test behavior of garbage after mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-03T01:51:49Z","receivedAt":"2012-04-03T01:51:49Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"Add 15 tests to see what happens when extra characters\nappear after a mark reference, in all places that take marks.\nTen of these fail.\n\nSigned-off-by: Pete Wyckoff \n---\n t/t9300-fast-import.sh | 267 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 267 insertions(+)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 0f5b5e5..621f02a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2635,4 +2635,271 @@ test_expect_success \\\n \t'n=$(grep $a verify | wc -l) &&\n \t test 1 = $n'\n \n+###\n+### series S\n+###\n+#\n+# Set up is roughly this. Commits marked 1,2,3,4. Blobs\n+# marked 100 + commit. Notes 200 +. Make sure missing spaces\n+# and EOLs after mark references cause errors.\n+#\n+# 1--2--4\n+# \\ /\n+# -3-\n+#\n+test_tick\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data < $GIT_COMMITTER_DATE\n+data <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/Snotes\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err\n+\tcommit refs/heads/S2\n+\tmark :3\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tmark :4\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\ttag refs/tags/Stag\n+\tfrom :2x\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcat-blob :2x\n+\tEOF\n+\tcat err &&\n+\tgrep -q \"Garbage after mark\" err\n+'\n+\n+#\n+# ls markref\n+#\n+test_expect_failure 'S: ls markref space' '\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls :2x hello.c\n+\tEOF\n+\tcat err &&\n+\tgrep -q \"Missing space after mark\" err\n+'\n+\n+test_expect_failure 'S: ls sha1 space' '\n+\tsha1=$(grep -w :2 marks | cut -d\\ -f2) &&\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls ${sha1}x hello.c\n+\tEOF\n+\tcat err &&\n+\tgrep -q \"Missing space after SHA1\" err\n+'\n+\n test_done\n-- \n1.7.10.rc2.2.g38670\n"},{"id":"188376","messageId":"1333417910-17955-3-git-send-email-pw@padd.com","threadId":"30123","inReplyTo":"1333417910-17955-1-git-send-email-pw@padd.com","subject":"[PATCHv2 2/2] fast-import: tighten parsing of mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-03T01:51:50Z","receivedAt":"2012-04-03T01:51:50Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"The syntax for the use of mark references in fast-import\ndemands either a SP (space) or LF (end-of-line) after\na mark reference. Fast-import does not complain when garbage\nappears after a mark reference in some cases.\n\nFactor out parsing of mark references and complain if\nerrant characters are found.\n\nBuggy input can cause fast-import to produce the wrong output,\nsilently, without error. This makes it difficult to track\ndown buggy generators of fast-import streams. An example is\nseen in the last line of this commit command:\n\n commit refs/heads/S2\n committer Name 1112912893 -0400\n data <\n---\n fast-import.c | 97 +++++++++++++++++++++++++++++++++++++-----------\n t/t9300-fast-import.sh | 20 +++++-----\n 2 files changed, 85 insertions(+), 32 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex a85275d..bd1b9d1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2207,6 +2207,57 @@ static uintmax_t change_note_fanout(struct tree_entry *root,\n \treturn do_change_note_fanout(root, root, hex_sha1, 0, path, 0, fanout);\n }\n \n+/*\n+ * Given a pointer into a string, parse a mark reference:\n+ *\n+ * idnum ::= ':' bigint;\n+ *\n+ * Return the first character after the value in *endptr.\n+ *\n+ * Complain if the following character is not what is expected,\n+ * either a space or end of the string.\n+ */\n+static uintmax_t parse_mark_ref(const char *p, char **endptr)\n+{\n+\tuintmax_t mark;\n+\n+\tassert(*p == ':');\n+\t++p;\n+\tmark = strtoumax(p, endptr, 10);\n+\tif (*endptr == p)\n+\t\tdie(\"No value after ':' in mark: %s\", command_buf.buf);\n+\treturn mark;\n+}\n+\n+/*\n+ * Parse the mark reference, and complain if this is not the end of\n+ * the string.\n+ */\n+static uintmax_t parse_mark_ref_eol(const char *p)\n+{\n+\tchar *end;\n+\tuintmax_t mark;\n+\n+\tmark = parse_mark_ref(p, &end);\n+\tif (*end != '\\0')\n+\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n+\treturn mark;\n+}\n+\n+/*\n+ * Parse the mark reference, demanding a trailing space. Return a\n+ * pointer to the space.\n+ */\n+static uintmax_t parse_mark_ref_space(const char *p, char **endptr)\n+{\n+\tuintmax_t mark;\n+\n+\tmark = parse_mark_ref(p, endptr);\n+\tif (**endptr != ' ')\n+\t\tdie(\"Missing space after mark: %s\", command_buf.buf);\n+\treturn mark;\n+}\n+\n static void file_change_m(struct branch *b)\n {\n \tconst char *p = command_buf.buf + 2;\n@@ -2236,20 +2287,24 @@ static void file_change_m(struct branch *b)\n \n \tif (*p == ':') {\n \t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n+\t\toe = find_mark(parse_mark_ref_space(p, &x));\n \t\thashcpy(sha1, oe->idx.sha1);\n \t\tp = x;\n \t} else if (!prefixcmp(p, \"inline\")) {\n \t\tinline_data = 1;\n \t\tp += 6;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after 'inline': %s\",\n+\t\t\t command_buf.buf);\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\t++p; /* skip space */\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -2408,20 +2463,24 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t/* or 'inline' */\n \tif (*p == ':') {\n \t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n+\t\toe = find_mark(parse_mark_ref_space(p, &x));\n \t\thashcpy(sha1, oe->idx.sha1);\n \t\tp = x;\n \t} else if (!prefixcmp(p, \"inline\")) {\n \t\tinline_data = 1;\n \t\tp += 6;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after 'inline': %s\",\n+\t\t\t command_buf.buf);\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\t++p; /* skip space */\n \n \t/* */\n \ts = lookup_branch(p);\n@@ -2430,7 +2489,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t\t\tdie(\"Can't add a note on empty branch.\");\n \t\thashcpy(commit_sha1, s->sha1);\n \t} else if (*p == ':') {\n-\t\tuintmax_t commit_mark = strtoumax(p + 1, NULL, 10);\n+\t\tuintmax_t commit_mark = parse_mark_ref_eol(p);\n \t\tstruct object_entry *commit_oe = find_mark(commit_mark);\n \t\tif (commit_oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n@@ -2537,7 +2596,7 @@ static int parse_from(struct branch *b)\n \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n \t} else if (*from == ':') {\n-\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\tstruct object_entry *oe = find_mark(idnum);\n \t\tif (oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2572,7 +2631,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\tif (s)\n \t\t\thashcpy(n->sha1, s->sha1);\n \t\telse if (*from == ':') {\n-\t\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\t\tstruct object_entry *oe = find_mark(idnum);\n \t\t\tif (oe->type != OBJ_COMMIT)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2735,7 +2794,7 @@ static void parse_new_tag(void)\n \t\ttype = OBJ_COMMIT;\n \t} else if (*from == ':') {\n \t\tstruct object_entry *oe;\n-\t\tfrom_mark = strtoumax(from + 1, NULL, 10);\n+\t\tfrom_mark = parse_mark_ref_eol(from);\n \t\toe = find_mark(from_mark);\n \t\ttype = oe->type;\n \t\thashcpy(sha1, oe->idx.sha1);\n@@ -2867,14 +2926,9 @@ static void parse_cat_blob(void)\n \t/* cat-blob SP LF */\n \tp = command_buf.buf + strlen(\"cat-blob \");\n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n-\t\tif (x == p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\toe = find_mark(parse_mark_ref_eol(p));\n \t\tif (!oe)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n-\t\tif (*x)\n-\t\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n \t\thashcpy(sha1, oe->idx.sha1);\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n@@ -2945,9 +2999,7 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n \n \tif (**p == ':') {\t/* */\n \t\tchar *endptr;\n-\t\te = find_mark(strtoumax(*p + 1, &endptr, 10));\n-\t\tif (endptr == *p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\te = find_mark(parse_mark_ref_space(*p, &endptr));\n \t\tif (!e)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n \t\t*p = endptr;\n@@ -2955,9 +3007,12 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n \t} else {\t/* */\n \t\tif (get_sha1_hex(*p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n-\t\te = find_object(sha1);\n \t\t*p += 40;\n+\t\tif (**p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\t\te = find_object(sha1);\n \t}\n+\t*p += 1; /* skip space */\n \n \twhile (!e || e->type != OBJ_TREE)\n \t\te = dereference(e, sha1);\n@@ -3008,8 +3063,6 @@ static void parse_ls(struct branch *b)\n \t\troot = new_tree_entry();\n \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n \t\tload_tree(root);\n-\t\tif (*p++ != ' ')\n-\t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n \t}\n \tif (*p == '\"') {\n \t\tstatic struct strbuf uq = STRBUF_INIT;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 621f02a..875ac7a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2693,7 +2693,7 @@ test_expect_success 'S: add commits 1 and 2, and blob 103' '\n #\n # filemodify, three datarefs\n #\n-test_expect_failure 'S: filemodify markref no space' '\n+test_expect_success 'S: filemodify markref no space' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/S\n \tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -2706,7 +2706,7 @@ test_expect_failure 'S: filemodify markref no space' '\n \tgrep -q \"Missing space after mark\" err\n '\n \n-test_expect_failure 'S: filemodify inline no space' '\n+test_expect_success 'S: filemodify inline no space' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/S\n \tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -2739,7 +2739,7 @@ test_expect_success 'S: filemodify sha1 no space' '\n #\n # notemodify, three ways to say dataref\n #\n-test_expect_failure 'S: notemodify dataref markref no space' '\n+test_expect_success 'S: notemodify dataref markref no space' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/S\n \tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -2752,7 +2752,7 @@ test_expect_failure 'S: notemodify dataref markref no space' '\n \tgrep -q \"Missing space after mark\" err\n '\n \n-test_expect_failure 'S: notemodify dataref inline no space' '\n+test_expect_success 'S: notemodify dataref inline no space' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/S\n \tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -2785,7 +2785,7 @@ test_expect_success 'S: notemodify dataref sha1 no space' '\n #\n # notemodify, mark in committish\n #\n-test_expect_failure 'S: notemodify committish markref junk at eol' '\n+test_expect_success 'S: notemodify committish markref junk at eol' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/Snotes\n \tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -2801,7 +2801,7 @@ test_expect_failure 'S: notemodify committish markref junk at eol' '\n #\n # from\n #\n-test_expect_failure 'S: from markref junk at eol' '\n+test_expect_success 'S: from markref junk at eol' '\n \t# no &&\n \tgit fast-import --import-marks=marks --export-marks=marks <<-EOF 2>err\n \tcommit refs/heads/S2\n@@ -2839,7 +2839,7 @@ test_expect_failure 'S: from markref junk at eol' '\n #\n # merge\n #\n-test_expect_failure 'S: merge markref junk at eol' '\n+test_expect_success 'S: merge markref junk at eol' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tcommit refs/heads/S\n \tmark :4\n@@ -2858,7 +2858,7 @@ test_expect_failure 'S: merge markref junk at eol' '\n #\n # tag, from markref\n #\n-test_expect_failure 'S: tag markref junk at eol' '\n+test_expect_success 'S: tag markref junk at eol' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \ttag refs/tags/Stag\n \tfrom :2x\n@@ -2885,7 +2885,7 @@ test_expect_success 'S: cat-blob markref junk at eol' '\n #\n # ls markref\n #\n-test_expect_failure 'S: ls markref space' '\n+test_expect_success 'S: ls markref space' '\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tls :2x hello.c\n \tEOF\n@@ -2893,7 +2893,7 @@ test_expect_failure 'S: ls markref space' '\n \tgrep -q \"Missing space after mark\" err\n '\n \n-test_expect_failure 'S: ls sha1 space' '\n+test_expect_success 'S: ls sha1 space' '\n \tsha1=$(grep -w :2 marks | cut -d\\ -f2) &&\n \ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n \tls ${sha1}x hello.c\n-- \n1.7.10.rc2.2.g38670\n"},{"id":"188377","messageId":"CAGdFq_h-x9bi35=HTuYnFVNb=ZPH04BjbZcz8_996ENXYQBNPw@mail.gmail.com","threadId":"30123","inReplyTo":"1333417910-17955-1-git-send-email-pw@padd.com","subject":"Re: [PATCHv2 0/2] fast-import: tighten parsing of mark references","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-04-03T02:00:58Z","receivedAt":"2012-04-03T02:00:58Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Mon, Apr 2, 2012 at 20:51, Pete Wyckoff wrote:\n> Also, I did the unit tests first, to make sure things were broken\n> as I expected.  You can squash it all together if you prefer.\n\nNice series. I agree that it's good to be strict in what we accept\n(breaking from the \"be liberal in what you accept\" mantra), due to the\nimportance of the input stream being parsed exactly right (as opposed\nto a browser, where it's better to not render something bold than to\njust break because a was missing).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"188414","messageId":"20120403140055.GC15589@burratino","threadId":"30123","inReplyTo":"1333417910-17955-2-git-send-email-pw@padd.com","subject":"Re: [PATCHv2 1/2] fast-import: test behavior of garbage after mark references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T14:00:55Z","receivedAt":"2012-04-03T14:00:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Pete Wyckoff wrote:\n\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -2635,4 +2635,271 @@ test_expect_success \\\n> \t'n=$(grep $a verify | wc -l) &&\n> \t test 1 = $n'\n> \n> +###\n> +### series S\n> +###\n> +#\n> +# Set up is roughly this. Commits marked 1,2,3,4. Blobs\n> +# marked 100 + commit. Notes 200 +. Make sure missing spaces\n> +# and EOLs after mark references cause errors.\n\nNit: \"Set up\" should be \"Setup\" when it is a noun.\n\n[...]\n> +test_expect_success 'S: add commits 1 and 2, and blob 103' '\n> +\tgit fast-import --export-marks=marks +'\n\nOk, this one sets up for later ones...\n\n> +\n> +#\n> +# filemodify, three datarefs\n> +#\n> +test_expect_failure 'S: filemodify markref no space' '\n\nWhat is this testing for? The ideal is that each test_expect_foo line\ncontains a proposition and the test checks whether that proposition is\ntrue or false. For example:\n\n\ttest_expect_failure 'S: filemodify with garbage after mark errors out' '\n\nLikewise in later tests.\n\n> +\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n> +\tcommit refs/heads/S\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata < +\tcommit N\n> +\tCOMMIT\n> +\tM 100644 :103x hello.c\n> +\tEOF\n> +\tcat err &&\n> +\tgrep -q \"Missing space after mark\" err\n\nIs this using \"grep -q\" to avoid repeating the same line in the output\ntwice? It seems better to use plain grep or test_i18ngrep.\n\nI'm also worried that if someone wants to change these messages\n(perhaps to make the 'm' in \"Missing\" lowercase or something), they\nwill have to change all of these tests. If we want to be absolutely\nsure that git detects the right error instead of something else, I\nwould suggest\n\n\ttest_i18ngrep \"space after mark\" message\n\nI'm also not convinced the error message is worth checking at all ---\nas long as fast-import errors out, won't the frontend author be able\nto look in the logs to find out the problematic line anyway?\n\n> +'\n> +\n> +test_expect_failure 'S: filemodify inline no space' '\n> +\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n> +\tcommit refs/heads/S\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata < +\tcommit N\n> +\tCOMMIT\n> +\tM 100644 inlineX hello.c\n> +\tdata < +\tinline\n> +\tBLOB\n> +\tEOF\n> +\tcat err &&\n> +\tgrep -q \"Missing space after .inline.\" err\n\nDoes this fail because the error message is \"Missing space after SHA1\"\ninstead? I'm not sure that's actually a bug, unless we want to\ncorrectly nitpick that the keyword \"inline\" that is a stand-in for an\nobject name is not itself one.\n\nI don't think the tests for exact error messages make too much sense\nwithout the next patch, so I would suggest leaving them out if this\npatch is supposed to be applicable on its own.\n\nThanks for some thorough tests.\n\nHope that helps,\nJonathan\n"},{"id":"188415","messageId":"20120403142001.GD15589@burratino","threadId":"30123","inReplyTo":"1333417910-17955-3-git-send-email-pw@padd.com","subject":"Re: [PATCHv2 2/2] fast-import: tighten parsing of mark references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T14:20:01Z","receivedAt":"2012-04-03T14:20:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Johan for noteimport code)\nPete Wyckoff wrote:\n\n> Fast-import does not complain when garbage\n> appears after a mark reference in some cases.\n\nThanks for fixing it.\n\n[...]\n> +++ b/fast-import.c\n[...]\n> @@ -2236,20 +2287,24 @@ static void file_change_m(struct branch *b)\n> \n> \tif (*p == ':') {\n> \t\tchar *x;\n> -\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n> +\t\toe = find_mark(parse_mark_ref_space(p, &x));\n> \t\thashcpy(sha1, oe->idx.sha1);\n> \t\tp = x;\n\nSimpler:\n\n\tif (*p == ':') {\n\t\toe = find_mark(parse_mark_ref_space(p, &p));\n\t\thashcpy(sha1, oe->idx.sha1);\n\t} else if ...\n\n> \t} else if (!prefixcmp(p, \"inline\")) {\n> \t\tinline_data = 1;\n> \t\tp += 6;\n> +\t\tif (*p != ' ')\n> +\t\t\tdie(\"Missing space after 'inline': %s\",\n> +\t\t\t command_buf.buf);\n> \t} else {\n> \t\tif (get_sha1_hex(p, sha1))\n> \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n\nIf I write\n\n\tM 100644 inliness some/path/to/file\n\nwas my mistake actually leaving out a space after 'inline' or\nwas it using an invalid ?\n\nI think the latter, so I would suggest\n\n\t} else if (!prefixcmp(p, \"inline \")) {\n\t\tinline_data = 1;\n\t\tp += strlen(\"inline\");\t/* advance to space */\n\t} else {\n\t\tif (get_sha1_hex(p, sha1))\n\t\t\t...\n\n[...]\n> \t}\n> -\tif (*p++ != ' ')\n> -\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n> +\t++p; /* skip space */\n\nI guess I'd suggest\n\n\tassert(*p == ' ');\n\tp++;\n\nas defense against coders introducing additional cases that\nare not as careful.\n\n> @@ -2408,20 +2463,24 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n> \t/* or 'inline' */\n> \tif (*p == ':') {\n> \t\tchar *x;\n> -\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n> +\t\toe = find_mark(parse_mark_ref_space(p, &x));\n> \t\thashcpy(sha1, oe->idx.sha1);\n> \t\tp = x;\n\nLikewise (btw, why doesn't this share code with the filemodify case?):\n\n\tif (*p == ':') {\n\t\toe = find_mark(parse_mark_with_trailing_space(p, &p));\n\t\thashcpy(sha1, oe->idx.sha1);\n\t} else if ...\n\nand so on.\n\n[...]\n> @@ -2430,7 +2489,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n> \t\t\tdie(\"Can't add a note on empty branch.\");\n> \t\thashcpy(commit_sha1, s->sha1);\n> \t} else if (*p == ':') {\n> -\t\tuintmax_t commit_mark = strtoumax(p + 1, NULL, 10);\n> +\t\tuintmax_t commit_mark = parse_mark_ref_eol(p);\n> \t\tstruct object_entry *commit_oe = find_mark(commit_mark);\n> \t\tif (commit_oe->type != OBJ_COMMIT)\n> \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n> @@ -2537,7 +2596,7 @@ static int parse_from(struct branch *b)\n> \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n> \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n> \t} else if (*from == ':') {\n> -\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n> +\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n\nThe title feature. Nice.\n\n[...]\n> @@ -2945,9 +2999,7 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n> \n> \tif (**p == ':') {\t/* */\n> \t\tchar *endptr;\n> -\t\te = find_mark(strtoumax(*p + 1, &endptr, 10));\n> -\t\tif (endptr == *p + 1)\n> -\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n> +\t\te = find_mark(parse_mark_ref_space(*p, &endptr));\n> \t\tif (!e)\n> \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n> \t\t*p = endptr;\n\nSimpler:\n\n\tif (**p == ':') {\n\t\te = find_mark(parse_mark_...(*p, p));\n\t\tif (!e)\n\t\t\tdie(...);\n\t} else {\n\n> @@ -2955,9 +3007,12 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n> \t} else {\t/* */\n> \t\tif (get_sha1_hex(*p, sha1))\n> \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n> -\t\te = find_object(sha1);\n> \t\t*p += 40;\n> +\t\tif (**p != ' ')\n> +\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n> +\t\te = find_object(sha1);\n\nThis seems dangerous. What if a new caller arises that wants to\nparse a representing a tree-ish at the end of the line?\n\nSo I think checking the character after the tree-ish should still\nbe the caller's responsibility.\n\n> \t}\n> +\t*p += 1; /* skip space */\n\nIf other patches in flight use the same function, they would expect\n*p to point to the space when parse_treeish_dataref returns. If we\nwanted to change that (as mentioned above I don't think we ought to)\nthen the function's name should be changed to force such new callers\nnot to compile.\n\n> @@ -3008,8 +3063,6 @@ static void parse_ls(struct branch *b)\n> \t\troot = new_tree_entry();\n> \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n> \t\tload_tree(root);\n> -\t\tif (*p++ != ' ')\n> -\t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n\n(here's the caller).\n\nExcept where noted above, this looks good.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"188463","messageId":"20120404004610.GA4124@padd.com","threadId":"30123","inReplyTo":"20120403140055.GC15589@burratino","subject":"Re: [PATCHv2 1/2] fast-import: test behavior of garbage after mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-04T00:46:10Z","receivedAt":"2012-04-04T00:46:10Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:00 -0500:\n> Pete Wyckoff wrote:\n> > +\n> > +#\n> > +# filemodify, three datarefs\n> > +#\n> > +test_expect_failure 'S: filemodify markref no space' '\n> \n> What is this testing for? The ideal is that each test_expect_foo line\n> contains a proposition and the test checks whether that proposition is\n> true or false. For example:\n> \n> \ttest_expect_failure 'S: filemodify with garbage after mark errors out' '\n> \n> Likewise in later tests.\n\nI've fixed these locally, thanks for reading them!\n\n> > +\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n> > +\tcommit refs/heads/S\n> > +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> > +\tdata < > +\tcommit N\n> > +\tCOMMIT\n> > +\tM 100644 :103x hello.c\n> > +\tEOF\n> > +\tcat err &&\n> > +\tgrep -q \"Missing space after mark\" err\n> \n> Is this using \"grep -q\" to avoid repeating the same line in the output\n> twice? It seems better to use plain grep or test_i18ngrep.\n> \n> I'm also worried that if someone wants to change these messages\n> (perhaps to make the 'm' in \"Missing\" lowercase or something), they\n> will have to change all of these tests. If we want to be absolutely\n> sure that git detects the right error instead of something else, I\n> would suggest\n> \n> \ttest_i18ngrep \"space after mark\" message\n> \n> I'm also not convinced the error message is worth checking at all ---\n> as long as fast-import errors out, won't the frontend author be able\n> to look in the logs to find out the problematic line anyway?\n\nI'm a bit confused. I read through 5e9637c (i18n: add\ninfrastructure for translating Git with gettext, 2011-11-18), and\nGETTEXT_POISON seems to be used to find untranslated messages.\nWhat I want to test here is that the functionality works: do the\nright untranslated messages get printed.\n\nChanging the \"Missing\" to \"missing\" would require fixing the\ntests, and that seems okay.\n\nI'm happy to drop the \"-q\" and drop the \"Missing\", but wonder if\nyou're looking for something deeper.\n\n> > +test_expect_failure 'S: filemodify inline no space' '\n> > +\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n> > +\tcommit refs/heads/S\n> > +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> > +\tdata < > +\tcommit N\n> > +\tCOMMIT\n> > +\tM 100644 inlineX hello.c\n> > +\tdata < > +\tinline\n> > +\tBLOB\n> > +\tEOF\n> > +\tcat err &&\n> > +\tgrep -q \"Missing space after .inline.\" err\n> \n> Does this fail because the error message is \"Missing space after SHA1\"\n> instead? I'm not sure that's actually a bug, unless we want to\n> correctly nitpick that the keyword \"inline\" that is a stand-in for an\n> object name is not itself one.\n\nThe form of the message matters. Datarefs can be inline, SHA1s,\nor marks. It is confusing to see an error message about a SHA1\nwhen the input stream has no SHA1s. The existing code always\nsays \"SHA1\", which is wrong if you gave it a mark or an inline.\n\n> I don't think the tests for exact error messages make too much sense\n> without the next patch, so I would suggest leaving them out if this\n> patch is supposed to be applicable on its own.\n> \n> Thanks for some thorough tests.\n\nYou think I should squash it all together, then? Or factor the\ntests into two chunks:\n - tests for behavior that silently accepts broken input; and,\n - tests for behavior where the bogus input is detcetd, but\n incorrect error messages are given?\n\n\t\t-- Pete\n"},{"id":"188464","messageId":"20120404012037.GB4124@padd.com","threadId":"30123","inReplyTo":"20120403142001.GD15589@burratino","subject":"Re: [PATCHv2 2/2] fast-import: tighten parsing of mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-04T01:20:37Z","receivedAt":"2012-04-04T01:20:37Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:20 -0500:\n> (cc-ing Johan for noteimport code)\n\nThanks, glad you noticed.\n\n> Pete Wyckoff wrote:\n> [...]\n> > +++ b/fast-import.c\n> [...]\n> > @@ -2236,20 +2287,24 @@ static void file_change_m(struct branch *b)\n> > \n> > \tif (*p == ':') {\n> > \t\tchar *x;\n> > -\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n> > +\t\toe = find_mark(parse_mark_ref_space(p, &x));\n> > \t\thashcpy(sha1, oe->idx.sha1);\n> > \t\tp = x;\n> \n> Simpler:\n> \n> \tif (*p == ':') {\n> \t\toe = find_mark(parse_mark_ref_space(p, &p));\n> \t\thashcpy(sha1, oe->idx.sha1);\n> \t} else if ...\n\nYes. I thought about just passing in plain old &p. Even though\nthese approaches would work, it is a bit more difficult for\nnovice C coders to read. Figured we should err on the side of\nhelping future code readers. I can add more cleverness if you\nfeel strongly.\n\n> > \t} else if (!prefixcmp(p, \"inline\")) {\n> > \t\tinline_data = 1;\n> > \t\tp += 6;\n> > +\t\tif (*p != ' ')\n> > +\t\t\tdie(\"Missing space after 'inline': %s\",\n> > +\t\t\t command_buf.buf);\n> > \t} else {\n> > \t\tif (get_sha1_hex(p, sha1))\n> > \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n> \n> If I write\n> \n> \tM 100644 inliness some/path/to/file\n> \n> was my mistake actually leaving out a space after 'inline' or\n> was it using an invalid ?\n> \n> I think the latter, so I would suggest\n> \n> \t} else if (!prefixcmp(p, \"inline \")) {\n> \t\tinline_data = 1;\n> \t\tp += strlen(\"inline\");\t/* advance to space */\n> \t} else {\n> \t\tif (get_sha1_hex(p, sha1))\n> \t\t\t...\n\nInsead of \"Missing space after 'inline'\", you'll get \"Invalid\nSHA1\". You misspelled \"inline\" with \"inliness\"? And would\nprefer to be told you provided an invalid SHA1?\n\nI'm tempted to guess that any string starting with \"inline\", e.g.\n\"inlinePath/To/File\" without a space is still a good indication\nthat they were trying to say \"inline \". The chance that they\nhorribly typed a SHA1, or really had a path staring with \"inline\"\nand forgot the dataref entirely, feel less likely.\n\n> [...]\n> > \t}\n> > -\tif (*p++ != ' ')\n> > -\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n> > +\t++p; /* skip space */\n> \n> I guess I'd suggest\n> \n> \tassert(*p == ' ');\n> \tp++;\n> \n> as defense against coders introducing additional cases that\n> are not as careful.\n\nGood suggestion, thanks.\n\n> > @@ -2408,20 +2463,24 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n> > \t/* or 'inline' */\n> > \tif (*p == ':') {\n> > \t\tchar *x;\n> > -\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n> > +\t\toe = find_mark(parse_mark_ref_space(p, &x));\n> > \t\thashcpy(sha1, oe->idx.sha1);\n> > \t\tp = x;\n> \n> Likewise (btw, why doesn't this share code with the filemodify case?):\n> \n> \tif (*p == ':') {\n> \t\toe = find_mark(parse_mark_with_trailing_space(p, &p));\n> \t\thashcpy(sha1, oe->idx.sha1);\n> \t} else if ...\n> \n> and so on.\n\nIt does feel like a good opportunity for some refactoring. Two\nout of the three callers to parse an mark followed by a space\ncould be put together here.\n\n[..]\n> > @@ -2955,9 +3007,12 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n> > \t} else {\t/* */\n> > \t\tif (get_sha1_hex(*p, sha1))\n> > \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n> > -\t\te = find_object(sha1);\n> > \t\t*p += 40;\n> > +\t\tif (**p != ' ')\n> > +\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n> > +\t\te = find_object(sha1);\n> \n> This seems dangerous. What if a new caller arises that wants to\n> parse a representing a tree-ish at the end of the line?\n> \n> So I think checking the character after the tree-ish should still\n> be the caller's responsibility.\n\nI was close to just moving parse_treeish_dataref() into its\nsingle caller, parse_ls(), just so we wouldn't have to think\nabout this.\n\nThere are two cases it handles: mark and sha1. The mark case\nuses the handy new parse_mark_ref_space(), which does the space\nchecking. The sha1 branch had no check in this function. So\nI hoisted the space check up to make the branches symmetrical.\n\n> > \t}\n> > +\t*p += 1; /* skip space */\n> \n> If other patches in flight use the same function, they would expect\n> *p to point to the space when parse_treeish_dataref returns. If we\n> wanted to change that (as mentioned above I don't think we ought to)\n> then the function's name should be changed to force such new callers\n> not to compile.\n> \n> > @@ -3008,8 +3063,6 @@ static void parse_ls(struct branch *b)\n> > \t\troot = new_tree_entry();\n> > \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n> > \t\tload_tree(root);\n> > -\t\tif (*p++ != ' ')\n> > -\t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n> \n> (here's the caller).\n\nI would prefer just to inline the whole thing. Or new name\nparse_ls_dataref() if you have a preference.\n\n> Except where noted above, this looks good.\n\nThanks.\n\n\t\t-- Pete\n"},{"id":"188467","messageId":"20120404053236.GA2460@burratino","threadId":"30123","inReplyTo":"20120404012037.GB4124@padd.com","subject":"Re: [PATCHv2 2/2] fast-import: tighten parsing of mark references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T05:32:37Z","receivedAt":"2012-04-04T05:32:37Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Pete Wyckoff wrote:\n> jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:20 -0500:\n\n>> Simpler:\n>> \n>> \tif (*p == ':') {\n>> \t\toe = find_mark(parse_mark_ref_space(p, &p));\n>> \t\thashcpy(sha1, oe->idx.sha1);\n>> \t} else if ...\n>\n> Yes. I thought about just passing in plain old &p. Even though\n> these approaches would work, it is a bit more difficult for\n> novice C coders to read. Figured we should err on the side of\n> helping future code readers. I can add more cleverness if you\n> feel strongly.\n\nIt would be clearest with one argument, like so:\n\n\t\toe = find_mark(parse_mark_...(&p));\n\t\thashcpy(sha1, oe->idx.sha1);\n\n[...]\n> Insead of \"Missing space after 'inline'\", you'll get \"Invalid\n> SHA1\". You misspelled \"inline\" with \"inliness\"? And would\n> prefer to be told you provided an invalid SHA1?\n\nIt wasn't a great example, but what I meant is that if someone\nasked me, a human, to parse\n\n\tM 100644 foobar path/to/file\n\nI would assume that foobar is a . Likewise, for any\nstring baz in\n\n\tM 100644 baz path/to/file\n\nincluding strings that start with \"inline\", except for \"inline\"\nitself.\n\nTo put it another way: checking for 'inline' at the start of a word as\na way to check for typos seems odd to me. We do not diagnose\n\n\tM 100644 Inline path/to/file\n\nas a misspelled version of \"inline\", nor\n\n\tM 100644inline path/to/file\n\nas an instance of a missing space character, and we shouldn't.\n\nThe goal in fast-import's behavior is usually predictability and\nsimplicity in terms of the mental model of the person writing a\nfrontend. Trying to guess the user's intention on malformed input\nonly takes away from that goal.\n\nWhy I care: if some day git permits other kinds of (for\nexample if it supports refnames some day), I do not want datarefs\nbeginning with \"inline\" to be forbidden.\n\n[...]\n> There are two cases it handles: mark and sha1. The mark case\n> uses the handy new parse_mark_ref_space(), which does the space\n> checking. The sha1 branch had no check in this function. So\n> I hoisted the space check up to make the branches symmetrical.\n\nI think it's ok to sacrifice symmetry here, but:\n\n[...]\n> I would prefer just to inline the whole thing. Or new name\n> parse_ls_dataref() if you have a preference.\n\nif changing the behavior of the function that parses a treeish dataref\nseems right, that's fine with me as long as its name or signature\nchanges.\n\nFor example, it could become\n\n\tstatic struct object_entry *parse_treeish(const char **p);\n\nHope that helps,\nJonathan\n"},{"id":"188468","messageId":"20120404054316.GB2460@burratino","threadId":"30123","inReplyTo":"20120404004610.GA4124@padd.com","subject":"Re: [PATCHv2 1/2] fast-import: test behavior of garbage after mark references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T05:43:17Z","receivedAt":"2012-04-04T05:43:17Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Tue, Apr 03, 2012 at 08:46:10PM -0400, Pete Wyckoff wrote:\n> jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:00 -0500:\n\n>> Is this using \"grep -q\" to avoid repeating the same line in the output\n>> twice? It seems better to use plain grep or test_i18ngrep.\n[...]\n> What I want to test here is that the functionality works: do the\n> right untranslated messages get printed.\n>\n> Changing the \"Missing\" to \"missing\" would require fixing the\n> tests, and that seems okay.\n\nLet me reiterate this a little then.\n\nSuppose I mark the messages in fast-import.c with _() so they get\ntranslated. Then your tests will fail, so I have to tweak them. Fine\n--- the test tweaks take some time, but they're doable. Nothing lost,\nright?\n\nNo, something major would be lost.\n\nTests normally save later coders time, by giving immediate feedback\nthat they would normally only get by letting a feature be used over a\nlong time by real users. They also dissuade people from changing\ngit's behavior without thinking carefully about the consequences ---\neach broken test represents a class of script or user expectation that\nis potentially being broken.\n\nSimilarly, a test that checks that git produces such-and-such exact\noutput is dissuading me from making certain behavior changes by adding\nto the work needed to make them (I have to adjust tests, too). So now\nI am less likely to\n\n (1) reword the message to make it clearer in some way in response to\n user feedback\n\n (2) mark it for translation so the operator can see a message in her\n native language\n\nHow is making that hard in any way a good thing?\n\nRelaxing the pattern addresses (1). Using test_i18ngrep instead of\ngrep addresses (2).\n\nJonathan\n"},{"id":"188545","messageId":"20120405015121.GA10945@padd.com","threadId":"30123","inReplyTo":"1333417910-17955-1-git-send-email-pw@padd.com","subject":"[PATCHv3] fast-import: tighten parsing of mark references","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-05T01:51:21Z","receivedAt":"2012-04-05T01:51:21Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"The syntax for the use of mark references in fast-import\ndemands either a SP (space) or LF (end-of-line) after\na mark reference. Fast-import does not complain when garbage\nappears after a mark reference in some cases.\n\nFactor out parsing of mark references and complain if\nerrant characters are found.\n\nBuggy input can cause fast-import to produce the wrong output,\nsilently, without error. This makes it difficult to track\ndown buggy generators of fast-import streams. An example is\nseen in the last line of this commit command:\n\n commit refs/heads/S2\n committer Name 1112912893 -0400\n data <idx.sha1);\n-\t\tp = x;\n-\t} else if (!prefixcmp(p, \"inline\")) {\n+\t} else if (!prefixcmp(p, \"inline \")) {\n \t\tinline_data = 1;\n-\t\tp += 6;\n+\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\tassert(*p == ' ');\n+\t++p; /* skip space */\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -2407,21 +2460,21 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t/* Now parse the notemodify command. */\n \t/* or 'inline' */\n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n+\t\toe = find_mark(parse_mark_ref_space(&p));\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t\tp = x;\n-\t} else if (!prefixcmp(p, \"inline\")) {\n+\t} else if (!prefixcmp(p, \"inline \")) {\n \t\tinline_data = 1;\n-\t\tp += 6;\n+\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n \t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\tassert(*p == ' ');\n+\t++p; /* skip space */\n \n \t/* */\n \ts = lookup_branch(p);\n@@ -2430,7 +2483,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t\t\tdie(\"Can't add a note on empty branch.\");\n \t\thashcpy(commit_sha1, s->sha1);\n \t} else if (*p == ':') {\n-\t\tuintmax_t commit_mark = strtoumax(p + 1, NULL, 10);\n+\t\tuintmax_t commit_mark = parse_mark_ref_eol(p);\n \t\tstruct object_entry *commit_oe = find_mark(commit_mark);\n \t\tif (commit_oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n@@ -2537,7 +2590,7 @@ static int parse_from(struct branch *b)\n \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n \t} else if (*from == ':') {\n-\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\tstruct object_entry *oe = find_mark(idnum);\n \t\tif (oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2572,7 +2625,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\tif (s)\n \t\t\thashcpy(n->sha1, s->sha1);\n \t\telse if (*from == ':') {\n-\t\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\t\tstruct object_entry *oe = find_mark(idnum);\n \t\t\tif (oe->type != OBJ_COMMIT)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2735,7 +2788,7 @@ static void parse_new_tag(void)\n \t\ttype = OBJ_COMMIT;\n \t} else if (*from == ':') {\n \t\tstruct object_entry *oe;\n-\t\tfrom_mark = strtoumax(from + 1, NULL, 10);\n+\t\tfrom_mark = parse_mark_ref_eol(from);\n \t\toe = find_mark(from_mark);\n \t\ttype = oe->type;\n \t\thashcpy(sha1, oe->idx.sha1);\n@@ -2867,14 +2920,9 @@ static void parse_cat_blob(void)\n \t/* cat-blob SP LF */\n \tp = command_buf.buf + strlen(\"cat-blob \");\n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n-\t\tif (x == p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\toe = find_mark(parse_mark_ref_eol(p));\n \t\tif (!oe)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n-\t\tif (*x)\n-\t\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n \t\thashcpy(sha1, oe->idx.sha1);\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n@@ -2944,13 +2992,9 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n \tstruct object_entry *e;\n \n \tif (**p == ':') {\t/* */\n-\t\tchar *endptr;\n-\t\te = find_mark(strtoumax(*p + 1, &endptr, 10));\n-\t\tif (endptr == *p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\te = find_mark(parse_mark_ref_space(p));\n \t\tif (!e)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n-\t\t*p = endptr;\n \t\thashcpy(sha1, e->idx.sha1);\n \t} else {\t/* */\n \t\tif (get_sha1_hex(*p, sha1))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 0f5b5e5..cbc0e81 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2635,4 +2635,280 @@ test_expect_success \\\n \t'n=$(grep $a verify | wc -l) &&\n \t test 1 = $n'\n \n+###\n+### series S\n+###\n+#\n+# Setup is roughly this. Commits marked 1,2,3,4. Blobs\n+# marked 100 + commit. Notes 200 +. Make sure missing spaces\n+# and EOLs after mark references cause errors.\n+#\n+# The error message looks like either:\n+# Missing space after ..\n+# or\n+# Garbage after ..\n+#\n+# 1--2--4\n+# \\ /\n+# -3-\n+#\n+test_tick\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data < $GIT_COMMITTER_DATE\n+data <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/Snotes\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err\n+\tcommit refs/heads/S2\n+\tmark :3\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tmark :4\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\ttag refs/tags/Stag\n+\tfrom :2x\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcat-blob :2x\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"after mark\" err\n+'\n+\n+#\n+# ls markref\n+#\n+test_expect_success 'S: ls with garbage after mark must fail' '\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls :2x hello.c\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"space after mark\" err\n+'\n+\n+test_expect_success 'S: ls with garbage after sha1 must fail' '\n+\tsha1=$(grep -w :2 marks | cut -d\\ -f2) &&\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls ${sha1}x hello.c\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"space after tree-ish\" err\n+'\n+\n test_done\n-- \n1.7.10.rc2.62.gac32b.dirty\n"},{"id":"188546","messageId":"20120405022422.GB20687@burratino","threadId":"30123","inReplyTo":"20120405015121.GA10945@padd.com","subject":"Re: [PATCHv3] fast-import: tighten parsing of mark references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-05T02:24:22Z","receivedAt":"2012-04-05T02:24:22Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Pete Wyckoff wrote:\n\n> This addresses all of Jonathan's comments, in particular:\n\nNice. Thanks much. I only have a few small worries left:\n\n[...]\n> +++ b/t/t9300-fast-import.sh\n> @@ -2635,4 +2635,280 @@ test_expect_success \\\n[...]\n> +test_expect_success 'S: filemodify with garbage after sha1 must fail' '\n> +\tsha1=$(grep -w :103 marks | cut -d\\ -f2) &&\n\n\"grep -w\" isn't used elsewhere in the testsuite. Is it portable?\n\n[...]\n> +# inline is misspelled; fast-import thinks it is some unknown dataref\n> +# and complains \"Invalid SHA1\"\n> +test_expect_success 'S: notemodify with garbage after inline dataref must fail' '\n> +\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n> +\tcommit refs/heads/S\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata < +\tcommit S note dataref inline\n> +\tCOMMIT\n> +\tN inlineX :2\n> +\tdata < +\tnote blob\n> +\tBLOB\n> +\tEOF\n> +\tcat err &&\n> +\ttest_i18ngrep \"nvalid SHA1\" err\n> +'\n\nIf I understood the discussion before correctly, this error message is\nsuboptimal and something like \"invalid dataref\" would be a little\nclearer, right?\n\nThat's orthogonal to what this patch is about so I'm not suggesting\nchanging it. But shouldn't the test just check that fast-import fails\nwithout committing to any particular message?\n\nCheers,\nJonathan\n"},{"id":"188574","messageId":"7vty0y5c55.fsf@alter.siamese.dyndns.org","threadId":"30123","inReplyTo":"20120405022422.GB20687@burratino","subject":"Re: [PATCHv3] fast-import: tighten parsing of mark references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-05T17:20:06Z","receivedAt":"2012-04-05T17:20:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Pete Wyckoff wrote:\n>\n>> This addresses all of Jonathan's comments, in particular:\n>\n> Nice. Thanks much. I only have a few small worries left:\n>\n> [...]\n>> +++ b/t/t9300-fast-import.sh\n>> @@ -2635,4 +2635,280 @@ test_expect_success \\\n> [...]\n>> +test_expect_success 'S: filemodify with garbage after sha1 must fail' '\n>> +\tsha1=$(grep -w :103 marks | cut -d\\ -f2) &&\n>\n> \"grep -w\" isn't used elsewhere in the testsuite. Is it portable?\n\nIt is not portable enough.\n\n> If I understood the discussion before correctly, this error message is\n> suboptimal and something like \"invalid dataref\" would be a little\n> clearer, right?\n>\n> That's orthogonal to what this patch is about so I'm not suggesting\n> changing it. But shouldn't the test just check that fast-import fails\n> without committing to any particular message?\n\nThat would certainly make more sense.\n\nThanks for being extra careful.\n"},{"id":"188739","messageId":"20120407225920.GA3948@padd.com","threadId":"30123","inReplyTo":"20120405015121.GA10945@padd.com","subject":"[PATCHv4] fast-import: tighten parsing of datarefs","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-04-07T22:59:20Z","receivedAt":"2012-04-07T22:59:20Z","isPatch":false,"sender":{"key":"pw@padd.com","avatar":null},"body":"The syntax for the use of mark references in fast-import\ndemands either a SP (space) or LF (end-of-line) after\na mark reference. Fast-import does not complain when garbage\nappears after a mark reference in some cases.\n\nFactor out parsing of mark references and complain if\nerrant characters are found. Also be a little more careful\nwhen parsing \"inline\" and SHA1s, complaining if extra\ncharacters appear or if the form of the dataref is unrecognized.\n\nBuggy input can cause fast-import to produce the wrong output,\nsilently, without error. This makes it difficult to track\ndown buggy generators of fast-import streams. An example is\nseen in the last line of this commit command:\n\n commit refs/heads/S2\n committer Name 1112912893 -0400\n data <\n---\n\nThanks for the patient comments. Changes from v3:\n\n - say \"invalid dataref\" when neither a sha1 nor\n \"inline \" for a mark could be correctly parsed, including\n requisite spaces\n\n - avoid \"grep -w\": rearrange marks used in the tests\n so they are easily greppable without -w\n\n fast-import.c | 110 +++++++++++++------\n t/t9300-fast-import.sh | 287 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 364 insertions(+), 33 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex a85275d..7f1fbed 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2207,6 +2207,59 @@ static uintmax_t change_note_fanout(struct tree_entry *root,\n \treturn do_change_note_fanout(root, root, hex_sha1, 0, path, 0, fanout);\n }\n \n+/*\n+ * Given a pointer into a string, parse a mark reference:\n+ *\n+ * idnum ::= ':' bigint;\n+ *\n+ * Return the first character after the value in *endptr.\n+ *\n+ * Complain if the following character is not what is expected,\n+ * either a space or end of the string.\n+ */\n+static uintmax_t parse_mark_ref(const char *p, char **endptr)\n+{\n+\tuintmax_t mark;\n+\n+\tassert(*p == ':');\n+\t++p;\n+\tmark = strtoumax(p, endptr, 10);\n+\tif (*endptr == p)\n+\t\tdie(\"No value after ':' in mark: %s\", command_buf.buf);\n+\treturn mark;\n+}\n+\n+/*\n+ * Parse the mark reference, and complain if this is not the end of\n+ * the string.\n+ */\n+static uintmax_t parse_mark_ref_eol(const char *p)\n+{\n+\tchar *end;\n+\tuintmax_t mark;\n+\n+\tmark = parse_mark_ref(p, &end);\n+\tif (*end != '\\0')\n+\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n+\treturn mark;\n+}\n+\n+/*\n+ * Parse the mark reference, demanding a trailing space. Return a\n+ * pointer to the space.\n+ */\n+static uintmax_t parse_mark_ref_space(const char **p)\n+{\n+\tuintmax_t mark;\n+\tchar *end;\n+\n+\tmark = parse_mark_ref(*p, &end);\n+\tif (*end != ' ')\n+\t\tdie(\"Missing space after mark: %s\", command_buf.buf);\n+\t*p = end;\n+\treturn mark;\n+}\n+\n static void file_change_m(struct branch *b)\n {\n \tconst char *p = command_buf.buf + 2;\n@@ -2235,21 +2288,21 @@ static void file_change_m(struct branch *b)\n \t}\n \n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n+\t\toe = find_mark(parse_mark_ref_space(&p));\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t\tp = x;\n-\t} else if (!prefixcmp(p, \"inline\")) {\n+\t} else if (!prefixcmp(p, \"inline \")) {\n \t\tinline_data = 1;\n-\t\tp += 6;\n+\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n-\t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n+\t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\tassert(*p == ' ');\n+\t++p; /* skip space */\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -2407,21 +2460,21 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t/* Now parse the notemodify command. */\n \t/* or 'inline' */\n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n+\t\toe = find_mark(parse_mark_ref_space(&p));\n \t\thashcpy(sha1, oe->idx.sha1);\n-\t\tp = x;\n-\t} else if (!prefixcmp(p, \"inline\")) {\n+\t} else if (!prefixcmp(p, \"inline \")) {\n \t\tinline_data = 1;\n-\t\tp += 6;\n+\t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n-\t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n+\t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n \t\tp += 40;\n+\t\tif (*p != ' ')\n+\t\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n \t}\n-\tif (*p++ != ' ')\n-\t\tdie(\"Missing space after SHA1: %s\", command_buf.buf);\n+\tassert(*p == ' ');\n+\t++p; /* skip space */\n \n \t/* */\n \ts = lookup_branch(p);\n@@ -2430,7 +2483,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t\t\tdie(\"Can't add a note on empty branch.\");\n \t\thashcpy(commit_sha1, s->sha1);\n \t} else if (*p == ':') {\n-\t\tuintmax_t commit_mark = strtoumax(p + 1, NULL, 10);\n+\t\tuintmax_t commit_mark = parse_mark_ref_eol(p);\n \t\tstruct object_entry *commit_oe = find_mark(commit_mark);\n \t\tif (commit_oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", commit_mark);\n@@ -2537,7 +2590,7 @@ static int parse_from(struct branch *b)\n \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n \t} else if (*from == ':') {\n-\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\tstruct object_entry *oe = find_mark(idnum);\n \t\tif (oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2572,7 +2625,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\tif (s)\n \t\t\thashcpy(n->sha1, s->sha1);\n \t\telse if (*from == ':') {\n-\t\t\tuintmax_t idnum = strtoumax(from + 1, NULL, 10);\n+\t\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\t\tstruct object_entry *oe = find_mark(idnum);\n \t\t\tif (oe->type != OBJ_COMMIT)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n@@ -2735,7 +2788,7 @@ static void parse_new_tag(void)\n \t\ttype = OBJ_COMMIT;\n \t} else if (*from == ':') {\n \t\tstruct object_entry *oe;\n-\t\tfrom_mark = strtoumax(from + 1, NULL, 10);\n+\t\tfrom_mark = parse_mark_ref_eol(from);\n \t\toe = find_mark(from_mark);\n \t\ttype = oe->type;\n \t\thashcpy(sha1, oe->idx.sha1);\n@@ -2867,18 +2920,13 @@ static void parse_cat_blob(void)\n \t/* cat-blob SP LF */\n \tp = command_buf.buf + strlen(\"cat-blob \");\n \tif (*p == ':') {\n-\t\tchar *x;\n-\t\toe = find_mark(strtoumax(p + 1, &x, 10));\n-\t\tif (x == p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\toe = find_mark(parse_mark_ref_eol(p));\n \t\tif (!oe)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n-\t\tif (*x)\n-\t\t\tdie(\"Garbage after mark: %s\", command_buf.buf);\n \t\thashcpy(sha1, oe->idx.sha1);\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n-\t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n+\t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\tif (p[40])\n \t\t\tdie(\"Garbage after SHA1: %s\", command_buf.buf);\n \t\toe = find_object(sha1);\n@@ -2944,17 +2992,13 @@ static struct object_entry *parse_treeish_dataref(const char **p)\n \tstruct object_entry *e;\n \n \tif (**p == ':') {\t/* */\n-\t\tchar *endptr;\n-\t\te = find_mark(strtoumax(*p + 1, &endptr, 10));\n-\t\tif (endptr == *p + 1)\n-\t\t\tdie(\"Invalid mark: %s\", command_buf.buf);\n+\t\te = find_mark(parse_mark_ref_space(p));\n \t\tif (!e)\n \t\t\tdie(\"Unknown mark: %s\", command_buf.buf);\n-\t\t*p = endptr;\n \t\thashcpy(sha1, e->idx.sha1);\n \t} else {\t/* */\n \t\tif (get_sha1_hex(*p, sha1))\n-\t\t\tdie(\"Invalid SHA1: %s\", command_buf.buf);\n+\t\t\tdie(\"Invalid dataref: %s\", command_buf.buf);\n \t\te = find_object(sha1);\n \t\t*p += 40;\n \t}\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 0f5b5e5..0125413 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2634,5 +2634,292 @@ test_expect_success \\\n \t'R: blob appears only once' \\\n \t'n=$(grep $a verify | wc -l) &&\n \t test 1 = $n'\n+ \n+###\n+### series S\n+###\n+#\n+# Make sure missing spaces and EOLs after mark references\n+# cause errors.\n+#\n+# Setup:\n+#\n+# 1--2--4\n+# \\ /\n+# -3-\n+#\n+# commit marks: 301, 302, 303, 304\n+# blob marks: 403, 404, resp.\n+# note mark: 202\n+#\n+# The error message when a space is missing not at the\n+# end of the line is:\n+#\n+# Missing space after ..\n+#\n+# or when extra characters come after the mark at the end\n+# of the line:\n+#\n+# Garbage after ..\n+#\n+# or when the dataref is neither \"inline \" or a known SHA1,\n+#\n+# Invalid dataref ..\n+#\n+test_tick\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data < $GIT_COMMITTER_DATE\n+data <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/Snotes\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err\n+\tcommit refs/heads/S2\n+\tmark :303\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcommit refs/heads/S\n+\tmark :304\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\ttag refs/tags/Stag\n+\tfrom :302x\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <err &&\n+\tcat-blob :403x\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"after mark\" err\n+'\n+\n+#\n+# ls markref\n+#\n+test_expect_success 'S: ls with garbage after mark must fail' '\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls :302x hello.c\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"space after mark\" err\n+'\n+\n+test_expect_success 'S: ls with garbage after sha1 must fail' '\n+\tsha1=$(grep :302 marks | cut -d\\ -f2) &&\n+\ttest_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&\n+\tls ${sha1}x hello.c\n+\tEOF\n+\tcat err &&\n+\ttest_i18ngrep \"space after tree-ish\" err\n+'\n \n test_done\n-- \n1.7.10.rc2.65.gbd86d\n"},{"id":"188911","messageId":"7vzkajnu3i.fsf@alter.siamese.dyndns.org","threadId":"30123","inReplyTo":"20120407225920.GA3948@padd.com","subject":"Re: [PATCHv4] fast-import: tighten parsing of datarefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-10T21:40:49Z","receivedAt":"2012-04-10T21:40:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pete Wyckoff writes:\n\n> The syntax for the use of mark references in fast-import\n> demands either a SP (space) or LF (end-of-line) after\n> a mark reference. Fast-import does not complain when garbage\n> appears after a mark reference in some cases.\n> \n> Factor out parsing of mark references and complain if\n> errant characters are found. Also be a little more careful\n> when parsing \"inline\" and SHA1s, complaining if extra\n> characters appear or if the form of the dataref is unrecognized.\n\n\n> +static uintmax_t parse_mark_ref(const char *p, char **endptr)\n> +{\n> +\tuintmax_t mark;\n> +\n> +\tassert(*p == ':');\n> +\t++p;\n> +\tmark = strtoumax(p, endptr, 10);\n> +\tif (*endptr == p)\n> +\t\tdie(\"No value after ':' in mark: %s\", command_buf.buf);\n> +\treturn mark;\n> +}\n\n> +static uintmax_t parse_mark_ref_eol(const char *p)\n> +{\n> +...\n> +}\n> +\n> +static uintmax_t parse_mark_ref_space(const char **p)\n> +{\n> +...\n> +}\n> +\n\nThe first helper looks sensible, but the two seemingly similar\nparse_mark_ref_WHATTOEXPECT() that have different interfaces are somewhat\ntasteless.\n\nI wonder if the calling sites in file_change_m(), note_change_n(),\nparse_merge(), parse_cat_blob() and parse_treeish_dataref() can be made to\nshare even more code by slight restructuring, though.\n"}]}