97/2026b9455288635bb72eb84b3996761052596e

   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 33765431FBC\r
   6         for <notmuch@notmuchmail.org>; Sat,  1 Dec 2012 07:46:39 -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 WvdhsGFClLxI for <notmuch@notmuchmail.org>;\r
  17         Sat,  1 Dec 2012 07:46:35 -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 16BB3431FAF\r
  22         for <notmuch@notmuchmail.org>; Sat,  1 Dec 2012 07:46:35 -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 1TepGy-0006u7-Rh; Sat, 01 Dec 2012 15:46:33 +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 1TepGy-0003mv-D9; Sat, 01 Dec 2012 15:46:32 +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: <87k3t1etll.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> <87k3t1etll.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 15:46:32 +0000\r
  43 Message-ID: <87vccl3hd3.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: 9ffeaf1ab7853fca02aec103141db535 (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 15:46:39 -0000\r
  76 \r
  77 On Sat, 01 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  78 > On Sat, 01 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  79 >> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  80 >>> On Sat, Dec 01 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  81 >>>\r
  82 >>>> On Sat, 01 Dec 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:\r
  83 >>>>> On Sat, Dec 01 2012, Mark Walters wrote:\r
  84 >>>>>\r
  85 >>>>>> Hi\r
  86 >>>>>>\r
  87 >>>>>> Overall I like the series: I think I agree with all of Jani's\r
  88 >>>>>> comments. \r
  89 >>>>>>\r
  90 >>>>>> My one extra comment is that I think we should decide on whether we also\r
  91 >>>>>> want a sexp plist version. I think we might want one for the emacs\r
  92 >>>>>> front-end as that currently uses plists for everything.\r
  93 >>>>>>\r
  94 >>>>>> If we do we might want to change the names a little, both for functions\r
  95 >>>>>> and options (eg sexp_a and sexp_p or something). Probably a lot of\r
  96 >>>>>> sprinter-sexp would be common to both versions.\r
  97 >>>>>\r
  98 >>>>> This is an important question that needs to be addressed fast: options\r
  99 >>>>> are:\r
 100 >>>>>\r
 101 >>>>> 1) have options to spit both alist & plist formats\r
 102 >>>>> 2) when converting emacs to use s-expressions, convert it to use alists\r
 103 >>>>> 3) start using plists instead of alists in Peter's android client\r
 104 >>>>\r
 105 >>>> Ok I have looked at this and the changes needed to output plist (or\r
 106 >>>> both) are pretty small: the only functions from sprinter-sexp.c that\r
 107 >>>> need to be changed are sexp_end and sexp_map_key. The total diff from\r
 108 >>>> alist to plist is about 10 lines. I have a version which allows both\r
 109 >>>> (the same sprinter file creates both possibilities) and have hooked it\r
 110 >>>> into emacs/notmuch-show.el and it all seems to work. \r
 111 >>>>\r
 112 >>>> (Search is more difficult as that uses the async parser; indeed even for\r
 113 >>>> show I used sexp-at-point as suggested by Tomi which seems rather\r
 114 >>>> underdocumented but does seem to work)\r
 115 >>>>\r
 116 >>>> Given the ease with which we can allow both I think that would be my\r
 117 >>>> preference: the biggest problem is that slightly more cluttered option\r
 118 >>>> list (i.e., we have to allow both --format=sexpa and --format=sexpp or\r
 119 >>>> similar).\r
 120 >>>>\r
 121 >>>> (I can post the patch doing the above but almost all of it is modifying the\r
 122 >>>> commands to choose alist or plist rather than modifying the\r
 123 >>>> sprinter-sexp itself)\r
 124 >>>\r
 125 >>> As the diff is so small I agree that supporting 2 formats is good\r
 126 >>> option.\r
 127 >>>\r
 128 >>> In case this is done I suggest that we proceed the following way:\r
 129 >>>\r
 130 >>> 1) Agree how we call these formats (sexpa & sexpp or something else)\r
 131 >>> 2) Peter does his updates, including to call the format as will be decided\r
 132 >>> 3) Mark posts his patches after Peter's work is pushed\r
 133 >>\r
 134 >> I think this seems a good approach. I should note that while the diff\r
 135 >> between alist and plist is only about 10 lines I do need to duplicate\r
 136 >> some of the surrounding code to allow both: its not huge but definitely\r
 137 >> more than 10 lines. \r
 138 >>\r
 139 >> I will clean it up a little and then post (as a diff on top of Peter's\r
 140 >> patches) just so people can judge if it looks basically acceptable.\r
 141 >\r
 142 > Here is my current diff: note this is just the sprinter diff: it does\r
 143 > not contain the plumbing into the cli functions. The largest part is the\r
 144 > duplicated create function: I couldn't persuade the compiler to let me\r
 145 > unify this (as the static const thing needs to be constant)\r
 146 >\r
 147 > The patch is -U6 as this makes it easier to see what is going on.\r
 148 \r
 149 At Tomi's suggestion (on irc) here is a much nicer version of the change\r
 150 needed for alist/plist.\r
 151 \r
 152 Best wishes\r
 153 \r
 154 Mark\r
 155 \r
 156 ---\r
 157  sprinter-sexp.c |   41 ++++++++++++++++++++++++++++++++++-------\r
 158  sprinter.h      |    6 +++++-\r
 159  2 files changed, 39 insertions(+), 8 deletions(-)\r
 160 \r
 161 diff --git a/sprinter-sexp.c b/sprinter-sexp.c\r
 162 index 8401c52..90ec9a0 100644\r
 163 --- a/sprinter-sexp.c\r
 164 +++ b/sprinter-sexp.c\r
 165 @@ -3,6 +3,11 @@\r
 166  #include <talloc.h>\r
 167  #include "sprinter.h"\r
 168  \r
 169 +typedef enum {\r
 170 +    SEXP_ALIST,\r
 171 +    SEXP_PLIST\r
 172 +} sexp_list_t;\r
 173 +\r
 174  struct sprinter_sexp {\r
 175      struct sprinter vtable;\r
 176      FILE *stream;\r
 177 @@ -10,6 +15,8 @@ struct sprinter_sexp {\r
 178       * inside any aggregate types. */\r
 179      struct sexp_state *state;\r
 180  \r
 181 +    /* A flag to say if the sprinter print alists or plists. */\r
 182 +    sexp_list_t list_type;\r
 183      /* A flag to signify that a separator should be inserted in the\r
 184       * output as soon as possible.\r
 185       */\r
 186 @@ -89,8 +96,9 @@ sexp_end (struct sprinter *sp)\r
 187      struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 188      struct sexp_state *state = sps->state;\r
 189  \r
 190 -    if (sps->state->in_map)\r
 191 -        fputc (')', sps->stream);\r
 192 +    if (sps->list_type == SEXP_ALIST)\r
 193 +       if (sps->state->in_map)\r
 194 +           fputc (' ', sps->stream);\r
 195      fputc (sps->state->close, sps->stream);\r
 196      sps->state = state->parent;\r
 197      talloc_free (state);\r
 198 @@ -184,9 +192,15 @@ sexp_map_key (struct sprinter *sp, const char *key)\r
 199  {\r
 200      struct sprinter_sexp *sps = (struct sprinter_sexp *) sp;\r
 201  \r
 202 -    if( sps->state->in_map && ! sps->state->first)\r
 203 -        fputs (") ", sps->stream);\r
 204 -    fputc ('(', sps->stream);\r
 205 +    if (sps->list_type == SEXP_ALIST) {\r
 206 +       if( sps->state->in_map && ! sps->state->first)\r
 207 +           fputs (") ", sps->stream);\r
 208 +       fputc ('(', sps->stream);\r
 209 +    } else {\r
 210 +       if( sps->state->in_map && ! sps->state->first)\r
 211 +           fputs (" ", sps->stream);\r
 212 +       fputc (':', sps->stream);\r
 213 +    }\r
 214      sexp_symbol (sp, key);\r
 215      fputc (' ', sps->stream);\r
 216  }\r
 217 @@ -204,8 +218,8 @@ sexp_separator (struct sprinter *sp)\r
 218      sps->insert_separator = TRUE;\r
 219  }\r
 220  \r
 221 -struct sprinter *\r
 222 -sprinter_sexp_create (const void *ctx, FILE *stream)\r
 223 +static struct sprinter *\r
 224 +sprinter_sexp_create (const void *ctx, FILE *stream, sexp_list_t list_type)\r
 225  {\r
 226      static const struct sprinter_sexp template = {\r
 227          .vtable = {\r
 228 @@ -231,5 +245,18 @@ sprinter_sexp_create (const void *ctx, FILE *stream)\r
 229  \r
 230      *res = template;\r
 231      res->stream = stream;\r
 232 +    res->list_type = list_type;\r
 233      return &res->vtable;\r
 234  }\r
 235 +\r
 236 +struct sprinter *\r
 237 +sprinter_sexp_alist_create (const void *ctx, FILE *stream)\r
 238 +{\r
 239 +    return sprinter_sexp_create (ctx, stream, SEXP_ALIST);\r
 240 +}\r
 241 +\r
 242 +struct sprinter *\r
 243 +sprinter_sexp_plist_create (const void *ctx, FILE *stream)\r
 244 +{\r
 245 +    return sprinter_sexp_create (ctx, stream, SEXP_PLIST);\r
 246 +}\r
 247 diff --git a/sprinter.h b/sprinter.h\r
 248 index 59776a9..9a496cd 100644\r
 249 --- a/sprinter.h\r
 250 +++ b/sprinter.h\r
 251 @@ -72,6 +72,10 @@ sprinter_json_create (const void *ctx, FILE *stream);\r
 252  \r
 253  /* Create a new structure printer that emits S-Expressions. */\r
 254  struct sprinter *\r
 255 -sprinter_sexp_create (const void *ctx, FILE *stream);\r
 256 +sprinter_sexp_alist_create (const void *ctx, FILE *stream);\r
 257 +\r
 258 +/* Create a new structure printer that emits S-Expressions. */\r
 259 +struct sprinter *\r
 260 +sprinter_sexp_plist_create (const void *ctx, FILE *stream);\r
 261  \r
 262  #endif // NOTMUCH_SPRINTER_H\r
 263 -- \r
 264 1.7.9.1\r
 265 \r
 266  \r
 267 \r
 268 \r
 269 \r