6c/b011fd7ac6c04c4b2dcfa8de93248249a00cf8

   1 Return-Path: <cworth@cworth.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 3FE9C429E20\r
   6         for <notmuch@notmuchmail.org>; Thu, 10 Mar 2011 19:48:57 -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.99\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.99 tagged_above=-999 required=5\r
  12         tests=[ALL_TRUSTED=-1, T_MIME_NO_TEXT=0.01] 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 6qO5R2pstLKb; Thu, 10 Mar 2011 19:48:55 -0800 (PST)\r
  16 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  17         by olra.theworths.org (Postfix) with ESMTP id 5CF39431FB5;\r
  18         Thu, 10 Mar 2011 19:48:55 -0800 (PST)\r
  19 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  20         id E505E54C0C4; Thu, 10 Mar 2011 19:48:54 -0800 (PST)\r
  21 From: Carl Worth <cworth@cworth.org>\r
  22 To: Austin Clements <amdragon@mit.edu>, notmuch@notmuchmail.org\r
  23 Subject: Re: [PATCH 0/5] Fetch all message metadata in a single pass\r
  24 In-Reply-To: <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>\r
  25 References: <1291928396-27937-1-git-send-email-amdragon@mit.edu>\r
  26         <AANLkTiknTgHruLKKYsVo-r9319VHxmuXcw5oxOLPzHQ_@mail.gmail.com>\r
  27 User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.2.1\r
  28         (i486-pc-linux-gnu)\r
  29 Date: Thu, 10 Mar 2011 19:48:54 -0800\r
  30 Message-ID: <87hbba8ggp.fsf@yoom.home.cworth.org>\r
  31 MIME-Version: 1.0\r
  32 Content-Type: multipart/signed; boundary="=-=-=";\r
  33         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  34 X-BeenThere: notmuch@notmuchmail.org\r
  35 X-Mailman-Version: 2.1.13\r
  36 Precedence: list\r
  37 List-Id: "Use and development of the notmuch mail system."\r
  38         <notmuch.notmuchmail.org>\r
  39 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  40         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  41 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  42 List-Post: <mailto:notmuch@notmuchmail.org>\r
  43 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  44 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  45         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  46 X-List-Received-Date: Fri, 11 Mar 2011 03:48:57 -0000\r
  47 \r
  48 --=-=-=\r
  49 \r
  50 On Sun, 13 Feb 2011 15:25:48 -0500, Austin Clements <amdragon@mit.edu> wrote:\r
  51 > I've rebased this against current master and fixed a few merge\r
  52 > conflicts.  The rebased patches are on the eager-metadata-v3 branch of\r
  53 >   http://awakening.csail.mit.edu/git/notmuch.git\r
  54 \r
  55 Hi Austin,\r
  56 \r
  57 This looks like a great set of optimizations and cleanup. Here is some\r
  58 (long-overdue) review.\r
  59 \r
  60 First, the patch series looks excellent and my review here is quite\r
  61 nit-picky. I feel bad reviewing this so late and not just immediately\r
  62 merging it, but I will commit to picking up a refreshed series very\r
  63 quickly.\r
  64 \r
  65 One very minor thing is that the word "metadata" might be confused with\r
  66 Xapian metadata which we are using already such as:\r
  67 \r
  68     version_string = notmuch->xapian_db->get_metadata ("version");\r
  69 \r
  70 So we might want to come up with a better name here. I don't have any\r
  71 concrete suggestion yet, so if you don't think of anything obvious, then\r
  72 don't worry about it.\r
  73 \r
  74 > +    /* Get tags */\r
  75 > +    if (!message->tag_list) {\r
  76 > +     message->tag_list =\r
  77 > +         _notmuch_get_terms_with_prefix (message, i, end, tag_prefix);\r
  78 > +     _notmuch_string_list_sort (message->tag_list);\r
  79 > +    }\r
  80 \r
  81 Looks like the above case is missing the assert to ensure proper prefix\r
  82 ordering.\r
  83 \r
  84 > +    if (!message->tag_list)\r
  85 > +     _notmuch_message_ensure_metadata (message);\r
  86 > +\r
  87 > +    /* We tell the iterator to add a talloc reference to the\r
  88 > +     * underlying list, rather than stealing it.  As a result, it's\r
  89 > +     * possible to modify the message tags while iterating because\r
  90 > +     * the iterator will keep the current list alive. */\r
  91 > +    return _notmuch_tags_create (message, message->tag_list, FALSE);\r
  92 \r
  93 The behavior here is great, but don't like Boolean function parameters\r
  94 being used to change the semantics. The problem with a Boolean argument\r
  95 is that it's impossible to know the semantic by just reading the calling\r
  96 code---you must also consult the implementation as well.\r
  97 \r
  98 For any case where it seems natural to implement something with a\r
  99 Boolean argument, I sometimes use an approach something like this:\r
 100 \r
 101         static void\r
 102         _foo_function_internal (foo_t *, ..., bool_t be_different)\r
 103         {\r
 104                 ...;\r
 105         }\r
 106 \r
 107         void\r
 108         foo_function (foo_t *, ...)\r
 109         {\r
 110                 return _foo_function_internal (foo, ..., FALSE);\r
 111         }\r
 112 \r
 113         void\r
 114         foo_function_different (foo_t *, ...)\r
 115         {\r
 116                 return _foo_function_internal (foo, ..., TRUE);\r
 117         }\r
 118 \r
 119 That is, I'm willing to accept the Boolean parameter in the case of a\r
 120 static function defined immediately next to all callers. Here, for any\r
 121 non-static callers the semantics should be quite clear. (And perhaps the\r
 122 function calling with the FALSE case will need a clarifying suffix as\r
 123 well---one might have some_function_foo and some_function_bar for the\r
 124 two cases).\r
 125 \r
 126 Of course, my proscription against Boolean parameter doesn't apply to\r
 127 something like a function that is setting some Boolean feature to TRUE\r
 128 or FALSE. The important criterion is that the function semantics should\r
 129 be evident to someone reading code calling the function. So if the\r
 130 Boolean argument is obviously tied to some portion of the function name,\r
 131 then that can be adequate.\r
 132 \r
 133 Enough with the generalities, back to _notmuch_tags_create...\r
 134 \r
 135 The caller above is the exceptional caller. It's the only one using\r
 136 passing FALSE, and it also already has a large comment. So it seems that\r
 137 the right fix here is to have the extra talloc_reference happen here\r
 138 where there's a comment talking about adding a talloc reference.\r
 139 \r
 140 So it would be great to see something here like:\r
 141 \r
 142         tags = _notmuch_tags_create (message, message->tag_list);\r
 143         talloc_reference (message, message->tag_list);\r
 144         return tags;\r
 145 \r
 146 Of course, that would mean that _notmuch_tags_create can't have done the\r
 147 talloc_steal. So perhaps all the other callers should be calling:\r
 148 \r
 149         _notmuch_tags_create_with_steal (ctx, tag_list);\r
 150 \r
 151 What do you think?\r
 152 \r
 153 > -notmuch_tags_t *\r
 154 > -_notmuch_convert_tags (void *ctx, Xapian::TermIterator &i,\r
 155 > -                    Xapian::TermIterator &end);\r
 156 > +notmuch_string_list_t *\r
 157 > +_notmuch_get_terms_with_prefix (void *ctx, Xapian::TermIterator &i,\r
 158 > +                             Xapian::TermIterator &end,\r
 159 > +                             const char *prefix);\r
 160 \r
 161 I don't really like the fact that _notmuch_get_terms_with_prefix doesn't\r
 162 make a clear indication of what file it's defined in, (the old function\r
 163 _notmuch_convert_tags had the same problem). It could be named\r
 164 _notmuch_database_convert_tags since it's in database.cc, but that would\r
 165 be odd in not actually accepting a notmuch_database_t * as the first\r
 166 parameter. Any suggestion here?\r
 167 \r
 168 > index 0000000..d92a0ba\r
 169 > --- /dev/null\r
 170 > +++ b/lib/strings.c\r
 171 > @@ -0,0 +1,94 @@\r
 172 > +/* strings.c - Iterator for a list of strings\r
 173 \r
 174 Similarly, this file might be better as string-list.c.\r
 175 \r
 176 > + * You should have received a copy of the GNU General Public License\r
 177 > + * along with this program.  If not, see http://www.gnu.org/licenses/ .\r
 178 > + *\r
 179 > + * Author: Carl Worth <cworth@cworth.org>\r
 180 \r
 181 Hey, wait a second, some of this code is mine, but I think the sort\r
 182 function is yours. Please do start annotating the Author tags on all\r
 183 files as appropriate. (There are probably lots of files missing your\r
 184 Author attribution---I just happened to notice this one here since you\r
 185 happened to have an Author line in the patch.)\r
 186 \r
 187 > -/* XXX: Should write some talloc-friendly list to avoid the need for\r
 188 > - * this. */\r
 189 \r
 190 Hurrah! I love patches that get to remove XXX comments.\r
 191 \r
 192 > +    /* Get thread */\r
 193 > +    if (!message->thread_id)\r
 194 > +     message->thread_id =\r
 195 > +         _notmuch_message_get_term (message, i, end, thread_prefix);\r
 196 > +\r
 197 > +    /* Get id */\r
 198 > +    assert (strcmp (thread_prefix, id_prefix) < 0);\r
 199 > +    if (!message->message_id)\r
 200 > +     message->message_id =\r
 201 > +         _notmuch_message_get_term (message, i, end, id_prefix);\r
 202 \r
 203 Missing asserts on the above two as well?\r
 204 \r
 205 -Carl\r
 206 \r
 207 --=-=-=\r
 208 Content-Type: application/pgp-signature\r
 209 \r
 210 -----BEGIN PGP SIGNATURE-----\r
 211 Version: GnuPG v1.4.10 (GNU/Linux)\r
 212 \r
 213 iD8DBQFNeZum6JDdNq8qSWgRAm0WAKCWSxswxte3awQvkHx121eP3euxHwCfa7As\r
 214 AeePsp1GmNL7pFoTzSdxptE=\r
 215 =u/mF\r
 216 -----END PGP SIGNATURE-----\r
 217 --=-=-=--\r