Fix padding calculation, closes #45

[peternewman]

Fixes #45

Untested, but maths checked externally

Could probably do with some tests to catch such issues in future.

[chabad360]

See the comments regarding the string delimiter

[chabad360]

LGTM.

[chabad360]

You should probably update the PR text to at least say fixes #45, this way it'll be automatically closed. Also, make sure to mark it ready for review.

[chabad360]

Oops, forgot one more thing: writePaddedString() needs to be modified as well, otherwise we still have this issue.

[peternewman]

You should probably update the PR text to at least say fixes #45, this way it'll be automatically closed. Also, make sure to mark it ready for review.

I thought it worked in the title too, but added to both to be sure.

[peternewman]

Oops, forgot one more thing: writePaddedString() needs to be modified as well, otherwise we still have this issue.

I don't see where @chabad360 . It uses the count from writeString which presumably already includes the null terminator. Or do you mean we need to ensure one is added?

[chabad360]

We need to ensure one is added.

I would just add

buf.WriteByte(0)
n += 1

right after buf.WriteString() (worked for me).

[peternewman]

We need to ensure one is added.

I would just add

buf.WriteByte(0)
n += 1

right after buf.WriteString() (worked for me).

Won't that get a double one if the string already has one (or is that impossible/guaranteed in Go)? Consider:
't', 'e', 's', '\0'
You'd end up sending:
't', 'e', 's', 0, 0, 0, 0, 0

[chabad360]

Unless you specifically craft a string to include a null byte string([]byte{0}), a string shouldn't contain one. However, because it's not simple to add a null byte efficiently (nor is it idiomatic), it's safer to assume it's not there. Additionally, all the tests I ran (and some analysis) showed that the library doesn't insert any null bytes.

[chabad360]

Alright, LGTM. @hypebeast?

[hypebeast]

Very nice. This PR looks good to me.

Thanks a lot for it. As soon as the build succeeds I'll merge it.

[peternewman]

Very nice. This PR looks good to me.

Thanks @hypebeast . It seems to works for all the test cases @chabad360 came up with and a few edge cases I conjured up too.

Thanks a lot for it.

No worries, not quite the easy fix I first thought, but hopefully should make things better and more standardised all round.

[peternewman]

This now passes aside from https://travis-ci.org/github/hypebeast/go-osc/jobs/764014122#L275-L279 which is also present in master @hypebeast .

Looks like this could fix it, but a bit outside my experience:
ipfs/kubo#2043

[chabad360]

I would say that issue is probably outside the scope of this PR.

[hypebeast]

Yes, this is outside of this PR. Let me see if I can fix it ;)

Thanks again for your work @peternewman and @chabad360