Re: [PATCH v2 1/8] hook: add internal state alloc/free callbacks

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

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> +	/*
> +	 * Ensure cb_data copy and free functions are either provided together,
> +	 * or neither one is provided.
> +	 */
> +	if ((options->feed_pipe_cb_data_alloc && !options->feed_pipe_cb_data_free) ||
> +	    (!options->feed_pipe_cb_data_alloc && options->feed_pipe_cb_data_free))
> +		BUG("feed_pipe_cb_data_alloc and feed_pipe_cb_data_free must be set together");

A way to avoid being repetitious may be to say

	if (!!options->feed_pipe_cb_data_alloc != !!options->feed_pipe_cb_data_free)

or

	if (!!options->feed_pipe_cb_data_alloc ^ !!options->feed_pipe_cb_data_free)

but it (especially the latter) might be a bit too cute for some
people's taste.

> diff --git a/refs.c b/refs.c
> index d432cfb78f..3c124fab29 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2514,24 +2514,38 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_
>  	return 0; /* no more input to feed */
>  }
>  
> +static void *transaction_feed_cb_data_alloc(void *feed_pipe_ctx UNUSED)
> +{
> +	struct transaction_feed_cb_data *data = xmalloc(sizeof(*data));
> +	strbuf_init(&data->buf, 0);
> +	data->index = 0;
> +	return data;
> +}
> +
> +static void transaction_feed_cb_data_free(void *data)
> +{
> +	struct transaction_feed_cb_data *d = data;
> +	if (!d)
> +		return;
> +	strbuf_release(&d->buf);
> +	free(d);
> +}
> +
>  static int run_transaction_hook(struct ref_transaction *transaction,
>  				const char *state)
>  {
>  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> -	struct transaction_feed_cb_data feed_ctx = { 0 };
>  	int ret = 0;
>  
>  	strvec_push(&opt.args, state);
>  
>  	opt.feed_pipe = transaction_hook_feed_stdin;
>  	opt.feed_pipe_ctx = transaction;
> -	opt.feed_pipe_cb_data = &feed_ctx;

So, as the proposed log message promised, our stack lost the
callback data, and instead uses an allocated piece of memory
pointed by the same opt.feed_pipe_cb_data member as before.

> -
> -	strbuf_init(&feed_ctx.buf, 0);
> +	opt.feed_pipe_cb_data_alloc = transaction_feed_cb_data_alloc;
> +	opt.feed_pipe_cb_data_free = transaction_feed_cb_data_free;
>  
>  	ret = run_hooks_opt(transaction->ref_store->repo, "reference-transaction", &opt);
>  
> -	strbuf_release(&feed_ctx.buf);

As the run_hooks_opt() machinery internally manages the
feed_pipe_cb_data, starting from the call to the custom alloc
function and concliding with the call to the custom free function,
this function no longer need to manage resources.  Instead, the
custom alloc/free functions take care of all the associated
resources, like the feed_ctx.buf strbuf, as we have seen earlier.

Nice.

> diff --git a/transport.c b/transport.c
> index ecf9e1f21c..1581aa0886 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1357,21 +1357,36 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void
>  	return 0;
>  }
>  
> +static void *pre_push_hook_data_alloc(void *feed_pipe_ctx)
> +{
> +...
> +}
> +
> +static void pre_push_hook_data_free(void *data)
> +{
> +...
> +}
> +
>  static int run_pre_push_hook(struct transport *transport,
>  			     struct ref *remote_refs)
>  {
>  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> -	struct feed_pre_push_hook_data data;
>  	int ret = 0;
>  
>  	strvec_push(&opt.args, transport->remote->name);
>  	strvec_push(&opt.args, transport->url);
>  
> -	strbuf_init(&data.buf, 0);
> -	data.refs = remote_refs;
> -
>  	opt.feed_pipe = pre_push_hook_feed_stdin;
> -	opt.feed_pipe_cb_data = &data;
> +	opt.feed_pipe_ctx = remote_refs;
> +	opt.feed_pipe_cb_data_alloc = pre_push_hook_data_alloc;
> +	opt.feed_pipe_cb_data_free = pre_push_hook_data_free;

The same structure is used here.  Nice.