[PATCH v3 01/20] cli: add stub for insert command
Peter Wang
novalazy at gmail.com
Wed Jan 23 00:04:24 PST 2013
On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani at 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
More information about the notmuch
mailing list