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

From: Shawn Pearce <spearce@spearce.org>
Date: Jul 12, 2016, 04:53 UTC
Message-ID: <CAJo=hJvFLonqiHtUfSdDOS53TFK_i847dMY1jPuKVinwEFh2Dw@mail.gmail.com>
In-Reply-To: <CAGZ79kaGJCL-HUNm1Rfgzr5E7curgQ9KLU07fonZF5YmiBW35w@mail.gmail.com>

On Sun, Jul 10, 2016 at 11:05 AM, Stefan Beller <sbeller@google.com> wrote:

Show 20 quoted lines

> On Sun, Jul 10, 2016 at 10:06 AM, Shawn Pearce <spearce@spearce.org> wrote:
>> On Fri, Jul 8, 2016 at 5:31 PM, Stefan Beller <sbeller@google.com> wrote:
>>> +
>>> +       /* NEEDSWORK: expose the limitations to be configurable. */
>>> +       int max_options = 32;
>>> +
>>> +       /*
>>> +        * NEEDSWORK: expose the limitations to be configurable;
>>> +        * Once the limit can be lifted, include a way for payloads
>>> +        * larger than one pkt, e.g allow a payload of up to
>>> +        * LARGE_PACKET_MAX - 1 only, and reserve the last byte
>>> +        * to indicate whether the next pkt continues with this
>>> +        * push option.
>>> +        */
>>> +       int max_size = 1024;
>>
>> Instead of this, what about a new config variable
>> receive.maxCommandBytes[1] that places a limit on the number of bytes
>> of pkt-line data the client can supply in both the command list ("old
>> new ref"), push signature framing, and option list?

FYI https://git.eclipse.org/r/77108 is my proposal in JGit.

> The design here with two bounds was used to not care about the oversized
> push options for now. (I mean we can still just reject larger push
> options even when
> having a receive.maxCommandBytes setting.)

I don't really see a problem with a single option being nearly 64 KiB in size. The pack file is going to (probably) need a lot more memory than that to be unpacked and have SHA-1s generated. The maxCommandBytes setting at 3 MiB neatly allows a few options to be up to the pkt-line 64 KiB size, which is 1/10th of what anyone would ever need. :)

> In an earlier discussion Jeff said roughly "either make it work well,
> or don't make it work at all, i.e. why are git push options better
> than a `git push .. && curl <server>/REST-API` thing?"

One reason this is less optimal is you need to setup authentication twice. git push might be working over SSH, or over a custom git-remote-NNN transport. But curl might not speak the same authentication. Even if git and curl are both using .netrc or an HTTP cookie file you need to pass the right options to make sure both use the same auth.

Another is we can do validation of the incoming Git data in context of what options you specified to us and refuse it while discarding the pack if we don't like what was asked. With `git push && curl` we have to save the data without necessarily knowing exactly what we should do with it, just in case the user comes back with a corresponding REST API call on another connection.