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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2016, 20:20 UTC
Message-ID: <xmqqeg75p5cn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160707011218.3690-2-sbeller@google.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.

Show 21 quoted lines

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

Show 47 quoted lines

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

Show 11 quoted lines

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