From: Jani Nikula <jani@nikula.org>
To: Peter Wang <novalazy@gmail.com>
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
Cc: notmuch@notmuchmail.org
Precedence: list

On Wed, 23 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> 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.

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.