plumbing: Use ReadBytes() rather than ReadSlice()

[lupine]

Fixes #249

[codecov]

Codecov Report

Merging #314 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   77.34%   77.34%           
=======================================
  Files         117      117           
  Lines        8011     8011           
=======================================
  Hits         6196     6196           
  Misses       1156     1156           
  Partials      659      659

Impacted Files	Coverage Δ
plumbing/object/commit.go	72.93% <100%> (ø)	⬆️
plumbing/object/tag.go	73.1% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62ad629...ce4ca45. Read the comment docs.

[mcuadros]

Can you provide a test for it? To avoid duplicate this bug again? A slice with more than 4096 chars without a newline should be enough for test the issue. Thanks!

[hansrodtang]

I am not sure what the original reason for using ReadSlice() was, but ReadBytes() makes copies of the data read, so won't this change put more pressure on the garbage collector?

Or is this deemed not an issue? I haven't looked into how much garbage is generated with the change, so this is just a quick observation that might be unimportant.

[lupine]

@mcuadros the existing test suite seems to rely heavily on a set of external fixtures: github.com/src-d/go-git-fixtures

Do I issue a PR against that repo to get a fixture added?

@hansrodtang it will result in an extra copy per line but we're already doing at least one copy per line, so I don't think it's a major issue - we're not changing the scaling behaviour fundamentally by this change.

[lupine]

Ah, never mind, I found a way to add tests without changing fixtures. Wdyt @mcuadros ? I verified that the tests fail without the fix.

[lupine]

If accepted, will this fix end up in gopkg.in/src-d/go-git.v4 automatically? Or is there a process / timeline for getting it added?

[mcuadros]

@hansrodtang thanks for pointing the extra memory usage, once we have a stable API, we will take care of the performance reviewing the code base to improve this from the root.

@lupine yep, I need update the v4 branch, but I will do it ASAP.

[mcuadros]

@lupine v4 has been updated, https://github.com/src-d/go-git/tree/v4.
BTW if you don't mind... may I know for what are you using go-git?

[lupine]

@mcuadros https://gitlab.com/gitlab-org/es-git-go at the moment. i'm starting to see uses for it elsewhere too, though :D

[lupine]

Thanks so much for the merge!