remote: sideband support #156
Conversation
Current coverage is 76.42% (diff: 77.55%)@@ master #156 diff @@
==========================================
Files 80 83 +3
Lines 5150 5344 +194
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3957 4084 +127
- Misses 749 810 +61
- Partials 444 450 +6
|
| } | ||
|
|
||
| // Demuxer demultiplex the progress reports and error info interleaved with the | ||
| // packfile itself. | ||
| // | ||
| // A sideband has three diferent channels the main one call PackData contains | ||
| // A sideband has three diferent channels the main one, call PackData, contains | ||
| // the packfile data, the ErrorMessage channel, that contains server errors and | ||
| // the last one ProgressMessage channel containing information about the ongoing | ||
| // tast happening in the server (optinal, can be suppressed sending NoProgress | ||
| // or Quiet capabilities to the server) |
There was a problem hiding this comment.
English grammar and typos, consider this alternate version:
Sideband information comes through 3 different channels:
- PackData: the packfile data itself.
- ErrorMessage: server errors.
- ProgressMessage: progress information about the packfile download
process. When the NoProgress and/or Quiet capabilities are in use,
this channel remains silent.
| // stored and can be read from `Progress` field, if any message is retrieved | ||
| // from the ErrorMessage channel an error is returned and we can assume that the | ||
| // written at `Progress` (if any), if any message is retrieved from the | ||
| // ErrorMessage channel an error is returned and we can assume that the | ||
| // conection has been closed. | ||
| type Demuxer struct { |
There was a problem hiding this comment.
This is confusing as the Demuxer type does not actually demultiplex the channels and it is not clear how are errors messages sent back to the user. Also a connection is mentioned in the comment, and some assumptions about it being closed, but it is not clear how this can be relevant for anything.
I think a better comment would be something like this:
[Foo] is a reader to the packfile data in the PackData channel. The progress information
is writen to the the Progress field while reading. If any message is received through the
ErrorMessage channel, the `Read` method will return it as an error and it will assume the
connection has been closed.
Although I am still not sure about the connection thing.
I would rather use a real demuxer though or even simply a method that returns 3 readers (or even better, 3 scanners): one for each channel, treating them as equal, with the same usage semantics. This will have a much simpler API and will let the user decide how to use each channel.
Another approach would be to use a single scanner, returning channel and data when read.
There was a problem hiding this comment.
Ok, this is not a PR of the sideband demux this was already merged. Your text doesn't explain that the Progress could be optional. I will review the text with the new phrase
| t: t, | ||
| r: r, | ||
| max: max, | ||
| s: pktline.NewScanner(r), | ||
| } | ||
| } | ||
|
|
||
| // Read reads up to len(p) bytes from the PackData channel into p, an error can | ||
| // be return if an error happends when reading or if a message is sent in the |
There was a problem hiding this comment.
you are reviewing an already merged comment
| // is zero, err will be nil unless an error reading happens. | ||
| // If a ProgressMessage is read, it won't be copied to b. Instead of this, | ||
| // Progress was set will be stored there. If the n value returned is zero, err | ||
| // will be nil unless an error reading happens. |
| @@ -126,13 +125,17 @@ func (d *Demuxer) nextPackData() ([]byte, error) { | |||
| case PackData: | |||
| return content[1:], nil | |||
| case ProgressMessage: | |||
| _, err := d.Progress.(io.Writer).Write(content[1:]) | |||
| return nil, err | |||
| if d.Progress != nil { | |||
There was a problem hiding this comment.
I think NewDemuxer should has a Progress as an argument.
There was a problem hiding this comment.
Is a optional argument, and not the most common behavior, so that's why is not part of the New method
| @@ -19,6 +19,8 @@ import ( | |||
| "gopkg.in/src-d/go-git.v4/storage/memory" | |||
| osfs "gopkg.in/src-d/go-git.v4/utils/fs/os" | |||
|
|
|||
| "bytes" | |||
Fix error message format
This is the support of
Remotefor sideband capable servers.When
Remote. Fetchis called If the request contained aside-band-64korside-bandasideband.Demuxis used inside ofRemoteto read the packfile data.If the human readable progress is needed a io.Reader/io.Writer should be provided to
Repository.Progressfield, this pointer will be pass to the remotes and be used when the packfile is read. If theRepository.Progressis not set theno-progresscapability is set if supported.Also a couple of bugs are solved at transport/client:
strconv.Itoainstead of just casting a integer to string.