protocol/packp: sideband muxer and demuxer #143
Conversation
| "errors" | ||
| "fmt" | ||
| "io" | ||
|
|
| case PackData: | ||
| return content[1:], err | ||
| case ProgressMessage: | ||
| _, err := d.Progress.(io.Writer).Write(content[1:]) |
There was a problem hiding this comment.
If Progress needs to be a Writer, shouldn't it be defined as io.ReadWriter instead of just io.Reader?
There was a problem hiding this comment.
Is preventing being written by external sources, also I can to declare the intent that should be used for reading
| _, err := d.Progress.(io.Writer).Write(content[1:]) | ||
| return nil, err | ||
| case ErrorMessage: | ||
| return nil, fmt.Errorf("unexepcted error: %s", content[1:]) |
| "testing" | ||
|
|
||
| . "gopkg.in/check.v1" | ||
| "gopkg.in/src-d/go-git.v4/plumbing/format/pktline" |
| } | ||
|
|
||
| if !d.s.Scan() { | ||
| return nil, io.EOF |
There was a problem hiding this comment.
if there is no more input, shouldn't the scanner error be checked instead of always returning io.EOF?
There was a problem hiding this comment.
And Error is always nil, when Scan returns io.EOF @alcortesm
There was a problem hiding this comment.
No, Error() is not always nil and Scan() does not return io.EOF, it returns a boolean instead: from pktline.Scanner.Scan(), plumbing/format/pktline/scanner.go:44 :
// Scan advances the Scanner to the next pkt-line, whose payload will
// then be available through the Bytes method. Scanning stops at EOF
// or the first I/O error. After Scan returns false, the Err method
// will return any error that occurred during scanning, except that if
// it was io.EOF, Err will return nil.
func (s *Scanner) Scan() bool {
We should definitely check the scanner error in this case and return io.EOF if the error is nil.
This is a similar behavior as bufio.Scanner, see https://golang.org/pkg/bufio/#example_Scanner_lines for example.
| "testing" | ||
|
|
||
| . "gopkg.in/check.v1" | ||
| "gopkg.in/src-d/go-git.v4/plumbing/format/pktline" |
| // be return if an error happends when reading or if a message is sent in the | ||
| // ErrorMessage channel. | ||
| // | ||
| // If a ProgressMessage is read, it's not copied into b, intead of this is |
| // If a ProgressMessage is read, it's not copied into b, intead of this is | ||
| // is stored, can be read through the reader Progress, the n value returned is | ||
| // zero, err is nil unless an error reading happends. | ||
| func (d *Demuxer) Read(b []byte) (n int, err error) { |
There was a problem hiding this comment.
Rewrite the second paragraph. An example:
If a ProgressMessage is read, it won't be copied to b. Instead of this, it is stored and can be read through the reader Progress. If the n value returned is zero, err will be nil unless an error reading happens.
| } | ||
| } | ||
|
|
||
| // Read reads up to len(p) bytes from the PackData channel into p, an error can |
There was a problem hiding this comment.
I think this description is not exact: I think this method reads from the 3 channels, not just the PackData channel.
I think this requires a better explanation, as there are 3 channels but only 2 readers. I would write a small table explaining how to read from each channel and how to know if there is data in any of them, maybe as an explanation to the package.
There was a problem hiding this comment.
You have a more detailed explanation of Demux at the struct, I think is clear enough, this function (specially for the user), only reads Packdata into p
| } | ||
|
|
||
| content := d.pending | ||
| d.pending = make([]byte, 0) |
There was a problem hiding this comment.
change this to:
d.pending = nil
to avoid allocating memory that will be thrown away when size > wanted in doRead.
This is convoluted and a little confusing, as the whole function does not really read and it is just two lines:
content := d.pending
d.pending = nil
And then we have to reslice in the calling function. Better remove the function and reslice directly.
| wanted := len(b) | ||
|
|
||
| if size > wanted { | ||
| d.pending = read[wanted:size] |
There was a problem hiding this comment.
simplify read[wanted:size] to read[wanted:].
|
|
||
| content := make([]byte, 26) | ||
| d := NewDemuxer(Sideband64k, buf) | ||
| n, err := d.Read(content) |
There was a problem hiding this comment.
change Read to ReadFull.
The same for all the tests.
|
|
||
| buf := bytes.NewBuffer(nil) | ||
| e := pktline.NewEncoder(buf) | ||
| e.Encode(append(PackData.Bytes(), expected[0:8]...)) |
There was a problem hiding this comment.
This is awesome.
I think you should write this line as an example in the comment of the Channel.Byte() method, so everybody understand, at a single glance, what is its intended use case.
You can also call the method func (c Channel) WithPayload(b []byte) []byte, so its usage will be a little more clear:
e.Encode(PackData.WithPayload(expected[0:8])
And make it return a multireader to save memory.
Current coverage is 73.57% (diff: 89.41%)@@ master #143 diff @@
==========================================
Files 78 81 +3
Lines 5073 5176 +103
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 3871 3808 -63
- Misses 756 936 +180
+ Partials 446 432 -14
|
* format/pakp: sideband demuxer * format/pakp: sideband muxer * format/pakp: sideband demuxer and muxer * protocol/pakp: sideband demuxer and muxer * documentation and improvements * improvements * handle scan errors properly
This is the implementation of the sideband muxer and the demuxer.