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

4 messages, 2 authors, 2016-08-05 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-08-04 16:14:15

Jeff King [off-list ref] writes:
The cost of write() may vary on other platforms, but the cost of memcpy
generally shouldn't. So I'm inclined to say that it is not really worth
micro-optimizing the interface.

I think the other issue is that format_packet() only lets you send
string data via "%s", so it cannot be used for arbitrary data that may
contain NULs. So we do need _some_ other interface to let you send a raw
data packet, and it's going to look similar to the direct_packet_write()
thing.
OK.  That is a much better argument than "I already stuff the length
bytes in my buffer" (which will invite "How about stop doing that?")
to justify a new "I have N bytes of data, send it out", whose
signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.




	

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

From: Lars Schneider <hidden>
Date: 2016-08-05 14:55:24

On 04 Aug 2016, at 18:14, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:
quoted
The cost of write() may vary on other platforms, but the cost of memcpy
generally shouldn't. So I'm inclined to say that it is not really worth
micro-optimizing the interface.

I think the other issue is that format_packet() only lets you send
string data via "%s", so it cannot be used for arbitrary data that may
contain NULs. So we do need _some_ other interface to let you send a raw
data packet, and it's going to look similar to the direct_packet_write()
thing.
OK.  That is a much better argument than "I already stuff the length
bytes in my buffer" (which will invite "How about stop doing that?")
to justify a new "I have N bytes of data, send it out", whose
signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.
Well, my argument wasn't meant to be offensive. It was just an idea that
I published the to get feedback. Now I understand that it wasn't a particular
good idea (thanks Peff for the performance test!).

However, besides the bogus performance argument I introduced that function
to allow packet writs to fail using the `gentle` parameter:
http://public-inbox.org/git/D116610C-F33A-43DA-A49D-0B33958822E5%40gmail.com/

Would you be OK if I introduce packet_write_gently() that returns `0` if the
write was OK and `-1` if it failed?

Thanks,
Lars

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

From: Lars Schneider <hidden>
Date: 2016-08-05 17:32:05

On 04 Aug 2016, at 18:14, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:
quoted
The cost of write() may vary on other platforms, but the cost of memcpy
generally shouldn't. So I'm inclined to say that it is not really worth
micro-optimizing the interface.

I think the other issue is that format_packet() only lets you send
string data via "%s", so it cannot be used for arbitrary data that may
contain NULs. So we do need _some_ other interface to let you send a raw
data packet, and it's going to look similar to the direct_packet_write()
thing.
OK.  That is a much better argument than "I already stuff the length
bytes in my buffer" (which will invite "How about stop doing that?")
to justify a new "I have N bytes of data, send it out", whose
signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.
"Sigh" means, a series preparation patch that renames "packet_write()" 
to "paket_write_fmt()" would not be a good idea? It is used 59 times 
currently...

- Lars

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

From: Junio C Hamano <hidden>
Date: 2016-08-05 17:42:19

On Fri, Aug 5, 2016 at 10:31 AM, Lars Schneider
[off-list ref] wrote:
quoted
On 04 Aug 2016, at 18:14, Junio C Hamano [off-list ref] wrote:

signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.
"Sigh" means, a series preparation patch that renames "packet_write()"
to "paket_write_fmt()" would not be a good idea? It is used 59 times
currently...
It would be a good idea in the longer term, I would think. I just wasn't
sure if you are willing to volunteer, and in-flight topics will tolerate, such
a change right now. I have a feeling that all the current callsites are
fairly stable and no in-flight topic touches them, so if you feel like doing
so, please go ahead ;-)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help