Return-Path: X-Original-To: notmuch@notmuchmail.org Delivered-To: notmuch@notmuchmail.org Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 39488431FB6 for ; Wed, 18 Jul 2012 04:52:37 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.699 X-Spam-Level: X-Spam-Status: No, score=-0.699 tagged_above=-999 required=5 tests=[HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6BMZQPrWO5QB for ; Wed, 18 Jul 2012 04:52:35 -0700 (PDT) Received: from mail-ob0-f181.google.com (mail-ob0-f181.google.com [209.85.214.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 88BD7431FAF for ; Wed, 18 Jul 2012 04:52:35 -0700 (PDT) Received: by obbup19 with SMTP id up19so2227710obb.26 for ; Wed, 18 Jul 2012 04:52:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-gm-message-state; bh=szFe+tbP4vhSsiVQcwK2pyRelN6tNIiQe+eJbuEH9AQ=; b=LKQYgyw8RquqVlBa/xN9l4M5WPfmvjsfb4y3TgEqHP+CgVvZlGgopYXfRkDrOxODuY ojaLUXEs5MpnaFqr1Z7vvVbSHDLM3vv41yObGl0oJropH8PxXvDUZ2VtajLOEuJb15hv 4i2Ob4Kn1/bLfJObtuKe3HBxEVxeNPczZ03pg8TEAowi2PHEZZQ4aG3ZjZyLm2sppwyz oUR9K7if9g9KbgWzhciK+fzhUJsnV0KWZweRlm8uJAwiyO50ZpWLD27x43UyEZhFyug+ iwb+KvBkbRyMXMUrYWvsgK2FQ767FBPp3Kffz9iQQqvtJnhvZtlkJBMKG5hZFRppNmHg hiug== MIME-Version: 1.0 Received: by 10.60.3.194 with SMTP id e2mr928976oee.1.1342612354849; Wed, 18 Jul 2012 04:52:34 -0700 (PDT) Received: by 10.76.10.102 with HTTP; Wed, 18 Jul 2012 04:52:34 -0700 (PDT) Received: by 10.76.10.102 with HTTP; Wed, 18 Jul 2012 04:52:34 -0700 (PDT) In-Reply-To: <1342503373-16979-1-git-send-email-dominik@with-h.at> References: <1342503373-16979-1-git-send-email-dominik@with-h.at> Date: Wed, 18 Jul 2012 14:52:34 +0300 Message-ID: Subject: Re: [PATCH] cli: Hooks for tag-command From: Jani Nikula To: Dominik Peteler Content-Type: multipart/alternative; boundary=e89a8fb202a07b599a04c5194c52 X-Gm-Message-State: ALoCoQnFyFI4a9F6koReLgdbcLuZMr7tmka2Y3JAIg9ess0UPa+mDB5WHWjsTmkrT+43jQKkOm0p Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jul 2012 11:52:37 -0000 --e89a8fb202a07b599a04c5194c52 Content-Type: text/plain; charset=UTF-8 On Jul 18, 2012 12:25 AM, "Dominik Peteler" 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, ¬much)) > 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@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch --e89a8fb202a07b599a04c5194c52 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Jul 18, 2012 12:25 AM, "Dominik Peteler" <dominik@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:
> =C2=A0* pre-tag: Run before tagging
> =C2=A0* 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.
> ---
> =C2=A0man/man1/notmuch-tag.1 =C2=A0 | 17 +++++++++++++++++
> =C2=A0man/man5/notmuch-hooks.5 | 23 +++++++++++++++++++++++
> =C2=A0notmuch-tag.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 25 ++++= +++++++++++++++++----
> =C2=A0test/hooks =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 36= ++++++++++++++++++++++++++++++++++++
> =C2=A04 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 matchi= ng the search terms
>
> =C2=A0.SH SYNOPSIS
> =C2=A0.B notmuch tag
> +.RB "[" --no-hooks "]"
> =C2=A0.RI =C2=A0"+<" tag> "|\-<" tag &quo= t;> [...] [\-\-] <" search-term ">..."
>
> =C2=A0.SH DESCRIPTION
> @@ -29,6 +30,22 @@ updates the maildir flags according to tag changes = if the
> =C2=A0configuration option is enabled. See \fBnotmuch-config\fR(1) for=
> =C2=A0details.
>
> +The
> +.B tag
> +command supports hooks. See =C2=A0\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
> +
> =C2=A0.SH SEE ALSO
>
> =C2=A0\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.
> =C2=A0Typically this hook is used to perform additional query\-based t= agging on the
> =C2=A0imported messages.
> =C2=A0.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 process= ing 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 synci= ng 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
> +
>
> =C2=A0.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 *a= rgv[])
> =C2=A0 =C2=A0 =C2=A0int tag_ops_count =3D 0;
> =C2=A0 =C2=A0 =C2=A0char *query_string;
> =C2=A0 =C2=A0 =C2=A0notmuch_config_t *config;
> + =C2=A0 =C2=A0const char *db_path;
> =C2=A0 =C2=A0 =C2=A0notmuch_database_t *notmuch;
> =C2=A0 =C2=A0 =C2=A0struct sigaction action;
> =C2=A0 =C2=A0 =C2=A0notmuch_bool_t synchronize_flags;
> + =C2=A0 =C2=A0notmuch_bool_t run_hooks =3D TRUE;
> =C2=A0 =C2=A0 =C2=A0int i;
> =C2=A0 =C2=A0 =C2=A0int ret;
>
> @@ -198,11 +200,12 @@ notmuch_tag_command (void *ctx, int argc, char *= argv[])
> =C2=A0 =C2=A0 =C2=A0}
>
> =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < argc; i++) {
> - =C2=A0 =C2=A0 =C2=A0 if (strcmp (argv[i], "--") =3D=3D 0) = {
> + =C2=A0 =C2=A0 =C2=A0 if (strcmp (argv[i], "--no-hooks") = =3D=3D 0) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 run_hooks =3D 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 woul= d 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.

> + =C2=A0 =C2=A0 =C2=A0 } else if (strcmp (argv[i], "--") = =3D=3D 0) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 i++;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> - =C2=A0 =C2=A0 =C2=A0 }
> - =C2=A0 =C2=A0 =C2=A0 if (argv[i][0] =3D=3D '+' || argv[i][0]= =3D=3D '-') {
> + =C2=A0 =C2=A0 =C2=A0 } else if (argv[i][0] =3D=3D '+' || arg= v[i][0] =3D=3D '-') {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tag_ops[tag_ops_count].tag = =3D argv[i] + 1;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tag_ops[tag_ops_count].remov= e =3D (argv[i][0] =3D=3D '-');
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tag_ops_count++;
> @@ -229,7 +232,15 @@ notmuch_tag_command (void *ctx, int argc, char *a= rgv[])
> =C2=A0 =C2=A0 =C2=A0if (config =3D=3D NULL)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
>
> - =C2=A0 =C2=A0if (notmuch_database_open (notmuch_config_get_database_= path (config),
> + =C2=A0 =C2=A0db_path =3D notmuch_config_get_database_path (config);<= br> > +
> + =C2=A0 =C2=A0if (run_hooks) {
> + =C2=A0 =C2=A0 =C2=A0 ret =3D notmuch_run_hook (db_path, "pre-ta= g");
> + =C2=A0 =C2=A0 =C2=A0 if (ret)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
> + =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0if (notmuch_database_open (db_path,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0NOTMUCH_DATABASE_MODE_READ_WRITE, = &notmuch))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
>
> @@ -239,5 +250,11 @@ notmuch_tag_command (void *ctx, int argc, char *a= rgv[])
>
> =C2=A0 =C2=A0 =C2=A0notmuch_database_destroy (notmuch);
>
> + =C2=A0 =C2=A0if (run_hooks) {

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

> + =C2=A0 =C2=A0 =C2=A0 ret =3D notmuch_run_hook (db_path, "pos= t-tag");
> + =C2=A0 =C2=A0 =C2=A0 if (ret)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
> + =C2=A0 =C2=A0}
> +
> =C2=A0 =C2=A0 =C2=A0return ret;
> =C2=A0}
> diff --git a/test/hooks b/test/hooks
> index 77e8569..ae857cc 100755
> --- a/test/hooks
> +++ b/test/hooks
> @@ -31,6 +31,7 @@ rm_hooks () {
> =C2=A0# add a message to generate mail dir and database
> =C2=A0add_message
>
> +# {pre,post}-new hooks
> =C2=A0test_begin_subtest "pre-new is run"
> =C2=A0rm_hooks
> =C2=A0generate_message
> @@ -101,4 +102,39 @@ EOF
> =C2=A0chmod +x "${HOOK_DIR}/pre-new"
> =C2=A0test_expect_code 1 "hook execution failure" "notm= uch 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 -- '*' =C2=A0> /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<= br> > +create_echo_hook "post-tag" post-tag.expected post-tag.outp= ut
> +notmuch tag +foo -- '*' =C2=A0> /dev/null
> +test_expect_equal_file post-tag.expected post-tag.output
> +
> +test_begin_subtest "pre-tag non-zero exit status (hook status)&q= uot;
> +rm_hooks
> +generate_message
> +create_failing_hook "pre-tag"
> +output=3D`notmuch tag +foo -- '*' =C2=A02>&1`
> +test_expect_equal "$output" "Error: pre-tag hook faile= d 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 -- '*'"
> +
> =C2=A0test_done
> --
> 1.7.11.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org=
> http://not= muchmail.org/mailman/listinfo/notmuch

--e89a8fb202a07b599a04c5194c52--