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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2016, 19:07 UTC
Message-ID: <xmqqfurcqbrw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZgJ_6fxZ19Hx6zd6KoTAd_+oMjb4fj1twY2ZGUHP7wHA@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 9 quoted lines

>>> +             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.