Return-Path: <novalazy@gmail.com>
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 428F1431FB6
	for <notmuch@notmuchmail.org>; Sat,  5 Jul 2014 20:57:44 -0700 (PDT)
X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
X-Spam-Flag: NO
X-Spam-Score: -0.799
X-Spam-Level: 
X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5
	tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,
	FREEMAIL_FROM=0.001, 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 qs09UJcXr8Xq for <notmuch@notmuchmail.org>;
	Sat,  5 Jul 2014 20:57:37 -0700 (PDT)
Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com
	[209.85.220.42]) (using TLSv1 with cipher RC4-SHA (128/128 bits))
	(No client certificate requested)
	by olra.theworths.org (Postfix) with ESMTPS id DE0AF431FAF
	for <notmuch@notmuchmail.org>; Sat,  5 Jul 2014 20:57:36 -0700 (PDT)
Received: by mail-pa0-f42.google.com with SMTP id lj1so3704622pab.1
	for <notmuch@notmuchmail.org>; Sat, 05 Jul 2014 20:57:36 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;
	h=date:message-id:from:to:cc:subject:in-reply-to:references
	:mime-version:content-type:content-disposition
	:content-transfer-encoding;
	bh=4zEpWlBODIKCL7+LnthhP5ZJlT/YM4xoRMvrncBiaDw=;
	b=ZvRhxU2Fz9kSXcPiwOTDmEvS/Wn5UtmuBo4lUgjBYzvFAnCqhnVNJ8XvcvAqi0KfCN
	0YYDpoivnpxOHk1l9YgANTq92TE0UUIetB7RTRcRrL7c6zzreYQgfVeqtjV15D19VkGx
	mqMLZeh1jkWdCwCtUbl6/iHHDq82dP0mY0tRa5G5aC6KHnPASAQhMaZRoGGTrGPwSgh5
	cfvAo/015b1jQYKtvYFgI0L00P27jnhEvhrUHj/NpFgTm5h0afNntBaEIrbcBg2LdFCi
	VV8rp/FzhdjnDTyhocalErCnrqzL6w83O+MG8y7JoWmDZxYvf2H9U9TDAFHftXpomq0w
	MqaA==
X-Received: by 10.68.160.163 with SMTP id xl3mr20977648pbb.39.1404619055900;
	Sat, 05 Jul 2014 20:57:35 -0700 (PDT)
Received: from localhost (215.42.233.220.static.exetel.com.au.
	[220.233.42.215])
	by mx.google.com with ESMTPSA id r3sm14819829pdd.8.2014.07.05.20.57.32
	for <multiple recipients>
	(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
	Sat, 05 Jul 2014 20:57:34 -0700 (PDT)
Date: Sun, 6 Jul 2014 13:57:28 +1000
Message-ID: <20140706135728.GE1142@hili.localdomain>
From: Peter Wang <novalazy@gmail.com>
To: David Bremner <david@tethera.net>
Subject: Re: [PATCH v2 06/10] cli: refactor insert
In-Reply-To: <87simgq702.fsf@maritornes.cs.unb.ca>
References: <1397653165-15620-1-git-send-email-novalazy@gmail.com>
	<1397653165-15620-7-git-send-email-novalazy@gmail.com>
	<87simgq702.fsf@maritornes.cs.unb.ca>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="uhW2wu+cthCu47Lg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
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."
	<notmuch.notmuchmail.org>
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
List-Archive: <http://notmuchmail.org/pipermail/notmuch>
List-Post: <mailto:notmuch@notmuchmail.org>
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=subscribe>
X-List-Received-Date: Sun, 06 Jul 2014 03:57:44 -0000


--uhW2wu+cthCu47Lg
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner <david@tethera.net> wrote:
> Peter Wang <novalazy@gmail.com> writes:
> 
> > -    cleanup_path = tmppath;
> > -
> > -    if (! copy_stdin (fdin, fdout))
> > -	goto FAIL;
> > +    if (! copy_stdin (fdin, fdout)) {
> > +	close (fdout);
> > +	unlink (tmppath);
> > +	return FALSE;
> > +    }
> 
> I'm not completely convinced by replacement of the "goto FAIL" with the
> multiple returns.  I'd lean to towards being consistent with the notmuch
> codebase unless the FAIL block is really horrendous

Eh, when I came back to the code I found it unnecessary convoluted.
However, you can squash in the attached patch if you like.
As an objective measure, the function with the FAIL block is longer.

> 
> Is there a good reason to use TRUE and FALSE for return values rather
> than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be
> overall slightly simpler in notmuch_insert_command.

Not sure what you have in mind.  I think CLI exit codes should be
confined to notmuch_insert_command.

Peter

--uhW2wu+cthCu47Lg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment; filename="0001-goto-fail.patch"
Content-Transfer-Encoding: 8bit

>From be07c53d6d5f22ced723a44f996323d2a982113e Mon Sep 17 00:00:00 2001
From: Peter Wang <novalazy@gmail.com>
Date: Sun, 6 Jul 2014 12:52:26 +1000
Subject: [PATCH] goto fail

---
 notmuch-insert.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7db4f73..8dfc8bb 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -342,26 +342,25 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
 {
     char *tmppath;
     char *newdir;
+    char *cleanup_path;
     int fdout;
 
     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir);
     if (fdout < 0)
 	return FALSE;
 
-    if (! copy_stdin (fdin, fdout)) {
-	close (fdout);
-	unlink (tmppath);
-	return FALSE;
-    }
+    cleanup_path = tmppath;
+
+    if (! copy_stdin (fdin, fdout))
+	goto FAIL;
 
     if (fsync (fdout) != 0) {
 	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
-	close (fdout);
-	unlink (tmppath);
-	return FALSE;
+	goto FAIL;
     }
 
     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
@@ -370,16 +369,21 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
      */
     if (rename (tmppath, *newpath) != 0) {
 	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
-	unlink (tmppath);
-	return FALSE;
+	goto FAIL;
     }
 
-    if (! sync_dir (newdir)) {
-	unlink (*newpath);
-	return FALSE;
-    }
+    cleanup_path = *newpath;
+
+    if (! sync_dir (newdir))
+	goto FAIL;
 
     return TRUE;
+
+  FAIL:
+    if (fdout >= 0)
+	close (fdout);
+    unlink (cleanup_path);
+    return FALSE;
 }
 
 int
-- 
1.8.4


--uhW2wu+cthCu47Lg--