threads / discuss / 22003

git fast-import not verifying commit author lines?

Subject: git fast-import not verifying commit author lines?

## tl;dr

5 messages between Dec 22, 2009 and Dec 31, 2009.

replies: 4people: 3as markdown or json

David Reiss· Dec 22, 2009, 04:22 UTC · lore

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

Shawn O. Pearce· Dec 22, 2009, 15:06 UTC · re: David Reiss · lore

Re: git fast-import not verifying commit author lines?

David Reiss <dreiss@facebook.com> wrote:

Show 22 quoted 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.

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.

David Reiss· Dec 30, 2009, 06:04 UTC · re: Shawn O. Pearce · lore

Re: git fast-import not verifying commit author lines?

>> 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)?

Shawn O. Pearce· Dec 30, 2009, 15:03 UTC · re: David Reiss · lore

[PATCH] fast-import: Document author/committer/tagger name is optional

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 <spearce@spearce.org>
---
   David Reiss <dreiss@facebook.com> 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.

Junio C Hamano· Dec 31, 2009, 23:08 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] fast-import: Document author/committer/tagger name is optional

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 10 quoted lines

>    David Reiss <dreiss@facebook.com> 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.