Return-Path: <jani@nikula.org>
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 <notmuch@notmuchmail.org>; 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 <notmuch@notmuchmail.org>;
	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 <notmuch@notmuchmail.org>; Tue, 31 Jul 2012 06:19:11 -0700 (PDT)
Received: by ghbz13 with SMTP id z13so7322316ghb.26
	for <notmuch@notmuchmail.org>; 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 <jani@nikula.org>
To: Dominik Peteler <dominik@with-h.at>, 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 <dominik@with-h.at>
X-BeenThere: notmuch@notmuchmail.org
X-Mailman-Version: 2.1.13
Precedence: list
List-Id: "Use and development of the notmuch mail system."
	<notmuch.notmuchmail.org>
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
List-Archive: <http://notmuchmail.org/pipermail/notmuch>
List-Post: <mailto:notmuch@notmuchmail.org>
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=subscribe>
X-List-Received-Date: Tue, 31 Jul 2012 13:19:15 -0000

On Wed, 18 Jul 2012, Dominik Peteler <dominik@with-h.at> wrote:
> hello,
>
> I improved my patch according to Janis mail:
>  * new cli syntax: notmuch tag [ --no-hooks ] -- <tag ops> [ -- ] <search terms>
>  * 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.
> +     *    <optional arguments> arg_delimiters[0] <tag ops> arg_delimiters[1] <search terms>
> +     *
> +     * 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, &notmuch))
>  	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