MultiFileDiffReader returns trailing content

[craigfurman]

Along with EOF. This is useful for handling mixed diff and non-diff
output. Note that "stray" content between diff files was already
included in the extended headers for the next diff. All this commit does
is return a partial FileDiff, with only extended headers and no actual
diff content, along with an EOF.

---

Fixes #59

[craigfurman]

Oops, I need to fix the tests.

[codecov]

Codecov Report

Merging #60 (b8a97d3) into master (82e3467) will increase coverage by 0.77%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   73.82%   74.59%   +0.77%     
==========================================
  Files           4        4              
  Lines         424      429       +5     
==========================================
+ Hits          313      320       +7     
+ Misses         63       62       -1     
+ Partials       48       47       -1     

Impacted Files	Coverage Δ
diff/parse.go	81.41% <71.42%> (+1.00%)	⬆️

---

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 82e3467...b8a97d3. Read the comment docs.

[mrnugget]

Hmm, I'm not sure yet. This seems a bit brittle and based on implicit side effects.

If I understand it correctly what's happening is that any non-diff content is parsed as "extended headers" into the next diff. But since there is no next diff an error is returned. Right?

What happens if non-diff content is found between two valid diffs? Or before the first diff -won't this break in those case?

Setting "request changes" here because at the very least we're going to need some tests :)

[mrnugget]

Another idea might be to make this API explicit by adding another new method to MultiFileReader, one that does return the already read content alongside a nil *FileDiff pointer and an error.

[craigfurman]

If I understand it correctly what's happening is that any non-diff content is parsed as "extended headers" into the next diff. But since there is no next diff an error is returned. Right?

Yes, this is my mental model of what the existing behaviour is, mostly unchanged by this PR.

What happens if non-diff content is found between two valid diffs? Or before the first diff -won't this break in those case?

I might be using the wrong terminology because of unfamiliarity with this codebase, but AFAIK non-diff content is indistinguishable from extended headers. "non-diff" content at the start of a file, or before a valid diff, will be parsed as extended headers for the following diff. This PR doesn't change any of that - it causes an additional "diff" to be returned at the end with trailing content.

Totally fair comments about brittleness. I can (and will) fix the brittleness aspect with tests, I just wanted to put something in front of maintainer eyes as quickly as possible before spending a lot of time on the idea.

@mrnugget re: adding another method, I can do that if you'd prefer. I was avoiding breaking the API, but extending it seems fine. I can push something that does this.

[mrnugget]

Thanks for adding the tests!

Left a comment, because I think we can maybe make the test case clearer, but not 100% sure.

[mrnugget]

Thanks for this! Feel free to hit the merge button once tests pass.

Edit: Woops, realised that we can only do that. So I'll do that once it's green :D

[craigfurman]

Thanks!