[pacman-dev] Potential bug in alpm-hooks; fix provided

[Andrew Gregory]

On 01/02/17 at 05:10pm, Stefan Klinger wrote:
> Hi,
> 
> I'm providing a fix [1] for the following issue.
> 
> I've noticed that exactly one `Exec` directive is allowed in
> alpm-hooks(5).  This is not explicitly stated in the manual (the word
> "required" does not usually imply "non-repeatable", as seems to be
> intended by the manual), nor does it rise an error.  Instead, multiple
> occurrences are silently ignored.
> 
> Use case:
> 
>     # When installed as a pacman hook [1], this will call the
>     # `my-update-pacman-mirrorlist` script every time the file
>     # `/etc/pacman.d/mirrorlist` should be updated.
> 
>     [Trigger]
>     Operation = Upgrade
>     Operation = Install
>     Type = File
>     Target = etc/pacman.d/mirrorlist
> 
>     [Action]
>     When = PostTransaction
>     Exec = /usr/bin/my-update-pacman-mirrorlist
>     Exec = /usr/bin/rm -f /etc/pacman.d/mirrorlist.pacnew
> 
> Only the last `Exec` is used.  I think it is semantically correct
> since the syntax looks like an assignment that overwrites the previous
> one.  But it is inconsistent with the use of `Operation` in the
> `Trigger` section.  I would expect the commands to be evaluated in
> order, or to see an error due to reusing `Exec`.
 
All options accept a single value unless the documentation says
otherwise.  For single-value options, using the last value is fairly
standard.  I'm not sure why you would expect hooks to behave any
differently.

> I think raising an error would be best, otherwise pacman would have to
> provide means to decide how to handle individually failing commands, a
> situation that is more flexibly handled by an external script, which
> should be called instead of using multiple `Exec`s.

I'm neutral on treating overwriting single-value options as an error.
I'm generally in favor of being strict when parsing hooks, but I'm
still considering allowing Include's in hooks, which would complicate
this.

> I have implemented this in [1] and added a test.  If you like that, I
> would update the documentation, and also look for other values that
> are silently overwritten when parsing a hook.  Also, I'd like to make
> `Operation` accept things like
> 
>     Operation = Upgrade Install
> 
> and in future versions deprecate the current use of multiple
> assignments which I consider rather unfortunate.

Multiple "assignments" are not going away.  Triggers can have multiple
Targets and forcing people to cram them all on one line is a terrible
idea.  This is also how multi-value options in pacman's configuration
are parsed.

> Cheers,
> Stefan
> 
> ____________________
> [1] https://github.com/s5k6/pacman/tree/fix-hook-exec