-------------------- Start of forwarded message --------------------
From: "Brennan Vincent" <brennan@umanwizard.com>
To: David Bremner <david@tethera.net>
Subject: Re: [PATCH v2] Add --emit-message-id flag to notmuch-insert.
Date: Thu, 26 Sep 2024 16:04:43 -0400

David Bremner <david@tethera.net> writes:

> 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.
I think someone might want to know the ID of a newly inserted message
for a lot of reasons that I can't predict now, not just the specific one
of the emacs function I wanted to write.

But I can make the message more detailed and include the emacs draftify
feature as an example use case.
>
>
>> ---
>>  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?
With --keep, it is not necessarily true that the id does not exist in
the database after a failure, because failures do not just happen during
indexing, but also during maildir flag synchronization.

So with --keep if indexing succeeds but maildir flag sync fails we will
not remove the message from the database, and so we need to print the
message ID.

I'll reword this to hopefully make it less confusing.
>
>> -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).
I don't remember why I did this. I've never used talloc before and
probably just got confused. I'll switch to using notmuch.
>>  
>>    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.
I'll add some tests.
-------------------- End of forwarded message --------------------

_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org