Re: [PATCH] push options: fail properly in the stateless case

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> When using non-builtin protocols relying on a transport helper
> (such as http), push options are not propagated to the helper.
>
> Fix this by propagating the push options to the transport helper and

The description up to this point is VERY readable and sensible.  But
that makes the title sound a bit strange.  I read it as if it were
saying "stateless case can never support push-options so fail if the
caller attempts to use one", but that does not seem to be what is
going on.

> adding a test that push options using http fail properly.

Sounds sensible.  What end-user visible effect does this fix have?
IOW, what feature do we use "push-option" for?

Ahh, OK, so you need to describe that there are two issues in order
to be understood by the readers:

 (1) the helper protocol does not propagate push-option
 (2) the http helper is not prepared to handle push-option

You fix (1), and you take advantage of the fact (2) to ensure that
(1) is fixed in the new test.

With such an understanding, the title makes (sort of) sense and you
wouldn't have to be asked "what end-user visible effect/benefit does
this have?"

> +'option push-option <c-string>::
> +	Transmit this push option.
> +

There is no "c-string" in the current documentation used or
defined.  The closest thing I found is

    ... that field will be quoted in the manner of a C string ...

in git-status page, but I do not think you send the value for an
push-option after running quote_c_style(), so I am puzzled.

I'd rather see 'option push-option <string>' as the bullet item, and
in its description say how arbitrary values (if you allow them, that
is) can be used, e.g. "Transmit <string> encoded in such and such
way a the value of the push-option".