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

From: Stefan Beller <sbeller@google.com>
Date: Feb 8, 2017, 20:51 UTC
Message-ID: <CAGZ79kaszczpnu+VEV3y+61qgji-0+vRFVLk8Q1+BbSDgiHz1Q@mail.gmail.com>
In-Reply-To: <xmqqpoiswo0l.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 8, 2017 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

> 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.

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.

Show 5 quoted lines

>
>> 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@google.com  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@google.com  http://gerrit...

    # This patch will make the second command fail, reporting
    # the http helper not supporting push options.

Show 13 quoted lines

>
> 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.

Show 12 quoted lines

>
>> +'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.

Show 5 quoted lines

>
> 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.