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

From: Stefan Beller <sbeller@google.com>
Date: Jul 14, 2016, 19:00 UTC
Message-ID: <CAGZ79kZgJ_6fxZ19Hx6zd6KoTAd_+oMjb4fj1twY2ZGUHP7wHA@mail.gmail.com>
In-Reply-To: <xmqqk2goqd4l.fsf@gitster.mtv.corp.google.com>

> packet_read_line() calls packet_read_line_generic() that calls
> packet_read() with the fixed packet_buffer[] that is sized to be
> LARGE_PACKET_MAX.

>
> Can this check even trigger?

I thought when len == LARGE_PACKET_MAX, this could trigger. Though on inspection of packet_read, we already reject packets that have size LARGE_PACKET_MAX, and the largest size allowed is LARGE_PACKET_MAX - 1.

I guess we can remove it altogether and still be future proof. If we ever want to allow larger push options we either need to have larger (variable sized) packets or a capability push-options-v2, so I'll rip this check out.

Show 7 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" ?

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.

Show 11 quoted lines

>
>> +
>> +             string_list_append(ret, line);
>> +     }
>> +
>> +     return ret;
>> +}
>
> Other than that, looks good to me.
>
> Thanks.