threads / discuss / 27845

SP in committer line in fast-import stream

Subject: SP in committer line in fast-import stream

## tl;dr

5 messages between Jul 18, 2011 and Jul 18, 2011.

replies: 4people: 2as markdown or json

SASAKI Suguru· Jul 18, 2011, 14:26 UTC · lore

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


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



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

Dmitry Ivankov· Jul 18, 2011, 15:38 UTC · re: SASAKI Suguru · lore

Re: SP in committer line in fast-import stream

Hi,

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

Show 41 quoted lines

> 
> 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,
>

SASAKI Suguru· Jul 18, 2011, 16:18 UTC · re: Dmitry Ivankov · lore

Re: SP in committer line in fast-import stream

Hi,

2011-07-19 Dmitry Ivankov <divanorama@gmail.com>:

Show 15 quoted lines

> 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

Dmitry Ivankov· Jul 18, 2011, 16:57 UTC · re: SASAKI Suguru · lore

Re: SP in committer line in fast-import stream

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

Show 6 quoted lines

> >
> > 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.

Show 7 quoted lines

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

SASAKI Suguru· Jul 18, 2011, 19:10 UTC · re: Dmitry Ivankov · lore

Re: SP in committer line in fast-import stream

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