From: Stefan Beller <sbeller@google.com>
Date: Fri, 01 Jul 2016 17:20:32 GMT
Subject: Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options
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:
>
> 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.


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