On Sat, 30 Mar 2013, Peter Wang <novalazy@gmail.com> wrote: > On Fri, 29 Mar 2013 19:59:56 -0400, David Bremner <david@tethera.net> wrote: >> >> It took longer than I thought (of course) but I finally finished looking >> at the first 6 patches. >> >> I already mentioned a minor man page issue in a seperate message. >> >> I took a second pass through 03/12, and I think I would prefer thethe >> control flow of insert_message be closer to the standard style in >> notmuch of using a return value variable and a single cleanup block at >> the end. Reasonable people can disagree about issues of style, but in >> the end consistency of the code base is also important. I think sync_dir() was worse than insert_message() in this regard. >> >> d > > static notmuch_bool_t > insert_message (void *ctx, notmuch_database_t *notmuch, int fdin, > const char *dir, tag_op_list_t *tag_ops) > { > char *tmppath; > char *newpath; > char *newdir; > int fdout; > char *cleanup_path; > > fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir); > if (fdout < 0) > return FALSE; > > cleanup_path = tmppath; > > if (! copy_stdin (fdin, fdout)) > goto DONE; > > if (fsync (fdout) != 0) { > fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno)); > goto DONE; > } > > close (fdout); > fdout = -1; > > /* Atomically move the new message file from the Maildir 'tmp' directory > * to the 'new' directory. We follow the Dovecot recommendation to > * simply use rename() instead of link() and unlink(). > * See also: http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery > */ > if (rename (tmppath, newpath) != 0) { > fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno)); > goto DONE; > } > > cleanup_path = newpath; > > if (! add_file_to_database (notmuch, newpath, tag_ops)) { > /* XXX add an option to keep the file in maildir? */ > goto DONE; > } > > if (! sync_dir (newdir)) > goto DONE; > > cleanup_path = NULL; /* success */ Put the happy day return TRUE here, and don't bother with the above statement. > > DONE: > if (fdout >= 0) > close (fdout); > > if (cleanup_path) { > unlink (cleanup_path); > return FALSE; > } > > return TRUE; Only have the return FALSE path here. You can unconditionally unlink (cleanup_path) too AFAICS. > } > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch