Re: [notmuch] [PATCH] notmuch-restore.c: only update tags for messages that differ...

diff --git a/f0/8e132ce7b7cae49149fa09edcc49cce6a745ca b/f0/8e132ce7b7cae49149fa09edcc49cce6a745ca
new file mode 100644 (file)
index 0000000..905b32f
--- /dev/null
+++ b/f0/8e132ce7b7cae49149fa09edcc49cce6a745ca
@@ -0,0 +1,138 @@
+Return-Path: <cworth@cworth.org>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 56148431FBF;\r
+       Tue,  8 Dec 2009 01:51:00 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id 9VujeyceIL2i; Tue,  8 Dec 2009 01:50:59 -0800 (PST)\r
+Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 33061431FAE;\r
+       Tue,  8 Dec 2009 01:50:59 -0800 (PST)\r
+Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
+       id C9C8C2542FB; Tue,  8 Dec 2009 01:50:58 -0800 (PST)\r
+From: Carl Worth <cworth@cworth.org>\r
+To: david@tethera.net, notmuch@notmuchmail.org\r
+In-Reply-To: <1260242088-15680-1-git-send-email-david@tethera.net>\r
+References: <1260124225-10830-1-git-send-email-david@tethera.net>\r
+       <1260242088-15680-1-git-send-email-david@tethera.net>\r
+Date: Tue, 08 Dec 2009 01:50:51 -0800\r
+Message-ID: <87ws0x23r8.fsf@yoom.home.cworth.org>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/signed; boundary="=-=-=";\r
+       micalg=pgp-sha1; protocol="application/pgp-signature"\r
+Cc: David Bremner <bremner@unb.ca>\r
+Subject: Re: [notmuch] [PATCH] notmuch-restore.c: only update tags for\r
+ messages that differ from dump file.\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.12\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Tue, 08 Dec 2009 09:51:00 -0000\r
+\r
+--=-=-=\r
+\r
+On Mon,  7 Dec 2009 23:14:48 -0400, david@tethera.net wrote:\r
+> The main feature of this patch is that it compares the list of current\r
+> tags on a message with those read by restore. Only if the two lists\r
+> differ is the tag list in the message replaced.  In my experiments this leads to\r
+> a large performance improvement.\r
+\r
+Hi David,\r
+\r
+This is going to be a nice performance fix. Thanks for working on it!\r
+\r
+I noticed that the code is in a style that is inconsistent with the\r
+prevailing style of the notmuch code. So here are a few points on some\r
+obvious style differences (there are other similar issues that I don't\r
+mention specifically).\r
+\r
+And I don't mean anything personal here. We all have different styles\r
+that we prefer and find most legible. I do think it's important that the\r
+code be in a consistent style throughout though, (and I will make a\r
+CODING_STYLE document for the source tree soon).\r
+\r
+> +/* for qsort */\r
+> +static int scmp( const void *sp1, const void *sp2 )\r
+\r
+The function name really needs to be on its own line (flush with the\r
+left-most column). I don't like the interior whitespace for the argument\r
+list. I really don't like a name as dramatically abbreviated as\r
+"scmp". I'd prefer something like strcmp_for_qsort or whatever this is\r
+actually doing.\r
+\r
+> +    return( strcmp(*(const char **)sp1, *(const char **)sp2) );\r
+\r
+More whitespace here. There should be a space *before* the '(' not\r
+after, (and then not before the ')'). Finally, with void* arguments, I\r
+like to take care of the ugly casts up front, (assigning to approriately\r
+named local variables) rather than cluttering a function call with casts\r
+like this.\r
+\r
+> +    char **tag_array=NULL;\r
+> +    int tag_array_size=DEFAULT_TAG_ARRAY_SIZE;\r
+\r
+Need whitespace on either side of '=' here, (and similar throughout).\r
+\r
+> +    while(*next){\r
+\r
+More missing space. I would prefer:\r
+\r
+       while (*next) {\r
+\r
+> +      while(*next && isspace(*next))\r
+> +        next++;\r
+\r
+Current indentation in notmuch is 4 columns per indent level, not 2.\r
+\r
+> +          tag_array=realloc(tag_array,tag_array_size*sizeof(char *));\r
+\r
+I can't read that at all with so much missing space. I'd prefer to see:\r
+\r
+             tag_array = realloc (tag_array, tag_array_size * sizeof (char *));\r
+\r
+> +    while (notmuch_tags_has_more (tag_list) && i<tag_count &&\r
+> +           (strcmp(notmuch_tags_get (tag_list),tag_array[i])==0)){\r
+> +      notmuch_tags_advance (tag_list);\r
+> +      i++;\r
+> +    }\r
+\r
+While I don't mind an opening brace on the same line as an "if" or\r
+"while" condition, I do mind it when the condition spans more than one\r
+line. In that case, the opening brace really needs to be on a line of\r
+its own. And again, lots of missing space in the above.\r
+\r
+> +      for (i=0; i<tag_count; i++) {\r
+\r
+More missing space. Should be:\r
+\r
+         for (i = 0; i < tag_count; i++) {\r
+\r
+-Carl\r
+\r
+PS. Why is the commit mentioning using calloc, not talloc? Is there a\r
+reason talloc is inappropriate here? Or were you just not familiar with\r
+how to use it? I'd be glad to answer any questions you have about use of\r
+talloc in notmuch.\r
+\r
+--=-=-=\r
+Content-Type: application/pgp-signature\r
+\r
+-----BEGIN PGP SIGNATURE-----\r
+Version: GnuPG v1.4.10 (GNU/Linux)\r
+\r
+iD8DBQFLHiF76JDdNq8qSWgRAr77AJ9ocvu1qn+YKcx7E4+FYjlsxhWMnQCeIinL\r
+VbsrGKaDHm2dso9RtehFAU4=\r
+=Vd9i\r
+-----END PGP SIGNATURE-----\r
+--=-=-=--\r