SP in committer line in fast-import stream

SP in committer line in fast-import stream

[SASAKI Suguru]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

Hi,


I'm working with data from `bzr fast-export` and `git fast-import`.
(bzr is 2.4b5, git is 1.7.5.4, on Debian GNU/Linux (sid))

Export and import themselves are OK,
but `git fsck --strict` exits with error, saying:

  error in commit 2e7a16fbe57b555c1c5954470ef66f3a2a089288: invalid
author/committer line - missing space before email

and pushing to remote like GitHub fails.


I found minimal OK-data unlike `bzr fast-export` outputs and NG-data
like `bzr fast-export`.
(Attached: test_NG.data.txt and test_OK.data.txt)

Only one difference between these is a space in committer line.
  * OK: 'committer' SP SP LT GT ...
  * NG: 'committer' SP    LT GT ...

`man git-fast-import` says:

  commit
    Create or update a branch with a new commit, recording one logical
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
      data
      ('from' SP <committish> LF)?
      ('merge' SP <committish> LF)?
      (filemodify | filedelete | filecopy | filerename | filedeleteall
| notemodify)*
      LF?

I think, from this notations, both data is OK.
What's the problem?

Regards,

-- 
SASAKI Suguru
  mailto:sss.sonik@gmail.com

[-- Attachment #2: test_OK.data.txt --]
[-- Type: text/plain, Size: 117 bytes --]

commit refs/heads/master
mark :1
committer  <> 1162316103 +0000
data 4
test
M 644 inline README
data 11
This is OK.


[-- Attachment #3: test_NG.data.txt --]
[-- Type: text/plain, Size: 116 bytes --]

commit refs/heads/master
mark :1
committer <> 1162316103 +0000
data 4
test
M 644 inline README
data 11
This is NG.

Re: SP in committer line in fast-import stream

[Dmitry Ivankov]

Hi,

SASAKI Suguru <sss.sonik <at> gmail.com> writes:

> 
> Hi,
> 
> I'm working with data from `bzr fast-export` and `git fast-import`.
> (bzr is 2.4b5, git is 1.7.5.4, on Debian GNU/Linux (sid))
> 
> Export and import themselves are OK,
> but `git fsck --strict` exits with error, saying:
> 
>   error in commit 2e7a16fbe57b555c1c5954470ef66f3a2a089288: invalid
> author/committer line - missing space before email
> 
> and pushing to remote like GitHub fails.
> 
> I found minimal OK-data unlike `bzr fast-export` outputs and NG-data
> like `bzr fast-export`.
> (Attached: test_NG.data.txt and test_OK.data.txt)
> 
> Only one difference between these is a space in committer line.
>   * OK: 'committer' SP SP LT GT ...
>   * NG: 'committer' SP    LT GT ...
> 
> `man git-fast-import` says:
> 
>   commit
>     Create or update a branch with a new commit, recording one logical
> 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
>       data
>       ('from' SP <committish> LF)?
>       ('merge' SP <committish> LF)?
>       (filemodify | filedelete | filecopy | filerename | filedeleteall
> | notemodify)*
>       LF?
> 
> I think, from this notations, both data is OK.
> What's the problem?
The problem is with git-fast-import that it doesn't verify the format strictly 
here.
For example following (no LT) will pass:
<name> SP <email> GT 
The second problem is that it generates "bad" committer, in fact name-email is 
used as-is, so at least it should convert absent name to a empty name. Or maybe 
just fix the format to make string obligatory.
There even is a third minor problem, fsck will report confusing "missing space" 
for the no-LT example.

Third one is a clear.
Your one is the second one, while internally it pulls the first one too.

The shortest fix is to read documentation as
'committer' SP <name> SP LT <email> GT SP <when> LF


> 
> Regards,
> 

Re: SP in committer line in fast-import stream

[SASAKI Suguru]

Hi,


2011-07-19 Dmitry Ivankov <divanorama@gmail.com>:
> The problem is with git-fast-import that it doesn't verify the format strictly
> here.
> For example following (no LT) will pass:
> <name> SP <email> GT
> The second problem is that it generates "bad" committer, in fact name-email is
> used as-is, so at least it should convert absent name to a empty name. Or maybe
> just fix the format to make string obligatory.
> There even is a third minor problem, fsck will report confusing "missing space"
> for the no-LT example.
>
> Third one is a clear.
> Your one is the second one, while internally it pulls the first one too.
>
> The shortest fix is to read documentation as
> 'committer' SP <name> SP LT <email> GT SP <when> LF

Thanks.  I understand what happens.
For now, I'll write some wrapper around git-fast-import as a workaound for this.

But, if git-fast-import successfully import and git-fsck will confuse,
aren't some fixes necessary?
It might be too done if git-fast-import will check as if git-fsck does,
but I think some simple checks will help us.

Any comments?


Regards,

-- 
SASAKI Suguru
  mailto:sss.sonik@gmail.com

Re: SP in committer line in fast-import stream

[Dmitry Ivankov]

SASAKI Suguru <sss.sonik <at> gmail.com> writes:

> >
> > The shortest fix is to read documentation as
> > 'committer' SP <name> SP LT <email> GT SP <when> LF
> 
> Thanks.  I understand what happens.
> For now, I'll write some wrapper around git-fast-import as a workaound for 
this.
> 
> But, if git-fast-import successfully import and git-fsck will confuse,
> aren't some fixes necessary?
> It might be too done if git-fast-import will check as if git-fsck does,
> but I think some simple checks will help us.
> 
> Any comments?
One patch is at the bottom, it makes fast-import behave well on proper input 
streams like yours.
Making fast-import stricter is worthy but will be a larger patch and effort. 
I'll try not to forget about and at least to write some failing tests.

> 
> Regards,
> 

Name cannot contain LT or GT and ident comes after SP in fast-import. So 
pretend there was a <empty name> SP if there is no name at all.

Parsing isn't strict still.
diff --git a/fast-import.c b/fast-import.c
index 78d9786..91a90e2 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1971,6 +1971,9 @@ static char *parse_ident(const char *buf)
        size_t name_len;
        char *ident;
 
+       /* ensure there is a space delimiter even if there is no name */
+       if (*buf == '<')
+               --buf;
        gt = strrchr(buf, '>');
        if (!gt)
                die("Missing > in ident string: %s", buf);

Re: SP in committer line in fast-import stream

[SASAKI Suguru]

Hi,


(2011-07-19 01:57), Dmitry Ivankov wrote:
> One patch is at the bottom,
 > it makes fast-import behave well on proper input streams like yours.

Thanks.
fast-import with you patch has worked well on these input streams.

> Making fast-import stricter is worthy but will be a larger patch and effort.

Exactly.

> I'll try not to forget about and at least to write some failing tests.

Addinng tests to t/t9300-fast-import.sh ?
If tests on input streams like these streams of mine will do,
I'll write some failing tests.  Will they?


Regards,

-- 
SASAKI Suguru
   mailto:sss.sonik@gmail.com