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

From: Stefan Beller <sbeller@google.com>
Date: Jul 1, 2016, 17:20 UTC
Message-ID: <CAGZ79kaDCLm3BBURJKfkYWKKvozkFTGCn0wGiQCtspUvtQBd+g@mail.gmail.com>
In-Reply-To: <20160701071410.GG5358@sigill.intra.peff.net>

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

Show 6 quoted lines

>
> Putting the data into a file does alleviate some issues. But it also
> makes parsing annoying, and opens up new issues (like what happens
> to content that has a newline in it?). Wouldn't multiple files be a bit
> more convenient? Especially for your example of eventually carrying
> larger binary content like images.

Yeah I did not think about having multiple files. I think that is the way to go now. In the environment variable you'd have the paths, such that

process_hook_option_from_file()
{
 ... # do a thing with $1
}

for p in $GIT_HOOK_OPTION_FILES ;
do
  process_hook_option_from_file $p
done

is a viable hook.

Show 6 quoted lines

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

Thanks, Stefan

>
> -Peff