> On 26 Aug 2016, at 00:27, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> larsxschneider@xxxxxxxxx writes:
>
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>>
>> packet_write_stream_with_flush_from_fd() and
>> packet_write_stream_with_flush_from_buf() write a stream of packets. All
>> content packets use the maximal packet size except for the last one.
>> After the last content packet a `flush` control packet is written.
>> packet_read_till_flush() reads arbitrary sized packets until it detects
>> a `flush` packet.
>
> These are awkwardly named and I couldn't guess what the input is (I
> can tell one is to read from fd and the other is <mem,len> buffer,
> but it is unclear if that is in packetized form or just raw data
> stream to be copied to the end from their names) without reading the
> implementation. I _think_ you read a raw stream of data through the
> end (either EOF or length limit) and write it out packetized, and
> use the flush packet to mark the end of the stream. In my mind,
> that is "writing a packetized stream". The words "packetizing" and
> "stream" imply that the stream could consist of more data than what
> would fit in a single packet, which in turn implies that there needs
> a way to mark the end of one data item, so with_flush does not
> necessarily have to be their names.
>
> The counter-part would be "reading a packetized stream".
>
>> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)
>> +{
>
> Especially this one I am tempted to suggest "copy-to-packetized-stream",
> as it reads a stream from one fd and then copies out while packetizing.
OK, what function names would be more clear from your point of view?
copy_to_packetized_stream_from_fd()
copy_to_packetized_stream_from_buf()
copy_to_packetized_stream_to_buf()
or
write_packetized_stream_from_fd()
write_packetized_stream_from_buf()
read_packetized_stream_to_buf()
?
>> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)
>> +{
>> + int err = 0;
>> + size_t bytes_written = 0;
>> + size_t bytes_to_write;
>> +
>> + while (!err) {
>> + if ((len - bytes_written) > sizeof(packet_write_buffer) - 4)
>> + bytes_to_write = sizeof(packet_write_buffer) - 4;
>> + else
>> + bytes_to_write = len - bytes_written;
>> + if (bytes_to_write == 0)
>> + break;
>
> The lack of COPY_WRITE_ERROR puzzled me briefly here. If you are
> assuming that your math at the beginning of this loop is correct and
> bytes_to_write will never exceed the write-buffer size, I think you
> should be able to (and it would be better to) assume that the math
> you do to tell xread() up to how many bytes it is allowed to read at
> once is also correct, losing the COPY_WRITE_ERROR check in the other
> function. You can choose to play safer and do a check in this
> function, too. Either way, we would want to be consistent.
OK. I'll remove the (I just realized meaningless) check in the other function:
+ if (bytes_to_write > sizeof(packet_write_buffer) - 4)
+ return COPY_WRITE_ERROR;
>
>> + err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
>> + bytes_written += bytes_to_write;
>> + }
>> + if (!err)
>> + err = packet_flush_gently(fd_out);
>> + return err;
>> +}
>
>> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)
>> +{
>> + int len, ret;
>> + int options = PACKET_READ_GENTLE_ON_EOF;
>> + char linelen[4];
>> +
>> + size_t oldlen = sb_out->len;
>> + size_t oldalloc = sb_out->alloc;
>> +
>> + for (;;) {
>> + /* Read packet header */
>> + ret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);
>> + if (ret < 0)
>> + goto done;
>> + len = packet_length(linelen);
>> + if (len < 0)
>> + die("protocol error: bad line length character: %.4s", linelen);
>> + if (!len) {
>> + /* Found a flush packet - Done! */
>> + packet_trace("0000", 4, 0);
>> + break;
>> + }
>> + len -= 4;
>> +
>> + /* Read packet content */
>> + strbuf_grow(sb_out, len);
>> + ret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);
>> + if (ret < 0)
>> + goto done;
>> + if (ret != len) {
>> + error("protocol error: incomplete read (expected %d, got %d)", len, ret);
>> + goto done;
>> + }
>> +
>> + packet_trace(sb_out->buf + sb_out->len, len, 0);
>
> All of the above seems to pretty much duplicate the logic in
> packet_read(), except that this user does not need options handling
> it has. Is optimizing that out the reason why you open-coded it
> here?
No.
> Or is it because you cannot tell if you got a truly empty packet or
> you got a flush from outside packet_read(), and you wanted to make
> sure that you won't be fooled by a normal packet with 0-length
> payload?
Correct!
>
> If the latter is the reason, it may be a viable alternative to
> update packet_read() to take PACKET_READ_IGNORE_EMPTY_PACKET, i.e. a
> new bit in its options parameter, so that a normal packet with
> 0-length payload is simply ignored there (i.e. even without
> returning, packet_read() would repeat from the beginning when it got
> such a packet). That way, the above would become
>
> strbuf_grow(); /* enough to hold max-packet-len more bytes */
> len = packet_read();
> if (!len)
> /* we cannot get 0 unless we see flush */
> break;
>
> which may be a lot cleaner?
Good idea! I will refactor it that way!
Thanks a lot for the review,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html