Re: [PATCH v2 06/10] cli: refactor insert

diff --git a/cc/2b44e56cbcd703632f38c9929724af5d202371 b/cc/2b44e56cbcd703632f38c9929724af5d202371
new file mode 100644 (file)
index 0000000..b094b1a
--- /dev/null
+++ b/cc/2b44e56cbcd703632f38c9929724af5d202371
@@ -0,0 +1,193 @@
+Return-Path: <novalazy@gmail.com>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+       by olra.theworths.org (Postfix) with ESMTP id 428F1431FB6\r
+       for <notmuch@notmuchmail.org>; Sat,  5 Jul 2014 20:57:44 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+       tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+       FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+       by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+       with ESMTP id qs09UJcXr8Xq for <notmuch@notmuchmail.org>;\r
+       Sat,  5 Jul 2014 20:57:37 -0700 (PDT)\r
+Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com\r
+       [209.85.220.42]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+       (No client certificate requested)\r
+       by olra.theworths.org (Postfix) with ESMTPS id DE0AF431FAF\r
+       for <notmuch@notmuchmail.org>; Sat,  5 Jul 2014 20:57:36 -0700 (PDT)\r
+Received: by mail-pa0-f42.google.com with SMTP id lj1so3704622pab.1\r
+       for <notmuch@notmuchmail.org>; Sat, 05 Jul 2014 20:57:36 -0700 (PDT)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
+       h=date:message-id:from:to:cc:subject:in-reply-to:references\r
+       :mime-version:content-type:content-disposition\r
+       :content-transfer-encoding;\r
+       bh=4zEpWlBODIKCL7+LnthhP5ZJlT/YM4xoRMvrncBiaDw=;\r
+       b=ZvRhxU2Fz9kSXcPiwOTDmEvS/Wn5UtmuBo4lUgjBYzvFAnCqhnVNJ8XvcvAqi0KfCN\r
+       0YYDpoivnpxOHk1l9YgANTq92TE0UUIetB7RTRcRrL7c6zzreYQgfVeqtjV15D19VkGx\r
+       mqMLZeh1jkWdCwCtUbl6/iHHDq82dP0mY0tRa5G5aC6KHnPASAQhMaZRoGGTrGPwSgh5\r
+       cfvAo/015b1jQYKtvYFgI0L00P27jnhEvhrUHj/NpFgTm5h0afNntBaEIrbcBg2LdFCi\r
+       VV8rp/FzhdjnDTyhocalErCnrqzL6w83O+MG8y7JoWmDZxYvf2H9U9TDAFHftXpomq0w\r
+       MqaA==\r
+X-Received: by 10.68.160.163 with SMTP id xl3mr20977648pbb.39.1404619055900;\r
+       Sat, 05 Jul 2014 20:57:35 -0700 (PDT)\r
+Received: from localhost (215.42.233.220.static.exetel.com.au.\r
+       [220.233.42.215])\r
+       by mx.google.com with ESMTPSA id r3sm14819829pdd.8.2014.07.05.20.57.32\r
+       for <multiple recipients>\r
+       (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\r
+       Sat, 05 Jul 2014 20:57:34 -0700 (PDT)\r
+Date: Sun, 6 Jul 2014 13:57:28 +1000\r
+Message-ID: <20140706135728.GE1142@hili.localdomain>\r
+From: Peter Wang <novalazy@gmail.com>\r
+To: David Bremner <david@tethera.net>\r
+Subject: Re: [PATCH v2 06/10] cli: refactor insert\r
+In-Reply-To: <87simgq702.fsf@maritornes.cs.unb.ca>\r
+References: <1397653165-15620-1-git-send-email-novalazy@gmail.com>\r
+       <1397653165-15620-7-git-send-email-novalazy@gmail.com>\r
+       <87simgq702.fsf@maritornes.cs.unb.ca>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/mixed; boundary="uhW2wu+cthCu47Lg"\r
+Content-Disposition: inline\r
+Content-Transfer-Encoding: 8bit\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+       <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+       <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Sun, 06 Jul 2014 03:57:44 -0000\r
+\r
+\r
+--uhW2wu+cthCu47Lg\r
+Content-Type: text/plain; charset=utf-8\r
+Content-Disposition: inline\r
+Content-Transfer-Encoding: 8bit\r
+\r
+On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner <david@tethera.net> wrote:\r
+> Peter Wang <novalazy@gmail.com> writes:\r
+> \r
+> > -    cleanup_path = tmppath;\r
+> > -\r
+> > -    if (! copy_stdin (fdin, fdout))\r
+> > -  goto FAIL;\r
+> > +    if (! copy_stdin (fdin, fdout)) {\r
+> > +  close (fdout);\r
+> > +  unlink (tmppath);\r
+> > +  return FALSE;\r
+> > +    }\r
+> \r
+> I'm not completely convinced by replacement of the "goto FAIL" with the\r
+> multiple returns.  I'd lean to towards being consistent with the notmuch\r
+> codebase unless the FAIL block is really horrendous\r
+\r
+Eh, when I came back to the code I found it unnecessary convoluted.\r
+However, you can squash in the attached patch if you like.\r
+As an objective measure, the function with the FAIL block is longer.\r
+\r
+> \r
+> Is there a good reason to use TRUE and FALSE for return values rather\r
+> than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be\r
+> overall slightly simpler in notmuch_insert_command.\r
+\r
+Not sure what you have in mind.  I think CLI exit codes should be\r
+confined to notmuch_insert_command.\r
+\r
+Peter\r
+\r
+--uhW2wu+cthCu47Lg\r
+Content-Type: text/x-diff; charset=utf-8\r
+Content-Disposition: attachment; filename="0001-goto-fail.patch"\r
+Content-Transfer-Encoding: 8bit\r
+\r
+>From be07c53d6d5f22ced723a44f996323d2a982113e Mon Sep 17 00:00:00 2001\r
+From: Peter Wang <novalazy@gmail.com>\r
+Date: Sun, 6 Jul 2014 12:52:26 +1000\r
+Subject: [PATCH] goto fail\r
+\r
+---\r
+ notmuch-insert.c | 32 ++++++++++++++++++--------------\r
+ 1 file changed, 18 insertions(+), 14 deletions(-)\r
+\r
+diff --git a/notmuch-insert.c b/notmuch-insert.c\r
+index 7db4f73..8dfc8bb 100644\r
+--- a/notmuch-insert.c\r
++++ b/notmuch-insert.c\r
+@@ -342,26 +342,25 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)\r
+ {\r
+     char *tmppath;\r
+     char *newdir;\r
++    char *cleanup_path;\r
+     int fdout;\r
+ \r
+     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir);\r
+     if (fdout < 0)\r
+       return FALSE;\r
+ \r
+-    if (! copy_stdin (fdin, fdout)) {\r
+-      close (fdout);\r
+-      unlink (tmppath);\r
+-      return FALSE;\r
+-    }\r
++    cleanup_path = tmppath;\r
++\r
++    if (! copy_stdin (fdin, fdout))\r
++      goto FAIL;\r
+ \r
+     if (fsync (fdout) != 0) {\r
+       fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));\r
+-      close (fdout);\r
+-      unlink (tmppath);\r
+-      return FALSE;\r
++      goto FAIL;\r
+     }\r
+ \r
+     close (fdout);\r
++    fdout = -1;\r
+ \r
+     /* Atomically move the new message file from the Maildir 'tmp' directory\r
+      * to the 'new' directory.  We follow the Dovecot recommendation to\r
+@@ -370,16 +369,21 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)\r
+      */\r
+     if (rename (tmppath, *newpath) != 0) {\r
+       fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));\r
+-      unlink (tmppath);\r
+-      return FALSE;\r
++      goto FAIL;\r
+     }\r
+ \r
+-    if (! sync_dir (newdir)) {\r
+-      unlink (*newpath);\r
+-      return FALSE;\r
+-    }\r
++    cleanup_path = *newpath;\r
++\r
++    if (! sync_dir (newdir))\r
++      goto FAIL;\r
+ \r
+     return TRUE;\r
++\r
++  FAIL:\r
++    if (fdout >= 0)\r
++      close (fdout);\r
++    unlink (cleanup_path);\r
++    return FALSE;\r
+ }\r
+ \r
+ int\r
+-- \r
+1.8.4\r
+\r
+\r
+--uhW2wu+cthCu47Lg--\r