3a/87df963a0f44c1b29aeb8eff101e56e7d0e92a

   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 A942F431FBD\r
   6         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 11:56:45 -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: -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 2m7YkggBlSyj for <notmuch@notmuchmail.org>;\r
  16         Wed, 17 Oct 2012 11:56:44 -0700 (PDT)\r
  17 Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com\r
  18         [209.85.217.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 6F3B3431FB6\r
  21         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 11:56:44 -0700 (PDT)\r
  22 Received: by mail-lb0-f181.google.com with SMTP id gg6so5832955lbb.26\r
  23         for <notmuch@notmuchmail.org>; Wed, 17 Oct 2012 11:56:41 -0700 (PDT)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=google.com; s=20120113;\r
  26         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  27         :mime-version:content-type:x-gm-message-state;\r
  28         bh=MutdBUVJQL7LcUYGv0M6vPG3sqjeS7+gJhRauSGOW7I=;\r
  29         b=HcdpiNLfAOgVwqEN4LE9VXJAj3pEMHav/RtjWNVbnjeVkffornnu8hMkKN131ujDht\r
  30         jQI5WjVqv8nmtg3xzEtEBhuHqcAh1qyQYCkpKk0CuXKTeKfic01gvQIbqJQH0XKTiYLq\r
  31         1yDTafMhuC8RXHaeT8UYnVu27kXAu0v8sGlQoxpv8BOJRpuwdRVThdoAWp3cnq92TgzG\r
  32         kXOn/3yXRBmf0QDCFzyPHS0ZMV4WWZkNqvFYER/ydOBUghuc4+eB38wQ5Y8apzgYd+jS\r
  33         za0d9qQWjEEU6DBS5Q7CYgt5AGOmRXhcjM+2kEq2altwl/RGpOXh/VUQU7iAwlNI4t8B\r
  34         BMcw==\r
  35 Received: by 10.152.105.103 with SMTP id gl7mr16352899lab.10.1350500201194;\r
  36         Wed, 17 Oct 2012 11:56:41 -0700 (PDT)\r
  37 Received: from localhost (dsl-hkibrasgw4-fe51df00-27.dhcp.inet.fi.\r
  38         [80.223.81.27])\r
  39         by mx.google.com with ESMTPS id jk8sm6827954lab.7.2012.10.17.11.56.38\r
  40         (version=SSLv3 cipher=OTHER); Wed, 17 Oct 2012 11:56:39 -0700 (PDT)\r
  41 From: Jani Nikula <jani@nikula.org>\r
  42 To: Ben Gamari <bgamari.foss@gmail.com>, notmuch@notmuchmail.org\r
  43 Subject: Re: [PATCH 1/3] Add notmuch_database_close_compact\r
  44 In-Reply-To: <1350487737-32058-2-git-send-email-bgamari.foss@gmail.com>\r
  45 References: <1350487737-32058-1-git-send-email-bgamari.foss@gmail.com>\r
  46         <1350487737-32058-2-git-send-email-bgamari.foss@gmail.com>\r
  47 User-Agent: Notmuch/0.14+46~g272a1f1 (http://notmuchmail.org) Emacs/23.3.1\r
  48         (i686-pc-linux-gnu)\r
  49 Date: Wed, 17 Oct 2012 21:56:37 +0300\r
  50 Message-ID: <874nlt2ahm.fsf@nikula.org>\r
  51 MIME-Version: 1.0\r
  52 Content-Type: text/plain; charset=us-ascii\r
  53 X-Gm-Message-State:\r
  54  ALoCoQkSMDMYS07Tm4AljSWrxj1HWExJl8BZb/mzlMChLSmQqsoToGrMNmixIbPaLiOhCGTfwVzK\r
  55 X-BeenThere: notmuch@notmuchmail.org\r
  56 X-Mailman-Version: 2.1.13\r
  57 Precedence: list\r
  58 List-Id: "Use and development of the notmuch mail system."\r
  59         <notmuch.notmuchmail.org>\r
  60 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  61         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  62 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  63 List-Post: <mailto:notmuch@notmuchmail.org>\r
  64 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  65 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  67 X-List-Received-Date: Wed, 17 Oct 2012 18:56:45 -0000\r
  68 \r
  69 \r
  70 Hi Ben -\r
  71 \r
  72 I'd like some meaningful commit messages here. Please find other\r
  73 comments inline.\r
  74 \r
  75 BR,\r
  76 Jani.\r
  77 \r
  78 On Wed, 17 Oct 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:\r
  79 > ---\r
  80 >  configure       |   21 ++++++++++++++++++++-\r
  81 >  lib/database.cc |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\r
  82 >  lib/notmuch.h   |   14 ++++++++++++++\r
  83 >  3 files changed, 88 insertions(+), 1 deletion(-)\r
  84 >\r
  85 > diff --git a/configure b/configure\r
  86 > index acb90a8..6551b13 100755\r
  87 > --- a/configure\r
  88 > +++ b/configure\r
  89 > @@ -270,7 +270,8 @@ printf "Checking for Xapian development files... "\r
  90 >  have_xapian=0\r
  91 >  for xapian_config in ${XAPIAN_CONFIG}; do\r
  92 >      if ${xapian_config} --version > /dev/null 2>&1; then\r
  93 > -     printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')\r
  94 > +     xapian_version=$(${xapian_config} --version | sed -e 's/.* //')\r
  95 > +     printf "Yes (%s).\n" ${xapian_version}\r
  96 >       have_xapian=1\r
  97 >       xapian_cxxflags=$(${xapian_config} --cxxflags)\r
  98 >       xapian_ldflags=$(${xapian_config} --libs)\r
  99 > @@ -282,6 +283,20 @@ if [ ${have_xapian} = "0" ]; then\r
 100 >      errors=$((errors + 1))\r
 101 >  fi\r
 102 >  \r
 103 > +have_xapian_compact=0\r
 104 > +if [ ${have_xapian} = "1" ]; then\r
 105 > +    printf "Checking for Xapian compact support... "\r
 106 > +    case "${xapian_version}" in\r
 107 > +        0.*|1.[01].*|1.2.[0-5])\r
 108 > +            printf "No.\n" ;;\r
 109 > +        [1-9]*.[0-9]*.[0-9]*) \r
 110 > +            have_xapian_compact=1\r
 111 > +            printf "Yes.\n" ;;\r
 112 > +        *)\r
 113 > +            printf "Unknown version.\n" ;;\r
 114 > +    esac\r
 115 > +fi\r
 116 > +\r
 117 >  printf "Checking for GMime development files... "\r
 118 >  have_gmime=0\r
 119 >  IFS=';'\r
 120 > @@ -679,6 +694,9 @@ LINKER_RESOLVES_LIBRARY_DEPENDENCIES = ${linker_resolves_library_dependencies}\r
 121 >  XAPIAN_CXXFLAGS = ${xapian_cxxflags}\r
 122 >  XAPIAN_LDFLAGS = ${xapian_ldflags}\r
 123 >  \r
 124 > +# Whether compact is supported by this version of Xapian\r
 125 > +HAVE_XAPIAN_COMPACT = ${have_xapian_compact}\r
 126 > +\r
 127 >  # Flags needed to compile and link against GMime-2.4\r
 128 >  GMIME_CFLAGS = ${gmime_cflags}\r
 129 >  GMIME_LDFLAGS = ${gmime_ldflags}\r
 130 > @@ -715,6 +733,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\\r
 131 >  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\\r
 132 >                    \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\\r
 133 >                    \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\\r
 134 > +                     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\\r
 135 >                       -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)\r
 136 >  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)\r
 137 >  EOF\r
 138 > diff --git a/lib/database.cc b/lib/database.cc\r
 139 > index 761dc1a..6e83a61 100644\r
 140 > --- a/lib/database.cc\r
 141 > +++ b/lib/database.cc\r
 142 > @@ -781,6 +781,60 @@ notmuch_database_close (notmuch_database_t *notmuch)\r
 143 >  }\r
 144 >  \r
 145 >  void\r
 146 > +notmuch_database_close_compact (notmuch_database_t *notmuch)\r
 147 \r
 148 It is clear that there are error conditions, so this should be of type\r
 149 notmuch_status_t. Even if you don't know what to do with the errors now,\r
 150 you'll find that changing the API is a PITA later.\r
 151 \r
 152 > +{\r
 153 > +    void *local = talloc_new (NULL);\r
 154 > +    Xapian::Compactor compactor;\r
 155 > +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;\r
 156 > +\r
 157 > +#if HAVE_XAPIAN_COMPACT\r
 158 > +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {\r
 159 > +     fprintf (stderr, "Out of memory\n");\r
 160 > +     goto DONE;\r
 161 > +    }\r
 162 \r
 163 You could include notmuch->path and .notmuch above into the asprintf\r
 164 below.\r
 165 \r
 166 A personal preference, I think this style would be much more readable:\r
 167 \r
 168     xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian");\r
 169     if (!xapian_path) {\r
 170         ...\r
 171     }\r
 172 \r
 173 > +\r
 174 > +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {\r
 175 > +     fprintf (stderr, "Out of memory\n");\r
 176 > +     goto DONE;\r
 177 > +    }\r
 178 > +\r
 179 > +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {\r
 180 > +     fprintf (stderr, "Out of memory\n");\r
 181 > +     goto DONE;\r
 182 > +    }\r
 183 > +\r
 184 > +    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {\r
 185 > +     fprintf (stderr, "Out of memory\n");\r
 186 > +     goto DONE;\r
 187 > +    }\r
 188 > +\r
 189 > +    try {\r
 190 > +     compactor.set_renumber(false);\r
 191 > +     compactor.add_source(xapian_path);\r
 192 > +     compactor.set_destdir(compact_xapian_path);\r
 193 > +     compactor.compact();\r
 194 > +\r
 195 > +     if (rename(xapian_path, old_xapian_path)) {\r
 196 > +         fprintf (stderr, "Error moving old database out of the way\n");\r
 197 > +         goto DONE;\r
 198 > +     }\r
 199 > +\r
 200 > +     if (rename(compact_xapian_path, xapian_path)) {\r
 201 > +         fprintf (stderr, "Error moving compacted database\n");\r
 202 > +         goto DONE;\r
 203 > +     }\r
 204 \r
 205 Please add strerror(errno) into the two error prints above. The user\r
 206 would probably like to know why the errors occurred.\r
 207 \r
 208 > +    } catch (Xapian::InvalidArgumentError e) {\r
 209 \r
 210 Should this be catch (const Xapian::Error &error) ?\r
 211 \r
 212 > +     fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());\r
 213 > +    }\r
 214 > +    \r
 215 > +#endif\r
 216 > +\r
 217 > +    notmuch_database_close(notmuch);\r
 218 \r
 219 The database gets closed on Xapian errors, but not on talloc or rename\r
 220 errors, and the caller has no way of knowing. See the return status\r
 221 above, but also read on...\r
 222 \r
 223 > +DONE:\r
 224 > +    talloc_free(local);\r
 225 > +}\r
 226 > +\r
 227 > +void\r
 228 >  notmuch_database_destroy (notmuch_database_t *notmuch)\r
 229 >  {\r
 230 >      notmuch_database_close (notmuch);\r
 231 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 232 > index 3633bed..50babfb 100644\r
 233 > --- a/lib/notmuch.h\r
 234 > +++ b/lib/notmuch.h\r
 235 > @@ -215,6 +215,20 @@ notmuch_database_open (const char *path,\r
 236 >  void\r
 237 >  notmuch_database_close (notmuch_database_t *database);\r
 238 >  \r
 239 > +/* Close the given notmuch database and then compact it.\r
 240 \r
 241 It's the other way round, isn't it?\r
 242 \r
 243 But do we want the call to do two things, one of which we already have a\r
 244 call for (closing the database)? That's not orthogonal. Maybe closing\r
 245 the database after compaction is the right thing to do (is it?) but even\r
 246 so, could we leave that responsibility to the API user? The caller does\r
 247 have to open the database too, and calls to open, compact, close seem\r
 248 good.\r
 249 \r
 250 > + *\r
 251 > + * After notmuch_database_close_compact has been called, calls to\r
 252 > + * other functions on objects derived from this database may either\r
 253 > + * behave as if the database had not been closed (e.g., if the\r
 254 > + * required data has been cached) or may fail with a\r
 255 > + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.\r
 256 > + *\r
 257 > + * notmuch_database_close_compact can be called multiple times.  Later\r
 258 > + * calls have no effect.\r
 259 > + */\r
 260 > +void\r
 261 > +notmuch_database_close_compact (notmuch_database_t *notmuch);\r
 262 > +\r
 263 >  /* Destroy the notmuch database, closing it if necessary and freeing\r
 264 >  * all associated resources. */\r
 265 >  void\r
 266 > -- \r
 267 > 1.7.10.4\r
 268 >\r
 269 > _______________________________________________\r
 270 > notmuch mailing list\r
 271 > notmuch@notmuchmail.org\r
 272 > http://notmuchmail.org/mailman/listinfo/notmuch\r