On Wed, Feb 8, 2017 at 10:11 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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.
Yes, the title was there first and then I massaged the commit message
until it was readable. Originally I thought the issue is with stateless
protocols, but that is wrong. The underlying issue is just the transport
helper communication lacking.
> 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.
Indeed the subject is wrong.
>
>> 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?
The Gerrit world started using push options for having a better git
experience, not needing to navigate to the web UI, e.g.:
# push for review and set me as a reviewer:
git push --push-option reviewer=sbeller@xxxxxxxxxx ssh://gerrit...
# same with http, it looks like it worked, but the push option
# never made it to the server
git push --push-option reviewer=sbeller@xxxxxxxxxx http://gerrit...
# This patch will make the second command fail, reporting
# the http helper not supporting push options.
>
> 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?"
Your analysis is correct.
>
>> +'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.
When implementing push options, we discussed that and according to
Documentation/git-push:
The given string must not contain a NUL or LF character.
>
> 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".
okay.