f4/93aae810faa18ce8340a5e60e64ec6e606d843

   1 Return-Path: <jani@nikula.org>\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 17CA6429E4E\r
   6         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 06:22:48 -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 98xfxJK0zxeB for <notmuch@notmuchmail.org>;\r
  16         Sat, 14 Jan 2012 06:22:47 -0800 (PST)\r
  17 Received: from mail-ey0-f181.google.com (mail-ey0-f181.google.com\r
  18         [209.85.215.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id CC636429E43\r
  21         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 06:22:46 -0800 (PST)\r
  22 Received: by eaah10 with SMTP id h10so709228eaa.26\r
  23         for <notmuch@notmuchmail.org>; Sat, 14 Jan 2012 06:22:44 -0800 (PST)\r
  24 Received: by 10.213.34.130 with SMTP id l2mr1409050ebd.99.1326550962570;\r
  25         Sat, 14 Jan 2012 06:22:42 -0800 (PST)\r
  26 Received: from localhost (dsl-hkibrasgw4-fe5cdc00-23.dhcp.inet.fi.\r
  27         [80.220.92.23])\r
  28         by mx.google.com with ESMTPS id b49sm44361894eec.9.2012.01.14.06.22.39\r
  29         (version=SSLv3 cipher=OTHER); Sat, 14 Jan 2012 06:22:40 -0800 (PST)\r
  30 From: Jani Nikula <jani@nikula.org>\r
  31 To: Austin Clements <amdragon@MIT.EDU>\r
  32 Subject: Re: [PATCH v4 1/5] cli: slightly refactor "notmuch reply" address\r
  33         scanning functions\r
  34 In-Reply-To: <20120112215905.GF18625@mit.edu>\r
  35 References: <cover.1325794371.git.jani@nikula.org>\r
  36         <cover.1326403905.git.jani@nikula.org>\r
  37         <9935c31d8727331b442ce266ae22469243b85f36.1326403905.git.jani@nikula.org>\r
  38         <20120112215905.GF18625@mit.edu>\r
  39 User-Agent: Notmuch/0.10.2+181~g99ec753 (http://notmuchmail.org) Emacs/23.3.1\r
  40         (i686-pc-linux-gnu)\r
  41 Date: Sat, 14 Jan 2012 16:22:37 +0200\r
  42 Message-ID: <87y5tanz2q.fsf@nikula.org>\r
  43 MIME-Version: 1.0\r
  44 Content-Type: text/plain; charset=us-ascii\r
  45 Cc: notmuch@notmuchmail.org\r
  46 X-BeenThere: notmuch@notmuchmail.org\r
  47 X-Mailman-Version: 2.1.13\r
  48 Precedence: list\r
  49 List-Id: "Use and development of the notmuch mail system."\r
  50         <notmuch.notmuchmail.org>\r
  51 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  52         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  53 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  54 List-Post: <mailto:notmuch@notmuchmail.org>\r
  55 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  56 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  57         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  58 X-List-Received-Date: Sat, 14 Jan 2012 14:22:48 -0000\r
  59 \r
  60 On Thu, 12 Jan 2012 16:59:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:\r
  61 > LGTM.  One thing you could fix below (and a few comments), but not\r
  62 > enough alone to warrant a new version.\r
  63 > \r
  64 > Quoth Jani Nikula on Jan 12 at 11:40 pm:\r
  65 > > Slightly refactor "notmuch reply" recipient and user from address scanning\r
  66 > > functions in preparation for reply-to-sender feature.\r
  67 > > \r
  68 > > Add support for not adding messages at all (just scan for user from\r
  69 > > address), and returning the number of messages added.\r
  70 > > \r
  71 > > No externally visible functional changes.\r
  72 > > \r
  73 > > Signed-off-by: Jani Nikula <jani@nikula.org>\r
  74 > > ---\r
  75 > >  notmuch-reply.c |   74 ++++++++++++++++++++++++++++--------------------------\r
  76 > >  1 files changed, 38 insertions(+), 36 deletions(-)\r
  77 > > \r
  78 > > diff --git a/notmuch-reply.c b/notmuch-reply.c\r
  79 > > index 000f6da..4fae66f 100644\r
  80 > > --- a/notmuch-reply.c\r
  81 > > +++ b/notmuch-reply.c\r
  82 > > @@ -168,22 +168,28 @@ address_is_users (const char *address, notmuch_config_t *config)\r
  83 > >      return 0;\r
  84 > >  }\r
  85 > >  \r
  86 > > -/* For each address in 'list' that is not configured as one of the\r
  87 > > - * user's addresses in 'config', add that address to 'message' as an\r
  88 > > - * address of 'type'.\r
  89 > > +/* Scan addresses in 'list'.\r
  90 > >   *\r
  91 > > - * The first address encountered that *is* the user's address will be\r
  92 > > - * returned, (otherwise NULL is returned).\r
  93 > > + * If 'message' is non-NULL, then for each address in 'list' that is not\r
  94 > > + * configured as one of the user's addresses in 'config', add that address to\r
  95 > > + * 'message' as an address of 'type'.\r
  96 > > + *\r
  97 > > + * If 'user_from' is non-NULL and *user_from is NULL, the first address\r
  98 > > + * encountered in 'list' that *is* the user's address will be set to *user_from.\r
  99 > > + *\r
 100 > > + * Return the number of addresses added to 'message'. (If 'message' is NULL, the\r
 101 > > + * function returns 0 by definition.)\r
 102 > \r
 103 > Ah, I like the return value.  Better than adding an umpteenth argument\r
 104 > like I was suggesting.\r
 105 > \r
 106 > >   */\r
 107 > > -static const char *\r
 108 > > -add_recipients_for_address_list (GMimeMessage *message,\r
 109 > > -                            notmuch_config_t *config,\r
 110 > > -                            GMimeRecipientType type,\r
 111 > > -                            InternetAddressList *list)\r
 112 > > +static unsigned int\r
 113 > > +scan_address_list (InternetAddressList *list,\r
 114 > > +              notmuch_config_t *config,\r
 115 > > +              GMimeMessage *message,\r
 116 > > +              GMimeRecipientType type,\r
 117 > > +              const char **user_from)\r
 118 > >  {\r
 119 > >      InternetAddress *address;\r
 120 > >      int i;\r
 121 > > -    const char *ret = NULL;\r
 122 > > +    unsigned int n = 0;\r
 123 > >  \r
 124 > >      for (i = 0; i < internet_address_list_length (list); i++) {\r
 125 > >     address = internet_address_list_get_address (list, i);\r
 126 > > @@ -196,8 +202,7 @@ add_recipients_for_address_list (GMimeMessage *message,\r
 127 > >         if (group_list == NULL)\r
 128 > >             continue;\r
 129 > >  \r
 130 > > -       add_recipients_for_address_list (message, config,\r
 131 > > -                                        type, group_list);\r
 132 > > +       n += scan_address_list (group_list, config, message, type, NULL);\r
 133 > \r
 134 > Should the NULL above be user_from?  You're being compatible with the\r
 135 > original code, which is the right thing to do in this patch, but the\r
 136 > new-found explicitness made me wonder if this is actually a bug.\r
 137 \r
 138 I didn't even know what a group list was, and if [1] is accurate, I\r
 139 don't think I've ever seen one either. This does smell like a bug, but\r
 140 I'm doubtful if anyone would ever notice... Anyway, as you say, it\r
 141 should be another patch, and independent of this series.\r
 142 \r
 143 BR,\r
 144 Jani.\r
 145 \r
 146 [1] http://www.cs.tut.fi/~jkorpela/rfc/822addr.html\r
 147 \r
 148 > \r
 149 > >     } else {\r
 150 > >         InternetAddressMailbox *mailbox;\r
 151 > >         const char *name;\r
 152 > > @@ -209,40 +214,40 @@ add_recipients_for_address_list (GMimeMessage *message,\r
 153 > >         addr = internet_address_mailbox_get_addr (mailbox);\r
 154 > >  \r
 155 > >         if (address_is_users (addr, config)) {\r
 156 > > -           if (ret == NULL)\r
 157 > > -               ret = addr;\r
 158 > > -       } else {\r
 159 > > +           if (user_from && *user_from == NULL)\r
 160 > > +               *user_from = addr;\r
 161 > > +       } else if (message) {\r
 162 > >             g_mime_message_add_recipient (message, type, name, addr);\r
 163 > > +           n++;\r
 164 > >         }\r
 165 > >     }\r
 166 > >      }\r
 167 > >  \r
 168 > > -    return ret;\r
 169 > > +    return n;\r
 170 > >  }\r
 171 > >  \r
 172 > > -/* For each address in 'recipients' that is not configured as one of\r
 173 > > - * the user's addresses in 'config', add that address to 'message' as\r
 174 > > - * an address of 'type'.\r
 175 > > +/* Scan addresses in 'recipients'.\r
 176 > >   *\r
 177 > > - * The first address encountered that *is* the user's address will be\r
 178 > > - * returned, (otherwise NULL is returned).\r
 179 > > + * See the documentation of scan_address_list() above. This function does\r
 180 > > + * exactly the same, but converts 'recipients' to an InternetAddressList first.\r
 181 > \r
 182 > Just bikeshedding, but comments in the notmuch codebase almost\r
 183 > universally wrap at 72 columns, not 80 (based on \r
 184 > egrep '[ /]\* .{70}' *.[ch]*)\r
 185 > \r
 186 > >   */\r
 187 > > -static const char *\r
 188 > > -add_recipients_for_string (GMimeMessage *message,\r
 189 > > -                      notmuch_config_t *config,\r
 190 > > -                      GMimeRecipientType type,\r
 191 > > -                      const char *recipients)\r
 192 > > +static unsigned int\r
 193 > > +scan_address_string (const char *recipients,\r
 194 > > +                notmuch_config_t *config,\r
 195 > > +                GMimeMessage *message,\r
 196 > > +                GMimeRecipientType type,\r
 197 > > +                const char **user_from)\r
 198 > >  {\r
 199 > >      InternetAddressList *list;\r
 200 > >  \r
 201 > >      if (recipients == NULL)\r
 202 > > -   return NULL;\r
 203 > > +   return 0;\r
 204 > >  \r
 205 > >      list = internet_address_list_parse_string (recipients);\r
 206 > >      if (list == NULL)\r
 207 > > -   return NULL;\r
 208 > > +   return 0;\r
 209 > >  \r
 210 > > -    return add_recipients_for_address_list (message, config, type, list);\r
 211 > > +    return scan_address_list (list, config, message, type, user_from);\r
 212 > >  }\r
 213 > >  \r
 214 > >  /* Does the address in the Reply-To header of 'message' already appear\r
 215 > > @@ -324,7 +329,7 @@ add_recipients_from_message (GMimeMessage *reply,\r
 216 > >      }\r
 217 > >  \r
 218 > >      for (i = 0; i < ARRAY_SIZE (reply_to_map); i++) {\r
 219 > > -   const char *addr, *recipients;\r
 220 > > +   const char *recipients;\r
 221 > >  \r
 222 > >     recipients = notmuch_message_get_header (message,\r
 223 > >                                              reply_to_map[i].header);\r
 224 > > @@ -332,11 +337,8 @@ add_recipients_from_message (GMimeMessage *reply,\r
 225 > >         recipients = notmuch_message_get_header (message,\r
 226 > >                                                  reply_to_map[i].fallback);\r
 227 > >  \r
 228 > > -   addr = add_recipients_for_string (reply, config,\r
 229 > > -                                     reply_to_map[i].recipient_type,\r
 230 > > -                                     recipients);\r
 231 > > -   if (from_addr == NULL)\r
 232 > > -       from_addr = addr;\r
 233 > > +   scan_address_string (recipients, config, reply,\r
 234 > > +                        reply_to_map[i].recipient_type, &from_addr);\r
 235 > >      }\r
 236 > >  \r
 237 > >      return from_addr;\r