Date: Wed, 23 Jan 2013 19:04:24 +1100
Message-ID: <20130123190424.GA1980@hili.localdomain>
From: Peter Wang <novalazy@gmail.com>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v3 01/20] cli: add stub for insert command
In-Reply-To: <87vcaoj3i0.fsf@nikula.org>
References: <1358643004-14522-1-git-send-email-novalazy@gmail.com>
	<1358643004-14522-2-git-send-email-novalazy@gmail.com>
	<87vcaoj3i0.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Cc: notmuch@notmuchmail.org
Precedence: list

On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani@nikula.org> 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.

> > +/* 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.

I'll fix the other problems you found.

Peter