From: Jani Nikula Date: Tue, 31 Jul 2012 13:19:04 +0000 (+0200) Subject: Re: [PATCHv2] cli: Hooks for tag-command X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=48c6e3eb4c32d51d0602e9ee8afc5ca543d6df25;p=notmuch-archives.git Re: [PATCHv2] cli: Hooks for tag-command --- diff --git a/42/d60343bb2df9f46f31c3a8c34d26aa59f1a39c b/42/d60343bb2df9f46f31c3a8c34d26aa59f1a39c new file mode 100644 index 000000000..956d6457a --- /dev/null +++ b/42/d60343bb2df9f46f31c3a8c34d26aa59f1a39c @@ -0,0 +1,385 @@ +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 D35DE431FAF + for ; Tue, 31 Jul 2012 06:19:14 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[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 hQo9YQGvdlyC for ; + Tue, 31 Jul 2012 06:19:11 -0700 (PDT) +Received: from mail-gh0-f181.google.com (mail-gh0-f181.google.com + [209.85.160.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 1489E431FBD + for ; Tue, 31 Jul 2012 06:19:11 -0700 (PDT) +Received: by ghbz13 with SMTP id z13so7322316ghb.26 + for ; Tue, 31 Jul 2012 06:19:09 -0700 (PDT) +X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; + d=google.com; s=20120113; + h=from:to:cc:subject:in-reply-to:references:user-agent:date + :message-id:mime-version:content-type:x-gm-message-state; + bh=OF3KX6cPhIB+K9k/6XQeI3217uNYLyNmFl7+SdzElXM=; + b=FcA6fJhD9e8lOQOUMpuVox0Xo8IladheSZTS+KGKEO0B14fpjNX5/3Dw//466Wxen9 + qFR+keM4uk0qZoi5EFCR3EyTEizdPS6KiKA6rL1ljUiAG03BfBuMqD4H2Up60bbiKr7i + 3IO8jRsJa2UtdV2TB0ekM82zylRbXQSFEgRDpjlqIKMoDKKw46OplYwlM5f76PdiDF8V + PrhoNHvfUvPyLS5npfjm725ilrnc24zWBvHqLjtMWPvqszZZRRu7Qt2N+8CdIOlH8jKY + g25TbS50XZFWVTWDM8JJ7/3o3KWEdAu8a1XiShJvfDAD/z7O42UmFsieAfgkH+oUbB/X + GiNQ== +Received: by 10.236.182.228 with SMTP id o64mr13323006yhm.85.1343740749265; + Tue, 31 Jul 2012 06:19:09 -0700 (PDT) +Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3]) + by mx.google.com with ESMTPS id d21sm100098yhe.22.2012.07.31.06.19.08 + (version=SSLv3 cipher=OTHER); Tue, 31 Jul 2012 06:19:08 -0700 (PDT) +From: Jani Nikula +To: Dominik Peteler , notmuch@notmuchmail.org +Subject: Re: [PATCHv2] cli: Hooks for tag-command +In-Reply-To: <1342634970-12991-1-git-send-email-dominik@with-h.at> +References: <1342634970-12991-1-git-send-email-dominik@with-h.at> +User-Agent: Notmuch/0.13.2+106~gb810aee (http://notmuchmail.org) Emacs/23.2.1 + (x86_64-pc-linux-gnu) +Date: Tue, 31 Jul 2012 15:19:04 +0200 +Message-ID: <87txwo9ign.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Gm-Message-State: + ALoCoQmcbn1wesTObaNE7RNf32SWL1nmU9nFDXss/xDHOV9IuGfchZfCBl6rOaLx8eRB7+5hBYr6 +Cc: Dominik Peteler +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: Tue, 31 Jul 2012 13:19:15 -0000 + +On Wed, 18 Jul 2012, Dominik Peteler wrote: +> hello, +> +> I improved my patch according to Janis mail: +> * new cli syntax: notmuch tag [ --no-hooks ] -- [ -- ] +> * adjusted man pages and wrote tests +> +> I had the idea to improve this feature by passing the message-ids or the filename to the hooks. +> What's your opinion about that ? Any suggestions ? + +There are probably races there that are difficult to solve. Imagine two +concurrent 'notmuch tag' invocations. The db can't be locked across the +hooks. Also, if you relied solely on the passed message-ids or filenames +to do whatever your hook would do, interrupting 'notmuch tag' before the +hook is run would leave you in an inconsistent state. + +> +> regards +> +> dominik + +Note that this part of the patch ends up as the commit message in the +git repository. Please have a look at +http://notmuchmail.org/patchformatting/. + +> +> +> +> 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. + +I am not sure if the example is such a hot idea. Moving a file in the +maildir means you'd need to run 'notmuch new' again to inform notmuch of +the change. I.e. after tagging you couldn't view the message before +you've run 'notmuch new'. + +Are there any other, perhaps more compelling reasons to have hooks in +'notmuch tag'? Can you give us some other examples, please? + +> --- +> man/man1/notmuch-tag.1 | 22 +++++++++++++++++++- +> man/man5/notmuch-hooks.5 | 19 ++++++++++++++++++ +> notmuch-tag.c | 52 +++++++++++++++++++++++++++++++++++++++++++++--- +> test/hooks | 36 +++++++++++++++++++++++++++++++++ +> test/tagging | 28 ++++++++++++++++++++++++++ +> 5 files changed, 153 insertions(+), 4 deletions(-) +> +> diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1 +> index d810e1b..e00e189 100644 +> --- a/man/man1/notmuch-tag.1 +> +++ b/man/man1/notmuch-tag.1 +> @@ -4,7 +4,11 @@ notmuch-tag \- add/remove tags for all messages matching the search terms +> +> .SH SYNOPSIS +> .B notmuch tag +> -.RI "+<" tag> "|\-<" tag "> [...] [\-\-] <" search-term ">..." +> +.RI "+<" tag "> |\-<" tag "> [...] [\-\-] <" search-term ">..." +> + +> +.B notmuch tag +> +.RB "[" --no-hooks "]" +> +.RI "\-\- +<" tag "> |\-<" tag "> [...] \-\- <" search-term ">..." +> +> .SH DESCRIPTION +> +> @@ -29,6 +33,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..e193ef5 100644 +> --- a/man/man5/notmuch-hooks.5 +> +++ b/man/man5/notmuch-hooks.5 +> @@ -38,6 +38,25 @@ 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. +> +.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. +> +.RE +> + +> +> .SH SEE ALSO +> +> diff --git a/notmuch-tag.c b/notmuch-tag.c +> index 7d18639..7572059 100644 +> --- a/notmuch-tag.c +> +++ b/notmuch-tag.c +> @@ -174,9 +174,17 @@ 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; +> + /* Points to the position of the "--" delimiters, e. g. +> + * arg_delimiters[0] arg_delimiters[1] +> + * +> + * arg_delimiters[0] may remain -1 if there are no arguments given +> + * arg_delimiters[0] may remain -1 if there is no delimiter between tag ops and search terms */ +> + int arg_delimiters[2] = {-1, -1}; +> + notmuch_bool_t run_hooks = TRUE; +> int i; +> int ret; +> +> @@ -197,11 +205,37 @@ notmuch_tag_command (void *ctx, int argc, char *argv[]) +> return 1; +> } +> +> + /* Determine position of delimiters */ +> for (i = 0; i < argc; i++) { +> if (strcmp (argv[i], "--") == 0) { +> - i++; +> - break; +> + if (arg_delimiters[1] == -1) { +> + arg_delimiters[1] = i; +> + } else if (arg_delimiters[0] == -1) { +> + arg_delimiters[0] = arg_delimiters[1]; +> + arg_delimiters[1] = i; +> + } else { +> + fprintf (stderr, "Error: 'notmuch tag' requires delimiter \"--\" at most two times.\n"); +> + return 1; +> + } +> } +> + } +> + +> + /* Process arguments if present */ +> + for (i = 0; i < arg_delimiters[0]; i++) { +> + if (strcmp (argv[i], "--no-hooks") == 0) { +> + run_hooks = FALSE; +> + } else { +> + fprintf (stderr, "Error: 'notmuch tag' doesn't recognize argument '%s'.\n", argv[i]); +> + return 1; +> + } +> + } +> + +> + /* Set arg_delimiters[1] to argc if no delimiters at all are present */ +> + if (arg_delimiters[1] == -1) +> + arg_delimiters[1] = argc; +> + +> + /* Read tag ops */ +> + for (i = arg_delimiters[0]+1; i < arg_delimiters[1]; i++) { + +As I said before, you should parse the arguments using the notmuch +argument parser. The above is getting needlessly complicated. Please see +the other notmuch commands for examples, and see the relevant part of +[1] for hints how to add this to 'notmuch tag'. + +[1] +id:"c560dc17781ea2e725cc75e00cb328822a65537f.1334404979.git.jani@nikula.org" +or +http://mid.gmane.org/c560dc17781ea2e725cc75e00cb328822a65537f.1334404979.git.jani@nikula.org + +> 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] == '-'); +> @@ -229,7 +263,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 +281,9 @@ notmuch_tag_command (void *ctx, int argc, char *argv[]) +> +> notmuch_database_destroy (notmuch); +> +> + if (!ret && run_hooks) { +> + ret = notmuch_run_hook (db_path, "post-tag"); +> + } +> + +> 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 +> diff --git a/test/tagging b/test/tagging +> index e4782ed..5167f4f 100755 +> --- a/test/tagging +> +++ b/test/tagging +> @@ -46,4 +46,32 @@ test_expect_equal "$output" "\ +> thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; One (:\" inbox tag1 unread) +> thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 tag4 unread)" +> +> +test_begin_subtest "Arguments mixed with tag ops" +> +notmuch tag +-no-hooks --no-hooks -- One +> +notmuch tag --no-hooks +-no-hooks -tag4 -- Two +> +output=$(notmuch search \* | notmuch_search_sanitize) +> +test_expect_equal "$output" "\ +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; One (:\" inbox tag1 unread) +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Two (-no-hooks inbox tag1 unread)" +> +notmuch tag --no-hooks -- Two +> + +> +test_begin_subtest "Arguments with correct position" +> +notmuch tag --no-hooks -- +tag4 -tag4 -- One +> +output=$(notmuch search \* | notmuch_search_sanitize) +> +test_expect_equal "$output" "\ +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; One (:\" inbox tag1 unread) +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 unread)" +> + +> +test_begin_subtest "Missing arguments" +> +notmuch tag -- +tag4 -tag4 -- One +> +output=$(notmuch search \* | notmuch_search_sanitize) +> +test_expect_equal "$output" "\ +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; One (:\" inbox tag1 unread) +> +thread:XXX 2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 unread)" +> + +> +test_begin_subtest "Unknown argument" +> +output=$(notmuch tag --no-blubb -- +tag4 -tag4 -- One 2>&1) +> +test_expect_equal "$output" "\ +> +Error: 'notmuch tag' doesn't recognize argument '--no-blubb'." +> + +> test_done +> -- +> 1.7.11.2 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch