> On 03 Aug 2016, at 22:18, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > larsxschneider@xxxxxxxxx writes: > >> From: Lars Schneider <larsxschneider@xxxxxxxxx> >> >> set_packet_header() converts an integer to a 4 byte hex string. Make >> this function locally available so that other pkt-line functions can >> use it. > > Didn't I say that this is a bad idea already in an earlier review? Yes, but in that earlier version I made this function *publicly* available. In this patch the function is only available and used within pkt-line.c. > The only reason why you want it, together with direct_packet_write() > (which I think is another bad idea), is because you use > packet_buf_write() to create a "<header><payload>" in a buf in the > usercode in step 11/12 like this: > > + packet_buf_write(&nbuf, "command=%s\n", filter_type); > + ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1); > > which would be totally unnecessary if you just did strbuf_addf() > into nbuf and used packet_write() like everybody else does. The usercode in step 11/12 could use packet_buf_write(). I am not worried about performance here. What I am worried about is that packet_buf_write() dies on error. Since direct_packet_write() has a "gentle" parameter in can handle these cases. This is important because a filter might be configured as "required=false" and then errors are OK. Would you prefer to see a packet_buf_write_gently() instead? Thanks, 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