From 517547883d1708a9b6a8d0db5b2134b83cff631b Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Wed, 23 Jan 2013 10:22:52 +0100 Subject: [PATCH] Re: [PATCH v3 01/20] cli: add stub for insert command --- 44/27f9df0f56de10cea99e93d2573fef83c2f267 | 175 ++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 44/27f9df0f56de10cea99e93d2573fef83c2f267 diff --git a/44/27f9df0f56de10cea99e93d2573fef83c2f267 b/44/27f9df0f56de10cea99e93d2573fef83c2f267 new file mode 100644 index 000000000..1b48c5d8d --- /dev/null +++ b/44/27f9df0f56de10cea99e93d2573fef83c2f267 @@ -0,0 +1,175 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +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. -- 2.26.2