Re: [PATCH] Display extra headers for emacs-mua - db config option

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

On Sat 2019-11-16 17:27:23 +0100, 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.
>
> The extra header list is configured by the database config option
> `show.extra_headers'.
>
> This is required in order for the elisp variable
> `notmuch-message-headers' to work.

Thanks for this work, Johan, and for your persistence on this
functionality.

> diff --git a/notmuch-show.c b/notmuch-show.c
> index 21792a57..4c77468f 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -18,11 +18,16 @@
>   * Author: Carl Worth <cworth@cworth.org>
>   */
>  
> +#include <string.h>
> +
>  #include "notmuch-client.h"
>  #include "gmime-filter-reply.h"
>  #include "sprinter.h"
>  #include "zlib-extra.h"
>  
> +static notmuch_database_t *notmuch = NULL;
> +
> +
>  static const char *
>  _get_tags_as_string (const void *ctx, notmuch_message_t *message)
>  {
> @@ -195,6 +200,38 @@ _is_from_line (const char *line)
>  	return 0;
>  }
>  
> +/* Output extra headers if configured with the `show.extra_headers'
> + * database configuration option
> + */
> +void
> +format_extra_headers_sprinter (sprinter_t *sp, GMimeMessage *message)
> +{
> +    GMimeHeaderList *header_list;
> +    GMimeHeader *header;
> +    char *extra_headers, *tofree, *header_name;
> +
> +    if (notmuch == NULL)
> +	return;
> +
> +    if (notmuch_database_get_config (notmuch, "show.extra_headers",
> +				     &extra_headers) != NOTMUCH_STATUS_SUCCESS)
> +	return;
> +
> +    header_list  = g_mime_object_get_header_list (GMIME_OBJECT(message));
> +
> +    tofree = extra_headers;
> +    while ( (header_name = strsep(&extra_headers, ";")) != NULL) {
> +
> +	header = g_mime_header_list_get_header (header_list, header_name);
> +	if (header == NULL)
> +	    continue;
> +
> +	sp->map_key (sp, g_mime_header_get_name(header));
> +	sp->string (sp, g_mime_header_get_value(header));
> +    }
> +    free (tofree);
> +}
> +
>  void
>  format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
>  			 bool reply, const _notmuch_message_crypto_t *msg_crypto)
> @@ -253,6 +290,9 @@ format_headers_sprinter (sprinter_t *sp, GMimeMessage *message,
>      } else {
>  	sp->map_key (sp, "Date");
>  	sp->string (sp, g_mime_message_get_date_string (sp, message));
> +
> +	/* Output extra headers the user has configured in the database, if any */
> +	format_extra_headers_sprinter (sp, message);
>      }
>  
>      sp->end (sp);
> @@ -1152,7 +1192,6 @@ static const notmuch_show_format_t *formatters[] = {
>  int
>  notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
>  {
> -    notmuch_database_t *notmuch;
>      notmuch_query_t *query;
>      char *query_string;
>      int opt_index, ret;

I'm a little weirded out by the move to a static notmuch_database_t
*notmuch object.  Are we doing this because we don't want to pass around
the database to internal functions?  I know that the scope of
nomtuch-show.c is basically "global scope", but i worry that it makes
the code more difficult to read and maintain.

It's also not a common idiom in the rest of the codebase (at least not
one that i've seen).

Is it that much worse to pass around the notmuch_database_t *?

     --dkg