After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773787 - Replace CamelHeaderRaw by CamelNameValueArray
Replace CamelHeaderRaw by CamelNameValueArray
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Mailer
3.23.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-mail-maintainers
Evolution QA team
Depends on:
Blocks: 764065
 
 
Reported: 2016-11-01 18:52 UTC by Corentin Noël
Modified: 2016-11-04 12:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Replace CamelHeaderRaw by CamelNameValueArray (82.11 KB, patch)
2016-11-01 18:52 UTC, Corentin Noël
none Details | Review
Replace CamelHeaderRaw by CamelNameValueArray (80.17 KB, patch)
2016-11-01 21:35 UTC, Corentin Noël
none Details | Review
Replace CamelHeaderRaw by CamelNameValueArray (79.30 KB, patch)
2016-11-01 23:17 UTC, Corentin Noël
needs-work Details | Review

Description Corentin Noël 2016-11-01 18:52:51 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…
Comment 1 Milan Crha 2016-11-01 18:59:48 UTC
(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
Comment 2 Corentin Noël 2016-11-01 21:35:33 UTC
Created attachment 338909 [details] [review]
Replace CamelHeaderRaw by CamelNameValueArray
Comment 3 Corentin Noël 2016-11-01 21:36:29 UTC
Here is a new patch, I found a solution for the goffset as we only need the offset for the X-Camel header
Comment 4 Corentin Noël 2016-11-01 23:17:04 UTC
Created attachment 338915 [details] [review]
Replace CamelHeaderRaw by CamelNameValueArray

This is the latest patch (with an internal CamelHeaderRaw to get the offset)
Comment 5 Milan Crha 2016-11-02 14:24:20 UTC
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'
Comment 6 Milan Crha 2016-11-04 12:19:01 UTC
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