Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options

From: Stefan Beller <sbeller@google.com>
Date: Jul 1, 2016, 18:40 UTC
Message-ID: <CAGZ79kZqB7bs1pyo4sn-_L1sbVRu4hiyFoduYDyZQ62hi9bbCg@mail.gmail.com>
In-Reply-To: <20160701175950.GB16235@sigill.intra.peff.net>

On Fri, Jul 1, 2016 at 10:59 AM, Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

> On Fri, Jul 01, 2016 at 10:20:32AM -0700, Stefan Beller wrote:
>
>> >> The rationale for keeping the actual options inside a file instead of
>> >> putting them directly into an environment variable has multiple reasons:
>> >
>> > Thanks for including this rationale; my first thought on seeing the
>> > patch was "wouldn't this be much more convenient for a hook if each
>> > value had its own environment variable?".
>>
>> That's what I thought as well. Office discussion ensued and I am still
>> offended by this solution, but it sucks less than multiple environment variables
>> (I tried writing a script to construct and evaluate the environment
>> variables and
>> that doesn't look nice)
>
> If you give up on having multiple incarnations of each variable, then I
> think:
>
>   GIT_PUSH_VAR_foo=value_for_foo
>   GIT_PUSH_VAR_bar=value_for_bar
>
> is quite elegant, and easy to use from hooks. It just cannot represent
> multiple such "foo" variables.

I see! Then you can have a single check for one feature

    if $GIT_PUSH_VAR_foo = "value_for_foo" ; then
        foo "bar"
    elif $GIT_PUSH_VAR_foo != "" ; then
        free_form_foo $GIT_PUSH_VAR_foo
    fi

and no worries about parsing.

Mind that we now put a user provided thing into the variable again. Which we may be fine with.

The question I still have is how much of enforcement we want to do? Does the client reject a push option if it doesn't contain a '=', such that the server doesn't try setting a weird "GIT_PUSH_VAR_bar_baz".

Mind that a different server may not handle these in environment variables, but read it off the wire and handle it in memory.

Show 13 quoted lines

>
>> If we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_<N>
>> but rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,
>> it may be easier to handle, but whether you read from a file or evaluate the
>> environment variable is only a minor step, the indirection is there anyway
>> and this would be very close to what we have above.
>
> It makes the server implementation a bit uglier. You have to create the
> temporary file, and you have to clean it up. What process is responsible
> for cleaning up stale files? Obviously receive-pack would try to clean
> up after itself, but what happens when it is "kill -9"'d, or the system
> goes down, etc? We clean up stale tmp files like tmp_obj_* in git-gc; I
> think we'd want something like that here.

Yeah that is one of the weaknesses with the file based solution.

>
> -Peff

Show 8 quoted lines

> Jeff writes:
>> Junio writes:
>> It still is not clear to me why the option to pass _COUNT and _VAR_<N> is
>> rejected.
> The "count" method gives you the flexibility to parse multiple keys as
> lists, last-one-wins, or whatever scheme you want. But it also gives you
> the _responsibility_ to do the parsing yourself, which is a pain when
> you want to do the simple thing.

We could ship Git with a default parser in the example hook that takes care of the responsibility in an opinionated way (like Git does with "multiple options are allowed", and "last option wins").

If we do that, then the _COUNT method may be favorable?