git fast-import not verifying commit author lines?

mtn git_export produces this output on a simple repo:

blob
mark :1
data 8
content

commit refs/heads/com.example.badexport
mark :2
author <somename> 1261454209 +0000
committer <somename> 1261454209 +0000
data 8
acommit

M 100644 :1 "afile"
progress revision 9b0e11e4d66eba8a3cf26095fb573116b886cd37 (1/1)
#############################################################

The author and committer lines are missing the names (I've filed this as a
bug with monotone).  git commit-tree refuses to to produce a commit object
like this, so it seems like git fast-import should detect and report this
instead of silently writing the invalid commit object to the repository.

--David

David Reiss [off-list ref] wrote:

mtn git_export produces this output on a simple repo:

blob
mark :1
data 8
content

commit refs/heads/com.example.badexport
mark :2
author <somename> 1261454209 +0000
committer <somename> 1261454209 +0000
data 8
acommit

M 100644 :1 "afile"
progress revision 9b0e11e4d66eba8a3cf26095fb573116b886cd37 (1/1)
#############################################################

The author and committer lines are missing the names (I've filed this as a
bug with monotone).  git commit-tree refuses to to produce a commit object
like this, so it seems like git fast-import should detect and report this
instead of silently writing the invalid commit object to the repository.

Nope.

These objects can still be processed by git commands, you just can't
normally create them with Git tools.  To some extent fast-import
allows the caller to import data that you wouldn't otherwise create
in Git because we know you are coming from a foreign system where
the data might not reasonably exist.

-- 
Shawn.

quoted

author <somename> 1261454209 +0000
committer <somename> 1261454209 +0000

a foreign system where the data might not reasonably exist.

But shouldn't there still be an extra space?  One to separate "author"
from the empty name, and one to separate the empty name from the email?
If not, then I think this change should be made.  (I couldn't find any
authoritative documentation on what constitutes a valid commit object.)

(Sorry, this has been sitting in my outbox for a week.)

--David

diff --git i/Documentation/git-fast-import.txt w/Documentation/git-fast-import.txt
index 288032c..6917739 100644
--- i/Documentation/git-fast-import.txt
+++ w/Documentation/git-fast-import.txt

@@ -312,6 +312,6 @@ change to the project.
        'commit' SP <ref> LF
        mark?
-       ('author' SP <name> SP LT <email> GT SP <when> LF)?
-       'committer' SP <name> SP LT <email> GT SP <when> LF
+       ('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
+       'committer' (SP <name>)? SP LT <email> GT SP <when> LF
        data
        ('from' SP <committish> LF)?

The fast-import parser does not validate that the author, committer
or tagger name component contains both a name and an email address.
Therefore the name component has always been optional.  Correct the
documentation to match the implementation.

Signed-off-by: Shawn O. Pearce <redacted>
---
   David Reiss [off-list ref] wrote:
   > >> author <somename> 1261454209 +0000
   > >> committer <somename> 1261454209 +0000
   > > a foreign system where the data might not reasonably exist.
   > But shouldn't there still be an extra space?  One to separate "author"
   > from the empty name, and one to separate the empty name from the email?
   > If not, then I think this change should be made.  (I couldn't find any
   > authoritative documentation on what constitutes a valid commit object.)
   
   Yes, we should do this.
    
 Documentation/git-fast-import.txt |    6 +++---
 fast-import.c                     |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 288032c..e6d364f 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt

@@ -311,8 +311,8 @@ change to the project.
 ....
 	'commit' SP <ref> LF
 	mark?
-	('author' SP <name> SP LT <email> GT SP <when> LF)?
-	'committer' SP <name> SP LT <email> GT SP <when> LF
+	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
+	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
 	data
 	('from' SP <committish> LF)?
 	('merge' SP <committish> LF)?

@@ -657,7 +657,7 @@ lightweight (non-annotated) tags see the `reset` command below.
 ....
 	'tag' SP <name> LF
 	'from' SP <committish> LF
-	'tagger' SP <name> SP LT <email> GT SP <when> LF
+	'tagger' (SP <name>)? SP LT <email> GT SP <when> LF
 	data
 ....
 

diff --git a/fast-import.c b/fast-import.c
index dd3c99d..cd87049 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -19,8 +19,8 @@ Format of STDIN stream:
 
   new_commit ::= 'commit' sp ref_str lf
     mark?
-    ('author' sp name sp '<' email '>' sp when lf)?
-    'committer' sp name sp '<' email '>' sp when lf
+    ('author' (sp name)? sp '<' email '>' sp when lf)?
+    'committer' (sp name)? sp '<' email '>' sp when lf
     commit_msg
     ('from' sp committish lf)?
     ('merge' sp committish lf)*

@@ -47,7 +47,7 @@ Format of STDIN stream:
 
   new_tag ::= 'tag' sp tag_str lf
     'from' sp committish lf
-    ('tagger' sp name sp '<' email '>' sp when lf)?
+    ('tagger' (sp name)? sp '<' email '>' sp when lf)?
     tag_msg;
   tag_msg ::= data;
 

-- 
1.6.6.307.gba67

-- 
Shawn.

"Shawn O. Pearce" [off-list ref] writes:

   David Reiss [off-list ref] wrote:
   > >> author <somename> 1261454209 +0000
   > >> committer <somename> 1261454209 +0000
   > > a foreign system where the data might not reasonably exist.
   > But shouldn't there still be an extra space?  One to separate "author"
   > from the empty name, and one to separate the empty name from the email?
   > If not, then I think this change should be made.  (I couldn't find any
   > authoritative documentation on what constitutes a valid commit object.)
   
   Yes, we should do this.

Thanks.