[PATCH] Fix format_headers_sprinter to return all headers

[PATCH] Fix format_headers_sprinter to return all headers

[Johan Parin]

Modify format_headers_sprinter so that it returns all headers in the
sexp, instead of a fixed set of headers.

This is required in order for the elisp variable
`notmuch-message-headers' to work.

See this bug report:

  https://notmuchmail.org/pipermail/notmuch/2017/026069.html
---
 notmuch-show.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/notmuch-show.c b/notmuch-show.c
index 21792a57..9652ed09 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -205,6 +205,15 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
     char *recipients_string;
     const char *reply_to_string;
     void *local = talloc_new (sp);
+    GMimeHeaderList *header_list;
+    GMimeHeader *header;
+    int header_count;
+    /* Headers requiring some special treatment in formatting */
+    const char* special_headers[] = {"Subject", "From", "To", "Cc", "Bcc",
+				     "Reply-To", "In-reply-to", "References",
+				     "Date"};
+    const int special_header_count = sizeof(special_headers) / sizeof(char *);
+    bool special;
 
     sp->begin_map (sp);
 
@@ -255,6 +264,24 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
 	sp->string (sp, g_mime_message_get_date_string (sp, message));
     }
 
+    header_list  = g_mime_object_get_header_list (GMIME_OBJECT(message));
+    header_count = g_mime_header_list_get_count(header_list);
+
+    for (int i = 0; i < header_count; i++) {
+        header = g_mime_header_list_get_header_at(header_list, i);
+	special = false;
+	for (int k = 0; k < special_header_count; k++)
+	    if (!STRNCMP_LITERAL(g_mime_header_get_name(header),
+				 special_headers[k])) {
+		special = true;
+		break;
+	    }
+	if (special)
+	    continue;
+	sp->map_key (sp, g_mime_header_get_name(header));
+	sp->string (sp, g_mime_header_get_value(header));
+    }
+
     sp->end (sp);
     talloc_free (local);
 }
-- 
2.21.0 (Apple Git-122)

Re: [PATCH] Fix format_headers_sprinter to return all headers

[Jani Nikula]

On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
> Modify format_headers_sprinter so that it returns all headers in the
> sexp, instead of a fixed set of headers.

I have to deal with plenty of long threads that already take a very long
time to open in notmuch-emacs. How's this going to impact the emacs
interface performance? For example the patch mail I'm replying to has
more header content than body content. There are tons of headers that I
can't imagine being useful to anyone.

I'm wondering if the right thing to do would be to make it possible to
specify which additional headers to include in notmuch-show output. This
would be based on notmuch-message-headers. We already have options to
include/exclude body content and html parts for much the same
reasons. (Though the command-line arguments for those are incoherent at
best.)

It is, of course, much more work than the patch at hand.

BR,
Jani.


>
> This is required in order for the elisp variable
>`notmuch-message-headers' to work.
>
> See this bug report:
>
>   https://notmuchmail.org/pipermail/notmuch/2017/026069.html
> ---
>  notmuch-show.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 21792a57..9652ed09 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -205,6 +205,15 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
>      char *recipients_string;
>      const char *reply_to_string;
>      void *local = talloc_new (sp);
> +    GMimeHeaderList *header_list;
> +    GMimeHeader *header;
> +    int header_count;
> +    /* Headers requiring some special treatment in formatting */
> +    const char* special_headers[] = {"Subject", "From", "To", "Cc", "Bcc",
> +				     "Reply-To", "In-reply-to", "References",
> +				     "Date"};
> +    const int special_header_count = sizeof(special_headers) / sizeof(char *);
> +    bool special;
>  
>      sp->begin_map (sp);
>  
> @@ -255,6 +264,24 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
>  	sp->string (sp, g_mime_message_get_date_string (sp, message));
>      }
>  
> +    header_list  = g_mime_object_get_header_list (GMIME_OBJECT(message));
> +    header_count = g_mime_header_list_get_count(header_list);
> +
> +    for (int i = 0; i < header_count; i++) {
> +        header = g_mime_header_list_get_header_at(header_list, i);
> +	special = false;
> +	for (int k = 0; k < special_header_count; k++)
> +	    if (!STRNCMP_LITERAL(g_mime_header_get_name(header),
> +				 special_headers[k])) {
> +		special = true;
> +		break;
> +	    }
> +	if (special)
> +	    continue;
> +	sp->map_key (sp, g_mime_header_get_name(header));
> +	sp->string (sp, g_mime_header_get_value(header));
> +    }
> +
>      sp->end (sp);
>      talloc_free (local);
>  }
> -- 
> 2.21.0 (Apple Git-122)
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Fix format_headers_sprinter to return all headers

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
>> Modify format_headers_sprinter so that it returns all headers in the
>> sexp, instead of a fixed set of headers.
>
> I have to deal with plenty of long threads that already take a very long
> time to open in notmuch-emacs. How's this going to impact the emacs
> interface performance? For example the patch mail I'm replying to has
> more header content than body content. There are tons of headers that I
> can't imagine being useful to anyone.

 Can we benchmark this somehow? I think we have suitable threads in the
 LKML corpus, but the challenge is timing emacs in some sensible way.

Re: [PATCH] Fix format_headers_sprinter to return all headers

[Jani Nikula]

