f0/8e132ce7b7cae49149fa09edcc49cce6a745ca

   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 56148431FBF;\r
   6         Tue,  8 Dec 2009 01:51:00 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 Received: from olra.theworths.org ([127.0.0.1])\r
   9         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  10         with ESMTP id 9VujeyceIL2i; Tue,  8 Dec 2009 01:50:59 -0800 (PST)\r
  11 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  12         by olra.theworths.org (Postfix) with ESMTP id 33061431FAE;\r
  13         Tue,  8 Dec 2009 01:50:59 -0800 (PST)\r
  14 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  15         id C9C8C2542FB; Tue,  8 Dec 2009 01:50:58 -0800 (PST)\r
  16 From: Carl Worth <cworth@cworth.org>\r
  17 To: david@tethera.net, notmuch@notmuchmail.org\r
  18 In-Reply-To: <1260242088-15680-1-git-send-email-david@tethera.net>\r
  19 References: <1260124225-10830-1-git-send-email-david@tethera.net>\r
  20         <1260242088-15680-1-git-send-email-david@tethera.net>\r
  21 Date: Tue, 08 Dec 2009 01:50:51 -0800\r
  22 Message-ID: <87ws0x23r8.fsf@yoom.home.cworth.org>\r
  23 MIME-Version: 1.0\r
  24 Content-Type: multipart/signed; boundary="=-=-=";\r
  25         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  26 Cc: David Bremner <bremner@unb.ca>\r
  27 Subject: Re: [notmuch] [PATCH] notmuch-restore.c: only update tags for\r
  28  messages that differ from dump file.\r
  29 X-BeenThere: notmuch@notmuchmail.org\r
  30 X-Mailman-Version: 2.1.12\r
  31 Precedence: list\r
  32 List-Id: "Use and development of the notmuch mail system."\r
  33         <notmuch.notmuchmail.org>\r
  34 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  35         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  36 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  37 List-Post: <mailto:notmuch@notmuchmail.org>\r
  38 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  39 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  40         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  41 X-List-Received-Date: Tue, 08 Dec 2009 09:51:00 -0000\r
  42 \r
  43 --=-=-=\r
  44 \r
  45 On Mon,  7 Dec 2009 23:14:48 -0400, david@tethera.net wrote:\r
  46 > The main feature of this patch is that it compares the list of current\r
  47 > tags on a message with those read by restore. Only if the two lists\r
  48 > differ is the tag list in the message replaced.  In my experiments this leads to\r
  49 > a large performance improvement.\r
  50 \r
  51 Hi David,\r
  52 \r
  53 This is going to be a nice performance fix. Thanks for working on it!\r
  54 \r
  55 I noticed that the code is in a style that is inconsistent with the\r
  56 prevailing style of the notmuch code. So here are a few points on some\r
  57 obvious style differences (there are other similar issues that I don't\r
  58 mention specifically).\r
  59 \r
  60 And I don't mean anything personal here. We all have different styles\r
  61 that we prefer and find most legible. I do think it's important that the\r
  62 code be in a consistent style throughout though, (and I will make a\r
  63 CODING_STYLE document for the source tree soon).\r
  64 \r
  65 > +/* for qsort */\r
  66 > +static int scmp( const void *sp1, const void *sp2 )\r
  67 \r
  68 The function name really needs to be on its own line (flush with the\r
  69 left-most column). I don't like the interior whitespace for the argument\r
  70 list. I really don't like a name as dramatically abbreviated as\r
  71 "scmp". I'd prefer something like strcmp_for_qsort or whatever this is\r
  72 actually doing.\r
  73 \r
  74 > +    return( strcmp(*(const char **)sp1, *(const char **)sp2) );\r
  75 \r
  76 More whitespace here. There should be a space *before* the '(' not\r
  77 after, (and then not before the ')'). Finally, with void* arguments, I\r
  78 like to take care of the ugly casts up front, (assigning to approriately\r
  79 named local variables) rather than cluttering a function call with casts\r
  80 like this.\r
  81 \r
  82 > +    char **tag_array=NULL;\r
  83 > +    int tag_array_size=DEFAULT_TAG_ARRAY_SIZE;\r
  84 \r
  85 Need whitespace on either side of '=' here, (and similar throughout).\r
  86 \r
  87 > +     while(*next){\r
  88 \r
  89 More missing space. I would prefer:\r
  90 \r
  91         while (*next) {\r
  92 \r
  93 > +       while(*next && isspace(*next))\r
  94 > +         next++;\r
  95 \r
  96 Current indentation in notmuch is 4 columns per indent level, not 2.\r
  97 \r
  98 > +           tag_array=realloc(tag_array,tag_array_size*sizeof(char *));\r
  99 \r
 100 I can't read that at all with so much missing space. I'd prefer to see:\r
 101 \r
 102               tag_array = realloc (tag_array, tag_array_size * sizeof (char *));\r
 103 \r
 104 > +     while (notmuch_tags_has_more (tag_list) && i<tag_count &&\r
 105 > +            (strcmp(notmuch_tags_get (tag_list),tag_array[i])==0)){\r
 106 > +       notmuch_tags_advance (tag_list);\r
 107 > +       i++;\r
 108 > +     }\r
 109 \r
 110 While I don't mind an opening brace on the same line as an "if" or\r
 111 "while" condition, I do mind it when the condition spans more than one\r
 112 line. In that case, the opening brace really needs to be on a line of\r
 113 its own. And again, lots of missing space in the above.\r
 114 \r
 115 > +       for (i=0; i<tag_count; i++) {\r
 116 \r
 117 More missing space. Should be:\r
 118 \r
 119           for (i = 0; i < tag_count; i++) {\r
 120 \r
 121 -Carl\r
 122 \r
 123 PS. Why is the commit mentioning using calloc, not talloc? Is there a\r
 124 reason talloc is inappropriate here? Or were you just not familiar with\r
 125 how to use it? I'd be glad to answer any questions you have about use of\r
 126 talloc in notmuch.\r
 127 \r
 128 --=-=-=\r
 129 Content-Type: application/pgp-signature\r
 130 \r
 131 -----BEGIN PGP SIGNATURE-----\r
 132 Version: GnuPG v1.4.10 (GNU/Linux)\r
 133 \r
 134 iD8DBQFLHiF76JDdNq8qSWgRAr77AJ9ocvu1qn+YKcx7E4+FYjlsxhWMnQCeIinL\r
 135 VbsrGKaDHm2dso9RtehFAU4=\r
 136 =Vd9i\r
 137 -----END PGP SIGNATURE-----\r
 138 --=-=-=--\r