{"thread":{"id":"28067","subject":"[PATCH v2 0/5] fixes for committer/author parsing/check","startedAt":"2011-08-11T10:21:05Z","lastAt":"2011-08-11T22:06:59Z","messageCount":7,"participants":["Dmitry Ivankov","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"173274","messageId":"1313058070-4774-1-git-send-email-divanorama@gmail.com","threadId":"28067","inReplyTo":null,"subject":"[PATCH v2 0/5] fixes for committer/author parsing/check","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T10:21:05Z","receivedAt":"2011-08-11T10:21:05Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"This is a second version of [1]. It features one more test for a\nfsck message and reworked commit messages.\n\nAside from fixing a parsing bug in fast-import and improving error\nmessages in git-fsck it makes them both to deny \"name> \"\nidentities.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/178035\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 Documentation/git-fast-import.txt | 4 +-\n fast-import.c | 33 ++++++++-----\n fsck.c | 10 ++--\n t/t1450-fsck.sh | 36 +++++++++++++\n t/t9300-fast-import.sh | 99 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 164 insertions(+), 18 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"173275","messageId":"1313058070-4774-2-git-send-email-divanorama@gmail.com","threadId":"28067","inReplyTo":"1313058070-4774-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v2 1/5] fast-import: add input format tests","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T10:21:06Z","receivedAt":"2011-08-11T10:21:06Z","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 f256475..0844e9d 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 <\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 7cc2262..ed1f7c9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1973,6 +1973,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 0844e9d..8f3938c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -352,7 +352,7 @@ data < SP)? LT GT\nwhere name is any string without LF and LT characters.\nBut fast-import just accepts any string up to first GT\ninstead of checking the whole format, and moreover just\nwrites it as is to the commit object.\n\ngit-fsck checks for [^<\\n]* <[^<>\\n]*> format. Note that the\nspace is mandatory. And the space quirk is already handled via\nextending the string to the left when needed.\n\nModify fast-import input identity format to a slightly stricter\none - deny LF, LT and GT in both and . And check\nfor it.\n\nThis is stricter then git-fsck as fsck accepts \"Name> \"\ncurrently, but soon fsck check will be adjusted likewise.\n\nSigned-off-by: Dmitry Ivankov \n---\n Documentation/git-fast-import.txt | 4 ++--\n fast-import.c | 29 +++++++++++++++++------------\n t/t9300-fast-import.sh | 10 +++++-----\n 3 files changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 2969388..ba16889 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -425,8 +425,8 @@ Here `` is the person's display name (for example\n (``cm@example.com''). `LT` and `GT` are the literal less-than (\\x3c)\n and greater-than (\\x3e) symbols. These are required to delimit\n the email address from the other fields in the line. Note that\n-`` is free-form and may contain any sequence of bytes, except\n-`LT` and `LF`. It is typically UTF-8 encoded.\n+`` and `` are free-form and may contain any sequence\n+of bytes, except `LT`, `GT` and `LF`. `` is typically UTF-8 encoded.\n \n The time of the change is specified by `` using the date format\n that was selected by the \\--date-format= command line option.\ndiff --git a/fast-import.c b/fast-import.c\nindex ed1f7c9..6d491b9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1969,7 +1969,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@@ -1977,28 +1977,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 8f3938c..e53ca90 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>\" or to \"\". It'd be nicer to say \"missing email\" for\nthe second string and \"name is bad\" (has > in it) for the first one.\nAdd a failing test for these messages.\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 | 36 ++++++++++++++++++++++++++++++++++++\n 1 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex bb01d5a..01ccefd 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -110,6 +110,42 @@ 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 'missing email is reported nicely' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/[a-z]* <[^>]*>//\" basis >bad-email-3 &&\n+\tnew=$(git hash-object -t commit -w --stdin out &&\n+\tcat out &&\n+\tgrep \"error in commit $new.* - missing email\" out\n+'\n+\n+test_expect_failure '> in name is reported' '\n+\tgit cat-file commit HEAD >basis &&\n+\tsed \"s/ bad-email-4 &&\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":"173277","messageId":"1313058070-4774-6-git-send-email-divanorama@gmail.com","threadId":"28067","inReplyTo":"1313058070-4774-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v2 5/5] fsck: improve committer/author check","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T10:21:10Z","receivedAt":"2011-08-11T10:21:10Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"fsck allows a name with > character in it like \"name> \". Also for\n\"name email>\" fsck says \"missing space before email\".\n\nMore precisely, it seeks for a first '<', checks that ' ' preceeds it.\nThen seeks to '<' or '>' and checks that it is the '>'. Missing space is\nreported if either '<' is not found or it's not preceeded with ' '.\n\nChange it to following. Seek to '<' or '>', check that it is '<' and is\npreceeded with ' '. Seek to '<' or '>' and check that it is '>'. So now\n\"name> \" is rejected as \"bad name\". More strict name check is the\nonly change in what is accepted.\n\nReport 'missing space' only if '<' is found and is not preceeded with a\nspace.\n\nSigned-off-by: Dmitry Ivankov \n---\n fsck.c | 10 ++++++----\n t/t1450-fsck.sh | 6 +++---\n 2 files changed, 9 insertions(+), 7 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 01ccefd..523ce9c 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 basis &&\n \tsed \"s/[a-z]* <[^>]*>//\" basis >bad-email-3 &&\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-4 &&\n \tnew=$(git hash-object -t commit -w --stdin writes:\n\n> diff --git a/fast-import.c b/fast-import.c\n> index 7cc2262..ed1f7c9 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1973,6 +1973,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\nThis is somewhat cryptic, even though it may be correct, especially if the\nreader of the code does not know that this function is called by the\ncaller after reading \"author \" (or committer/tagger) and buf points at one\nbyte beyond that SP after the string that specifies the kind of the\nperson, hence \"--buf\" [*1*] makes the subsequent strncpy() start copying\nfrom that SP which makes the result correct.\n\nPerhaps an additional comment before the function is in order?\n\nThanks.\n\n[Footnote]\n\n*1* by the way, as pure style thing, I think our codebase favors to use\npost-*crement, i.e. \"buf--\", if you are doing pre/post-*crement purely for\nits side effect.\n"}]}