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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> As we first want to focus on getting simple strings to work
> reliably, we go with the last option for now.

OK.

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index d82e912..c875cde 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -247,6 +247,9 @@ Both standard output and standard error output are forwarded to
>  'git send-pack' on the other end, so you can simply `echo` messages
>  for the user.
>  
> +The number of push options are available in the variable GIT_PUSH_OPTION_COUNT
> +and the options itself are in GIT_PUSH_OPTION_{0,1,...}.
> +
>  [[update]]
>  update
>  ~~~~~~
> @@ -322,6 +325,9 @@ a sample script `post-receive-email` provided in the `contrib/hooks`
>  directory in Git distribution, which implements sending commit
>  emails.
>  
> +The number of push options are available in the variable GIT_PUSH_OPTION_COUNT
> +and the options itself are in GIT_PUSH_OPTION_{0,1,...}.
> +

The mention of "push options" in these two entries sounded a bit too
abrupt, at least to me.  Perhaps add a short phrase before the
sentence, e.g.

    When 'git push --push-option=...' is used, the number of push
    options are avaiable ...

or

    The number of push options given on the command line of "git
    push --push-option=..." can be read from the environment
    variable GIT_PUSH_OPTION_COUNT, and the options themselves are
    found in GIT_PUSH_OPTION_0, GIT_PUSH_OPTION_1,...

We can read any of the above variants in two ways to describe what
should happen when "git push" is run without "--push-option=...".
Is GIT_PUSH_OPTION_COUNT unset, or set to 0?  Either way, it may be
better to be a bit more explicit.

The hook driver code does not explicitly clear these environment
variables; it is safe to assume that these won't seep in from the
environment, I think, but I am not sure.

> @@ -1756,6 +1771,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  
>  	if ((commands = read_head_info(&shallow)) != NULL) {
>  		const char *unpack_status = NULL;
> +		struct string_list *push_options = NULL;
>  
>  		prepare_shallow_info(&si, &shallow);
>  		if (!si.nr_ours && !si.nr_theirs)
> @@ -1764,13 +1780,19 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  			unpack_status = unpack_with_sideband(&si);
>  			update_shallow_info(commands, &si, &ref);
>  		}
> -		execute_commands(commands, unpack_status, &si);
> +		execute_commands(commands, unpack_status, &si,
> +				 push_options);
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);
>  		if (report_status)
>  			report(commands, unpack_status);
> -		run_receive_hook(commands, "post-receive", 1);
> +		run_receive_hook(commands, "post-receive", 1,
> +				 push_options);
>  		run_update_post_hook(commands);
> +		if (push_options) {
> +			string_list_clear(push_options, 0);
> +			free(push_options);
> +		}
>  		if (auto_gc) {
>  			const char *argv_gc_auto[] = {
>  				"gc", "--auto", "--quiet", NULL,
> diff --git a/templates/hooks--pre-receive.sample b/templates/hooks--pre-receive.sample
> new file mode 100644
> index 0000000..e4d3edc
> --- /dev/null
> +++ b/templates/hooks--pre-receive.sample
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +#
> +# An example hook script to make use of push options.
> +# The example simply echoes all push options that start with 'echoback='
> +# and rejects all pushes when the "reject" push option is used.
> +#
> +# To enable this hook, rename this file to "pre-receive".
> +
> +if test -n "$GIT_PUSH_OPTION_COUNT"; then
> +	i=0
> +	while test "$i" -lt "$GIT_PUSH_OPTION_COUNT"; do


Style:

        if test -n "$GIT_PUSH_OPTION_COUNT"
        then
		...

	while test ...
        do
		...

> +		eval "value=\$GIT_PUSH_OPTION_$i"
> +		case "$value" in
> +		echoback=*)
> +			echo "echo from the pre-receive-hook ${value#*=}" >&2
> +			;;
> +		reject)
> +			exit 1
> +		esac
> +		i=$((i + 1))
> +	done
> +fi

What is suboptimal about the structure of the series is that we
won't bisect down to any of the potential bugs in the above code
even if we ever see any bug in the future.  It also does not hint
where push_options is expected to be read in the code in the
subsequent patches in the series.  If I were doing this series, I
would probably have done 2/4 first without plumbing it through
(i.e. it is sent and accumulated in a string list at the receiver,
and then cleared and freed without being used), and then added the
processing (i.e. this step) as the second patch.