Re: [PATCH] Fix format_headers_sprinter to return all headers (v2)

[Jani Nikula <jani@nikula.org>]

On Sun, 10 Nov 2019, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Subject line not updated
>
> On Sat, Nov 09 2019, Johan Parin wrote:
>
>> So this version only returns a fixed limited set of extra
>> headers. Hopefully this eliminates any concern for a performance
>> penalty. Of course it limits the usefulness of the
>> notmuch-message-headers variable.
>
> headers to request based on command line parameter (and default to current
> set) could be an option (kinda Jani already suggested). Quick (not perhaps
> well thought) one could be:
>
>    --message-headers=To,Cc,X-Face

Yes, something like this is what I had in mind.

But this needs to be carefully thought out. Obviously we need a default
set. Is it possible to remove headers from the default set? Does
--message-headers=Foo override the default set too? Should we force the
inclusion of certain "fundamental" headers? Etc. As I said, the
notmuch-show interface is a bit confusing already, better not make it
more confusing.

> and in the other mail -- mostly style comments (for future reference)
>
> On Sat, Nov 09 2019, Johan Parin wrote:
>
>
>> Modify format_headers_sprinter so that it returns some additional headers in the
>> sexp, instead of a small fixed set of headers.
>>
>> This version includes the following headers:
>>
>> - Maildir
>> - Mailing-list
>> - Tags
>> - Attachments
>> - Signature
>> - Decryption
>> - User-agent
>> - X-Mailer
>>
>> This is required in order for the elisp variable
>> `notmuch-message-headers' to work to some extent.
>>
>> See this bug report:
>>
>>   https://notmuchmail.org/pipermail/notmuch/2017/026069.html
>> ---
>>  notmuch-show.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/notmuch-show.c b/notmuch-show.c
>> index 21792a57..86ddb491 100644
>> --- a/notmuch-show.c
>> +++ b/notmuch-show.c
>> @@ -205,6 +205,12 @@ 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;
>
> could be dropped here and had only in scope where it is used below...
>
>> +    const char* interesting_headers[] = {
>> +	"Maildir", "Mailing-list", "Tags", "Attachments", "Signature",
>> +	"Decryption", "User-agent", "X-Mailer"};
>> +    const int interesting_header_count = sizeof(interesting_headers) / sizeof(char *);
>
> more "idiomatic" could be ... sizeof (interesting_headers) / sizeof (interesting_headers[0]);

Yes, but please use ARRAY_SIZE() instead. ;)

BR,
Jani.

>
>>      sp->begin_map (sp);
>>  
>> @@ -255,6 +261,19 @@ 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));
>
> two (2) spaces, then no space after GMIME_OBJECT
>
>> +
>> +    for (int i = 0; i < interesting_header_count; i++) {
>> +
>
> ... here GMimeHeader *header; is only used
>
>> +	header = g_mime_header_list_get_header(
>> +	    header_list, interesting_headers[i]);
>> +	if (header == NULL)
>> +	    continue;
>> +
>> +	sp->map_key (sp, g_mime_header_get_name(header));
>> +	sp->string (sp, g_mime_header_get_value(header));
>
> spaces before '('
>
>> +    }
>> +
>>      sp->end (sp);
>>      talloc_free (local);
>>  }
>> -- 
>> 2.21.0 (Apple Git-122)
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> https://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch