3e/8dd3409f580a8e0286a2eef0c2526ac063eb35

   1 Return-Path: <sojkam1@fel.cvut.cz>\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 1C8E1431FAF\r
   6         for <notmuch@notmuchmail.org>; Mon, 13 Oct 2014 14:38:44 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -2.3\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-2.3 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_MED=-2.3] 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 d50R1f3cgZVk for <notmuch@notmuchmail.org>;\r
  16         Mon, 13 Oct 2014 14:38:40 -0700 (PDT)\r
  17 Received: from max.feld.cvut.cz (max.feld.cvut.cz [147.32.192.36])\r
  18         by olra.theworths.org (Postfix) with ESMTP id 80EB9431FAE\r
  19         for <notmuch@notmuchmail.org>; Mon, 13 Oct 2014 14:38:40 -0700 (PDT)\r
  20 Received: from localhost (unknown [192.168.200.7])\r
  21         by max.feld.cvut.cz (Postfix) with ESMTP id 4D9475CCF4A;\r
  22         Mon, 13 Oct 2014 23:38:35 +0200 (CEST)\r
  23 X-Virus-Scanned: IMAP STYX AMAVIS\r
  24 Received: from max.feld.cvut.cz ([192.168.200.1])\r
  25         by localhost (styx.feld.cvut.cz [192.168.200.7]) (amavisd-new,\r
  26         port 10044)\r
  27         with ESMTP id bY1nH3zVLXiJ; Mon, 13 Oct 2014 23:38:31 +0200 (CEST)\r
  28 Received: from imap.feld.cvut.cz (imap.feld.cvut.cz [147.32.192.34])\r
  29         by max.feld.cvut.cz (Postfix) with ESMTP id 2F68B5CCEAE;\r
  30         Mon, 13 Oct 2014 23:38:31 +0200 (CEST)\r
  31 Received: from wsh by steelpick.2x.cz with local (Exim 4.84)\r
  32         (envelope-from <sojkam1@fel.cvut.cz>)\r
  33         id 1XdnK2-0005jf-7H; Mon, 13 Oct 2014 23:38:30 +0200\r
  34 From: Michal Sojka <sojkam1@fel.cvut.cz>\r
  35 To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org\r
  36 Subject: Re: [PATCH v3 3/4] cli: Extend the search command for\r
  37         --output={sender, recipients}\r
  38 In-Reply-To: <m238arrdzp.fsf@guru.guru-group.fi>\r
  39 References: <87zjd51phx.fsf@steelpick.2x.cz>\r
  40         <1413150093-8383-1-git-send-email-sojkam1@fel.cvut.cz>\r
  41         <1413150093-8383-4-git-send-email-sojkam1@fel.cvut.cz>\r
  42         <m238arrdzp.fsf@guru.guru-group.fi>\r
  43 User-Agent: Notmuch/0.18.1+101~g56b0ff0 (http://notmuchmail.org) Emacs/24.3.1\r
  44         (x86_64-pc-linux-gnu)\r
  45 Date: Mon, 13 Oct 2014 23:38:30 +0200\r
  46 Message-ID: <871tqbejl5.fsf@steelpick.2x.cz>\r
  47 MIME-Version: 1.0\r
  48 Content-Type: text/plain\r
  49 X-BeenThere: notmuch@notmuchmail.org\r
  50 X-Mailman-Version: 2.1.13\r
  51 Precedence: list\r
  52 List-Id: "Use and development of the notmuch mail system."\r
  53         <notmuch.notmuchmail.org>\r
  54 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  55         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  56 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  57 List-Post: <mailto:notmuch@notmuchmail.org>\r
  58 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  59 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  60         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  61 X-List-Received-Date: Mon, 13 Oct 2014 21:38:44 -0000\r
  62 \r
  63 On Mon, Oct 13 2014, Tomi Ollila wrote:\r
  64 > On Mon, Oct 13 2014, Michal Sojka <sojkam1@fel.cvut.cz> wrote:\r
  65 >\r
  66 >> The new outputs allow printing senders, recipients or both of matching\r
  67 >> messages. The --output option is converted from "keyword" argument to\r
  68 >> "flags" argument, which means that the user can use --output=sender and\r
  69 >> --output=recipients simultaneously, to print both. Other combinations\r
  70 >> produce an error.\r
  71 >>\r
  72 >> This code based on a patch from Jani Nikula.\r
  73 >> ---\r
  74 >>  completion/notmuch-completion.bash |   2 +-\r
  75 >>  completion/notmuch-completion.zsh  |   3 +-\r
  76 >>  doc/man1/notmuch-search.rst        |  22 +++++++-\r
  77 >>  notmuch-search.c                   | 110 ++++++++++++++++++++++++++++++++++---\r
  78 >>  test/T090-search-output.sh         |  64 +++++++++++++++++++++\r
  79 >>  5 files changed, 189 insertions(+), 12 deletions(-)\r
  80 >>\r
  81 >> diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash\r
  82 >> index 0571dc9..cfbd389 100644\r
  83 >> --- a/completion/notmuch-completion.bash\r
  84 >> +++ b/completion/notmuch-completion.bash\r
  85 >> @@ -294,7 +294,7 @@ _notmuch_search()\r
  86 >>          return\r
  87 >>          ;;\r
  88 >>      --output)\r
  89 >> -        COMPREPLY=( $( compgen -W "summary threads messages files tags" -- "${cur}" ) )\r
  90 >> +        COMPREPLY=( $( compgen -W "summary threads messages files tags sender recipients" -- "${cur}" ) )\r
  91 >>          return\r
  92 >>          ;;\r
  93 >>      --sort)\r
  94 >> diff --git a/completion/notmuch-completion.zsh b/completion/notmuch-completion.zsh\r
  95 >> index 67a9aba..3e52a00 100644\r
  96 >> --- a/completion/notmuch-completion.zsh\r
  97 >> +++ b/completion/notmuch-completion.zsh\r
  98 >> @@ -52,7 +52,8 @@ _notmuch_search()\r
  99 >>    _arguments -s : \\r
 100 >>      '--max-threads=[display only the first x threads from the search results]:number of threads to show: ' \\r
 101 >>      '--first=[omit the first x threads from the search results]:number of threads to omit: ' \\r
 102 >> -    '--sort=[sort results]:sorting:((newest-first\:"reverse chronological order" oldest-first\:"chronological order"))'\r
 103 >> +    '--sort=[sort results]:sorting:((newest-first\:"reverse chronological order" oldest-first\:"chronological order"))' \\r
 104 >> +    '--output=[select what to output]:output:((summary threads messages files tags sender recipients))'\r
 105 >>  }\r
 106 >>\r
 107 >>  _notmuch()\r
 108 >> diff --git a/doc/man1/notmuch-search.rst b/doc/man1/notmuch-search.rst\r
 109 >> index 90160f2..c9d38b1 100644\r
 110 >> --- a/doc/man1/notmuch-search.rst\r
 111 >> +++ b/doc/man1/notmuch-search.rst\r
 112 >> @@ -35,7 +35,7 @@ Supported options for **search** include\r
 113 >>          intended for programs that invoke **notmuch(1)** internally. If\r
 114 >>          omitted, the latest supported version will be used.\r
 115 >>\r
 116 >> -    ``--output=(summary|threads|messages|files|tags)``\r
 117 >> +    ``--output=(summary|threads|messages|files|tags|sender|recipients)``\r
 118 >>\r
 119 >>          **summary**\r
 120 >>              Output a summary of each thread with any message matching\r
 121 >> @@ -78,6 +78,26 @@ Supported options for **search** include\r
 122 >>              by null characters (--format=text0), as a JSON array\r
 123 >>              (--format=json), or as an S-Expression list (--format=sexp).\r
 124 >>\r
 125 >> +    **sender**\r
 126 >> +            Output all addresses from the *From* header that appear on\r
 127 >> +            any message matching the search terms, either one per line\r
 128 >> +            (--format=text), separated by null characters\r
 129 >> +            (--format=text0), as a JSON array (--format=json), or as\r
 130 >> +            an S-Expression list (--format=sexp).\r
 131 >> +\r
 132 >> +        Note: Searching for **sender** should be much faster than\r
 133 >> +        searching for **recipients**, because sender addresses are\r
 134 >> +        cached directly in the database whereas other addresses\r
 135 >> +        need to be fetched from message files.\r
 136 >> +\r
 137 >> +    **recipients**\r
 138 >> +            Like **sender** but for addresses from *To*, *Cc* and\r
 139 >> +        *Bcc* headers.\r
 140 >> +\r
 141 >> +    This option can be given multiple times to combine different\r
 142 >> +    outputs. Curently, this is only supported for **sender** and\r
 143 >> +    **recipients** outputs.\r
 144 >> +\r
 145 >>      ``--sort=``\ (**newest-first**\ \|\ **oldest-first**)\r
 146 >>          This option can be used to present results in either\r
 147 >>          chronological order (**oldest-first**) or reverse chronological\r
 148 >> diff --git a/notmuch-search.c b/notmuch-search.c\r
 149 >> index 5ac2a26..74588f8 100644\r
 150 >> --- a/notmuch-search.c\r
 151 >> +++ b/notmuch-search.c\r
 152 >> @@ -23,11 +23,14 @@\r
 153 >>  #include "string-util.h"\r
 154 >>\r
 155 >>  typedef enum {\r
 156 >> -    OUTPUT_SUMMARY,\r
 157 >> -    OUTPUT_THREADS,\r
 158 >> -    OUTPUT_MESSAGES,\r
 159 >> -    OUTPUT_FILES,\r
 160 >> -    OUTPUT_TAGS\r
 161 >> +    OUTPUT_SUMMARY  = 1 << 0,\r
 162 >> +    OUTPUT_THREADS  = 1 << 1,\r
 163 >> +    OUTPUT_MESSAGES = 1 << 2,\r
 164 >> +    OUTPUT_FILES    = 1 << 3,\r
 165 >> +    OUTPUT_TAGS             = 1 << 4,\r
 166 >> +    OUTPUT_SENDER   = 1 << 5,\r
 167 >> +    OUTPUT_RECIPIENTS       = 1 << 6,\r
 168 >> +    OUTPUT_ADDRESSES        = OUTPUT_SENDER | OUTPUT_RECIPIENTS,\r
 169 >\r
 170 > leftover, like mentioned below (this comment added just before\r
 171 > sending)\r
 172 \r
 173 This is needed to suppress a warning about unknown enum value in the\r
 174 switch statement.\r
 175 \r
 176 >>  } output_t;\r
 177 >>\r
 178 >>  typedef struct {\r
 179 >> @@ -220,6 +223,67 @@ do_search_threads (search_options_t *o)\r
 180 >>      return 0;\r
 181 >>  }\r
 182 >>\r
 183 >> +static void\r
 184 >> +print_address_list (const search_options_t *o, InternetAddressList *list)\r
 185 >> +{\r
 186 >> +    InternetAddress *address;\r
 187 >> +    int i;\r
 188 >> +\r
 189 >> +    for (i = 0; i < internet_address_list_length (list); i++) {\r
 190 >> +    address = internet_address_list_get_address (list, i);\r
 191 >> +    if (INTERNET_ADDRESS_IS_GROUP (address)) {\r
 192 >> +        InternetAddressGroup *group;\r
 193 >> +        InternetAddressList *group_list;\r
 194 >> +\r
 195 >> +        group = INTERNET_ADDRESS_GROUP (address);\r
 196 >> +        group_list = internet_address_group_get_members (group);\r
 197 >> +        if (group_list == NULL)\r
 198 >> +            continue;\r
 199 >> +\r
 200 >> +        print_address_list (o, group_list);\r
 201 >> +    } else {\r
 202 >> +        InternetAddressMailbox *mailbox;\r
 203 >> +        const char *name;\r
 204 >> +        const char *addr;\r
 205 >> +        char *full_address;\r
 206 >> +\r
 207 >> +        mailbox = INTERNET_ADDRESS_MAILBOX (address);\r
 208 >> +\r
 209 >> +        name = internet_address_get_name (address);\r
 210 >> +        addr = internet_address_mailbox_get_addr (mailbox);\r
 211 >> +\r
 212 >> +        if (name && *name)\r
 213 >> +            full_address = talloc_asprintf (o->format, "%s <%s>", name, addr);\r
 214 >> +        else\r
 215 >> +            full_address = talloc_strdup (o->format, addr);\r
 216 >> +\r
 217 >> +        if (!full_address) {\r
 218 >\r
 219 > Apart from minor style issue like space after ! and the leftover ADDRESSES\r
 220 > parts (w/o that I would not have commented about !<SPC>) the first 3\r
 221 > patches look pretty good to me. I have not tested those yet.\r
 222 \r
 223 If you want, I can send another version.\r
 224 \r
 225 > But we keep to have some disagreement w/ unique/duplicate/filter-by\r
 226 > handling ;/\r
 227 >\r
 228 > I (currently) rest the case of first/last/most common handling to just\r
 229 > how the --sort=(newest-first|oldest-first) affects the order...\r
 230 >\r
 231 > Let's consider the following list of output if no /deduplication/ is done:\r
 232 >\r
 233 >   John Doe <john@example.com>\r
 234 >   Dr. John Doe <john@example.com>\r
 235 >   John Doe <JOHN@EXAMPLE.COM>\r
 236 >   John Doe <john@doe.name>\r
 237 >   Dr. John Doe <john@doe.name>\r
 238 >   John Doe <JOHN@doe.name.example.com>\r
 239 >   John Doe <john@doe.name>\r
 240 >   Dr. John Doe <john@example.com>\r
 241 >   Dr. John Doe <john@example.com>\r
 242 >   Dr. John Doe <john@doe.name>\r
 243 >   John Doe <john@example.com>\r
 244 >   John Doe <john@doe.name.example.com>\r
 245 >\r
 246 > To stir the pool a little more, this could be the output when\r
 247 > --duplicate=all (the default) is given.\r
 248 \r
 249 This seems counter-intuitive. A command without --duplicate gives you a\r
 250 certain number of addresses and when you add --duplicate, you get less\r
 251 addresses?\r
 252 \r
 253 Moreover, --duplicate is already used with --output=files and has\r
 254 different meaning.\r
 255 \r
 256 > With --duplicate=none the output could be (first match by unique\r
 257 > case-insensitive address):\r
 258 >\r
 259 >   John Doe <john@example.com>\r
 260 >   John Doe <john@doe.name>\r
 261 >   John Doe <john@doe.name.example.com>\r
 262 \r
 263 This seems confusing to me. If I see "duplicate none", I would simply\r
 264 expect that duplicate lines are removed from the --duplicate=all list\r
 265 above. Not that just duplicate addresses would be removed.\r
 266 \r
 267 > (many people may have the same name, but email address is unique per person\r
 268 > -- therefore I think 'none' limiting that just to John Doe <john@example.com>\r
 269 > would be too little)\r
 270 \r
 271 Here I agree that filtering by address part is perhaps most useful.\r
 272 \r
 273 > and with --duplicate=address\r
 274 >\r
 275 >   John Doe <john@example.com>\r
 276 >   Dr. John Doe <john@example.com>\r
 277 >   John Doe <john@doe.name>\r
 278 >   Dr. John Doe <john@doe.name>\r
 279 >   John Doe <JOHN@doe.name.example.com>\r
 280 \r
 281 I don't get this. You say --duplicate=address but the output contains\r
 282 also duplicate names.\r
 283 \r
 284 > (from this output user can choose how the recipient is to be called\r
 285 > (like "pseudonyms" mentioned in id:20141010113202.GE28601@TP_L520.localdomain )\r
 286 > when sending email)\r
 287 >\r
 288 > and --duplicate=address-casesensitive\r
 289 >\r
 290 >   John Doe <john@example.com>\r
 291 >   Dr. John Doe <john@example.com>\r
 292 >   John Doe <JOHN@EXAMPLE.COM>\r
 293 >   John Doe <john@doe.name>\r
 294 >   Dr. John Doe <john@doe.name>\r
 295 >   John Doe <JOHN@doe.name.example.com>\r
 296 >   John Doe <john@doe.name.example.com>\r
 297 >\r
 298 > This reuse of --duplicate was thought out after Jani's irc mention of it.\r
 299 > This scheme would leave no room tho the filter-by=name suggestion -- for\r
 300 > completeness that would make this look:\r
 301 >\r
 302 >   John Doe <john@example.com>\r
 303 >   Dr. John Doe <john@example.com>\r
 304 >\r
 305 > This doesn't look too bad in this particular case but not having ability to\r
 306 > see all potential addresses (perhaps the only working address is now\r
 307 > hidden) isn't not much for general use. Well, maybe for some specific use\r
 308 > --duplicate=no-unique-addresses could be useful :O\r
 309 >\r
 310 > Ok, this took an hour to get written to (w/ some interruptions). Healthy\r
 311 > criticism appreciated :D\r
 312 \r
 313 I don't know. You seem to think about this in the opposite way than how\r
 314 it is implemented. The implementation really filters things out whereas\r
 315 you specify what not to filter.\r
 316 \r
 317 My feeling is that if you would implement your proposal, the code would\r
 318 be more complex than in my patch, because the mapping between command\r
 319 line options and the actual algorithm would require some extra code. And\r
 320 in a previous comment, you preferred simplicity.\r
 321 \r
 322 Hopefully, you consider the above as "healthy" criticism.\r
 323 \r
 324 Thanks for quick review.\r
 325 -Michal\r