d0/d18a92cbc523b56a29cc23c817aa29e5d5a544

   1 Return-Path: <dmitry.kurochkin@gmail.com>\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 72C84429E25\r
   6         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 08:42:33 -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.799\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] 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 YSBl6dNifbZ8 for <notmuch@notmuchmail.org>;\r
  17         Sun, 11 Dec 2011 08:42:32 -0800 (PST)\r
  18 Received: from mail-bw0-f53.google.com (mail-bw0-f53.google.com\r
  19         [209.85.214.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  20         (No client certificate requested)\r
  21         by olra.theworths.org (Postfix) with ESMTPS id 47642431FB6\r
  22         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 08:42:32 -0800 (PST)\r
  23 Received: by bkat8 with SMTP id t8so4903768bka.26\r
  24         for <notmuch@notmuchmail.org>; Sun, 11 Dec 2011 08:42:31 -0800 (PST)\r
  25 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
  26         h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
  27         :message-id:mime-version:content-type;\r
  28         bh=p1GgUP57VUKGtHWcK/z6atIyTDQcUKGFnpw5ek4STG0=;\r
  29         b=CFJTfJnBeMSrHp8qwfDNFOGCNFVAM6JUIkauzg9bfzAli6S+zVXlRZTuxwCWecZrrI\r
  30         lbYfw8KDsJZrjX6Ek4zWw4KT6KT864/FuqfjKiuknihybo1NFu5E5E+WbkIeIdFG7YSq\r
  31         fBEGEzpnfRLyTcradsAzsgBrnpWjXE3OL0MWg=\r
  32 Received: by 10.204.155.65 with SMTP id r1mr7893415bkw.110.1323621749518;\r
  33         Sun, 11 Dec 2011 08:42:29 -0800 (PST)\r
  34 Received: from localhost ([91.144.186.21])\r
  35         by mx.google.com with ESMTPS id l5sm24929718bkv.9.2011.12.11.08.42.27\r
  36         (version=TLSv1/SSLv3 cipher=OTHER);\r
  37         Sun, 11 Dec 2011 08:42:28 -0800 (PST)\r
  38 From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
  39 To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org\r
  40 Subject: Re: [PATCH] cli: factor out config handling code to get/set lists.\r
  41 In-Reply-To: <1323619671-14111-1-git-send-email-david@tethera.net>\r
  42 References: <87sjkstbiu.fsf@zancas.localnet>\r
  43         <1323619671-14111-1-git-send-email-david@tethera.net>\r
  44 User-Agent: Notmuch/0.10.2+82~g96a629c (http://notmuchmail.org) Emacs/23.3.1\r
  45         (x86_64-pc-linux-gnu)\r
  46 Date: Sun, 11 Dec 2011 20:41:53 +0400\r
  47 Message-ID: <874nx76oxa.fsf@gmail.com>\r
  48 MIME-Version: 1.0\r
  49 Content-Type: text/plain; charset=us-ascii\r
  50 Cc: David Bremner <bremner@debian.org>\r
  51 X-BeenThere: notmuch@notmuchmail.org\r
  52 X-Mailman-Version: 2.1.13\r
  53 Precedence: list\r
  54 List-Id: "Use and development of the notmuch mail system."\r
  55         <notmuch.notmuchmail.org>\r
  56 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  57         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  58 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  59 List-Post: <mailto:notmuch@notmuchmail.org>\r
  60 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  61 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  62         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  63 X-List-Received-Date: Sun, 11 Dec 2011 16:42:33 -0000\r
  64 \r
  65 Hi David.\r
  66 \r
  67 On Sun, 11 Dec 2011 12:07:51 -0400, David Bremner <david@tethera.net> wrote:\r
  68 > From: David Bremner <bremner@debian.org>\r
  69 > \r
  70 > Two new internal routines are created _config_get_list and\r
  71 > _config_set_list; the notmuch_config_get_* functions that deal with\r
  72 > lists are simply wrappers for these functions.\r
  73 \r
  74 Looks good to me.  Some comments below.\r
  75 \r
  76 > ---\r
  77 >  notmuch-config.c |  130 +++++++++++++++++++++++++++--------------------------\r
  78 >  1 files changed, 66 insertions(+), 64 deletions(-)\r
  79 > \r
  80 > diff --git a/notmuch-config.c b/notmuch-config.c\r
  81 > index 1a7ed58..e98b6a3 100644\r
  82 > --- a/notmuch-config.c\r
  83 > +++ b/notmuch-config.c\r
  84 > @@ -520,92 +520,94 @@ notmuch_config_set_user_primary_email (notmuch_config_t *config,\r
  85 >      config->user_primary_email = NULL;\r
  86 >  }\r
  87 >  \r
  88 > -const char **\r
  89 > -notmuch_config_get_user_other_email (notmuch_config_t *config,\r
  90 > -                                  size_t *length)\r
  91 > +static const char **\r
  92 > +_config_get_list (notmuch_config_t *config,\r
  93 > +               const char *section, const char *key,\r
  94 > +               const char ***outlist, size_t *list_length, size_t *ret_length)\r
  95 >  {\r
  96 \r
  97 It seems weird that the function both takes list as an output parameter\r
  98 and returns it.  Having two length parameters which are set to the same\r
  99 value is confusing as well.  I understand that it is done to make getter\r
 100 functions smaller.  So perhaps it is ok.\r
 101 \r
 102 > -    char **emails;\r
 103 > -    size_t emails_length;\r
 104 > +    char **inlist;\r
 105 >      unsigned int i;\r
 106 >  \r
 107 \r
 108 Please move variable declarations inside the if (!*outlist) and if\r
 109 (inlist) blocks.  (I saw other code in notmuch that does it, so it must\r
 110 be ok.)\r
 111 \r
 112 > -    if (config->user_other_email == NULL) {\r
 113 > -     emails = g_key_file_get_string_list (config->key_file,\r
 114 > -                                          "user", "other_email",\r
 115 > -                                          &emails_length, NULL);\r
 116 > -     if (emails) {\r
 117 > -         config->user_other_email = talloc_size (config,\r
 118 > -                                                 sizeof (char *) *\r
 119 > -                                                 (emails_length + 1));\r
 120 > -         for (i = 0; i < emails_length; i++)\r
 121 > -             config->user_other_email[i] = talloc_strdup (config->user_other_email,\r
 122 > -                                                          emails[i]);\r
 123 > -         config->user_other_email[i] = NULL;\r
 124 > -\r
 125 > -         g_strfreev (emails);\r
 126 > -\r
 127 > -         config->user_other_email_length = emails_length;\r
 128 > +    if (*outlist == NULL) {\r
 129 > +     inlist = g_key_file_get_string_list (config->key_file,\r
 130 > +                                          section, key,\r
 131 > +                                          list_length, NULL);\r
 132 > +     if (inlist) {\r
 133 > +         *outlist = talloc_size (config, sizeof (char *) *\r
 134 > +                                 (*list_length + 1));\r
 135 > +         for (i = 0; i < *list_length; i++)\r
 136 > +             (*outlist)[i] = talloc_strdup (*outlist, inlist[i]);\r
 137 > +         (*outlist)[i] = NULL;\r
 138 > +\r
 139 > +         g_strfreev (inlist);\r
 140 >       }\r
 141 >      }\r
 142 >  \r
 143 > -    *length = config->user_other_email_length;\r
 144 > -    return config->user_other_email;\r
 145 > +    if (ret_length) *ret_length = *list_length;\r
 146 \r
 147 I would prefer to have this on separate lines.\r
 148 \r
 149 > +    return *outlist;\r
 150 >  }\r
 151 >  \r
 152 > -void\r
 153 > -notmuch_config_set_user_other_email (notmuch_config_t *config,\r
 154 > -                                  const char *other_email[],\r
 155 > -                                  size_t length)\r
 156 > +const char **\r
 157 > +notmuch_config_get_user_other_email (notmuch_config_t *config,   size_t *length)\r
 158 >  {\r
 159 > -    g_key_file_set_string_list (config->key_file,\r
 160 > -                             "user", "other_email",\r
 161 > -                             other_email, length);\r
 162 > +    return _config_get_list (config, "user", "other_email",\r
 163 > +                          &(config->user_other_email),\r
 164 > +                          &(config->user_other_email_length), length);\r
 165 > +}\r
 166 >  \r
 167 > -    talloc_free (config->user_other_email);\r
 168 > -    config->user_other_email = NULL;\r
 169 > +const char **\r
 170 > +notmuch_config_get_new_tags (notmuch_config_t *config,   size_t *length)\r
 171 > +{\r
 172 > +    return _config_get_list (config, "new", "tags",\r
 173 > +                          &(config->new_tags),\r
 174 > +                          &(config->new_tags_length), length);\r
 175 >  }\r
 176 >  \r
 177 >  const char **\r
 178 > -notmuch_config_get_new_tags (notmuch_config_t *config,\r
 179 > -                          size_t *length)\r
 180 > +notmuch_config_get_log_subscribers (notmuch_config_t *config,   size_t *length)\r
 181 >  {\r
 182 > -    char **tags;\r
 183 > -    size_t tags_length;\r
 184 > -    unsigned int i;\r
 185 > +    return _config_get_list (config, "log", "subscribers",\r
 186 > +                          &(config->new_tags),\r
 187 > +                          &(config->new_tags_length), length);\r
 188 > +}\r
 189 >  \r
 190 \r
 191 This should be part of a separate patch which adds the logging feature,\r
 192 I guess.\r
 193 \r
 194 Regards,\r
 195   Dmitry\r
 196 \r
 197 > -    if (config->new_tags == NULL) {\r
 198 > -     tags = g_key_file_get_string_list (config->key_file,\r
 199 > -                                        "new", "tags",\r
 200 > -                                        &tags_length, NULL);\r
 201 > -     if (tags) {\r
 202 > -         config->new_tags = talloc_size (config,\r
 203 > -                                         sizeof (char *) *\r
 204 > -                                         (tags_length + 1));\r
 205 > -         for (i = 0; i < tags_length; i++)\r
 206 > -             config->new_tags[i] = talloc_strdup (config->new_tags,\r
 207 > -                                                  tags[i]);\r
 208 > -         config->new_tags[i] = NULL;\r
 209 > -\r
 210 > -         g_strfreev (tags);\r
 211 > -\r
 212 > -         config->new_tags_length = tags_length;\r
 213 > -     }\r
 214 > -    }\r
 215 >  \r
 216 > -    *length = config->new_tags_length;\r
 217 > -    return config->new_tags;\r
 218 > +static void\r
 219 > +_config_set_list (notmuch_config_t *config,\r
 220 > +               const char *group, const char *name,\r
 221 > +               const char *list[],\r
 222 > +               size_t length, const char ***config_var )\r
 223 > +{\r
 224 > +    g_key_file_set_string_list (config->key_file, group, name, list, length);\r
 225 > +    talloc_free (*config_var);\r
 226 > +    *config_var = NULL;\r
 227 > +}\r
 228 > +\r
 229 > +void\r
 230 > +notmuch_config_set_user_other_email (notmuch_config_t *config,\r
 231 > +                                  const char *list[],\r
 232 > +                                  size_t length)\r
 233 > +{\r
 234 > +    _config_set_list (config, "user", "other_email", list, length,\r
 235 > +                  &(config->user_other_email));\r
 236 >  }\r
 237 >  \r
 238 >  void\r
 239 >  notmuch_config_set_new_tags (notmuch_config_t *config,\r
 240 > -                          const char *new_tags[],\r
 241 > -                          size_t length)\r
 242 > +                                  const char *list[],\r
 243 > +                                  size_t length)\r
 244 >  {\r
 245 > -    g_key_file_set_string_list (config->key_file,\r
 246 > -                             "new", "tags",\r
 247 > -                             new_tags, length);\r
 248 > +    _config_set_list (config, "new", "tags", list, length,\r
 249 > +                  &(config->new_tags));\r
 250 > +}\r
 251 >  \r
 252 > -    talloc_free (config->new_tags);\r
 253 > -    config->new_tags = NULL;\r
 254 > +void\r
 255 > +notmuch_config_set_log_subscribers (notmuch_config_t *config,\r
 256 > +                                 const char *list[],\r
 257 > +                                 size_t length)\r
 258 > +{\r
 259 > +    _config_set_list (config, "log", "subscribers", list, length,\r
 260 > +                  &(config->log_subscribers));\r
 261 >  }\r
 262 >  \r
 263 >  /* Given a configuration item of the form <group>.<key> return the\r
 264 > -- \r
 265 > 1.7.7.3\r
 266 > \r
 267 > _______________________________________________\r
 268 > notmuch mailing list\r
 269 > notmuch@notmuchmail.org\r
 270 > http://notmuchmail.org/mailman/listinfo/notmuch\r