[pacman-dev] [PATCH] alpm-hooks: add Description field

[Olivier Brunel]

On Mon, 14 Dec 2015 20:26:05 +1000
Allan McRae <allan at archlinux.org> wrote:

> The "Description" field allows a hook to provide a some text for
> frontends to use in describing what the hook is doing.  For example:
> 
> Description = updating info page directory
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---
> 
> I'm not sure if "Description" is the best label, but could not think
> of anything better.
> 
>  doc/alpm-hooks.5.txt | 5 +++++
>  lib/libalpm/hook.c   | 8 +++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/alpm-hooks.5.txt b/doc/alpm-hooks.5.txt
> index 3729387..f33ff8c 100644
> --- a/doc/alpm-hooks.5.txt
> +++ b/doc/alpm-hooks.5.txt
> @@ -19,6 +19,7 @@ Type = File|Package (Required)
>  Target = <Path|PkgName> (Required, Repeatable)
>  
>  [Action] (Required)
> +Description = ... (Optional)
>  When = PreTransaction|PostTransaction (Required)
>  Exec = <Command> (Required)
>  Depends = <PkgName> (Optional)
> @@ -63,6 +64,10 @@ defined the hook will run if the transaction
> matches *any* of the triggers. ACTIONS
>  -------
>  
> +* Description =* ...::
> +	An optional description that describes the action being
> taken by the
> +	hook for use in front-end output.
> +
>  *Exec =* <command>::
>  	Command to run.  Command arguments are split on whitespace.
> Values containing whitespace should be enclosed in quotes.  Required.
> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
> index acd571d..7d0f4f2 100644
> --- a/lib/libalpm/hook.c
> +++ b/lib/libalpm/hook.c
> @@ -48,6 +48,7 @@ struct _alpm_trigger_t {
>  
>  struct _alpm_hook_t {
>  	char *name;
> +	char *desc;
>  	alpm_list_t *triggers;
>  	alpm_list_t *depends;
>  	char **cmd;
> @@ -84,6 +85,7 @@ static void _alpm_hook_free(struct _alpm_hook_t
> *hook) {
>  	if(hook) {
>  		free(hook->name);
> +		free(hook->desc);
>  		_alpm_wordsplit_free(hook->cmd);
>  		alpm_list_free_inner(hook->triggers,
> (alpm_list_fn_free) _alpm_trigger_free);
> alpm_list_free(hook->triggers); @@ -316,6 +318,10 @@ static int
> _alpm_hook_parse_cb(const char *file, int line, } else {
>  				error(_("hook %s line %d: invalid
> value %s\n"), file, line, value); }
> +		} else if(strcmp(key, "Description") == 0) {
> +			char *val;
> +			STRDUP(val, value, return 1);
> +			hook->desc = val;
>  		} else if(strcmp(key, "Depends") == 0) {
>  			char *val;
>  			STRDUP(val, value, return 1);
> @@ -734,7 +740,7 @@ int _alpm_hook_run(alpm_handle_t *handle,
> alpm_hook_when_t when) _alpm_log(handle, ALPM_LOG_DEBUG, "running
> hook %s\n", hook->name); 
>  		hook_event.type = ALPM_EVENT_HOOK_RUN_START;
> -		hook_event.desc = hook->name;
> +		hook_event.desc = hook->desc ? hook->desc :
> hook->name; EVENT(handle, &hook_event);

I don't like this, specifically I feel this should be an addition to
the event data. That is, there should always be a hook_event.name with
the hook's name, and optionaly (as in can be NULL) a hook_event.desc --
Let's leave it to the frontend to decide what/how to show the user.

Speaking of which, in the patch I just sent (re:
ALPM_EVENT_HOOK_RUN_FAILED) I think pacman should include the hook name
in the message, because that's how the user can actually identify the
faulty hook...

>  
>  		if(_alpm_hook_run_hook(handle, hook) != 0 &&
> hook->abort_on_fail) {