From: David Bremner <david@tethera.net>
To: Anton Khirnov <anton@khirnov.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH v5 0/6] indexing attachment contents
In-Reply-To: <20260815140127.26199-2-anton@khirnov.net>
References: <20260815140127.26199-2-anton@khirnov.net>
Date: Sun, 16 Aug 2026 09:29:58 -0300
Message-ID: <87lda6uu2h.fsf@tethera.net>
MIME-Version: 1.0
Message-ID-Hash: OPLSYA7EAN4PR7QL3RM75UX5A7CQHHZF
Precedence: list
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Anton Khirnov <anton@khirnov.net> writes:

> Hi,
> another iteration of the set adding support for indexing attachment
> contents. I believe I've addressed all the review comments from the
> previous round [1].
>
> The first patch now contains a simple performance test that runs reindex
> on all emails with attachments - twice with no filter, and once with a
> trivial /bin/true filter. By placing the test early in the series it is
> easy to see that the following patches do not make the filter-less runs
> slower.

I have applied v5.1 (with your updated patch 4) to master. I realized
while doing my final checks that there were some whitespace
disagreements with uncrustify. Rather than doing another round I just
amended the patches. The difference follows.

diff --git a/lib/index.cc b/lib/index.cc
index b04f7b73..c1c675b0 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -454,8 +454,8 @@ _filter_attachment_communicate (int *child_stdin, int *child_stdout,
 	    /* Use plain realloc(), since the attachment body we are replacing
 	     * needs to be freed with a plain free().
 	     * +1 for the terminating 0 */
-	    tmp = (char *)realloc (filtered, filtered_len + bytes_read + 1);
-	    if (!tmp)
+	    tmp = (char *) realloc (filtered, filtered_len + bytes_read + 1);
+	    if (! tmp)
 		goto FAIL;
 	    filtered = tmp;
 	    memcpy (filtered + filtered_len, out, bytes_read);
@@ -493,7 +493,7 @@ _filter_attachment_communicate (int *child_stdin, int *child_stdout,
     *data_len = filtered_len;
     return;
 
-FAIL:
+  FAIL:
     free (filtered);
     free (*data);
     *data = NULL;
@@ -530,7 +530,7 @@ _filter_attachment (notmuch_message_t *message,
     void (*sigpipe_handler_prev)(int) = NULL;
 
     /* take ownership of input data, so it won't get indexed
-       if filtering fails */
+     * if filtering fails */
     data = *pdata;
     data_len = *pdata_len;
     *pdata = NULL;
@@ -545,7 +545,7 @@ _filter_attachment (notmuch_message_t *message,
     }
 
     /* split the commandline */
-    if (!g_shell_parse_argv (filter, NULL, &cmdline, &err)) {
+    if (! g_shell_parse_argv (filter, NULL, &cmdline, &err)) {
 	_notmuch_database_log (db, "Error splitting the commandline: %s\n",
 			       err->message);
 	g_error_free (err);
@@ -558,41 +558,41 @@ _filter_attachment (notmuch_message_t *message,
 
     local = talloc_new (db);
     /* +2 for MIME_TYPE and MESSAGE_ID
-       maybe +1 for FILENAME, if present
-       +1 for terminating NULL */
-    env = talloc_array (local, char*, num_env + 2 + !!filename + 1);
-    if (!env)
+     * maybe +1 for FILENAME, if present
+     +1 for terminating NULL */
+    env = talloc_array (local, char *, num_env + 2 + ! ! filename + 1);
+    if (! env)
 	goto CLOSE;
     for (num_env = 0; environ[num_env]; num_env++) {
 	env[num_env] = talloc_strdup (local, environ[num_env]);
-	if (!env[num_env])
+	if (! env[num_env])
 	    goto CLOSE;
     }
 
     env[num_env] = talloc_asprintf (env, "NOTMUCH_FILTER_MIME_TYPE=%s", mime_type);
-    if (!env[num_env])
+    if (! env[num_env])
 	goto CLOSE;
     num_env++;
 
     env[num_env] = talloc_asprintf (env, "NOTMUCH_FILTER_MESSAGE_ID=%s", msgid);
-    if (!env[num_env])
+    if (! env[num_env])
 	goto CLOSE;
     num_env++;
 
     if (filename) {
 	env[num_env] = talloc_asprintf (env, "NOTMUCH_FILTER_FILENAME=%s", filename);
-	if (!env[num_env])
+	if (! env[num_env])
 	    goto CLOSE;
 	num_env++;
     }
 
     env[num_env] = NULL;
 
-    if (!g_spawn_async_with_pipes_and_fds (NULL, cmdline, env, G_SPAWN_DO_NOT_REAP_CHILD,
-					   NULL, NULL,
-					   -1, -1, -1, NULL, NULL, 0, &pid,
-					   &pipes[0], &pipes[1],
-					   NULL, &err)) {
+    if (! g_spawn_async_with_pipes_and_fds (NULL, cmdline, env, G_SPAWN_DO_NOT_REAP_CHILD,
+					    NULL, NULL,
+					    -1, -1, -1, NULL, NULL, 0, &pid,
+					    &pipes[0], &pipes[1],
+					    NULL, &err)) {
 	_notmuch_database_log (db, "Error spawning the filter process: %s\n",
 			       err->message);
 	g_error_free (err);
@@ -621,7 +621,7 @@ _filter_attachment (notmuch_message_t *message,
 
     _filter_attachment_communicate (&pipes[0], &pipes[1], &data, &data_len, db);
 
-CLOSE:
+  CLOSE:
     for (int i = 0; i < 2; i++)
 	if (pipes[i] >= 0)
 	    close (pipes[i]);
@@ -789,7 +789,7 @@ _index_mime_part (notmuch_message_t *message,
     filter = g_mime_stream_filter_new (stream);
     content_type = g_mime_object_get_content_type (part);
 
-    if (!attachment_filter) {
+    if (! attachment_filter) {
 	GMimeFilter *discard_non_term_filter;
 
 	discard_non_term_filter = notmuch_filter_discard_non_term_new (content_type);
diff --git a/lib/indexopts.c b/lib/indexopts.c
index 8e497ba8..5fc275cb 100644
--- a/lib/indexopts.c
+++ b/lib/indexopts.c
@@ -65,14 +65,14 @@ notmuch_database_get_default_indexopts (notmuch_database_t *db)
     if (filter_cmd && *filter_cmd) {
 	ret->filter_cmd = talloc_strdup (ret, filter_cmd);
 	free (filter_cmd);
-	if (!ret->filter_cmd)
+	if (! ret->filter_cmd)
 	    goto FAIL;
     } else
 	free (filter_cmd);
 
     return ret;
 
-FAIL:
+  FAIL:
     talloc_free (ret);
     return NULL;
 }
@@ -101,7 +101,7 @@ notmuch_indexopts_set_filter (notmuch_indexopts_t *indexopts,
 {
     talloc_free (indexopts->filter_cmd);
     indexopts->filter_cmd = talloc_strdup (indexopts, filter_cmd);
-    if (!indexopts->filter_cmd)
+    if (! indexopts->filter_cmd)
 	return NOTMUCH_STATUS_OUT_OF_MEMORY;
     return NOTMUCH_STATUS_SUCCESS;
 }
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org