Fix data handling

[felipec]

The length should be exactly the same.

[felipec]

Please have a look at what constitutes a good commit message

I have hundreds of commits in git.git, I think I know what a good commit message is.

I'm particularity missing why this change is necessary "Fix data handling" doesn't tell me much.

That's why I added the explanation "The length should be exactly the same".

What does that mean?

It means the length of data %d should be the actual length of the data, not len(gitmodules)+1, that's a hack.

The removed comment that is present since day 1 suggests it was always a hack:

wr(b'data %d' % (len(desc)+1)) # wtf?

It should be len(desc) (exactly the same). I thought that part was clear.

The only reason the hack works, is that after the data is written, an extra wr() is issued, essentially adding \n to the stream. But this is wrong, the extra new line should be added to the data (desc).

The reason why that wtf comment is there is that commit messages in git typically end with a new line, therefore the wr() replicated what a typical commit message looks like in the stream, but it doesn't align with the original data (mercurial commits don't have that).

Adding an extra b'\n' to desc does the same thing, but correctly.

BTW, I'm the author of git-remote-hg, I fixed that problem in 2013, and the output is compatible with hg-git, who fixed the problem in 2009: added a missing newline back to git conversion.

I'm not so certain about the .gitmodules, but I bet git adds a newline, which is why the +1 is there, but I don't see you adding a newline in the stream, so presumably this screws up the stream, and thus there's a bug.

Does this explanation answer you questions?

[felipec]

As the contents of .gitmodules is generated to end with a \n, you can drop the gitmodules += b'\n'.

I actually looked at the data and the newline is already included so there wasn't any need to add an extra \n, or use wr(): it should be wr_no_nl().

[frej]

As the contents of .gitmodules is generated to end with a \n, you can drop the gitmodules += b'\n'.

I actually looked at the data and the newline is already included so there wasn't any need to add an extra \n, or use wr(): it should be wr_no_nl().

For readability it is, especially when we go to wr_data().

[felipec]

The updated wr_data() would be readable.

[frej]

Merged in 4edea927fb94e2b1ee9d52cb61a12a59fc60f5d2