Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

From: Lars Schneider <larsxschneider@gmail.com>
Date: Aug 3, 2016, 21:56 UTC
Message-ID: <D116610C-F33A-43DA-A49D-0B33958822E5@gmail.com>
In-Reply-To: <xmqqd1lp8v2o.fsf@gitster.mtv.corp.google.com>

Show 11 quoted lines

> On 03 Aug 2016, at 22:18, Junio C Hamano <gitster@pobox.com> wrote:
> 
> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> 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.

Show 10 quoted lines

> 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