refactor(plumbing): improve revfile encode/decode #1750
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the revfile encoder and decoder to improve safety and API design by converting them from reusable objects to pure functions. The changes eliminate potential concurrent misuse by making encoder/decoder types unexported and creating fresh state for each operation, allowing the removal of mutexes and panic recovery.
Key improvements include:
- Encoder and decoder structs are now unexported with pure functional APIs (Encode/Decode functions)
- Enhanced error handling for non-EOF errors during trailing data checks in decoder
- Better EOF context in error messages (e.g., "unexpected EOF at object N")
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plumbing/format/revfile/encoder.go | Converted Encoder to unexported encoder type; exposed Encode function; removed mutex, panic recovery, and unused stateFn field; simplified return in writeRevChecksum |
| plumbing/format/revfile/decoder.go | Converted Decoder to unexported decoder type; exposed Decode function accepting io.Reader; improved EOF error handling with context; fixed bug where non-EOF errors during trailing check were not properly handled |
| plumbing/format/revfile/encoder_test.go | Updated tests to use new Encode function API; removed bufio import; adopted error channel pattern for goroutine error handling |
| plumbing/format/revfile/decoder_test.go | Updated tests to use new Decode function API; added errorAfterReader test helper; added test for read errors during EOF check; removed bufio and sync imports; removed "closed chan" test (now documented in API that caller must not close channel) |
| storage/filesystem/dotgit/writers.go | Updated to use new Encode function API directly instead of creating encoder instance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
pjbgf
left a comment
pjbgf
left a comment
There was a problem hiding this comment.
@ferhatelmas thanks for working on this. 🙇 The comments are largely for future reference, no action needed.
| ) | ||
|
|
||
| // errorAfterReader wraps a reader and returns an error after n bytes have been read. | ||
| type errorAfterReader struct { |
There was a problem hiding this comment.
This would certainly be handy in other parts of the code - including other {en,de}coders. But we can extract it on a follow-up PR when it gets used around other tests.
| }() | ||
| br, ok := r.(*bufio.Reader) | ||
| if !ok { | ||
| br = bufio.NewReader(r) |
There was a problem hiding this comment.
I'm not a massive fan of the additional allocation that comes with this trade-off. But considering that on using a io.Reader makes it more flexible, it sounds like a reasonable thing to do.
|
@ferhatelmas this commit also needs to be reworded so that I can go ahead and approve/merge. |
related to go-git#1731 * Make encode and decode a function and unexport encoder/decoder to hide state so that there is less opportunity to misuse concurrently, and drop mutex and recover as a result. * Handle non EOF errors in decode trailing check. * Add context in object decode for EOF. * Drop unused stateFn in encoder/decoder. * Accept io.Reader since it's more idiomatic. For decode, internally, we create buffered reader if already not buffered but not doing it for writing since flushing is required and it's generally controlled by caller (file, buffer, etc). Keeping explicit check for io.EOF because of convention golang/go#39155 Signed-off-by: ferhat elmas <[email protected]>
ferhatelmas
force-pushed
the
ferhat/refactor-revfile
branch
from
b8f2943 to
a1bf299
Compare
|
Updated this as well. Aside, shall we update commit message validation to include type of work such as docs, fix, refactor, etc. ? |
Feel free to propose an update to the contributing guidelines then we can loop in other folks on it. |
2 participants
related to #1731
there is less opportunity to misuse concurrently,
and drop mutex and recover as a result.
if already not buffered but not doing it for
writing since flushing is required and it's
generally controlled by caller (file, buffer, etc).
Keeping explicit check for io.EOF because of
convention golang/go#39155