GNOME Bugzilla – Bug 773787
Replace CamelHeaderRaw by CamelNameValueArray
Last modified: 2016-11-04 12:19:14 UTC
Created attachment 338906 [details] [review] Replace CamelHeaderRaw by CamelNameValueArray Here is the patch replacing CamelHeaderRaw by CamelNameValueArray There are two functions on which I'm unsure I've it ported correctly: * mbox_summary_sync_quick : It seems to use the offset property of CamelHeaderRaw, I need to look deeper into it * header_append_mempool : It seems that CamelNameValueArray isn't made to work with the Mempool…
(In reply to Corentin Noël from comment #0) > * mbox_summary_sync_quick : It seems to use the offset property of > CamelHeaderRaw, I need to look deeper into it Right. The current change is not correct. The thing is that the mbox provider saves here only flags, which has enough room in the file already, thus instead of rewriting whole file the file is only updated by seeking into the right 'offset' in it and rewriting only those prefilled bits. An option is to store offsets in a separate structure, can be even CamelMboxMessageInfo. > * header_append_mempool : It seems that CamelNameValueArray isn't made to > work with the Mempool… The mempool is history, it should be avoided whenever possible
Created attachment 338909 [details] [review] Replace CamelHeaderRaw by CamelNameValueArray
Here is a new patch, I found a solution for the goffset as we only need the offset for the X-Camel header
Created attachment 338915 [details] [review] Replace CamelHeaderRaw by CamelNameValueArray This is the latest patch (with an internal CamelHeaderRaw to get the offset)
Review of attachment 338915 [details] [review]: As I wrote on IRC, what about providing also 'const CamelNameValueArray camel_medium_get_headers (CamelMedium *medium);', thus the places which only read from it can avoid the allocation? ::: src/camel/camel-filter-search.c @@ +945,1 @@ + headers = camel_medium_dup_headers (CAMEL_MEDIUM (message)); missing free() call here ::: src/camel/camel-folder-search.c @@ +363,3 @@ } + truth = camel_name_value_array_get_length (headers) > 0; this is not accurate. The trick was that if the 'for' was stopped with non-NULL raw_header, then it meant that there was found a header which satisfied criteria. Easier to add 'header_found = TRUE;' before the above 'break;' call and use it here (eventually 'ii < get_length()'). ::: src/camel/camel-folder-summary.c @@ +2624,1 @@ + value = g_strdup (camel_name_value_array_get_named (h, TRUE, name)); I prefer the previous version, without useless memory allocation. The code only skips leading spaces, instead of allocating memory twice (the second is the return value of the camel_header_unfold()). @@ +2649,1 @@ + value = g_strdup (camel_name_value_array_get_named (h, TRUE, name)); similar note about the allocation as above @@ +2661,3 @@ static CamelMessageInfo * message_info_new_from_header (CamelFolderSummary *summary, + CamelNameValueArray *h) do not be shy and rename it to 'headers' ;) @@ +2967,3 @@ CamelFolderSummaryPrivate *p = summary->priv; CamelContentType *ct; + CamelNameValueArray *header; 'headerS' plural form. @@ +2996,3 @@ } + header = camel_medium_dup_headers (CAMEL_MEDIUM (object)); missing free() @@ +2998,3 @@ + header = camel_medium_dup_headers (CAMEL_MEDIUM (object)); + for (i = 0; camel_name_value_array_get (header, i, &header_name, &header_value); i++) { + gchar *value = g_strdup (header_value); unneeded allocation ::: src/camel/camel-folder-summary.h @@ +264,3 @@ camel_message_info_new_from_header (CamelFolderSummary *summary, + CamelNameValueArray *header); 'headerS' ::: src/camel/camel-message-info.c @@ +2726,3 @@ * + * Returns: (transfer none) (nullable) (element-type guint64): A #GArray of + * guint64 encoded Message-ID-s; or %NULL when none are available. indent by two spaces, as it was before the change. @@ +2759,3 @@ + * Returns: (transfer full) (nullable) (element-type guint64): A #GArray of + * guint64 encoded Message-ID-s; or %NULL when none are available. Free returned + * array with g_array_unref() when no longer needed. indent by two spaces ::: src/camel/camel-mime-message.c @@ +1218,3 @@ camel_mime_message_build_mbox_from (CamelMimeMessage *message) { + CamelNameValueArray *header = NULL; 'headerS' ::: src/camel/camel-mime-parser.c @@ +345,3 @@ * until the next call to parser_step(), or parser_drop_step(). * + * Returns: (transfer full): The headers, or NULL if there are no headers %NULL @@ +346,3 @@ * + * Returns: (transfer full): The headers, or NULL if there are no headers + * defined for the current part or state. free it with camel_name_value_array_free(). . Free it @@ +1933,3 @@ +header_raw_find (CamelHeaderRaw **list, + const gchar *name, + gint *offset) incorrect indentation of the two lines above ::: src/camel/camel-mime-utils.c @@ +4598,3 @@ * TODO: Document me. * + * Returns: (nullable): The mailing list header of %NULL if none are found , or %NULL, if none is found ::: src/camel/camel-mime-utils.h @@ +97,3 @@ CamelHeaderAddress *camel_header_address_new_name (const gchar *name, const gchar *addr); CamelHeaderAddress *camel_header_address_new_group (const gchar *name); +CamelHeaderAddress *camel_header_address_ref (CamelHeaderAddress *h); one-letter variables are hard to search for (can be seen multiple times in the patch). What was wrong with addrlist? @@ +152,3 @@ gchar *camel_header_unfold (const gchar *in); +gchar *camel_header_get_mailing_list (CamelNameValueArray *header); what about "_dup_" instead of "_get_"? ::: src/camel/camel-movemail.c @@ +432,3 @@ static gint solaris_header_write (gint fd, + CamelNameValueArray *header) 'headerS' @@ +512,3 @@ from = g_strdup (camel_mime_parser_from_line (mp)); if (camel_mime_parser_step (mp, &buffer, &len) != CAMEL_MIME_PARSER_STATE_FROM_END) { + CamelNameValueArray *header; 'headerS' ::: src/camel/providers/local/camel-local-summary.c @@ +388,3 @@ gint camel_local_summary_write_headers (gint fd, + CamelNameValueArray *header, 'headerS' ::: src/camel/providers/nntp/camel-nntp-folder.c @@ +406,3 @@ gint ret; + guint u, ii; + CamelNameValueArray *previous_header = NULL; 'previous_headerS' ::: src/camel/providers/sendmail/camel-sendmail-transport.c @@ +113,3 @@ GError **error) { + CamelNameValueArray *previous_header = NULL; 'previous_headerS' ::: src/camel/providers/smtp/camel-smtp-transport.c @@ +1640,3 @@ GError **error) { + CamelNameValueArray *previous_header = NULL; 'previous_headerS'
I finished the patch, and added some more changes along the way, and committed it: eds: wip/camel-more-gobject fc2e5c7 evo: wip/camel-more-gobject 2564e4a ews: wip/camel-more-gobject 690ad2a ema: wip/camel-more-gobject e7734fa and finally also evolution-rspam, whose patch is at bug #764065 comment #22