Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

Stefan Beller [off-list ref] writes:

quoted

quoted

+             lf = strchr(line, '\n');
+             if (lf)
+                     *lf = '\0';

packet_read_line() -> packet_read_line_generic() calls packet_read()
with PACKET_READ_CHOMP_NEWLINE flag bit; do we need this check?

This check was not about "option with lf at end\n", but rather we want to chop
off "option\nover\nmultiple\nlines" ?

Ahh, I did misread the check.

Although as you remarked in another email, this would not pose a problem for
the shell variable, so we could also drop it to allow multi line
options. will do.

One thing to note is that I do not think there is a guarantee that
packet_buf[] is NUL-terminated, and when len == LAGE_PACKET_MAX, you
do not have room to NUL-terminate it yourself.

string_list_append(ret, line) that assumes the "string" is NUL
terminated may become an issue that you need to solve by appending
the result of xmemdupz() into a non-duping string list.

On Thu, Jul 14, 2016 at 12:07:15PM -0700, Junio C Hamano wrote:

quoted

Although as you remarked in another email, this would not pose a problem for
the shell variable, so we could also drop it to allow multi line
options. will do.

One thing to note is that I do not think there is a guarantee that
packet_buf[] is NUL-terminated, and when len == LAGE_PACKET_MAX, you
do not have room to NUL-terminate it yourself.

packet_read() does NUL-terminate for you. It gets the extra bytes
because it doesn't store the 4-byte size in the output (whereas the
client does not ever send anything over LARGE_PACKET_MAX, _including_
those bytes, so we always have room to store its result in our
LARGE_PACKET_MAX buffer, plus the NUL, with 3 bytes to spare).

-Peff