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