[PATCH] cli: Hooks for tag-command

[Jani Nikula]

On Jul 18, 2012 12:25 AM, "Dominik Peteler" <dominik at with-h.at> wrote:
>
> hello,
>
> I attached some modifications which I made to notmuch. Comes with man
pages and test.
>

Hi Dominik, please find a couple of comments below. I'm on the road,
replying on a phone, so this is not very thorough...

BR,
Jani.

> regards
>
> dominik
>
>
>
> There are two hooks:
>  * pre-tag: Run before tagging
>  * post-tag: Run after
>
> This allows users to react on changes of tags. For example,
> you might want to move a message to a special Maildir
> depending on its notmuch tags.
> ---
>  man/man1/notmuch-tag.1   | 17 +++++++++++++++++
>  man/man5/notmuch-hooks.5 | 23 +++++++++++++++++++++++
>  notmuch-tag.c            | 25 +++++++++++++++++++++----
>  test/hooks               | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 97 insertions(+), 4 deletions(-)
>
> diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
> index d810e1b..8d8b7b2 100644
> --- a/man/man1/notmuch-tag.1
> +++ b/man/man1/notmuch-tag.1
> @@ -4,6 +4,7 @@ notmuch-tag \- add/remove tags for all messages matching
the search terms
>
>  .SH SYNOPSIS
>  .B notmuch tag
> +.RB "[" --no-hooks "]"
>  .RI  "+<" tag> "|\-<" tag "> [...] [\-\-] <" search-term ">..."
>
>  .SH DESCRIPTION
> @@ -29,6 +30,22 @@ updates the maildir flags according to tag changes if
the
>  configuration option is enabled. See \fBnotmuch-config\fR(1) for
>  details.
>
> +The
> +.B tag
> +command supports hooks. See  \fBnotmuch-hooks(5)\fR
> +for more details on hooks.
> +
> +Supported options for
> +.B tag
> +include
> +.RS 4
> +.TP 4
> +.BR \-\-no\-hooks
> +
> +Prevents hooks from being run.
> +.RE
> +.RE
> +
>  .SH SEE ALSO
>
>  \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
> diff --git a/man/man5/notmuch-hooks.5 b/man/man5/notmuch-hooks.5
> index b914a29..7399627 100644
> --- a/man/man5/notmuch-hooks.5
> +++ b/man/man5/notmuch-hooks.5
> @@ -38,6 +38,29 @@ the scan or import.
>  Typically this hook is used to perform additional query\-based tagging
on the
>  imported messages.
>  .RE
> +.RS 4
> +.TP 4
> +.B pre\-tag
> +This hook is invoked by the
> +.B tag
> +command before tagging messages. If this
> +hook exits with a non-zero status, notmuch will abort further processing
of the
> +.B tag
> +command.
> +
> +Typically this hook is used for syncing the Maildir with notmuch tags.

Maildir syncing usually refers to the maildir flag and notmuch tag syncing
in the notmuch context. The above is bound to be confusing.

> +.RE
> +.RS 4
> +.TP 4
> +.B post\-tag
> +This hook is invoked by the
> +.B tag
> +command after messages have been tagged. The hook will not be run if
there have been any errors during
> +the tagging.
> +
> +Typically this hook is used for syncing the Maildir with notmuch tags.

Ditto.

