hook: add internal state alloc/free callbacks

Some hooks use opaque structs to keep internal state between callbacks.

Because hooks ran sequentially (jobs == 1) with one command per hook,
these internal states could be allocated on the stack for each hook run.

Next commits add the ability to run multiple commands for each hook, so
the states cannot be shared or stored on the stack anymore, especially
since down the line we will also enable parallel execution (jobs > 1).

Add alloc/free helpers for each hook, doing a "deep" alloc/init & free
of their internal opaque struct.

The alloc callback takes a context pointer, to initialize the struct at
at the time of resource acquisition.

These callbacks must always be provided together: no alloc without free
and no free without alloc, otherwise a BUG() is triggered.

Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b5379a489569944097e00f7b72520e54d37c79b2..5259f097884871b68a78f4a6c975e03f946a283d 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -901,6 +901,26 @@ static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_
        return state->cmd ? 0 : 1;  /* 0 = more to come, 1 = EOF */
 }
 
+static void *receive_hook_feed_state_alloc(void *feed_pipe_ctx)
+{
+       struct receive_hook_feed_state *init_state = feed_pipe_ctx;
+       struct receive_hook_feed_state *data = xcalloc(1, sizeof(*data));
+       data->report = init_state->report;
+       data->cmd = init_state->cmd;
+       data->skip_broken = init_state->skip_broken;
+       strbuf_init(&data->buf, 0);
+       return data;
+}
+
+static void receive_hook_feed_state_free(void *data)
+{
+       struct receive_hook_feed_state *d = data;
+       if (!d)
+               return;
+       strbuf_release(&d->buf);
+       free(d);
+}
+
 static int run_receive_hook(struct command *commands,
                            const char *hook_name,
                            int skip_broken,
@@ -908,7 +928,7 @@ static int run_receive_hook(struct command *commands,
 {
        struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
        struct command *iter = commands;
-       struct receive_hook_feed_state feed_state;
+       struct receive_hook_feed_state feed_init_state = { 0 };
        struct async sideband_async;
        int sideband_async_started = 0;
        int saved_stderr = -1;
@@ -938,16 +958,15 @@ static int run_receive_hook(struct command *commands,
        prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started);
 
        /* set up stdin callback */
-       feed_state.cmd = commands;
-       feed_state.skip_broken = skip_broken;
-       feed_state.report = NULL;
-       strbuf_init(&feed_state.buf, 0);
-       opt.feed_pipe_cb_data = &feed_state;
+       feed_init_state.cmd = commands;
+       feed_init_state.skip_broken = skip_broken;
+       opt.feed_pipe_ctx = &feed_init_state;
        opt.feed_pipe = feed_receive_hook_cb;
+       opt.feed_pipe_cb_data_alloc = receive_hook_feed_state_alloc;
+       opt.feed_pipe_cb_data_free = receive_hook_feed_state_free;
 
        ret = run_hooks_opt(the_repository, hook_name, &opt);
 
-       strbuf_release(&feed_state.buf);
        finish_sideband_async(&sideband_async, saved_stderr, sideband_async_started);
 
        return ret;

diff --git a/hook.c b/hook.c
index cde719841250d16c8fc798ab9f3d2d7619bc0c7b..83ff658866627f7c79b351fe49603a0230a69537 100644 (file)
--- a/hook.c
+++ b/hook.c
@@ -133,6 +133,8 @@ static int notify_hook_finished(int result,
 
 static void run_hooks_opt_clear(struct run_hooks_opt *options)
 {
+       if (options->feed_pipe_cb_data_free)
+               options->feed_pipe_cb_data_free(options->feed_pipe_cb_data);
        strvec_clear(&options->env);
        strvec_clear(&options->args);
 }
@@ -172,6 +174,17 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
        if (!options->jobs)
                BUG("run_hooks_opt must be called with options.jobs >= 1");
 
+       /*
+        * 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");
+
+       if (options->feed_pipe_cb_data_alloc)
+               options->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx);
+
        if (options->invoked_hook)
                *options->invoked_hook = 0;
 

diff --git a/hook.h b/hook.h
index 20eb56fd634c8ddf6036c1be7c3e0d30ce1c6f1f..a6bdc6f90fd05a19a9ff505a08d5d29db77d039d 100644 (file)
--- a/hook.h
+++ b/hook.h
@@ -5,6 +5,9 @@
 
 struct repository;
 
+typedef void (*cb_data_free_fn)(void *data);
+typedef void *(*cb_data_alloc_fn)(void *init_ctx);
+
 struct run_hooks_opt
 {
        /* Environment vars to be set for each hook */
@@ -88,10 +91,30 @@ struct run_hooks_opt
         * It can be accessed directly via the third callback arg 'pp_task_cb':
         * struct ... *state = pp_task_cb;
         *
-        * The caller is responsible for managing the memory for this data.
+        * The caller is responsible for managing the memory for this data by
+        * providing alloc/free callbacks to `run_hooks_opt`.
+        *
         * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it.
         */
        void *feed_pipe_cb_data;
+
+       /**
+        * Some hooks need to create a fresh `feed_pipe_cb_data` internal state,
+        * so they can keep track of progress without affecting one another.
+        *
+        * If provided, this function will be called to alloc & initialize the
+        * `feed_pipe_cb_data` for each hook.
+        *
+        * The `feed_pipe_ctx` pointer can be used to pass initialization data.
+        */
+       cb_data_alloc_fn feed_pipe_cb_data_alloc;
+
+       /**
+        * Called to free the memory initialized by `feed_pipe_cb_data_alloc`.
+        *
+        * Must always be provided when `feed_pipe_cb_data_alloc` is provided.
+        */
+       cb_data_free_fn feed_pipe_cb_data_free;
 };
 
 #define RUN_HOOKS_OPT_INIT { \

diff --git a/refs.c b/refs.c
index 1e2ac90018c1f7a1872cdfdea696e3a56cbd48a6..e06a137c296c7693c3e18d9d8ab42a52df8c44bc 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2511,24 +2511,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;
-
-       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);
        return ret;
 }
 

diff --git a/transport.c b/transport.c
index e876cc9189d33ffe025cde09fb62c3fb1728a38d..d25e9a4fb09ae23d55d544d6742653f48f33fc66 100644 (file)
--- 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)
+{
+       struct feed_pre_push_hook_data *data = xmalloc(sizeof(*data));
+       strbuf_init(&data->buf, 0);
+       data->refs = (struct ref *)feed_pipe_ctx;
+       return data;
+}
+
+static void pre_push_hook_data_free(void *data)
+{
+       struct feed_pre_push_hook_data *d = data;
+       if (!d)
+               return;
+       strbuf_release(&d->buf);
+       free(d);
+}
+
 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;
 
        /*
         * pre-push hooks expect stdout & stderr to be separate, so don't merge
@@ -1381,8 +1396,6 @@ static int run_pre_push_hook(struct transport *transport,
 
        ret = run_hooks_opt(the_repository, "pre-push", &opt);
 
-       strbuf_release(&data.buf);
-
        return ret;
 }