Re: [PATCH 3/4] push: accept push options

From: Stefan Beller <sbeller@google.com>
Date: Jul 8, 2016, 22:59 UTC
Message-ID: <CAGZ79kbkbb=qYUYmKnuKwJCBtidymXua3eAZGANpXDHT56pqGQ@mail.gmail.com>
In-Reply-To: <xmqq60shp3w9.fsf@gitster.mtv.corp.google.com>

On Thu, Jul 7, 2016 at 1:52 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +-L::
>> +--push-option::
>> +     Transmit the given string to the server, which passes them to
>> +     the pre-receive as well as the post-receive hook. Only C strings
>> +     containing no new lines are allowed.
>
> This is to affect what happens at the remote end, so I would have
> understood "-R".  I also would have understood "-P" as a short-hand
> for "--push-option".  What is the justification of "-L"?

It was made up. The actual code took -o for option. Changed that.

>
> What does "C strings" mean?  Did you mean to say "A sequence of
> bytes excluding NUL is passed verbatim"?

>
> I do not think I saw anything in the code I reviewed so far that
> requires "no LF" limitation.

It is enforced server side, but an additional client side enforcement may be better indeed.

The rationale for no enforcement on the client side is an easier way forward if we allow it on the server as the client would "just work" and it's up to the server to complain.

That makes me wonder if we want to document that, i.e.:

-o::
--push-option::
    Transmit the given argument to the server, which passes them to
    the pre-receive as well as the post-receive hook. As it is up to the
    server to react on these push options, it may reject push options
    that contain new line or NUL characters. .

Show 6 quoted lines

>
> ... Ahh, OK, you want to make sure that push-options are
> one-per-line in the push certificate.  While I do not think it is
> absolutely necessary, starting with a possibly tighter than
> necessary limitation is much better than starting loose and having
> to tighten it later.

This is not what I had in mind, but rather the pain of dealing with multi line environment variables.

Show 7 quoted lines

>>                       transport_get(remote, NULL);
>> -
>> +             if (flags & TRANSPORT_PUSH_OPTIONS)
>> +                     transport->push_options = push_options;
>
> The result would be easier to read without the removal of a blank
> that separates decl/defn and stmt here.

ok

Show 6 quoted lines

>> +             OPT_STRING_LIST('o', "push-option", &push_options, N_("server-specific"), N_("option to transmit")),
>
> Here it seems to expect "-o".  If we really want a short option,
> "-o" would probably be OK, as I do not think "git push" wants to
> have "send the output to this file" option.
>

Ok, will update the documentation.

Show 11 quoted lines

>
> by adding this between the two lines in the pre-context of this
> hunk, i.e.
>
>         if (push_cert_nonce[0])
>                 strbuf_addf(&cert, "nonce %s\n", push_cert_nonce);
>         if (args->push_options)
>                 for_each_string_list_item(item, args->push_options)
>                         strbuf_addf(&cert, "push-option %s\n", item->string);
>         strbuf_addstr(&cert, "\n");
>

makes sense.