> +.RE
> +
>
>  .SH SEE ALSO
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 7d18639..e98d3a0 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -174,9 +174,11 @@ notmuch_tag_command (void *ctx, int argc, char
*argv[])
>      int tag_ops_count = 0;
>      char *query_string;
>      notmuch_config_t *config;
> +    const char *db_path;
>      notmuch_database_t *notmuch;
>      struct sigaction action;
>      notmuch_bool_t synchronize_flags;
> +    notmuch_bool_t run_hooks = TRUE;
>      int i;
>      int ret;
>
> @@ -198,11 +200,12 @@ notmuch_tag_command (void *ctx, int argc, char
*argv[])
>      }
>
>      for (i = 0; i < argc; i++) {
> -       if (strcmp (argv[i], "--") == 0) {
> +       if (strcmp (argv[i], "--no-hooks") == 0) {
> +               run_hooks = FALSE;

This is subtler than it looks. This would allow --no-hooks to be placed in
the middle of tag operations. Same for any future arguments. And it would
prevent removal of a hypothetical -no-hooks tag... I think we'll want to
support specifying "--" twice to separate arguments from tag ops, and ops
from query. And it must be forbidden to mix them.

> +       } else if (strcmp (argv[i], "--") == 0) {
>             i++;
>             break;
> -       }
> -       if (argv[i][0] == '+' || argv[i][0] == '-') {
> +       } else if (argv[i][0] == '+' || argv[i][0] == '-') {
>             tag_ops[tag_ops_count].tag = argv[i] + 1;
>             tag_ops[tag_ops_count].remove = (argv[i][0] == '-');
>             tag_ops_count++;
> @@ -229,7 +232,15 @@ notmuch_tag_command (void *ctx, int argc, char
*argv[])
>      if (config == NULL)
>         return 1;
>
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> +    db_path = notmuch_config_get_database_path (config);
> +
> +    if (run_hooks) {
> +       ret = notmuch_run_hook (db_path, "pre-tag");
> +       if (ret)
> +           return ret;
> +    }
> +
> +    if (notmuch_database_open (db_path,
>                                NOTMUCH_DATABASE_MODE_READ_WRITE,
&notmuch))
>         return 1;
>
> @@ -239,5 +250,11 @@ notmuch_tag_command (void *ctx, int argc, char
*argv[])
>
>      notmuch_database_destroy (notmuch);
>
> +    if (run_hooks) {

Can't check further context atm, are you sure not to run post-tag if
there's an error?

> +       ret = notmuch_run_hook (db_path, "post-tag");
> +       if (ret)
> +           return ret;
> +    }
> +
>      return ret;
>  }
> diff --git a/test/hooks b/test/hooks
> index 77e8569..ae857cc 100755
> --- a/test/hooks
> +++ b/test/hooks
> @@ -31,6 +31,7 @@ rm_hooks () {
>  # add a message to generate mail dir and database
>  add_message
>
> +# {pre,post}-new hooks
>  test_begin_subtest "pre-new is run"
>  rm_hooks
>  generate_message
> @@ -101,4 +102,39 @@ EOF
>  chmod +x "${HOOK_DIR}/pre-new"
>  test_expect_code 1 "hook execution failure" "notmuch new"
>
> +
> +
> +# {pre,post}-tag hooks
> +test_begin_subtest "pre-tag is run"
> +rm_hooks
> +generate_message
> +create_echo_hook "pre-tag" expected output
> +notmuch tag +foo -- '*' > /dev/null
> +test_expect_equal_file expected output
> +
> +test_begin_subtest "post-tag is run"
> +rm_hooks
> +generate_message
> +create_echo_hook "post-tag" expected output
> +notmuch tag +foo -- '*'  > /dev/null
> +test_expect_equal_file expected output
> +
> +test_begin_subtest "pre-tag is run before post-new"
> +rm_hooks
> +generate_message
> +create_echo_hook "pre-tag" pre-tag.expected pre-tag.output
> +create_echo_hook "post-tag" post-tag.expected post-tag.output
> +notmuch tag +foo -- '*'  > /dev/null
> +test_expect_equal_file post-tag.expected post-tag.output
> +
> +test_begin_subtest "pre-tag non-zero exit status (hook status)"
> +rm_hooks
> +generate_message
> +create_failing_hook "pre-tag"
> +output=`notmuch tag +foo -- '*'  2>&1`
> +test_expect_equal "$output" "Error: pre-tag hook failed with status 13"
> +
> +# depends on the previous subtest leaving broken hook behind
> +test_expect_code 1 "pre-tag non-zero exit status (notmuch status)"
"notmuch tag +foo -- '*'"
> +
>  test_done
> --
> 1.7.11.2
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120718/cd0913ea/attachment.html>