Re: [PATCH v3 1/2] cli/show: add --format=pretty

[Hannu Hartikainen]

Thanks for reviewing the patch!

On Tue, 13 Jul 2021 07:24:43 -0300, David Bremner  wrote:
> David Bremner  writes:
> > I don't know what g_mime_message_get_message_id will return if there is
> > no message-id, but that case can and does arise. 

It returns null and the printing function prints out `(null)` IIRC so
it's not a crash (I checked that), but good point nonetheless.

> I expect that would require somehow making the notmuch_database_t object
> available inside the sprinter. We want this for at least one other
> feature request (configurable headers in json / sexpr output), so it's
> not as much of a "waste" of effort as it might seem at first.

That's good to know. I was afraid of making big changes for no reason. I
think I had a version that added a function parameter but thought it's
too ugly and complicated to submit.

> The options are adding extra arguments to functions and stashing a
> copy of the pointer in some struct (perhaps the sprinter struct).

I'll see if I find a good way to do the latter when I have time. Thanks
for the pointers!

Hannu
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: [PATCH v3 1/2] cli/show: add --format=pretty

[David Bremner]

David Bremner  writes:

>
> I don't know what g_mime_message_get_message_id will return if there is
> no message-id, but that case can and does arise. Notmuch constructs a
> message-id by sha1 hashing the message. That message-id is in the
> database, so I think you should probably print out the message-id from
> the database.
>

I expect that would require somehow making the notmuch_database_t object
available inside the sprinter. We want this for at least one other
feature request (configurable headers in json / sexpr output), so it's
not as much of a "waste" of effort as it might seem at first.  The
options are adding extra arguments to functions and stashing a copy of
the pointer in some struct (perhaps the sprinter struct). I think David
E has a prototype of the former. My own instincts is that the latter
will be more general.  The previous consensus was that we did not want
to add static (file scope) variables, even though that would be the
quick and dirty solution.

d
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: [PATCH v3 1/2] cli/show: add --format=pretty

[David Bremner]

Hannu Hartikainen  writes:


> +static notmuch_status_t
> +format_part_pretty (const void *ctx, sprinter_t *sp, mime_node_t *node,
> + int indent, const notmuch_show_params_t *params)
> +{
> +/* The disposition and content-type metadata are associated with
> + * the envelope for message parts */
> +GMimeObject *meta = node->envelope_part ? (
> + GMIME_OBJECT (node->envelope_part) ) : node->part;
> +GMimeContentType *content_type = g_mime_object_get_content_type (meta);
> +GMimeStream *stream = params->out_stream;
> +int i;
> +
> +if (GMIME_IS_MESSAGE (node->part)) {
> + GMimeMessage *message = GMIME_MESSAGE (node->part);
> + char *recipients_string;
> + char *date_string;
> +
> + g_mime_stream_printf (stream, "Subject: %s\n", 
> g_mime_message_get_subject (message));
> + g_mime_stream_printf (stream, "From: %s\n", 
> g_mime_message_get_from_string (message));
> + recipients_string = g_mime_message_get_address_string (message, 
> GMIME_ADDRESS_TYPE_TO);
> + if (recipients_string)
> + g_mime_stream_printf (stream, "To: %s\n", recipients_string);
> + g_free (recipients_string);
> + recipients_string = g_mime_message_get_address_string (message, 
> GMIME_ADDRESS_TYPE_CC);
> + if (recipients_string)
> + g_mime_stream_printf (stream, "Cc: %s\n", recipients_string);
> + g_free (recipients_string);
> + date_string = g_mime_message_get_date_string (node, message);
> + g_mime_stream_printf (stream, "Date: %s\n", date_string);
> + g_mime_stream_printf (stream, "Message-ID: <%s>\n\n", 
> g_mime_message_get_message_id (
> +   message));

I don't know what g_mime_message_get_message_id will return if there is
no message-id, but that case can and does arise. Notmuch constructs a
message-id by sha1 hashing the message. That message-id is in the
database, so I think you should probably print out the message-id from
the database.

> +static const notmuch_show_format_t format_pretty = {
> +.new_sprinter = sprinter_text_create,
> +.part = format_part_pretty,
> +};

I think this deserves a comment to explain briefly why it works.

> diff --git a/test/T520-show.sh b/test/T520-show.sh
> index 6f42ca12..c9bcf81d 100755
> --- a/test/T520-show.sh
> +++ b/test/T520-show.sh
> @@ -27,4 +27,36 @@ notmuch show --entire-thread=true --sort=newest-first 
> $QUERY > EXPECTED
>  notmuch show --entire-thread=true --sort=oldest-first $QUERY > OUTPUT
>  test_expect_equal_file EXPECTED OUTPUT
>  
> +test_begin_subtest "--format=pretty"
> +output=$(notmuch show --format=pretty 
> id:cf0c4d610911171136h1713aa59w9cf9aa31f052a...@mail.gmail.com 2>&1 && echo 
> OK)
> +test_expect_equal "$output" "Subject: [notmuch] preliminary FreeBSD support
> +From: Alex Botero-Lowry 
> +To: notmuch@notmuchmail.org
> +Date: Tue, 17 Nov 2009 11:36:14 -0800
> +Message-ID: 
> +

I would prefer the
cat < EXPECTED
...
EOF

style here rather then the multiline strings, finishing with
"test_expect_equal_file EXPECTED OUTPUT".

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org