44/27f9df0f56de10cea99e93d2573fef83c2f267

   1 Return-Path: <jani@nikula.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 7276C431FC0\r
   6         for <notmuch@notmuchmail.org>; Wed, 23 Jan 2013 01:23:04 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id kJVg3s2eBvDR for <notmuch@notmuchmail.org>;\r
  16         Wed, 23 Jan 2013 01:23:03 -0800 (PST)\r
  17 Received: from mail-bk0-f44.google.com (mail-bk0-f44.google.com\r
  18         [209.85.214.44]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id 84482431FAF\r
  21         for <notmuch@notmuchmail.org>; Wed, 23 Jan 2013 01:23:03 -0800 (PST)\r
  22 Received: by mail-bk0-f44.google.com with SMTP id j4so575558bkw.3\r
  23         for <notmuch@notmuchmail.org>; Wed, 23 Jan 2013 01:23:01 -0800 (PST)\r
  24 X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;\r
  25         d=google.com; s=20120113;\r
  26         h=x-received:from:to:cc:subject:in-reply-to:references:user-agent\r
  27         :date:message-id:mime-version:content-type:x-gm-message-state;\r
  28         bh=Jxd3Fova8Rbz2hmyuSpqZ60s1n7LMfjz5SCgJWKCW0M=;\r
  29         b=lVV0ECi9chlpBjY2c/ANFbc/jMRTDKO6wGG5GFJyRp7aTwNJgjsUoVdlz95V26V2Hi\r
  30         kZcbck/JOpJLMjEhM7k7F208/yqdvIRCLx2AKCEPJbrSu1sa9xnYhtCgftecLnbwNqnp\r
  31         Md1BCWv2Hbu6WlSuOpkoIMTU9cj3NQdWluQZKA5kYy8z/PTxn0fTOeIb8wHAIIeqsDWU\r
  32         g/67NvlG6z2/sXLjoRU6dbDx4/O9O5eRymXnANeHmbAc1TShUor2OHPY0F1vpXLp3VOR\r
  33         Nis/+4pgpWQCvWxLhnoIdzTrSL0ubKcBvvIkBiAVgHmZiaZbSumnrf0yDRDdU5vDDsvc\r
  34         HGjw==\r
  35 X-Received: by 10.204.5.135 with SMTP id 7mr169887bkv.48.1358932980780;\r
  36         Wed, 23 Jan 2013 01:23:00 -0800 (PST)\r
  37 Received: from localhost ([2001:4b98:dc0:43:216:3eff:fe1b:25f3])\r
  38         by mx.google.com with ESMTPS id u3sm13365518bkw.9.2013.01.23.01.22.59\r
  39         (version=TLSv1.1 cipher=RC4-SHA bits=128/128);\r
  40         Wed, 23 Jan 2013 01:22:59 -0800 (PST)\r
  41 From: Jani Nikula <jani@nikula.org>\r
  42 To: Peter Wang <novalazy@gmail.com>\r
  43 Subject: Re: [PATCH v3 01/20] cli: add stub for insert command\r
  44 In-Reply-To: <20130123190424.GA1980@hili.localdomain>\r
  45 References: <1358643004-14522-1-git-send-email-novalazy@gmail.com>\r
  46         <1358643004-14522-2-git-send-email-novalazy@gmail.com>\r
  47         <87vcaoj3i0.fsf@nikula.org>\r
  48         <20130123190424.GA1980@hili.localdomain>\r
  49 User-Agent: Notmuch/0.14+259~gdee88db (http://notmuchmail.org) Emacs/23.2.1\r
  50         (x86_64-pc-linux-gnu)\r
  51 Date: Wed, 23 Jan 2013 10:22:52 +0100\r
  52 Message-ID: <87622ol0cz.fsf@nikula.org>\r
  53 MIME-Version: 1.0\r
  54 Content-Type: text/plain; charset=us-ascii\r
  55 X-Gm-Message-State:\r
  56  ALoCoQl0VO7nYv67ZAV34DASaOfy1bdpihDeyGVQ6hpIDn2bPWXSw8Qm8iojtUvTES88yT3sUWY0\r
  57 Cc: notmuch@notmuchmail.org\r
  58 X-BeenThere: notmuch@notmuchmail.org\r
  59 X-Mailman-Version: 2.1.13\r
  60 Precedence: list\r
  61 List-Id: "Use and development of the notmuch mail system."\r
  62         <notmuch.notmuchmail.org>\r
  63 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  64         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  65 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  66 List-Post: <mailto:notmuch@notmuchmail.org>\r
  67 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  68 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  69         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  70 X-List-Received-Date: Wed, 23 Jan 2013 09:23:04 -0000\r
  71 \r
  72 On Wed, 23 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:\r
  73 > On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani@nikula.org> wrote:\r
  74 >> > Though it could be used as an alternative to notmuch new, the reason\r
  75 >> > I want this is to allow my notmuch frontend to add postponed or sent\r
  76 >> > messages to the mail store and notmuch database, without resorting to\r
  77 >> > another tool (e.g. notmuch-deliver) nor directly modifying the maildir.\r
  78 >> \r
  79 >> This review is based on the following patches squashed together:\r
  80 >> \r
  81 >>      cli: add stub for insert command\r
  82 >>      insert: open Maildir tmp file\r
  83 >>      insert: copy stdin to Maildir tmp file\r
  84 >>      insert: move file from Maildir tmp to new\r
  85 >>      insert: add new message to database\r
  86 >>      insert: apply default tags to new message\r
  87 >>      insert: parse and apply command-line tag operations\r
  88 >>      insert: fsync after writing tmp file\r
  89 >>      insert: trap SIGINT and clean up\r
  90 >>      insert: add copyright line from notmuch-deliver\r
  91 >> \r
  92 >> It's much easier for me to grasp the big picture this way.\r
  93 >> \r
  94 >\r
  95 > So there *is* a limit to how fine-grained notmuchers want their patches ;-)\r
  96 \r
  97 :)\r
  98 \r
  99 >\r
 100 >> > +static notmuch_bool_t\r
 101 >> > +maildir_move_tmp_to_new (const char *tmppath, const char *newpath)\r
 102 >> > +{\r
 103 >> > +    if (rename (tmppath, newpath) != 0) {\r
 104 >> > +  fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));\r
 105 >> > +  return FALSE;\r
 106 >> > +    }\r
 107 >> > +\r
 108 >> > +    return TRUE;\r
 109 >> > +}\r
 110 >> \r
 111 >> IMO you could just use rename() inline in the caller, without a wrapper.\r
 112 >\r
 113 > A subsequent patch fsyncs the directory here.\r
 114 \r
 115 Ah, right.\r
 116 \r
 117 You'll probably need to rework the patch a bit to apply here (if you\r
 118 take my advice of postponing the --folder etc. options). While at it,\r
 119 please try to see if you can reduce the amount of paths allocated and\r
 120 passed around. Given the filename, one can figure out the rest. See\r
 121 lib/message.cc for examples. In the end, go for clarity if this\r
 122 suggestion seems messy.\r
 123 \r
 124 >\r
 125 >> > +/* Copy the contents of standard input (fdin) into fdout. */\r
 126 >> > +static notmuch_bool_t\r
 127 >> > +copy_stdin (int fdin, int fdout)\r
 128 >> \r
 129 >> The comment and the function name imply the function has something to do\r
 130 >> with stdin, while it only cares about file descriptors.\r
 131 >\r
 132 > Tomi pointed out that the error message refers to standard input.\r
 133 >\r
 134 >> > +/* Add the specified message file to the notmuch database, applying tags.\r
 135 >> > + * The file is renamed to encode notmuch tags as maildir flags. */\r
 136 >> > +static notmuch_bool_t\r
 137 >> > +add_file_to_database (notmuch_database_t *notmuch, const char *path,\r
 138 >> > +                tag_op_list_t *tag_ops)\r
 139 >> > +{\r
 140 >> > +    notmuch_message_t *message;\r
 141 >> > +    notmuch_status_t status;\r
 142 >> > +\r
 143 >> > +    status = notmuch_database_add_message (notmuch, path, &message);\r
 144 >> > +    switch (status) {\r
 145 >> > +    case NOTMUCH_STATUS_SUCCESS:\r
 146 >> > +  break;\r
 147 >> > +    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:\r
 148 >> > +  fprintf (stderr, "Warning: duplicate message.\n");\r
 149 >> \r
 150 >> This is not uncommon. Why the warning?\r
 151 >> \r
 152 >\r
 153 > I put in the warning because I wasn't sure, then forgot to revisit it.\r
 154 >\r
 155 >> Also, notmuch new does not apply new.tags in this case. Are you sure we\r
 156 >> want to do that here? (You get mail, you read and archive it, you get\r
 157 >> the dupe, it pops up unread in your inbox again.)\r
 158 >\r
 159 > Should command-line tags to be applied to duplicate messages?\r
 160 > I'm thinking not.\r
 161 \r
 162 Agreed; a future patch might add an option to choose.\r
 163 \r
 164 I've thought about having a config option to have notmuch new add a\r
 165 separate set of tags to duplicates (defaulting to no tags added). That\r
 166 could be reused here too (but is different from the command-line\r
 167 tags). But again, future work.\r
 168 \r
 169 > I'll fix the other problems you found.\r
 170 >\r
 171 > Peter\r
 172 \r
 173 \r
 174 BR,\r
 175 Jani.\r