Re: [PATCH v5 04/15] pkt-line: add packet_write_gently()

[Junio C Hamano]

Jeff King  writes:

> On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:
>
>> > So now we have packet_write() and packet_write_gently(), but they differ
>> > in more than just whether they are gentle. That seems like a weird
>> > interface.
>> > 
>> > Should we either be picking a new name (e.g., packet_write_mem() or
>> > something), or migrating packet_write() to packet_write_fmt()?
>> 
>> Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to 
>> packet_write_fmt()"
>
> Ah, OK. Generally I'd suggest to reorder things so that each patch looks
> like a step forward (and so the early patches become preparatory steps,
> and the justification in them is something like "we're going to add more
> write functions, so let's give this a more descriptive name").

I am guilty for saying "packet_write() should have been similar to
write(2)".  We may want to have a time-period during which there is
no "packet_write()" in the codebase, before we get to that stage.
I.e. rename it to packet_write_fmt() to vacate the name and add
packet_write_mem(), and then later rename packet_write_mem() to its
final name packet_write(), or something like that.  The two-step
process would reduce the chance of misconversion.
--
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 v5 04/15] pkt-line: add packet_write_gently()

[Lars Schneider]

> On 10 Aug 2016, at 15:28, Jeff King  wrote:
> 
> On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider 
>> 
>> packet_write() 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.
>> 
>> Add packet_write_gently() which writes arbitrary data and returns `0` for
>> success and `-1` for an error.
> 
> So now we have packet_write() and packet_write_gently(), but they differ
> in more than just whether they are gentle. That seems like a weird
> interface.
> 
> Should we either be picking a new name (e.g., packet_write_mem() or
> something), or migrating packet_write() to packet_write_fmt()?

Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()"


>> diff --git a/pkt-line.c b/pkt-line.c
>> index e6b8410..4f25748 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -3,6 +3,7 @@
>> #include "run-command.h"
>> 
>> char packet_buffer[LARGE_PACKET_MAX];
>> +char packet_write_buffer[LARGE_PACKET_MAX];
> 
> Should this be static? I.e., are random other bits of the code allowed
> to write into it (I guess not because it's not declared in pkt-line.h).

static is better!


>> +int packet_write_gently(const int fd_out, const char *buf, size_t size)
>> +{
>> +if (size > PKTLINE_DATA_MAXLEN)
>> +return -1;
>> +packet_trace(buf, size, 1);
>> +memmove(packet_write_buffer + 4, buf, size);
> 
> It looks like this iteration drops the idea of callers using a
> LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
> PKTLINE_DATA_MAXLEN bytes (which is fine).
> 
> I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
> obscuring things. The magic number "4" still appears separately here,
> and it actually makes it harder to see that things are correct.  I.e.,
> doing:
> 
>  if (size > sizeof(packet_write_buffer) - 4)
>   return -1;
>  memmove(packet_write_buffer + 4, buf, size);
> 
> is more obviously correct, because you do not have to wonder about the
> relationship between the size of your buffer and the macro.
> 
> It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
> callers use it to size their input to packet_write_gently().

I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,
too. I will change it to your suggestion.

For now I would remove PKTLINE_DATA_MAXLEN because it should be an 
implementation
detail of pkt-line.c (plus it is not used by anyone).

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

[PATCH v5 04/15] pkt-line: add packet_write_gently()

[larsxschneider]

From: Lars Schneider 

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

Add packet_write_gently() which writes arbitrary data and returns `0` for
success and `-1` for an error.

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 12 
 pkt-line.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index e6b8410..4f25748 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -3,6 +3,7 @@
 #include "run-command.h"
 
 char packet_buffer[LARGE_PACKET_MAX];
+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);
@@ -141,6 +142,17 @@ void packet_write(int fd, const char *fmt, ...)
write_or_die(fd, buf.buf, buf.len);
 }
 
+int packet_write_gently(const int fd_out, const char *buf, size_t size)
+{
+   if (size > PKTLINE_DATA_MAXLEN)
+   return -1;
+   packet_trace(buf, size, 1);
+   memmove(packet_write_buffer + 4, buf, size);
+   size += 4;
+   set_packet_header(packet_write_buffer, size);
+   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;
diff --git a/pkt-line.h b/pkt-line.h
index 3cb9d91..88584f1 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -77,6 +77,7 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, 
int *size);
 
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
+#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)
 extern char packet_buffer[LARGE_PACKET_MAX];
 
 #endif
-- 
2.9.2

--
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 v5 04/15] pkt-line: add packet_write_gently()

[Jeff King]

On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> packet_write() 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.
> 
> Add packet_write_gently() which writes arbitrary data and returns `0` for
> success and `-1` for an error.

So now we have packet_write() and packet_write_gently(), but they differ
in more than just whether they are gentle. That seems like a weird
interface.

