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 7276C431FC0 for ; Wed, 23 Jan 2013 01:23:04 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled 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 kJVg3s2eBvDR for ; Wed, 23 Jan 2013 01:23:03 -0800 (PST) Received: from mail-bk0-f44.google.com (mail-bk0-f44.google.com [209.85.214.44]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 84482431FAF for ; Wed, 23 Jan 2013 01:23:03 -0800 (PST) Received: by mail-bk0-f44.google.com with SMTP id j4so575558bkw.3 for ; Wed, 23 Jan 2013 01:23:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:from:to:cc:subject:in-reply-to:references:user-agent :date:message-id:mime-version:content-type:x-gm-message-state; bh=Jxd3Fova8Rbz2hmyuSpqZ60s1n7LMfjz5SCgJWKCW0M=; b=lVV0ECi9chlpBjY2c/ANFbc/jMRTDKO6wGG5GFJyRp7aTwNJgjsUoVdlz95V26V2Hi kZcbck/JOpJLMjEhM7k7F208/yqdvIRCLx2AKCEPJbrSu1sa9xnYhtCgftecLnbwNqnp Md1BCWv2Hbu6WlSuOpkoIMTU9cj3NQdWluQZKA5kYy8z/PTxn0fTOeIb8wHAIIeqsDWU g/67NvlG6z2/sXLjoRU6dbDx4/O9O5eRymXnANeHmbAc1TShUor2OHPY0F1vpXLp3VOR Nis/+4pgpWQCvWxLhnoIdzTrSL0ubKcBvvIkBiAVgHmZiaZbSumnrf0yDRDdU5vDDsvc HGjw== X-Received: by 10.204.5.135 with SMTP id 7mr169887bkv.48.1358932980780; Wed, 23 Jan 2013 01:23:00 -0800 (PST) Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3]) by mx.google.com with ESMTPS id u3sm13365518bkw.9.2013.01.23.01.22.59 (version=TLSv1.1 cipher=RC4-SHA bits=128/128); Wed, 23 Jan 2013 01:22:59 -0800 (PST) From: Jani Nikula To: Peter Wang Subject: Re: [PATCH v3 01/20] cli: add stub for insert command In-Reply-To: <20130123190424.GA1980@hili.localdomain> References: <1358643004-14522-1-git-send-email-novalazy@gmail.com> <1358643004-14522-2-git-send-email-novalazy@gmail.com> <87vcaoj3i0.fsf@nikula.org> <20130123190424.GA1980@hili.localdomain> User-Agent: Notmuch/0.14+259~gdee88db (http://notmuchmail.org) Emacs/23.2.1 (x86_64-pc-linux-gnu) Date: Wed, 23 Jan 2013 10:22:52 +0100 Message-ID: <87622ol0cz.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Gm-Message-State: ALoCoQl0VO7nYv67ZAV34DASaOfy1bdpihDeyGVQ6hpIDn2bPWXSw8Qm8iojtUvTES88yT3sUWY0 Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 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: Wed, 23 Jan 2013 09:23:04 -0000 On Wed, 23 Jan 2013, Peter Wang wrote: > On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula wrote: >> > Though it could be used as an alternative to notmuch new, the reason >> > I want this is to allow my notmuch frontend to add postponed or sent >> > messages to the mail store and notmuch database, without resorting to >> > another tool (e.g. notmuch-deliver) nor directly modifying the maildir. >> >> This review is based on the following patches squashed together: >> >> cli: add stub for insert command >> insert: open Maildir tmp file >> insert: copy stdin to Maildir tmp file >> insert: move file from Maildir tmp to new >> insert: add new message to database >> insert: apply default tags to new message >> insert: parse and apply command-line tag operations >> insert: fsync after writing tmp file >> insert: trap SIGINT and clean up >> insert: add copyright line from notmuch-deliver >> >> It's much easier for me to grasp the big picture this way. >> > > So there *is* a limit to how fine-grained notmuchers want their patches ;-) :) > >> > +static notmuch_bool_t >> > +maildir_move_tmp_to_new (const char *tmppath, const char *newpath) >> > +{ >> > + if (rename (tmppath, newpath) != 0) { >> > + fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno)); >> > + return FALSE; >> > + } >> > + >> > + return TRUE; >> > +} >> >> IMO you could just use rename() inline in the caller, without a wrapper. > > A subsequent patch fsyncs the directory here. Ah, right. You'll probably need to rework the patch a bit to apply here (if you take my advice of postponing the --folder etc. options). While at it, please try to see if you can reduce the amount of paths allocated and passed around. Given the filename, one can figure out the rest. See lib/message.cc for examples. In the end, go for clarity if this suggestion seems messy. > >> > +/* Copy the contents of standard input (fdin) into fdout. */ >> > +static notmuch_bool_t >> > +copy_stdin (int fdin, int fdout) >> >> The comment and the function name imply the function has something to do >> with stdin, while it only cares about file descriptors. > > Tomi pointed out that the error message refers to standard input. > >> > +/* Add the specified message file to the notmuch database, applying tags. >> > + * The file is renamed to encode notmuch tags as maildir flags. */ >> > +static notmuch_bool_t >> > +add_file_to_database (notmuch_database_t *notmuch, const char *path, >> > + tag_op_list_t *tag_ops) >> > +{ >> > + notmuch_message_t *message; >> > + notmuch_status_t status; >> > + >> > + status = notmuch_database_add_message (notmuch, path, &message); >> > + switch (status) { >> > + case NOTMUCH_STATUS_SUCCESS: >> > + break; >> > + case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID: >> > + fprintf (stderr, "Warning: duplicate message.\n"); >> >> This is not uncommon. Why the warning? >> > > I put in the warning because I wasn't sure, then forgot to revisit it. > >> Also, notmuch new does not apply new.tags in this case. Are you sure we >> want to do that here? (You get mail, you read and archive it, you get >> the dupe, it pops up unread in your inbox again.) > > Should command-line tags to be applied to duplicate messages? > I'm thinking not. Agreed; a future patch might add an option to choose. I've thought about having a config option to have notmuch new add a separate set of tags to duplicates (defaulting to no tags added). That could be reused here too (but is different from the command-line tags). But again, future work. > I'll fix the other problems you found. > > Peter BR, Jani.