20/16cd86500f09f73ce7f5aebaf0701eebc2c7f5

   1 Return-Path: <m.walters@qmul.ac.uk>\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 D5A56431FAF\r
   6         for <notmuch@notmuchmail.org>; Sat,  1 Dec 2012 06:26:54 -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: -1.098\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled\r
  14 Received: from olra.theworths.org ([127.0.0.1])\r
  15         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  16         with ESMTP id kPv0EXB5WJNN for <notmuch@notmuchmail.org>;\r
  17         Sat,  1 Dec 2012 06:26:51 -0800 (PST)\r
  18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  19         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 87E30431FBC\r
  22         for <notmuch@notmuchmail.org>; Sat,  1 Dec 2012 06:26:50 -0800 (PST)\r
  23 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  24         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  25         (envelope-from <m.walters@qmul.ac.uk>)\r
  26         id 1Teo1m-0005fa-Lc; Sat, 01 Dec 2012 14:26:47 +0000\r
  27 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
  28         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  29         (envelope-from <m.walters@qmul.ac.uk>)\r
  30         id 1Teo1m-0002dU-3I; Sat, 01 Dec 2012 14:26:46 +0000\r
  31 From: Mark Walters <markwalters1009@gmail.com>\r
  32 To: Tomi Ollila <tomi.ollila@iki.fi>, Peter Feigl <craven@gmx.net>,\r
  33         notmuch@notmuchmail.org\r
  34 Subject: Re: [PATCH 1/3] Adding an S-expression structured output printer.\r
  35 In-Reply-To: <874nk5ansj.fsf@qmul.ac.uk>\r
  36 References: <1354264143-30173-1-git-send-email-craven@gmx.net>\r
  37         <871ufa9jpm.fsf@qmul.ac.uk> <m2txs6vwdy.fsf@guru.guru-group.fi>\r
  38         <87d2yu559y.fsf@qmul.ac.uk> <m2fw3pq4sf.fsf@guru.guru-group.fi>\r
  39         <874nk5ansj.fsf@qmul.ac.uk>\r
  40 User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1\r
  41         (x86_64-pc-linux-gnu)\r
  42 Date: Sat, 01 Dec 2012 14:26:46 +0000\r
  43 Message-ID: <87k3t1etll.fsf@qmul.ac.uk>\r
  44 MIME-Version: 1.0\r
  45 Content-Type: text/plain; charset=us-ascii\r
  46 X-Sender-Host-Address: 93.97.24.31\r
  47 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  48 X-QM-Body-MD5: 6402eb8e03afa4bd0b3f006e5504ea54 (of first 20000 bytes)\r
  49 X-SpamAssassin-Score: -1.7\r
  50 X-SpamAssassin-SpamBar: -\r
  51 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  52         determine if it is\r
  53         spam. We require at least 5.0 points to mark a message as spam.\r
  54         This message scored -1.7 points.\r
  55         Summary of the scoring: \r
  56         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  57         *      medium trust\r
  58         *      [138.37.6.40 listed in list.dnswl.org]\r
  59         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  60         provider *      (markwalters1009[at]gmail.com)\r
  61         *  0.6 AWL AWL: From: address is in the auto white-list\r
  62 X-QM-Scan-Virus: ClamAV says the message is clean\r
  63 X-BeenThere: notmuch@notmuchmail.org\r
  64 X-Mailman-Version: 2.1.13\r
  65 Precedence: list\r
  66 List-Id: "Use and development of the notmuch mail system."\r
  67         <notmuch.notmuchmail.org>\r
  68 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  69         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  70 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  71 List-Post: <mailto:notmuch@notmuchmail.org>\r
  72 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  73 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  74         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  75 X-List-Received-Date: Sat, 01 Dec 2012 14:26:55 -0000\r
  76 \r
  77 On Sat, 01 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  78 > On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  79 >> On Sat, Dec 01 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  80 >>\r
  81 >>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  82 >>>> On Sat, Dec 01 2012, Mark Walters wrote:\r
  83 >>>>\r
  84 >>>>> Hi\r
  85 >>>>>\r
  86 >>>>> Overall I like the series: I think I agree with all of Jani's\r
  87 >>>>> comments. \r
  88 >>>>>\r
  89 >>>>> My one extra comment is that I think we should decide on whether we also\r
  90 >>>>> want a sexp plist version. I think we might want one for the emacs\r
  91 >>>>> front-end as that currently uses plists for everything.\r
  92 >>>>>\r
  93 >>>>> If we do we might want to change the names a little, both for functions\r
  94 >>>>> and options (eg sexp_a and sexp_p or something). Probably a lot of\r
  95 >>>>> sprinter-sexp would be common to both versions.\r
  96 >>>>\r
  97 >>>> This is an important question that needs to be addressed fast: options\r
  98 >>>> are:\r
  99 >>>>\r
 100 >>>> 1) have options to spit both alist & plist formats\r
 101 >>>> 2) when converting emacs to use s-expressions, convert it to use alists\r
 102 >>>> 3) start using plists instead of alists in Peter's android client\r
 103 >>>\r
 104 >>> Ok I have looked at this and the changes needed to output plist (or\r
 105 >>> both) are pretty small: the only functions from sprinter-sexp.c that\r
 106 >>> need to be changed are sexp_end and sexp_map_key. The total diff from\r
 107 >>> alist to plist is about 10 lines. I have a version which allows both\r
 108 >>> (the same sprinter file creates both possibilities) and have hooked it\r
 109 >>> into emacs/notmuch-show.el and it all seems to work. \r
 110 >>>\r
 111 >>> (Search is more difficult as that uses the async parser; indeed even for\r
 112 >>> show I used sexp-at-point as suggested by Tomi which seems rather\r
 113 >>> underdocumented but does seem to work)\r
 114 >>>\r
 115 >>> Given the ease with which we can allow both I think that would be my\r
 116 >>> preference: the biggest problem is that slightly more cluttered option\r
 117 >>> list (i.e., we have to allow both --format=sexpa and --format=sexpp or\r
 118 >>> similar).\r
 119 >>>\r
 120 >>> (I can post the patch doing the above but almost all of it is modifying the\r
 121 >>> commands to choose alist or plist rather than modifying the\r
 122 >>> sprinter-sexp itself)\r
 123 >>\r
 124 >> As the diff is so small I agree that supporting 2 formats is good\r
 125 >> option.\r
 126 >>\r
 127 >> In case this is done I suggest that we proceed the following way:\r
 128 >>\r
 129 >> 1) Agree how we call these formats (sexpa & sexpp or something else)\r
 130 >> 2) Peter does his updates, including to call the format as will be decided\r
 131 >> 3) Mark posts his patches after Peter's work is pushed\r
 132 >\r
 133 > I think this seems a good approach. I should note that while the diff\r
 134 > between alist and plist is only about 10 lines I do need to duplicate\r
 135 > some of the surrounding code to allow both: its not huge but definitely\r
 136 > more than 10 lines. \r
 137 >\r
 138 > I will clean it up a little and then post (as a diff on top of Peter's\r
 139 > patches) just so people can judge if it looks basically acceptable.\r
 140 \r
 141 Here is my current diff: note this is just the sprinter diff: it does\r
 142 not contain the plumbing into the cli functions. The largest part is the\r
 143 duplicated create function: I couldn't persuade the compiler to let me\r
 144 unify this (as the static const thing needs to be constant)\r
 145 \r
 146 The patch is -U6 as this makes it easier to see what is going on.\r
 147 \r
 148 Best wishes\r
 149 \r
 150 Mark\r
 151 \r
 152 ---\r
 153  sprinter-sexp.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++-----\r
 154  sprinter.h      |    6 +++-\r
 155  2 files changed, 73 insertions(+), 8 deletions(-)\r
 156 \r
 157 diff --git a/sprinter-sexp.c b/sprinter-sexp.c\r
 158 index 8401c52..51bab68 100644\r
 159 --- a/sprinter-sexp.c\r
 160 +++ b/sprinter-sexp.c\r
 161 @@ -1,11 +1,16 @@\r
 162  #include <stdbool.h>\r
 163  #include <stdio.h>\r
 164  #include <talloc.h>\r
 165  #include "sprinter.h"\r
 166  \r
 167 +typedef enum {\r
 168 +    SEXP_ALIST,\r
 169 +    SEXP_PLIST\r
 170 +} sexp_list_t;\r
 171 +\r
 172  struct sprinter_sexp {\r
 173      struct sprinter vtable;\r
 174      FILE *stream;\r
 175      /* Top of the state stack, or NULL if the printer is not currently\r
 176       * inside any aggregate types. */\r
 177      struct sexp_state *state;\r
 178 @@ -81,26 +86,39 @@ static void\r
 179  sexp_begin_list (struct sprinter *sp)\r
 180  {\r
 181      sexp_begin_aggregate (sp, '(', ')');\r
 182  }\r
 183  \r
 184  static void\r
 185 -sexp_end (struct sprinter *sp)\r
 186 +sexp_end (struct sprinter *sp, sexp_list_t list_type)\r
 187  {\r
 188      struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 189      struct sexp_state *state = sps->state;\r
 190  \r
 191 -    if (sps->state->in_map)\r
 192 -        fputc (')', sps->stream);\r
 193 +    if (list_type == SEXP_ALIST)\r
 194 +       if (sps->state->in_map)\r
 195 +           fputc (' ', sps->stream);\r
 196      fputc (sps->state->close, sps->stream);\r
 197      sps->state = state->parent;\r
 198      talloc_free (state);\r
 199      if (sps->state == NULL)\r
 200          fputc ('\n', sps->stream);\r
 201  }\r
 202  \r
 203 +static void\r
 204 +sexp_end_alist (struct sprinter *sp)\r
 205 +{\r
 206 +    sexp_end(sp, SEXP_ALIST);\r
 207 +}\r
 208 +\r
 209 +static void\r
 210 +sexp_end_plist (struct sprinter *sp)\r
 211 +{\r
 212 +    sexp_end(sp, SEXP_PLIST);\r
 213 +}\r
 214 +\r
 215  /* This implementation supports embedded NULs as allowed by the JSON\r
 216   * specification and Unicode.  Support for *parsing* embedded NULs\r
 217   * varies, but is generally not a problem outside of C-based parsers\r
 218   * (Python's json module and Emacs' json.el take embedded NULs in\r
 219   * stride). */\r
 220  static void\r
 221 @@ -177,24 +195,36 @@ sexp_null (struct sprinter *sp)\r
 222      struct sprinter_sexp *sps = sexp_begin_value (sp);\r
 223  \r
 224      fputs ("nil", sps->stream);\r
 225  }\r
 226  \r
 227  static void\r
 228 -sexp_map_key (struct sprinter *sp, const char *key)\r
 229 +sexp_map_key_alist (struct sprinter *sp, const char *key)\r
 230  {\r
 231      struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 232  \r
 233      if( sps->state->in_map && ! sps->state->first)\r
 234          fputs (") ", sps->stream);\r
 235      fputc ('(', sps->stream);\r
 236      sexp_symbol (sp, key);\r
 237      fputc (' ', sps->stream);\r
 238  }\r
 239  \r
 240  static void\r
 241 +sexp_map_key_plist (struct sprinter *sp, const char *key)\r
 242 +{\r
 243 +    struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 244 +\r
 245 +    if( sps->state->in_map && ! sps->state->first)\r
 246 +        fputs (" ", sps->stream);\r
 247 +    fputc (':', sps->stream);\r
 248 +    sexp_symbol (sp, key);\r
 249 +    fputc (' ', sps->stream);\r
 250 +}\r
 251 +\r
 252 +static void\r
 253  sexp_set_prefix (unused (struct sprinter *sp), unused (const char *name))\r
 254  {\r
 255  }\r
 256  \r
 257  static void\r
 258  sexp_separator (struct sprinter *sp)\r
 259 @@ -202,25 +232,56 @@ sexp_separator (struct sprinter *sp)\r
 260      struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 261  \r
 262      sps->insert_separator = TRUE;\r
 263  }\r
 264  \r
 265  struct sprinter *\r
 266 -sprinter_sexp_create (const void *ctx, FILE *stream)\r
 267 +sprinter_sexp_alist_create (const void *ctx, FILE *stream)\r
 268 +{\r
 269 +    static const struct sprinter_sexp template = {\r
 270 +        .vtable = {\r
 271 +            .begin_map = sexp_begin_map,\r
 272 +            .begin_list = sexp_begin_list,\r
 273 +            .end = sexp_end_alist,\r
 274 +            .string = sexp_string,\r
 275 +            .string_len = sexp_string_len,\r
 276 +            .integer = sexp_integer,\r
 277 +            .boolean = sexp_boolean,\r
 278 +            .null = sexp_null,\r
 279 +            .map_key = sexp_map_key_alist,\r
 280 +            .separator = sexp_separator,\r
 281 +            .set_prefix = sexp_set_prefix,\r
 282 +            .is_text_printer = FALSE,\r
 283 +        }\r
 284 +    };\r
 285 +    struct sprinter_sexp *res;\r
 286 +\r
 287 +    res = talloc (ctx, struct sprinter_sexp);\r
 288 +    if (! res)\r
 289 +        return NULL;\r
 290 +\r
 291 +    *res = template;\r
 292 +    res->stream = stream;\r
 293 +    return &res->vtable;\r
 294 +}\r
 295 +\r
 296 +\r
 297 +struct sprinter *\r
 298 +sprinter_sexp_plist_create (const void *ctx, FILE *stream)\r
 299  {\r
 300      static const struct sprinter_sexp template = {\r
 301          .vtable = {\r
 302              .begin_map = sexp_begin_map,\r
 303              .begin_list = sexp_begin_list,\r
 304 -            .end = sexp_end,\r
 305 +            .end = sexp_end_plist,\r
 306              .string = sexp_string,\r
 307              .string_len = sexp_string_len,\r
 308              .integer = sexp_integer,\r
 309              .boolean = sexp_boolean,\r
 310              .null = sexp_null,\r
 311 -            .map_key = sexp_map_key,\r
 312 +            .map_key = sexp_map_key_plist,\r
 313              .separator = sexp_separator,\r
 314              .set_prefix = sexp_set_prefix,\r
 315              .is_text_printer = FALSE,\r
 316          }\r
 317      };\r
 318      struct sprinter_sexp *res;\r
 319 diff --git a/sprinter.h b/sprinter.h\r
 320 index 59776a9..9a496cd 100644\r
 321 --- a/sprinter.h\r
 322 +++ b/sprinter.h\r
 323 @@ -69,9 +69,13 @@ sprinter_text_create (const void *ctx, FILE *stream);\r
 324  /* Create a new structure printer that emits JSON. */\r
 325  struct sprinter *\r
 326  sprinter_json_create (const void *ctx, FILE *stream);\r
 327  \r
 328  /* Create a new structure printer that emits S-Expressions. */\r
 329  struct sprinter *\r
 330 -sprinter_sexp_create (const void *ctx, FILE *stream);\r
 331 +sprinter_sexp_alist_create (const void *ctx, FILE *stream);\r
 332 +\r
 333 +/* Create a new structure printer that emits S-Expressions. */\r
 334 +struct sprinter *\r
 335 +sprinter_sexp_plist_create (const void *ctx, FILE *stream);\r
 336  \r
 337  #endif // NOTMUCH_SPRINTER_H\r
 338 -- \r
 339 1.7.9.1\r
 340 \r