Should we either be picking a new name (e.g., packet_write_mem() or
something), or migrating packet_write() to packet_write_fmt()?

> diff --git a/pkt-line.c b/pkt-line.c
> index e6b8410..4f25748 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -3,6 +3,7 @@
>  #include "run-command.h"
>  
>  char packet_buffer[LARGE_PACKET_MAX];
> +char packet_write_buffer[LARGE_PACKET_MAX];

Should this be static? I.e., are random other bits of the code allowed
to write into it (I guess not because it's not declared in pkt-line.h).

> +int packet_write_gently(const int fd_out, const char *buf, size_t size)
> +{
> + if (size > PKTLINE_DATA_MAXLEN)
> + return -1;
> + packet_trace(buf, size, 1);
> + memmove(packet_write_buffer + 4, buf, size);

It looks like this iteration drops the idea of callers using a
LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
PKTLINE_DATA_MAXLEN bytes (which is fine).

I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
obscuring things. The magic number "4" still appears separately here,
and it actually makes it harder to see that things are correct.  I.e.,
doing:

  if (size > sizeof(packet_write_buffer) - 4)
return -1;
  memmove(packet_write_buffer + 4, buf, size);

is more obviously correct, because you do not have to wonder about the
relationship between the size of your buffer and the macro.

It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
callers use it to size their input to packet_write_gently().

-Peff
--
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 v5 04/15] pkt-line: add packet_write_gently()

[Lars Schneider]

> On 10 Aug 2016, at 20:21, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 10 Aug 2016, at 19:17, Junio C Hamano  wrote:
>>> 
>> OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()"
>> rename as is in this series?
> 
> I didn't really check what order you are doing things to answer
> that.
> 
> If the function that is introduced in this step is a version of
> packet_write_fmt() that does its thing only gently, you would want
> to do the rename s/packet_write/packet_write_fmt/ before this step,
> and then add the new function as packet_write_fmt_gently(), I would
> think.

OK - will fix. I did it that way because I thought it would be easier
if we decide to drop the big rename patch.

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 v5 04/15] pkt-line: add packet_write_gently()

[Jeff King]

On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:

> > So now we have packet_write() and packet_write_gently(), but they differ
> > in more than just whether they are gentle. That seems like a weird
> > interface.
> > 
> > Should we either be picking a new name (e.g., packet_write_mem() or
> > something), or migrating packet_write() to packet_write_fmt()?
> 
> Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to 
> packet_write_fmt()"

Ah, OK. Generally I'd suggest to reorder things so that each patch looks
like a step forward (and so the early patches become preparatory steps,
and the justification in them is something like "we're going to add more
write functions, so let's give this a more descriptive name").

> I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,
> too. I will change it to your suggestion.
> 
> For now I would remove PKTLINE_DATA_MAXLEN because it should be an 
> implementation
> detail of pkt-line.c (plus it is not used by anyone).

Sounds reasonable.

-Peff
--
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 v5 04/15] pkt-line: add packet_write_gently()

[Lars Schneider]

> On 10 Aug 2016, at 19:17, Junio C Hamano  wrote:
> 
> Jeff King  writes:
> 
>> On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:
>> 
 So now we have packet_write() and packet_write_gently(), but they differ
 in more than just whether they are gentle. That seems like a weird
 interface.
 
 Should we either be picking a new name (e.g., packet_write_mem() or
 something), or migrating packet_write() to packet_write_fmt()?
>>> 
>>> Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to 
>>> packet_write_fmt()"
>> 
>> Ah, OK. Generally I'd suggest to reorder things so that each patch looks
>> like a step forward (and so the early patches become preparatory steps,
>> and the justification in them is something like "we're going to add more
>> write functions, so let's give this a more descriptive name").
> 
> I am guilty for saying "packet_write() should have been similar to
> write(2)".  We may want to have a time-period during which there is
> no "packet_write()" in the codebase, before we get to that stage.
> I.e. rename it to packet_write_fmt() to vacate the name and add
> packet_write_mem(), and then later rename packet_write_mem() to its
> final name packet_write(), or something like that.  The two-step
> process would reduce the chance of misconversion.

OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()"
rename as is in this series?

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 v5 04/15] pkt-line: add packet_write_gently()

[Junio C Hamano]

Lars Schneider  writes:

>> On 10 Aug 2016, at 19:17, Junio C Hamano  wrote:
>> 
> OK. Does this mean I can leave the "packet_write()" to "packet_write_fmt()"
> rename as is in this series?

I didn't really check what order you are doing things to answer
that.

If the function that is introduced in this step is a version of
packet_write_fmt() that does its thing only gently, you would want
to do the rename s/packet_write/packet_write_fmt/ before this step,
and then add the new function as packet_write_fmt_gently(), I would
think.
--
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