On Sat, 09 Nov 2019, David Bremner <david@tethera.net> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
>>> Modify format_headers_sprinter so that it returns all headers in the
>>> sexp, instead of a fixed set of headers.
>>
>> I have to deal with plenty of long threads that already take a very long
>> time to open in notmuch-emacs. How's this going to impact the emacs
>> interface performance? For example the patch mail I'm replying to has
>> more header content than body content. There are tons of headers that I
>> can't imagine being useful to anyone.
>
>  Can we benchmark this somehow? I think we have suitable threads in the
>  LKML corpus, but the challenge is timing emacs in some sensible way.

notmuch-emacs performance tests? :o

I can of course try this stuff to get a gut feeling, but it's subjective
and doesn't really scale. :)

BR,
Jani.

Re: [PATCH] Fix format_headers_sprinter to return all headers

[Johan Parin]

Jani Nikula <jani@nikula.org> writes:

> On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
>> Modify format_headers_sprinter so that it returns all headers in the
>> sexp, instead of a fixed set of headers.
>
> I have to deal with plenty of long threads that already take a very
> long time to open in notmuch-emacs. How's this going to impact the
> emacs interface performance? For example the patch mail I'm replying
> to has more header content than body content. There are tons of
> headers that I can't imagine being useful to anyone.
>

How long are these threads may I ask? Would it be possible for you to
apply the patch and see if you see a noticable difference?

I have a hard time seeing that the C code would cause any noticable
performance impact, but perhaps the elisp. But my general understanding
of the code base is low, so I may be wrong.

I guess it's in the following type of code the performance concern lies?

    (mapc (lambda (header)
	    (let* ((header-symbol (intern (concat ":" header)))
		   (header-value (plist-get headers header-symbol)))
	      (if (and header-value
		       (not (string-equal "" header-value)))
		  (notmuch-show-insert-header header header-value))))
	  notmuch-message-headers)

Don't know how efficient plists are in general.

> I'm wondering if the right thing to do would be to make it possible to
> specify which additional headers to include in notmuch-show
> output. This would be based on notmuch-message-headers. We already
> have options to include/exclude body content and html parts for much
> the same reasons. (Though the command-line arguments for those are
> incoherent at best.)
>

This would be possible of course.

Another option I guess is to hard code some extra "interesting"
headers. But what is interesting is subjective and it is very nice to
have the flexibility to control this is in elisp.

mu4e has the customization variable `mu4e-view-fields', but it can only
take on values from a restricted list, namely:

:from :to :cc :subject :flags :date :maildir :mailing-list :tags
:attachments :signature :decryption :user-agent

So a compromise would be to restrict the returned headers to a list like
this. This of course is if we think the potential performance issue is
in the elisp.

Personally I would be content for the moment to be able to see
user-agent and x-mailer. But that's just me.

/Johan

Re: [PATCH] Fix format_headers_sprinter to return all headers

[Jani Nikula]

On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
> Jani Nikula <jani@nikula.org> writes:
>
>> On Sat, 09 Nov 2019, Johan Parin <johanparin@gmail.com> wrote:
>>> Modify format_headers_sprinter so that it returns all headers in the
>>> sexp, instead of a fixed set of headers.
>>
>> I have to deal with plenty of long threads that already take a very
>> long time to open in notmuch-emacs. How's this going to impact the
>> emacs interface performance? For example the patch mail I'm replying
>> to has more header content than body content. There are tons of
>> headers that I can't imagine being useful to anyone.
>>
>
> How long are these threads may I ask? Would it be possible for you to
> apply the patch and see if you see a noticable difference?

I'll need to try this with my work email which is the bigger problem.

> I have a hard time seeing that the C code would cause any noticable
> performance impact, but perhaps the elisp. But my general understanding
> of the code base is low, so I may be wrong.

It's indeed the lisp part that I'm worried about.

> I guess it's in the following type of code the performance concern lies?
>
>     (mapc (lambda (header)
> 	    (let* ((header-symbol (intern (concat ":" header)))
> 		   (header-value (plist-get headers header-symbol)))
> 	      (if (and header-value
> 		       (not (string-equal "" header-value)))
> 		  (notmuch-show-insert-header header header-value))))
> 	  notmuch-message-headers)
>
> Don't know how efficient plists are in general.
>
>> I'm wondering if the right thing to do would be to make it possible to
>> specify which additional headers to include in notmuch-show
>> output. This would be based on notmuch-message-headers. We already
>> have options to include/exclude body content and html parts for much
>> the same reasons. (Though the command-line arguments for those are
>> incoherent at best.)
>>
>
> This would be possible of course.
>
> Another option I guess is to hard code some extra "interesting"
> headers. But what is interesting is subjective and it is very nice to
> have the flexibility to control this is in elisp.
>
> mu4e has the customization variable `mu4e-view-fields', but it can only
> take on values from a restricted list, namely:
>
> :from :to :cc :subject :flags :date :maildir :mailing-list :tags
> :attachments :signature :decryption :user-agent
>
> So a compromise would be to restrict the returned headers to a list like
> this. This of course is if we think the potential performance issue is
> in the elisp.
>
> Personally I would be content for the moment to be able to see
> user-agent and x-mailer. But that's just me.

I failed to mention that I do think the feature itself would be useful
and welcome. Currently I just hit V and find the header in the raw
message...

BR,
Jani.