{"thread":{"id":"27941","subject":"[PATCH 0/5] fixes for committer/author parsing/check","startedAt":"2011-07-28T05:43:59Z","lastAt":"2011-08-02T19:50:20Z","messageCount":11,"participants":["Dmitry Ivankov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"172228","messageId":"1311831844-13123-1-git-send-email-divanorama@gmail.com","threadId":"27941","inReplyTo":null,"subject":"[PATCH 0/5] fixes for committer/author parsing/check","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-07-28T05:43:59Z","receivedAt":"2011-07-28T05:43:59Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"fast-import part is clear and should be safe to apply. The test\nscript is a bit ugly as it uses fsck and prune on a shared (with\nnearby tests) repo. But it should work, any hints on simplifying\nit are welcome.\n\nfsck part fixes an uncaught bad committer, but also changes error\nmessages for some of bad committer strings. The messages choice is\na subject for discussion most likely.\n\nDmitry Ivankov (5):\n fast-import: add input format tests\n fast-import: don't fail on omitted committer name\n fast-import: check committer name more strictly\n fsck: add a few committer name tests\n fsck: improve committer/author check\n\n fast-import.c | 33 ++++++++++------\n fsck.c | 10 +++--\n t/t1450-fsck.sh | 24 ++++++++++++\n t/t9300-fast-import.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 150 insertions(+), 16 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"172229","messageId":"1311831844-13123-2-git-send-email-divanorama@gmail.com","threadId":"27941","inReplyTo":"1311831844-13123-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 1/5] fast-import: add input format tests","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-07-28T05:44:00Z","receivedAt":"2011-07-28T05:44:00Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Documentation/git-fast-import.txt says that git-fast-import is strict\nabout it's input format. But committer/author field parsing is a bit\nloose. Invalid values can be unnoticed and written out to the commit,\neither with format-conforming input or with non-format-conforming one.\n\nAdd one passing and one failing test for empty/absent committer name\nwith well-formed input. And a failed test with unnoticed ill-formed\ninput.\n\nReported-by: SASAKI Suguru \nSigned-off-by: Dmitry Ivankov \n---\n t/t9300-fast-import.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 99 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 2a53640..a659dd4 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -324,6 +324,105 @@ test_expect_success \\\n \t test `git rev-parse master` = `git rev-parse TEMP_TAG^`'\n rm -f .git/TEMP_TAG\n \n+git gc 2>/dev/null >/dev/null\n+git prune 2>/dev/null >/dev/null\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data </dev/null >/dev/null\n+git prune 2>/dev/null >/dev/null\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data </dev/null >/dev/null\n+git prune 2>/dev/null >/dev/null\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data <input <> $GIT_COMMITTER_DATE\n+data <input <input < $GIT_COMMITTER_DATE\n+data <' ident was passed.\n\nThis fixes a ident parsing bug for a well-formed fast-import input.\nThough the parsing is still loose and can accept a ill-formed input.\n\nSigned-off-by: Dmitry Ivankov \n---\n fast-import.c | 4 ++++\n t/t9300-fast-import.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 9e8d186..3194f4e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)\n \tsize_t name_len;\n \tchar *ident;\n \n+\t/* ensure there is a space delimiter even if there is no name */\n+\tif (*buf == '<')\n+\t\t--buf;\n+\n \tgt = strrchr(buf, '>');\n \tif (!gt)\n \t\tdie(\"Missing > in ident string: %s\", buf);\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex a659dd4..09ef6ba 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -352,7 +352,7 @@ data <\n---\n fast-import.c | 29 +++++++++++++++++------------\n t/t9300-fast-import.sh | 10 +++++-----\n 2 files changed, 22 insertions(+), 17 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 3194f4e..419744f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1968,7 +1968,7 @@ static int validate_raw_date(const char *src, char *result, int maxlen)\n \n static char *parse_ident(const char *buf)\n {\n-\tconst char *gt;\n+\tconst char *ltgt;\n \tsize_t name_len;\n \tchar *ident;\n \n@@ -1976,28 +1976,33 @@ static char *parse_ident(const char *buf)\n \tif (*buf == '<')\n \t\t--buf;\n \n-\tgt = strrchr(buf, '>');\n-\tif (!gt)\n+\tltgt = buf + strcspn(buf, \"<>\");\n+\tif (*ltgt != '<')\n+\t\tdie(\"Missing < in ident string: %s\", buf);\n+\tif (ltgt != buf && ltgt[-1] != ' ')\n+\t\tdie(\"Missing space before < in ident string: %s\", buf);\n+\tltgt = ltgt + 1 + strcspn(ltgt + 1, \"<>\");\n+\tif (*ltgt != '>')\n \t\tdie(\"Missing > in ident string: %s\", buf);\n-\tgt++;\n-\tif (*gt != ' ')\n+\tltgt++;\n+\tif (*ltgt != ' ')\n \t\tdie(\"Missing space after > in ident string: %s\", buf);\n-\tgt++;\n-\tname_len = gt - buf;\n+\tltgt++;\n+\tname_len = ltgt - buf;\n \tident = xmalloc(name_len + 24);\n \tstrncpy(ident, buf, name_len);\n \n \tswitch (whenspec) {\n \tcase WHENSPEC_RAW:\n-\t\tif (validate_raw_date(gt, ident + name_len, 24) < 0)\n-\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", gt, buf);\n+\t\tif (validate_raw_date(ltgt, ident + name_len, 24) < 0)\n+\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_RFC2822:\n-\t\tif (parse_date(gt, ident + name_len, 24) < 0)\n-\t\t\tdie(\"Invalid rfc2822 date \\\"%s\\\" in ident: %s\", gt, buf);\n+\t\tif (parse_date(ltgt, ident + name_len, 24) < 0)\n+\t\t\tdie(\"Invalid rfc2822 date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_NOW:\n-\t\tif (strcmp(\"now\", gt))\n+\t\tif (strcmp(\"now\", ltgt))\n \t\t\tdie(\"Date in ident must be 'now': %s\", buf);\n \t\tdatestamp(ident + name_len, 24);\n \t\tbreak;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 09ef6ba..18441f8 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -370,7 +370,7 @@ data <\" for committer string equal\nto \"name email>\". It'd be nicer to say \"missing < in email\" or \"name is\nbad\" (has > in it). The second option looks a bit better, add such a\nfailing test.\n\nFor \"name> \" no error is reported. Looks like a bug, so add\nsuch a failing test.\"\n\nSigned-off-by: Dmitry Ivankov \n---\n t/t1450-fsck.sh | 24 ++++++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex bb01d5a..fc7ee8e 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -110,6 +110,30 @@ test_expect_success 'email with embedded > is not okay' '\n \tgrep \"error in commit $new\" out\n '\n \n+test_expect_failure 'missing < email delimiter is reported nicely' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/bad-email-2 &&\n+\tnew=$(git hash-object -t commit -w --stdin out &&\n+\tcat out &&\n+\tgrep \"error in commit $new.* - bad name\" out\n+'\n+\n+test_expect_failure '> in name is reported' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/ bad-email-3 &&\n+\tnew=$(git hash-object -t commit -w --stdin out &&\n+\tcat out &&\n+\tgrep \"error in commit $new\" out\n+'\n+\n test_expect_success 'tag pointing to nonexistent' '\n \tcat >invalid-tag <<-\\EOF &&\n \tobject ffffffffffffffffffffffffffffffffffffffff\n-- \n1.7.3.4\n"},{"id":"172233","messageId":"1311831844-13123-6-git-send-email-divanorama@gmail.com","threadId":"27941","inReplyTo":"1311831844-13123-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 5/5] fsck: improve committer/author check","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-07-28T05:44:04Z","receivedAt":"2011-07-28T05:44:04Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Neither name nor email should contain < or >, so split the string with\nthese and check they come in that order and < is preceeded with a space.\n\nIf < is missing don't say a confusing \"missing space\", say \"bad name\" if\n> isn't missing and \"missing email\" if both < and > are missing.\n\nSigned-off-by: Dmitry Ivankov \n---\n fsck.c | 10 ++++++----\n t/t1450-fsck.sh | 4 ++--\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 60bd4bb..6c855f8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -224,13 +224,15 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \n static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n {\n-\tif (**ident == '<' || **ident == '\\n')\n-\t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n-\t*ident += strcspn(*ident, \"<\\n\");\n-\tif ((*ident)[-1] != ' ')\n+\tif (**ident == '<')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n+\t*ident += strcspn(*ident, \"<>\\n\");\n+\tif (**ident == '>')\n+\t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - bad name\");\n \tif (**ident != '<')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing email\");\n+\tif ((*ident)[-1] != ' ')\n+\t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n \t(*ident)++;\n \t*ident += strcspn(*ident, \"<>\\n\");\n \tif (**ident != '>')\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex fc7ee8e..0b92d0f 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -110,7 +110,7 @@ test_expect_success 'email with embedded > is not okay' '\n \tgrep \"error in commit $new\" out\n '\n \n-test_expect_failure 'missing < email delimiter is reported nicely' '\n+test_expect_success 'missing < email delimiter is reported nicely' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/bad-email-2 &&\n \tnew=$(git hash-object -t commit -w --stdin in name is reported' '\n+test_expect_success '> in name is reported' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/ bad-email-3 &&\n \tnew=$(git hash-object -t commit -w --stdin writes:\n\n> fast-import format declares 'committer_name SP' to be optional. But SP\n> between empty or not name and a email is obligatory and checked by\n\nSorry, cannot parse this.\n\n> git-fsck, so fast-import must prepend the SP if the name is omitted.\n> Currently it doesn't.\n>\n> Name cannot contain LT or GT and ident always comes after SP in\n> fast-import. So reuse that SP as if a valid 'SP ' ident was passed.\n>\n> This fixes a ident parsing bug for a well-formed fast-import input.\n> Though the parsing is still loose and can accept a ill-formed input.\n>\n> Signed-off-by: Dmitry Ivankov \n> ---\n> fast-import.c | 4 ++++\n> t/t9300-fast-import.sh | 2 +-\n> 2 files changed, 5 insertions(+), 1 deletions(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index 9e8d186..3194f4e 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)\n> \tsize_t name_len;\n> \tchar *ident;\n> \n> +\t/* ensure there is a space delimiter even if there is no name */\n> +\tif (*buf == '<')\n> +\t\t--buf;\n> +\n> \tgt = strrchr(buf, '>');\n> \tif (!gt)\n> \t\tdie(\"Missing > in ident string: %s\", buf);\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index a659dd4..09ef6ba 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -352,7 +352,7 @@ data < empty commit\n> COMMIT\n> INPUT_END\n> -test_expect_failure 'B: accept and fixup committer with no name' '\n> +test_expect_success 'B: accept and fixup committer with no name' '\n> \tgit fast-import \tout=$(git fsck) &&\n> \techo \"$out\" &&\n"},{"id":"172671","messageId":"7vfwljg33p.fsf@alter.siamese.dyndns.org","threadId":"27941","inReplyTo":"1311831844-13123-6-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 5/5] fsck: improve committer/author check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T17:00:58Z","receivedAt":"2011-08-02T17:00:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Ivankov writes:\n\n> Neither name nor email should contain < or >, so split the string with\n> these and check they come in that order and < is preceeded with a space.\n>\n> If < is missing don't say a confusing \"missing space\", say \"bad name\" if\n>> isn't missing and \"missing email\" if both < and > are missing.\n>\n> Signed-off-by: Dmitry Ivankov \n> ---\n\nSame comment as 3/5; before starting to talk about how you implemented\nyour validation, please state what rules you are enforcing.\n\nThanks.\n"},{"id":"172672","messageId":"7v8vrbg32f.fsf@alter.siamese.dyndns.org","threadId":"27941","inReplyTo":"1311831844-13123-4-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 3/5] fast-import: check committer name more strictly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T17:01:44Z","receivedAt":"2011-08-02T17:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Ivankov writes:\n\n> Signed-off-by: Dmitry Ivankov \n> ---\n\nPlease describe how you check this field \"more strictly\" in the body of\nthe log message, iow, against what rule you are validating, perhaps\nsomething like:\n\n The identifier must be either \"
\" or \"Name
\" where\n neither address nor Name can contain '<' nor '>'; otherwise the input\n stream is rejected.\n\nAs fast-import is used to _create_ new objects, its input is a simple text\nfile that can be fixed-up as needed, it is a good idea to validate the\ninput more strictly and rejecting bad ones.\n"},{"id":"172673","messageId":"CA+gfSn-15+z+PYhFH6=RQF7HKC7i_1p=fyJqtepv3xPdUnTVdQ@mail.gmail.com","threadId":"27941","inReplyTo":"7voc07g3fr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/5] fast-import: don't fail on omitted committer name","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-02T17:07:25Z","receivedAt":"2011-08-02T17:07:25Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Tue, Aug 2, 2011 at 10:53 PM, Junio C Hamano wrote:\n> Dmitry Ivankov writes:\n>\n>> fast-import format declares 'committer_name SP' to be optional. But SP\n>> between empty or not name and a email is obligatory and checked by\n>\n> Sorry, cannot parse this.\nOk, the point is fast-import input format for identities is declared to be\n'(name SP)? LT email GT' (followed by a datetime)\nwhere name and email are allowed to be empty (and should not have\nLF, LT, GT characters).\nWhile git-fsck checks identities to be in form\n'name SP LT email GT' (followed by a datetime)\nwhere name and email are allowed to be empty (and should not have\nLF, LT, GT characters).\nSo fast-import must prepend a space if the name part is omitted. This\npatch makes it do so.\n\n>\n>> git-fsck, so fast-import must prepend the SP if the name is omitted.\n>> Currently it doesn't.\n>>\n>> Name cannot contain LT or GT and ident always comes after SP in\n>> fast-import. So reuse that SP as if a valid 'SP ' ident was passed.\n>>\n>> This fixes a ident parsing bug for a well-formed fast-import input.\n>> Though the parsing is still loose and can accept a ill-formed input.\n>>\n>> Signed-off-by: Dmitry Ivankov \n>> ---\n>>  fast-import.c          |    4 ++++\n>>  t/t9300-fast-import.sh |    2 +-\n>>  2 files changed, 5 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/fast-import.c b/fast-import.c\n>> index 9e8d186..3194f4e 100644\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)\n>>       size_t name_len;\n>>       char *ident;\n>>\n>> +     /* ensure there is a space delimiter even if there is no name */\n>> +     if (*buf == '<')\n>> +             --buf;\n>> +\n>>       gt = strrchr(buf, '>');\n>>       if (!gt)\n>>               die(\"Missing > in ident string: %s\", buf);\n>> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n>> index a659dd4..09ef6ba 100755\n>> --- a/t/t9300-fast-import.sh\n>> +++ b/t/t9300-fast-import.sh\n>> @@ -352,7 +352,7 @@ data <>  empty commit\n>>  COMMIT\n>>  INPUT_END\n>> -test_expect_failure 'B: accept and fixup committer with no name' '\n>> +test_expect_success 'B: accept and fixup committer with no name' '\n>>       git fast-import >       out=$(git fsck) &&\n>>       echo \"$out\" &&\n>\n"},{"id":"172694","messageId":"CA+gfSn8xp3yj0Odu-7DSQyPq4mxVOdOzpS98nSx9gUpfrx-5Jg@mail.gmail.com","threadId":"27941","inReplyTo":"7vfwljg33p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/5] fsck: improve committer/author check","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-02T19:50:20Z","receivedAt":"2011-08-02T19:50:20Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Tue, Aug 2, 2011 at 11:00 PM, Junio C Hamano wrote:\n> Dmitry Ivankov writes:\n>\n>> Neither name nor email should contain < or >, so split the string with\n>> these and check they come in that order and < is preceeded with a space.\n>>\n>> If < is missing don't say a confusing \"missing space\", say \"bad name\" if\n>>> isn't missing and \"missing email\" if both < and > are missing.\n>>\n>> Signed-off-by: Dmitry Ivankov \n>> ---\n>\n> Same comment as 3/5; before starting to talk about how you implemented\n> your validation, please state what rules you are enforcing.\nThanks, will apply and reroll.\nBut before this, is it ok to reject \"Name> \" idents in fsck and\nfast-import?\nfsck already denies '<' and '>' inside email, and '<' in name. But\naccepts '>' in name.\nIt just hit me that Documentation/fast-import.c doesn't deny '>' in\nname and it is\nconsistent with fsck, so there may be a reason behind it.\n\n>\n> Thanks.\n>\n"}]}