[PATCH v3 01/20] cli: add stub for insert command
The notmuch insert command should read a message from standard input
and deliver it to a Maildir folder, and then incorporate the message
into the notmuch database. Essentially it moves the functionality of
notmuch-deliver into notmuch.
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.
---
Makefile.local | 1 +
notmuch-client.h | 3 +++
notmuch-insert.c | 43 +++
notmuch.c| 3 +++
4 files changed, 50 insertions(+)
create mode 100644 notmuch-insert.c
diff --git a/Makefile.local b/Makefile.local
index c274f07..bb2381d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -261,6 +261,7 @@ notmuch_client_srcs = \
notmuch-config.c\
notmuch-count.c \
notmuch-dump.c \
+ notmuch-insert.c\
notmuch-new.c \
notmuch-reply.c \
notmuch-restore.c \
diff --git a/notmuch-client.h b/notmuch-client.h
index 5f28836..af7d094 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -175,6 +175,9 @@ int
notmuch_dump_command (void *ctx, int argc, char *argv[]);
int
+notmuch_insert_command (void *ctx, int argc, char *argv[]);
+
+int
notmuch_new_command (void *ctx, int argc, char *argv[]);
int
diff --git a/notmuch-insert.c b/notmuch-insert.c
new file mode 100644
index 000..c1d359c
--- /dev/null
+++ b/notmuch-insert.c
@@ -0,0 +1,43 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2013 Peter Wang
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Peter Wang
+ */
+
+#include "notmuch-client.h"
+
+int
+notmuch_insert_command (void *ctx, int argc, char *argv[])
+{
+notmuch_config_t *config;
+notmuch_database_t *notmuch;
+const char *db_path;
+
+config = notmuch_config_open (ctx, NULL, NULL);
+if (config == NULL)
+ return 1;
+
+db_path = notmuch_config_get_database_path (config);
+
+if (notmuch_database_open (notmuch_config_get_database_path (config),
+ NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much))
+ return 1;
+
+notmuch_database_destroy (notmuch);
+
+return 1;
+}
diff --git a/notmuch.c b/notmuch.c
index 4fc0973..1c3b893 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -53,6 +53,9 @@ static command_t commands[] = {
{ "new", notmuch_new_command,
"[options...]",
"Find and import new messages to the notmuch database." },
+{ "insert", notmuch_insert_command,
+ "[options...] [--] [+|- ...] < message",
+ "Add a new message into the maildir and notmuch database." },
{ "search", notmuch_search_command,
"[options...] [...]",
"Search for messages matching the given search terms." },
--
1.7.12.1
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH v3 01/20] cli: add stub for insert command
The notmuch insert command should read a message from standard input
and deliver it to a Maildir folder, and then incorporate the message
into the notmuch database. Essentially it moves the functionality of
notmuch-deliver into notmuch.
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.
---
Makefile.local | 1 +
notmuch-client.h | 3 +++
notmuch-insert.c | 43 +++
notmuch.c| 3 +++
4 files changed, 50 insertions(+)
create mode 100644 notmuch-insert.c
diff --git a/Makefile.local b/Makefile.local
index c274f07..bb2381d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -261,6 +261,7 @@ notmuch_client_srcs = \
notmuch-config.c\
notmuch-count.c \
notmuch-dump.c \
+ notmuch-insert.c\
notmuch-new.c \
notmuch-reply.c \
notmuch-restore.c \
diff --git a/notmuch-client.h b/notmuch-client.h
index 5f28836..af7d094 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -175,6 +175,9 @@ int
notmuch_dump_command (void *ctx, int argc, char *argv[]);
int
+notmuch_insert_command (void *ctx, int argc, char *argv[]);
+
+int
notmuch_new_command (void *ctx, int argc, char *argv[]);
int
diff --git a/notmuch-insert.c b/notmuch-insert.c
new file mode 100644
index 000..c1d359c
--- /dev/null
+++ b/notmuch-insert.c
@@ -0,0 +1,43 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright ? 2013 Peter Wang
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Peter Wang
+ */
+
+#include "notmuch-client.h"
+
+int
+notmuch_insert_command (void *ctx, int argc, char *argv[])
+{
+notmuch_config_t *config;
+notmuch_database_t *notmuch;
+const char *db_path;
+
+config = notmuch_config_open (ctx, NULL, NULL);
+if (config == NULL)
+ return 1;
+
+db_path = notmuch_config_get_database_path (config);
+
+if (notmuch_database_open (notmuch_config_get_database_path (config),
+ NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much))
+ return 1;
+
+notmuch_database_destroy (notmuch);
+
+return 1;
+}
diff --git a/notmuch.c b/notmuch.c
index 4fc0973..1c3b893 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -53,6 +53,9 @@ static command_t commands[] = {
{ "new", notmuch_new_command,
"[options...]",
"Find and import new messages to the notmuch database." },
+{ "insert", notmuch_insert_command,
+ "[options...] [--] [+|- ...] < message",
+ "Add a new message into the maildir and notmuch database." },
{ "search", notmuch_search_command,
"[options...] [...]",
"Search for messages matching the given search terms." },
--
1.7.12.1
[PATCH v3 01/20] cli: add stub for insert command
Hi Peter -
On Sun, 20 Jan 2013, Peter Wang wrote:
> The notmuch insert command should read a message from standard input
> and deliver it to a Maildir folder, and then incorporate the message
> into the notmuch database. Essentially it moves the functionality of
> notmuch-deliver into notmuch.
Bikeshedding first, I'd prefer "notmuch deliver" for the command too...
> 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.
> ---
> Makefile.local |1 +
> notmuch-client.h |3 +
> notmuch-insert.c | 316
> ++
> notmuch.c|3 +
> 4 files changed, 323 insertions(+)
> create mode 100644 notmuch-insert.c
>
> diff --git a/Makefile.local b/Makefile.local
> index c274f07..bb2381d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -261,6 +261,7 @@ notmuch_client_srcs = \
> notmuch-config.c\
> notmuch-count.c \
> notmuch-dump.c \
> + notmuch-insert.c\
> notmuch-new.c \
> notmuch-reply.c \
> notmuch-restore.c \
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 5f28836..af7d094 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -175,6 +175,9 @@ int
> notmuch_dump_command (void *ctx, int argc, char *argv[]);
>
> int
> +notmuch_insert_command (void *ctx, int argc, char *argv[]);
> +
> +int
> notmuch_new_command (void *ctx, int argc, char *argv[]);
>
> int
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> new file mode 100644
> index 000..0e74be0
> --- /dev/null
> +++ b/notmuch-insert.c
> @@ -0,0 +1,316 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright ? 2013 Peter Wang
> + *
> + * Based in part on notmuch-deliver
> + * Copyright ? 2010 Ali Polatel
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Peter Wang
> + */
> +
> +#include "notmuch-client.h"
> +#include "tag-util.h"
> +
> +#include
> +#include
> +#include
> +
> +static volatile sig_atomic_t interrupted;
> +
> +static void
> +handle_sigint (unused (int sig))
> +{
> +static char msg[] = "Stopping... \n";
> +
> +/* This write is "opportunistic", so it's okay to ignore the
> + * result. It is not required for correctness, and if it does
> + * fail or produce a short write, we want to get out of the signal
> + * handler as quickly as possible, not retry it. */
> +IGNORE_RESULT (write (2, msg, sizeof (msg) - 1));
> +interrupted = 1;
> +}
> +
> +/* Like gethostname but guarantees that a null-terminated hostname is
> + * returned, even if it has to make one up.
> + * Returns true unless hostname contains a slash. */
> +static notmuch_bool_t
> +safe_gethostname (char *hostname, size_t len)
> +{
> +if (gethostname (hostname, len) == -1) {
> + strncpy (hostname, "unknown", len);
> +}
> +hostname[len - 1] = '\0';
> +
> +return (strchr (hostname, '/') == NULL);
You could just replace all chars you don't accept with something you
do. Add ':' to the list of unacceptable chars.
> +}
> +
> +/* Open a unique file in the Maildir 'tmp' directory.
> + * Returns the file descriptor on success, or -1 on failure.
> + * On success, file paths for the message in the 'tmp' and 'new'
> + * directories are returned via tmppath and newpath. */
> +static int
> +maildir_open_tmp_file (void *ctx, const char *dir,
> +char **tmppath, char **newpath)
> +{
> +pid_t pid;
> +char host
[PATCH v3 01/20] cli: add stub for insert command
On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula 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
[PATCH v3 01/20] cli: add stub for insert command
On Wed, Jan 23 2013, Peter Wang wrote: > On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula wrote: > >> > +/* 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. Was that me or some other Tomi ;)... Nevertheless the function here called 'copy_stdin' copies fdin to fdout, whatever the file descriptors are, it doesn't enforce fdin to be fd 0 (default stdin). I.e. the function name should be more generic (or then drop fdin argument -- should better name then be 'copy_from_stdin' or copy_stdin_to.. or copy_[from_]stdin_to_some_magic_fd -- ok, no more bikeshedding now >;)). > Peter Tomi
[PATCH v3 01/20] cli: add stub for insert command
On Wed, 23 Jan 2013, Peter Wang wrote:
> On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula 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.
Re: [PATCH v3 01/20] cli: add stub for insert command
Hi Peter -
On Sun, 20 Jan 2013, Peter Wang wrote:
> The notmuch insert command should read a message from standard input
> and deliver it to a Maildir folder, and then incorporate the message
> into the notmuch database. Essentially it moves the functionality of
> notmuch-deliver into notmuch.
Bikeshedding first, I'd prefer "notmuch deliver" for the command too...
> 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.
> ---
> Makefile.local |1 +
> notmuch-client.h |3 +
> notmuch-insert.c | 316
> ++
> notmuch.c|3 +
> 4 files changed, 323 insertions(+)
> create mode 100644 notmuch-insert.c
>
> diff --git a/Makefile.local b/Makefile.local
> index c274f07..bb2381d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -261,6 +261,7 @@ notmuch_client_srcs = \
> notmuch-config.c\
> notmuch-count.c \
> notmuch-dump.c \
> + notmuch-insert.c\
> notmuch-new.c \
> notmuch-reply.c \
> notmuch-restore.c \
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 5f28836..af7d094 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -175,6 +175,9 @@ int
> notmuch_dump_command (void *ctx, int argc, char *argv[]);
>
> int
> +notmuch_insert_command (void *ctx, int argc, char *argv[]);
> +
> +int
> notmuch_new_command (void *ctx, int argc, char *argv[]);
>
> int
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> new file mode 100644
> index 000..0e74be0
> --- /dev/null
> +++ b/notmuch-insert.c
> @@ -0,0 +1,316 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright © 2013 Peter Wang
> + *
> + * Based in part on notmuch-deliver
> + * Copyright © 2010 Ali Polatel
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Peter Wang
> + */
> +
> +#include "notmuch-client.h"
> +#include "tag-util.h"
> +
> +#include
> +#include
> +#include
> +
> +static volatile sig_atomic_t interrupted;
> +
> +static void
> +handle_sigint (unused (int sig))
> +{
> +static char msg[] = "Stopping... \n";
> +
> +/* This write is "opportunistic", so it's okay to ignore the
> + * result. It is not required for correctness, and if it does
> + * fail or produce a short write, we want to get out of the signal
> + * handler as quickly as possible, not retry it. */
> +IGNORE_RESULT (write (2, msg, sizeof (msg) - 1));
> +interrupted = 1;
> +}
> +
> +/* Like gethostname but guarantees that a null-terminated hostname is
> + * returned, even if it has to make one up.
> + * Returns true unless hostname contains a slash. */
> +static notmuch_bool_t
> +safe_gethostname (char *hostname, size_t len)
> +{
> +if (gethostname (hostname, len) == -1) {
> + strncpy (hostname, "unknown", len);
> +}
> +hostname[len - 1] = '\0';
> +
> +return (strchr (hostname, '/') == NULL);
You could just replace all chars you don't accept with something you
do. Add ':' to the list of unacceptable chars.
> +}
> +
> +/* Open a unique file in the Maildir 'tmp' directory.
> + * Returns the file descriptor on success, or -1 on failure.
> + * On success, file paths for the message in the 'tmp' and 'new'
> + * directories are returned via tmppath and newpath. */
> +static int
> +maildir_open_tmp_file (void *ctx, const char *dir,
> +char **tmppath, char **newpath)
> +{
> +pid_t pid;
> +char host
Re: [PATCH v3 01/20] cli: add stub for insert command
On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula 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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v3 01/20] cli: add stub for insert command
On Wed, Jan 23 2013, Peter Wang wrote: > On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula wrote: > >> > +/* 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. Was that me or some other Tomi ;)... Nevertheless the function here called 'copy_stdin' copies fdin to fdout, whatever the file descriptors are, it doesn't enforce fdin to be fd 0 (default stdin). I.e. the function name should be more generic (or then drop fdin argument -- should better name then be 'copy_from_stdin' or copy_stdin_to.. or copy_[from_]stdin_to_some_magic_fd -- ok, no more bikeshedding now >;)). > Peter Tomi ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v3 01/20] cli: add stub for insert command
On Wed, 23 Jan 2013, Peter Wang wrote:
> On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula 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.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
