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

On Fri, Jul 8, 2016 at 11:39 AM, Junio C Hamano [off-list ref] wrote:

Jonathan Nieder [off-list ref] writes:

quoted

Hi,

Junio C Hamano wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

quoted

quoted

More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?

Sorry to butt into the conversation late, but: I am not yet convinced.

Is the idea that if the push options were very large, this would save
the client from the cost of sending them?

Not really.  I have no strong opinion on the benefit of limiting
number/size.  Stefan limited the number/size at the receiving end
and made receiving end die with its message.

Jeff claimed we'd need some sort of DoS protection for this feature,
so I considered just die-ing enough for an initial implementation.

I was merely trying to
tweak the arrangement so that the sending end can complain with its
own message, possibly in its own language, especially because it was
unclear to me if the die() message on the receiving end would always
go back to the sending end correctly.

I'm currently implementing Jonathans suggestion as it seems to be a reasonable
trade off (client hasn't sent a lot of data when it is decided it
doesn't go through,
the server can complain with a reasonable error message, only downside: no
i18n localisation support on the client side as the server will
currently report the
error in English).

That method will make heavy use of rp_error that uses the side band for
communicating the actual error message.

quoted

But this comes with a
downside: the server doesn't get to send an error message about
where
the maximum number of push options can come from (e.g., with a link to
a page where the limit can be adjusted, or with an explanation of when
clients tend to run into this problem and what they should do
instead).

Hmm, interesting point.  That would be better told by the receiving
end, as the way to configure it (if offered) would be different from
installation to installation.

quoted

So I'd like to propose an alternative. What if the client tells the
server the number of push options early on (and possibly also a cap on
the length of those push options)?  That way, the client doesn't have
to waste bit sending the push options but the server gets an
opportunity to send a helpful error message on sideband 3.

      server> HEAD\0push-options ...
      client> ... commands ...
      client> push-options 2
      client> my-first-option
      client> my-second-option

Another (slightly offtopic) observation:
If in the future we'll need to transmit push options >64k, instead of
splitting the push option to multiple packets, we could invent "large
packets". The current upper bound for packets is artificailly low, such
that the server is able to interleave sideband information with the actual
data in a fetch and have the client display the progress in a timely manner.
When pushing to the server we'd not need to have progress information
(the server doesn't care, and the client knows the size it is pushing).

As of today a packet consists of 4 bytes (hex characters) to indicate
the length and then the payload follows. So instead we could transmit
"v" (that is not a hex character) followed by a variable length integer for
the length and then the payload which has no upper bound.

In the release notes for 2.3 you wrote:

* "git push" and "git fetch" did not communicate an overlong refname
  correctly.  Now it uses 64kB sideband to accommodate longer ones.

That could also make use of these "large packets" instead.

Thanks,
Stefan