[pacman-dev] [PATCH 1/3] Collect all triggered hooks before running them

[Andrew Gregory]

On 12/14/15 at 01:45pm, Allan McRae wrote:
> Having a first pass that checks which hooks are triggered followed by a
> second pass of the triggered hooks allows up to only provide output when
> a hook is actually triggered.
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---
>  lib/libalpm/hook.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
> index f5627af..ca90c4d 100644
> --- a/lib/libalpm/hook.c
> +++ b/lib/libalpm/hook.c
> @@ -610,9 +610,9 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook)
>  
>  int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when)
>  {
> -	alpm_list_t *i, *hooks = NULL;
> +	alpm_list_t *i, *hooks = NULL, *hooks_triggered = NULL;
>  	const char *suffix = ".hook";
> -	size_t suflen = strlen(suffix);
> +	size_t suflen = strlen(suffix), triggered = 0;
>  	int ret = 0;
>  
>  	for(i = alpm_list_last(handle->hookdirs); i; i = alpm_list_previous(i)) {
> @@ -712,13 +712,25 @@ int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when)
>  	for(i = hooks; i; i = i->next) {
>  		struct _alpm_hook_t *hook = i->data;
>  		if(hook && hook->when == when && _alpm_hook_triggered(handle, hook)) {
> -			_alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name);
> -			if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) {
> -				ret = -1;
> -			}
> +			hooks_triggered = alpm_list_add(hooks_triggered, hook);
> +			triggered++;
> +		}
> +	}
> +
> +	if(triggered == 0) {

triggered is redundant in this patch.  We can just check
'hooks_triggered == NULL' here which is safer since triggered could,
at least theoretically, overflow and wrap back to 0.

> +		goto cleanup;
> +	}
> +
> +	for(i = hooks_triggered; i; i = i->next) {
> +		struct _alpm_hook_t *hook = i->data;
> +		_alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name);
> +		if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) {
> +			ret = -1;
>  		}
>  	}
>  
> +	alpm_list_free(hooks_triggered);

Just a minor style suggestion:  I would reverse the above conditional
and move all of this inside it to avoid the unnecessary goto.

> +
>  cleanup:
>  	alpm_list_free_inner(hooks, (alpm_list_fn_free) _alpm_hook_free);
>  	alpm_list_free(hooks);
> -- 
> 2.6.4

-- 
apg