Re: [PATCH v2] Add --emit-message-id flag to notmuch-insert.

[David Bremner <david@tethera.net>]

Brennan Vincent <brennan@umanwizard.com> writes:

First, thanks for sending a contribution to notmuch.

Usually we insist on a bit more verbose (see
https://notmuchmail.org/contributing/#index5h2 for details).

Here it's probably enough to mention the overall goal of the patch
series, that this feature will be used to add a draftify feature, but
maybe you have a more general "why" to discuss.


> ---
>  doc/man1/notmuch-insert.rst |  5 +++++
>  notmuch-insert.c            | 26 +++++++++++++++++++++-----
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
> index e05bd0b5..3848b8f6 100644
> --- a/doc/man1/notmuch-insert.rst
> +++ b/doc/man1/notmuch-insert.rst
> @@ -66,6 +66,11 @@ Supported options for **insert** include
>     umask.  By default, delivered mail is only readable by the current
>     user.
>  
> +.. option:: --emit-message-id
> +
> +   On successful exit (or with ``--keep``),
> +   print the message ID of the newly indexed message.
> +

I'm a bit confused here. If indexing fails with --keep, the
printed id will not exist in the database, right? So how is the user
supposed to detect that kind of failure?

> -add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
> +add_file (const void *ctx, notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,

Since notmuch is already a talloc context, you should address the need
for a new context somewhere (either a comment or the commit message).
>  
>    DONE:
> -    notmuch_message_destroy (message);
> -
>      if (status) {
>  	if (keep) {
>  	    status = NOTMUCH_STATUS_SUCCESS;
> @@ -458,6 +457,15 @@ add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
>  	    }
>  	}
>      }
> +    if (message_id_out) {
> +	if (!status) {
I can't quite explain why I don't mind if (status), but find "if
(!status)" for success a bit jarring/confusing, but an explicit test
against NOTMUCH_STATUS_SUCCESS would be nicer, IMHO.


A more substantial comment is that for new CLI features we need at least one
or two tests. Hopefully test/T070-insert.sh gives you enough clues, but
feel free to ask for help.