52/a238b2fd71f97127f49fa783afde51ab1ab015

   1 Return-Path: <amdragon@mit.edu>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id B0D51429E26\r
   6         for <notmuch@notmuchmail.org>; Sat,  3 Dec 2011 19:40:26 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id FOIjlAIUbDq0 for <notmuch@notmuchmail.org>;\r
  16         Sat,  3 Dec 2011 19:40:25 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-5.mit.edu (DMZ-MAILSEC-SCANNER-5.MIT.EDU\r
  18         [18.7.68.34])\r
  19         by olra.theworths.org (Postfix) with ESMTP id A044F429E25\r
  20         for <notmuch@notmuchmail.org>; Sat,  3 Dec 2011 19:40:25 -0800 (PST)\r
  21 X-AuditID: 12074422-b7ff56d00000092f-b5-4edaeba8fed2\r
  22 Received: from mailhub-auth-4.mit.edu ( [18.7.62.39])\r
  23         by dmz-mailsec-scanner-5.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 84.91.02351.8ABEADE4; Sat,  3 Dec 2011 22:40:24 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id pB43eNjH001306; \r
  27         Sat, 3 Dec 2011 22:40:24 -0500\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id pB43eM8P010210\r
  32         (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT);\r
  33         Sat, 3 Dec 2011 22:40:23 -0500 (EST)\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1RX2xu-0004PX-T8; Sat, 03 Dec 2011 22:42:10 -0500\r
  37 Date: Sat, 3 Dec 2011 22:42:10 -0500\r
  38 From: Austin Clements <amdragon@MIT.EDU>\r
  39 To: Jani Nikula <jani@nikula.org>\r
  40 Subject: Re: [PATCH v2 1/2] cli: introduce the concept of user defined hooks\r
  41 Message-ID: <20111204034210.GA16405@mit.edu>\r
  42 References:\r
  43  <7fbe6befcf31881a9bca672f55b93501249a220c.1322859389.git.jani@nikula.org>\r
  44         <6688b09fffa2a66b496af78008102f88ab4e9450.1322953841.git.jani@nikula.org>\r
  45 MIME-Version: 1.0\r
  46 Content-Type: text/plain; charset=iso-8859-1\r
  47 Content-Disposition: inline\r
  48 Content-Transfer-Encoding: 8bit\r
  49 In-Reply-To:\r
  50  <6688b09fffa2a66b496af78008102f88ab4e9450.1322953841.git.jani@nikula.org>\r
  51 User-Agent: Mutt/1.5.21 (2010-09-15)\r
  52 X-Brightmail-Tracker:\r
  53  H4sIAAAAAAAAA+NgFlrOKsWRmVeSWpSXmKPExsUixG6nrrvi9S0/g2kXdSyapjtbXL85k9mB\r
  54         yePW/dfsHs9W3WIOYIrisklJzcksSy3St0vgymj4PIGxoF294se0PuYGxg65LkZODgkBE4n2\r
  55         fS+YIGwxiQv31rN1MXJxCAnsY5SY/fApWEJIYD2jxPeH2hCJE0wSd7ceYodwljBKPP06B6yK\r
  56         RUBF4t71SWA2m4CGxLb9yxlBbBEBRYnNJ/eD2cwC0hLffjeD1QgL+Eh0/9zFDmLzCuhItNxb\r
  57         DjV0IaNE07+bLBAJQYmTM5+wQDTrSOzcegfoPg6wQcv/cUCE5SWat85mBrE5BcIk3mybCjZf\r
  58         FOieKSe3sU1gFJ6FZNIsJJNmIUyahWTSAkaWVYyyKblVurmJmTnFqcm6xcmJeXmpRbqmermZ\r
  59         JXqpKaWbGMFx4KK0g/HnQaVDjAIcjEo8vJknbvkJsSaWFVfmHmKU5GBSEuX9/QooxJeUn1KZ\r
  60         kVicEV9UmpNafIhRgoNZSYS36yFQjjclsbIqtSgfJiXNwaIkzsu108FPSCA9sSQ1OzW1ILUI\r
  61         JivDwaEkwXsSZKhgUWp6akVaZk4JQpqJgxNkOA/QcKPXIMOLCxJzizPTIfKnGHU5tr7oP8Mo\r
  62         xJKXn5cqJc7rC1IkAFKUUZoHNweWvl4xigO9JcxrDlLFA0x9cJNeAS1hAlqi2HgDZElJIkJK\r
  63         qoGxbpfF3VdzUsIqq/e+ubPbqCJcilEk6tOO43NbN33oZD0e+tDq2/UXeWE1r+0+zBVd89d7\r
  64         5l7zQ17cB23FPihs4g+6s/L88SUTjBdtWHR9y2SDn27P/hq7yt7QKPU88vRP5EdTa26vW7s+\r
  65         a9c4h8qHPppxkbffYtse82qurqbOrFe/tByjr7y4qcRSnJFoqMVcVJwIAJ2key86AwAA\r
  66 Cc: notmuch@notmuchmail.org\r
  67 X-BeenThere: notmuch@notmuchmail.org\r
  68 X-Mailman-Version: 2.1.13\r
  69 Precedence: list\r
  70 List-Id: "Use and development of the notmuch mail system."\r
  71         <notmuch.notmuchmail.org>\r
  72 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  73         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  74 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  75 List-Post: <mailto:notmuch@notmuchmail.org>\r
  76 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  77 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  78         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  79 X-List-Received-Date: Sun, 04 Dec 2011 03:40:26 -0000\r
  80 \r
  81 I like it.  See below for some nits.\r
  82 \r
  83 Quoth Jani Nikula on Dec 04 at  1:16 am:\r
  84 > Add mechanism for running user defined hooks. Hooks are executables or\r
  85 > symlinks to executables stored under the new notmuch hooks directory,\r
  86 > <database-path>/.notmuch/hooks.\r
  87 > \r
  88 > No hooks are introduced here, but adding support for a hook is now a simple\r
  89 > matter of calling the new notmuch_run_hook() function at an appropriate\r
  90 > location with the hook name.\r
  91 > \r
  92 > Signed-off-by: Jani Nikula <jani@nikula.org>\r
  93 > \r
  94 > ---\r
  95 > \r
  96 > v2: Switch to git style hooks in a hook directory as suggested by Austin\r
  97 > Clements in IRC. Update manpage and add polish.\r
  98 > ---\r
  99 >  Makefile.local   |    1 +\r
 100 >  notmuch-client.h |    3 ++\r
 101 >  notmuch-hook.c   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
 102 >  3 files changed, 67 insertions(+), 0 deletions(-)\r
 103 >  create mode 100644 notmuch-hook.c\r
 104 > \r
 105 > diff --git a/Makefile.local b/Makefile.local\r
 106 > index c94402b..a1665e1 100644\r
 107 > --- a/Makefile.local\r
 108 > +++ b/Makefile.local\r
 109 > @@ -302,6 +302,7 @@ notmuch_client_srcs =             \\r
 110 >       notmuch-config.c        \\r
 111 >       notmuch-count.c         \\r
 112 >       notmuch-dump.c          \\r
 113 > +     notmuch-hook.c          \\r
 114 >       notmuch-new.c           \\r
 115 >       notmuch-reply.c         \\r
 116 >       notmuch-restore.c       \\r
 117 > diff --git a/notmuch-client.h b/notmuch-client.h\r
 118 > index b50cb38..a91ad6c 100644\r
 119 > --- a/notmuch-client.h\r
 120 > +++ b/notmuch-client.h\r
 121 > @@ -235,6 +235,9 @@ void\r
 122 >  notmuch_config_set_maildir_synchronize_flags (notmuch_config_t *config,\r
 123 >                                             notmuch_bool_t synchronize_flags);\r
 124 >  \r
 125 > +int\r
 126 > +notmuch_run_hook (const char *db_path, const char *hook);\r
 127 > +\r
 128 >  notmuch_bool_t\r
 129 >  debugger_is_active (void);\r
 130 >  \r
 131 > diff --git a/notmuch-hook.c b/notmuch-hook.c\r
 132 > new file mode 100644\r
 133 > index 0000000..fc32044\r
 134 > --- /dev/null\r
 135 > +++ b/notmuch-hook.c\r
 136 > @@ -0,0 +1,63 @@\r
 137 > +/* notmuch - Not much of an email program, (just index and search)\r
 138 > + *\r
 139 > + * This file is part of notmuch.\r
 140 > + *\r
 141 > + * Copyright © 2011 Jani Nikula\r
 142 > + *\r
 143 > + * This program is free software: you can redistribute it and/or modify\r
 144 > + * it under the terms of the GNU General Public License as published by\r
 145 > + * the Free Software Foundation, either version 3 of the License, or\r
 146 > + * (at your option) any later version.\r
 147 > + *\r
 148 > + * This program is distributed in the hope that it will be useful,\r
 149 > + * but WITHOUT ANY WARRANTY; without even the implied warranty of\r
 150 > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the\r
 151 > + * GNU General Public License for more details.\r
 152 > + *\r
 153 > + * You should have received a copy of the GNU General Public License\r
 154 > + * along with this program.  If not, see http://www.gnu.org/licenses/ .\r
 155 > + *\r
 156 > + * Author: Jani Nikula <jani@nikula.org>\r
 157 > + */\r
 158 > +\r
 159 > +#include "notmuch-client.h"\r
 160 > +\r
 161 > +int\r
 162 > +notmuch_run_hook (const char *db_path, const char *hook)\r
 163 > +{\r
 164 > +    char *hook_path;\r
 165 > +    int status = 0;\r
 166 \r
 167 You use status as both a notmuch_status_t and for generic C library\r
 168 results.  This seems a little weird.  You may or may not want to do\r
 169 anything about it.\r
 170 \r
 171 > +\r
 172 > +    if (asprintf (&hook_path, "%s/%s/%s/%s", db_path, ".notmuch", "hooks",\r
 173 > +               hook) == -1)\r
 174 \r
 175 asprintf isn't very portable.  Perhaps talloc_asprintf would be\r
 176 better?  (And more idiomatic.)\r
 177 \r
 178 > +     return NOTMUCH_STATUS_OUT_OF_MEMORY;\r
 179 > +\r
 180 > +    if (access (hook_path, X_OK) == -1) {\r
 181 > +     /* Ignore ENOENT. It's okay not to have a hook, hook dir, or even\r
 182 > +      * notmuch dir. Dangling symbolic links also result in ENOENT, but\r
 183 > +      * we'll ignore that too for simplicity. */\r
 184 > +     if (errno != ENOENT) {\r
 185 > +         fprintf (stderr, "Error: %s hook access failed: %s\n", hook,\r
 186 > +                  strerror (errno));\r
 187 > +         status = NOTMUCH_STATUS_FILE_ERROR;\r
 188 > +     }\r
 189 > +     goto DONE;\r
 190 > +    }\r
 191 > +\r
 192 > +    status = system (hook_path);\r
 193 \r
 194 It would probably be better to fork/execl.  system is sensitive to all\r
 195 sorts of weird things because it invokes the shell (e.g., spaces or\r
 196 funny characters in db_path) and it plays funny games with signals.\r
 197 \r
 198 Really proper error handling with fork/exec is a pain, but I think you\r
 199 can get away with something simpler and even get rid of the access\r
 200 call in the process.  Something like\r
 201 \r
 202 ret = fork();\r
 203 if (ret < 0) ...\r
 204 else if (ret == 0) {\r
 205   ret = execl(hook_path, hook_path, NULL);\r
 206   if (ret != ENOENT && ret != EACCESS)\r
 207     print a real error message\r
 208   exit(0);\r
 209 } else {\r
 210   waitpid(ret, &status, 0);\r
 211   if (status) .. checks you do now ..\r
 212 }\r
 213 \r
 214 I guess this wastes a fork if there is no hook script, so maybe the\r
 215 access call is worth doing anyway.\r
 216 \r
 217 > +    if (status) {\r
 218 > +     if (WIFSIGNALED(status))\r
 219 > +         fprintf(stderr, "Error: %s hook terminated with signal %d\n", hook,\r
 220 > +                 WTERMSIG(status));\r
 221 > +     else\r
 222 > +         fprintf(stderr, "Error: %s hook failed with status %d\n", hook,\r
 223 > +                 WEXITSTATUS(status));\r
 224 > +\r
 225 > +     status = NOTMUCH_STATUS_FILE_ERROR; /* Close enough */\r
 226 > +    }\r
 227 > +\r
 228 > +  DONE:\r
 229 > +    free (hook_path);\r
 230 > +\r
 231 > +    return status;\r
 232 > +}\r