Remove dependency to bufio.Reader in internal carv1 package

[masih]

Remove dependency to bufio.Reader in internal carv1 package that
seems to be mainly used for peeking a byte to return appropriate error
when stream abruptly ends, relating to #36. This allows simplification
of code across the repo and remove all unnecessary wrappings of
io.Reader with bufio.Reader. This will also aid simplify the
internal IO utilities which will be done in future PRs. For now we
simply remove dependency to bufio.Reader.

Relates to:

• Improve EOF handling #36

[rvagg]

oh, so we have a carv1 inside carv2? is that going to be deduped at some point?

I have some memory of this Peek() code, trying to make it give proper errors on EOF vs "can't decode this uvarint". Other than that, I don't see a good reason to hold on to bufio.Reader; so go ahead and yeet it! This looks good to me. I'd be happy to see this migrate into the top level package too, I don't think there's a strong reason to keep it otherwise since that Peek() is not protecting an unbounded reader or anything.

[masih]

oh, so we have a carv1 inside carv2? is that going to be deduped at some point?

Yep the context behind this is captured in #104. TLDR; go-ipld-prime dependency in FileCoin side needs to be upgraded before we can deduplicate. The plan is certainly to deduplicate. The same dependency issue on FileCoin side is blocking the implementation of Selective Car in v2. Forking the v1 into v2 also allows us to do such refactors.

I'd be happy to see this migrate into the top level package too

Yeah that would be nice; the bad thing is we can't roll out breaking changes so we have to leave the buffo.Reader taking APIs in v1 in place. But we can certainly mark them as deprecated and roll out new versions. Not sure how nice the naming of new functions would be since existing ones are named things like ReadHeader.

[mvdan]

SGTM. For what it's worth I'm not convinced we can simply deduplicate the carv1 packages; the v0 ones in master have APIs that limit what we can do, like func ReadHeader(br *bufio.Reader) (*CarHeader, error) still requiring a buffered reader.

We can certainly try to deduplicate some of it though.

[rvagg]

the bad thing is we can't roll out breaking changes

.. but isn't bufio.Reader a superset of io.Reader? if we switched to just r *io.Reader wouldn't it still work if someone passed in a bufio.Reader?

[masih]

the bad thing is we can't roll out breaking changes

.. but isn't bufio.Reader a superset of io.Reader? if we switched to just r *io.Reader wouldn't it still work if someone passed in a bufio.Reader?

mm good point; is that OK as far as Go API etiquette goes? @mvdan what do you think?

[mvdan]

It's technically a breaking change, as one could e.g. pass ReadHeader as a parameter to a func that expects the bufio type.

It's likely that it would break noone in practice though, so we could always try and see. It's a v0 after all. I haven't checked if any other bits of the API needed changing for their uses in carv2.