Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()

2016-08-29 Thread Lars Schneider

> On 26 Aug 2016, at 19:15, Junio C Hamano  wrote:
> 
> 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()

2016-08-26 Thread Junio C Hamano
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.


--
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()

2016-08-26 Thread Lars Schneider

> On 25 Aug 2016, at 23:50, Junio C Hamano  wrote:
> 
> 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()

2016-08-25 Thread Junio C Hamano
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