Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()
> On 26 Aug 2016, at 19:15, Junio C Hamanowrote: > > Lars Schneider writes: > >>> Do you anticipate future need of non-gently variant of this >>> function? If so, perhaps a helper that takes a boolean "am I >>> working for the gently variant?" may help share more code. >> >> With helper you mean "an additional boolean parameter"? I don't >> see a need for a non-gently variant right now but I will >> add this parameter if you think it is a good idea. How would the >> signature look like? >> >> int packet_write_gently(const int fd_out, const char *buf, size_t size, int >> gentle) >> >> This would follow type_from_string_gently() in object.h. > > I actually imagined it would be more like your packet_write_fmt vs > packet_write_fmt_gently pair of functions. If you do not have an > immediate need for a non-gentle packet_write() right now, but you > still forsee that it is likely some other caller may want one, you > could still prepare for it by doing a static > > packet_write_1((const int fd_out, const char *buf, size_t size, int > gentle) > > and make packet_write_gently() call it with gentle=1, without > actually introducing packet_write() nobody yet calls. I see. In that case I would like to keep packet_write_gently() as is because I don't see the need for a non-gently variant right now. If there is a need for packet_write() then we could just add it and move the packet_write_gently() code to packet_write_1() following your suggestion. No caller would need to change for this refactoring. If you strongly disagree then I would use the "two function" approach you suggested above right away, though. Thanks, Lars
Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()
Lars Schneiderwrites: >> Do you anticipate future need of non-gently variant of this >> function? If so, perhaps a helper that takes a boolean "am I >> working for the gently variant?" may help share more code. > > With helper you mean "an additional boolean parameter"? I don't > see a need for a non-gently variant right now but I will > add this parameter if you think it is a good idea. How would the > signature look like? > > int packet_write_gently(const int fd_out, const char *buf, size_t size, int > gentle) > > This would follow type_from_string_gently() in object.h. I actually imagined it would be more like your packet_write_fmt vs packet_write_fmt_gently pair of functions. If you do not have an immediate need for a non-gentle packet_write() right now, but you still forsee that it is likely some other caller may want one, you could still prepare for it by doing a static packet_write_1((const int fd_out, const char *buf, size_t size, int gentle) and make packet_write_gently() call it with gentle=1, without actually introducing packet_write() nobody yet calls. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()
> On 25 Aug 2016, at 23:50, Junio C Hamanowrote: > > larsxschnei...@gmail.com writes: > >> From: Lars Schneider >> >> packet_write_fmt() has two shortcomings. First, it uses format_packet() >> which lets the caller only send string data via "%s". That means it >> cannot be used for arbitrary data that may contain NULs. Second, it will >> always die on error. > > As you introduced _gently in 3/13, the latter is no longer a valid > excuse to add this function. Just remove the sentence "Second, ...". Agreed! >> Add packet_write_gently() which writes arbitrary data and returns `0` >> for success and `-1` for an error. This function is used by other >> pkt-line functions in a subsequent patch. >> >> Signed-off-by: Lars Schneider >> --- >> pkt-line.c | 12 >> 1 file changed, 12 insertions(+) >> >> diff --git a/pkt-line.c b/pkt-line.c >> index cad26df..7e8a803 100644 >> --- a/pkt-line.c >> +++ b/pkt-line.c >> @@ -3,6 +3,7 @@ >> #include "run-command.h" >> >> char packet_buffer[LARGE_PACKET_MAX]; >> +static char packet_write_buffer[LARGE_PACKET_MAX]; >> static const char *packet_trace_prefix = "git"; >> static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET); >> static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE); >> @@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, >> ...) >> return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); >> } >> >> +int packet_write_gently(const int fd_out, const char *buf, size_t size) >> +{ >> +if (size > sizeof(packet_write_buffer) - 4) >> +return -1; >> +packet_trace(buf, size, 1); >> +memmove(packet_write_buffer + 4, buf, size); >> +size += 4; >> +set_packet_header(packet_write_buffer, size); > > It may not matter all that much, but from code-reader's point of > view, when you know packet_write_buffer[] will contain things A and > B in this order, and when you have enough information to compute A > before stasrting to fill packet_write_buffer[], I would prefer to > see you actually fill the buffer in that natural order. I did that because when packet_write_stream_with_flush_from_fd() calls packet_write_gently() then buf[] is actually packet_write_buffer[]: https://github.com/larsxschneider/git/blob/d474e6a4c2523b87624a07111eb7a4f2dcd12426/pkt-line.c#L185-L192 Therefore I would override the first 4 bytes. However, the code evolved for some reason in that way but looking at it now I think that is an obscure, likely meaningless optimization. I'll use a separate buffer in packet_write_stream_with_flush_from_fd() and then fix the order here following your advice. > Do you anticipate future need of non-gently variant of this > function? If so, perhaps a helper that takes a boolean "am I > working for the gently variant?" may help share more code. With helper you mean "an additional boolean parameter"? I don't see a need for a non-gently variant right now but I will add this parameter if you think it is a good idea. How would the signature look like? int packet_write_gently(const int fd_out, const char *buf, size_t size, int gentle) This would follow type_from_string_gently() in object.h. Thanks, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()
larsxschnei...@gmail.com writes: > From: Lars Schneider> > packet_write_fmt() has two shortcomings. First, it uses format_packet() > which lets the caller only send string data via "%s". That means it > cannot be used for arbitrary data that may contain NULs. Second, it will > always die on error. As you introduced _gently in 3/13, the latter is no longer a valid excuse to add this function. Just remove the sentence "Second, ...". > Add packet_write_gently() which writes arbitrary data and returns `0` > for success and `-1` for an error. This function is used by other > pkt-line functions in a subsequent patch. > > Signed-off-by: Lars Schneider > --- > pkt-line.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/pkt-line.c b/pkt-line.c > index cad26df..7e8a803 100644 > --- a/pkt-line.c > +++ b/pkt-line.c > @@ -3,6 +3,7 @@ > #include "run-command.h" > > char packet_buffer[LARGE_PACKET_MAX]; > +static char packet_write_buffer[LARGE_PACKET_MAX]; > static const char *packet_trace_prefix = "git"; > static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET); > static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE); > @@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...) > return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1); > } > > +int packet_write_gently(const int fd_out, const char *buf, size_t size) > +{ > + if (size > sizeof(packet_write_buffer) - 4) > + return -1; > + packet_trace(buf, size, 1); > + memmove(packet_write_buffer + 4, buf, size); > + size += 4; > + set_packet_header(packet_write_buffer, size); It may not matter all that much, but from code-reader's point of view, when you know packet_write_buffer[] will contain things A and B in this order, and when you have enough information to compute A before stasrting to fill packet_write_buffer[], I would prefer to see you actually fill the buffer in that natural order. Do you anticipate future need of non-gently variant of this function? If so, perhaps a helper that takes a boolean "am I working for the gently variant?" may help share more code. > + return (write_in_full(fd_out, packet_write_buffer, size) == size ? 0 : > -1); > +} > + > void packet_buf_write(struct strbuf *buf, const char *fmt, ...) > { > va_list args; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html