[RFC 0/5] globbing on new.ignore etc.

[RFC 0/5] globbing on new.ignore etc.

[Jani Nikula]

This series adds globbing support to new.ignore using fnmatch(3). Just
an RFC for starters because it's missing tests and man pages
etc. There's some cleanup patches first, and then 'notmuch config
append' command to make it easier to add new ignores to the list.

BR,
Jani.


Jani Nikula (5):
  cli: fix function name in notmuch new debug logging
  cli: don't call _entry_in_ignore_list twice in count files debug
  cli: support shell globbing patterns in new.ignore
  cli: add notmuch config append command to append to string list
    configs
  cli: print number of ignored non-mails encountered in notmuch new scan

 notmuch-config.c | 47 +++++++++++++++++++++++++++++++++++++
 notmuch-new.c    | 71 ++++++++++++++++++++++++++++++++++++++------------------
 test/T050-new.sh | 40 +++++++++++++++----------------
 3 files changed, 115 insertions(+), 43 deletions(-)

-- 
2.1.4

[RFC 1/5] cli: fix function name in notmuch new debug logging

[Jani Nikula]

add_files_recursive has been renamed add_files long ago.
---
 notmuch-new.c    | 15 +++++++--------
 test/T050-new.sh | 40 ++++++++++++++++++++--------------------
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index d45d0af8642a..e5037761c8ee 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -445,7 +445,7 @@ add_files (notmuch_database_t *notmuch,
 	 */
 	if (_entry_in_ignore_list (entry->d_name, state)) {
 	    if (state->debug)
-		printf ("(D) add_files_recursive, pass 1: explicitly ignoring %s/%s\n",
+		printf ("(D) add_files, pass 1: explicitly ignoring %s/%s\n",
 			path, entry->d_name);
 	    continue;
 	}
@@ -513,9 +513,8 @@ add_files (notmuch_database_t *notmuch,
 	/* Ignore files & directories user has configured to be ignored */
 	if (_entry_in_ignore_list (entry->d_name, state)) {
 	    if (state->debug)
-		printf ("(D) add_files_recursive, pass 2: explicitly ignoring %s/%s\n",
-			path,
-			entry->d_name);
+		printf ("(D) add_files, pass 2: explicitly ignoring %s/%s\n",
+			path, entry->d_name);
 	    continue;
 	}
 
@@ -529,7 +528,7 @@ add_files (notmuch_database_t *notmuch,
 					      notmuch_filenames_get (db_files));
 
 	    if (state->debug)
-		printf ("(D) add_files_recursive, pass 2: queuing passed file %s for deletion from database\n",
+		printf ("(D) add_files, pass 2: queuing passed file %s for deletion from database\n",
 			absolute);
 
 	    _filename_list_add (state->removed_files, absolute);
@@ -547,7 +546,7 @@ add_files (notmuch_database_t *notmuch,
 		char *absolute = talloc_asprintf (state->removed_directories,
 						  "%s/%s", path, filename);
 		if (state->debug)
-		    printf ("(D) add_files_recursive, pass 2: queuing passed directory %s for deletion from database\n",
+		    printf ("(D) add_files, pass 2: queuing passed directory %s for deletion from database\n",
 			absolute);
 
 		_filename_list_add (state->removed_directories, absolute);
@@ -618,7 +617,7 @@ add_files (notmuch_database_t *notmuch,
 					  "%s/%s", path,
 					  notmuch_filenames_get (db_files));
 	if (state->debug)
-	    printf ("(D) add_files_recursive, pass 3: queuing leftover file %s for deletion from database\n",
+	    printf ("(D) add_files, pass 3: queuing leftover file %s for deletion from database\n",
 		    absolute);
 
 	_filename_list_add (state->removed_files, absolute);
@@ -633,7 +632,7 @@ add_files (notmuch_database_t *notmuch,
 					  notmuch_filenames_get (db_subdirs));
 
 	if (state->debug)
-	    printf ("(D) add_files_recursive, pass 3: queuing leftover directory %s for deletion from database\n",
+	    printf ("(D) add_files, pass 3: queuing leftover directory %s for deletion from database\n",
 		    absolute);
 
 	_filename_list_add (state->removed_directories, absolute);
diff --git a/test/T050-new.sh b/test/T050-new.sh
index 81cf2fad10ad..93a6fa94ae7c 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -64,7 +64,7 @@ generate_message
 notmuch new > /dev/null
 mv "$gen_msg_filename" "${gen_msg_filename}"-renamed
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 2: queuing passed file ${gen_msg_filename} for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 2: queuing passed file ${gen_msg_filename} for deletion from database
 No new mail. Detected 1 file rename."
 
 
@@ -72,7 +72,7 @@ test_begin_subtest "Deleted message"
 
 rm "${gen_msg_filename}"-renamed
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 3: queuing leftover file ${gen_msg_filename}-renamed for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 3: queuing leftover file ${gen_msg_filename}-renamed for deletion from database
 No new mail. Removed 1 message."
 
 
@@ -88,7 +88,7 @@ notmuch new > /dev/null
 mv "${MAIL_DIR}"/dir "${MAIL_DIR}"/dir-renamed
 
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 2: queuing passed directory ${MAIL_DIR}/dir for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 2: queuing passed directory ${MAIL_DIR}/dir for deletion from database
 No new mail. Detected 3 file renames."
 
 
@@ -96,7 +96,7 @@ test_begin_subtest "Deleted directory"
 rm -rf "${MAIL_DIR}"/dir-renamed
 
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 2: queuing passed directory ${MAIL_DIR}/dir-renamed for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 2: queuing passed directory ${MAIL_DIR}/dir-renamed for deletion from database
 No new mail. Removed 3 messages."
 
 
@@ -115,7 +115,7 @@ test_begin_subtest "Deleted directory (end of list)"
 rm -rf "${MAIL_DIR}"/zzz
 
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 3: queuing leftover directory ${MAIL_DIR}/zzz for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 3: queuing leftover directory ${MAIL_DIR}/zzz for deletion from database
 No new mail. Removed 3 messages."
 
 
@@ -166,7 +166,7 @@ test_begin_subtest "Deleted two-level directory"
 rm -rf "${MAIL_DIR}"/two
 
 output=$(NOTMUCH_NEW --debug)
-test_expect_equal "$output" "(D) add_files_recursive, pass 3: queuing leftover directory ${MAIL_DIR}/two for deletion from database
+test_expect_equal "$output" "(D) add_files, pass 3: queuing leftover directory ${MAIL_DIR}/two for deletion from database
 No new mail. Removed 3 messages."
 
 test_begin_subtest "Support single-message mbox"
@@ -227,20 +227,20 @@ mkdir -p "${MAIL_DIR}"/one/two/three/.git
 touch "${MAIL_DIR}"/{one,one/two,one/two/three}/ignored_file
 output=$(NOTMUCH_NEW --debug 2>&1 | sort)
 test_expect_equal "$output" \
-"(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/.git
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/.ignored_hidden_file
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/ignored_file
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/one/ignored_file
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
-(D) add_files_recursive, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/.git
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/.ignored_hidden_file
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/ignored_file
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/ignored_file
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
-(D) add_files_recursive, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
+"(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/.git
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/.ignored_hidden_file
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/ignored_file
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/one/ignored_file
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
+(D) add_files, pass 1: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/.git
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/.ignored_hidden_file
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/ignored_file
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/one/ignored_file
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/ignored_file
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/.git
+(D) add_files, pass 2: explicitly ignoring ${MAIL_DIR}/one/two/three/ignored_file
 No new mail."
 
 
-- 
2.1.4

[RFC 2/5] cli: don't call _entry_in_ignore_list twice in count files debug

[Jani Nikula]

Split file ignores in count_files to fixed and user configured in
order to not have to call _entry_in_ignore_list twice when debugging
is enabled. Minor detail.
---
 notmuch-new.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index e5037761c8ee..da1d1be2ed3b 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -734,18 +734,20 @@ count_files (const char *path, int *count, add_files_state_t *state)
         entry = fs_entries[i];
 
 	/* Ignore special directories to avoid infinite recursion.
-	 * Also ignore the .notmuch directory and files/directories
-	 * the user has configured to be ignored.
+	 * Also ignore the .notmuch directory.
 	 */
 	if (strcmp (entry->d_name, ".") == 0 ||
 	    strcmp (entry->d_name, "..") == 0 ||
-	    strcmp (entry->d_name, ".notmuch") == 0 ||
-	    _entry_in_ignore_list (entry->d_name, state))
-	{
-	    if (state->debug && _entry_in_ignore_list (entry->d_name, state))
+	    strcmp (entry->d_name, ".notmuch") == 0)
+	    continue;
+
+	/* Ignore any files/directories the user has configured to be
+	 * ignored
+	 */
+	if (_entry_in_ignore_list (entry->d_name, state)) {
+	    if (state->debug)
 		printf ("(D) count_files: explicitly ignoring %s/%s\n",
-			path,
-			entry->d_name);
+			path, entry->d_name);
 	    continue;
 	}
 
-- 
2.1.4

[RFC 3/5] cli: support shell globbing patterns in new.ignore

[Jani Nikula]

Keep the existing strcmp on the basename for backwards compatibility,
and additionally use the new.ignore entries as fnmatch(3) patterns on
the absolute filename. Note that it's not enough to add e.g. "foo*bar"
to the list; you will need to do "*/foo*bar" to match the path also.
---
 notmuch-new.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index da1d1be2ed3b..d06f9c906fc6 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -22,6 +22,7 @@
 #include "tag-util.h"
 
 #include <unistd.h>
+#include <fnmatch.h>
 
 typedef struct _filename_node {
     char *filename;
@@ -236,15 +237,31 @@ _entries_resemble_maildir (const char *path, struct dirent **entries, int count)
 /* Test if the file/directory is to be ignored.
  */
 static notmuch_bool_t
-_entry_in_ignore_list (const char *entry, add_files_state_t *state)
+_entry_in_ignore_list (const char *path, const char *entry,
+		       add_files_state_t *state)
 {
+    char *abspath;
+    notmuch_bool_t ret = FALSE;
     size_t i;
 
-    for (i = 0; i < state->new_ignore_length; i++)
-	if (strcmp (entry, state->new_ignore[i]) == 0)
-	    return TRUE;
+    if (state->new_ignore_length == 0)
+	return FALSE;
+
+    abspath = talloc_asprintf (NULL, "%s/%s", path, entry);
+    if (! abspath)
+	return FALSE;
 
-    return FALSE;
+    for (i = 0; i < state->new_ignore_length; i++) {
+	if (strcmp (entry, state->new_ignore[i]) == 0 ||
+	    fnmatch (state->new_ignore[i], abspath, 0) == 0) {
+	    ret = TRUE;
+	    break;
+	}
+    }
+
+    talloc_free (abspath);
+
+    return ret;
 }
 
 /* Add a single file to the database. */
@@ -443,7 +460,7 @@ add_files (notmuch_database_t *notmuch,
 	 * and because we don't care if dirent_type fails on entries
 	 * that are explicitly ignored.
 	 */
-	if (_entry_in_ignore_list (entry->d_name, state)) {
+	if (_entry_in_ignore_list (path, entry->d_name, state)) {
 	    if (state->debug)
 		printf ("(D) add_files, pass 1: explicitly ignoring %s/%s\n",
 			path, entry->d_name);
@@ -511,7 +528,7 @@ add_files (notmuch_database_t *notmuch,
         entry = fs_entries[i];
 
 	/* Ignore files & directories user has configured to be ignored */
-	if (_entry_in_ignore_list (entry->d_name, state)) {
+	if (_entry_in_ignore_list (path, entry->d_name, state)) {
 	    if (state->debug)
 		printf ("(D) add_files, pass 2: explicitly ignoring %s/%s\n",
 			path, entry->d_name);
@@ -744,7 +761,7 @@ count_files (const char *path, int *count, add_files_state_t *state)
 	/* Ignore any files/directories the user has configured to be
 	 * ignored
 	 */
-	if (_entry_in_ignore_list (entry->d_name, state)) {
+	if (_entry_in_ignore_list (path, entry->d_name, state)) {
 	    if (state->debug)
 		printf ("(D) count_files: explicitly ignoring %s/%s\n",
 			path, entry->d_name);
-- 
2.1.4

[RFC 4/5] cli: add notmuch config append command to append to string list configs

[Jani Nikula]

Make it easy to add new entries to string list configs. For example,
'notmuch config append new.ignore my-non-mail-file'.
---
 notmuch-config.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/notmuch-config.c b/notmuch-config.c
index d252bb25d4bb..96c73b0cc2ac 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -831,6 +831,46 @@ notmuch_config_command_set (notmuch_config_t *config, char *item, int argc, char
 }
 
 static int
+notmuch_config_command_append (notmuch_config_t *config, char *item,
+			       int argc, char *argv[])
+{
+    char **value, **new_value;
+    size_t i, length, new_length;
+    char *group, *key;
+
+    if (_item_split (item, &group, &key))
+	return 1;
+
+    value = g_key_file_get_string_list (config->key_file, group, key,
+					&length, NULL);
+    if (value == NULL)
+	length = 0;
+
+    new_length = length + argc;
+    new_value = talloc_size (NULL, new_length * sizeof (char *));
+    if (! new_value)
+	return 1;
+
+    for (i = 0; i < new_length; i++) {
+	if (i < length)
+	    new_value[i] = value[i];
+	else
+	    new_value[i] = argv[i - length];
+    }
+
+    if (new_length == 1)
+	g_key_file_set_string (config->key_file, group, key, new_value[0]);
+    else
+	g_key_file_set_string_list (config->key_file, group, key,
+				    (const gchar **) new_value, new_length);
+
+    g_strfreev (value);
+    talloc_free (new_value);
+
+    return notmuch_config_save (config);
+}
+
+static int
 notmuch_config_command_list (notmuch_config_t *config)
 {
     char **groups;
@@ -905,6 +945,13 @@ notmuch_config_command (notmuch_config_t *config, int argc, char *argv[])
 	    return EXIT_FAILURE;
 	}
 	ret = notmuch_config_command_set (config, argv[1], argc - 2, argv + 2);
+    } else if (strcmp (argv[0], "append") == 0) {
+	if (argc < 3) {
+	    fprintf (stderr, "Error: notmuch config append requires at least "
+		     "two arguments.\n");
+	    return EXIT_FAILURE;
+	}
+	ret = notmuch_config_command_append (config, argv[1], argc - 2, argv + 2);
     } else if (strcmp (argv[0], "list") == 0) {
 	ret = notmuch_config_command_list (config);
     } else {
-- 
2.1.4

[RFC 5/5] cli: print number of ignored non-mails encountered in notmuch new scan

[Jani Nikula]

Also advise the user how to ignore them explicitly.
---
 notmuch-new.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/notmuch-new.c b/notmuch-new.c
index d06f9c906fc6..8d942507364a 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -54,6 +54,7 @@ typedef struct {
     int total_files;
     int processed_files;
     int added_messages, removed_messages, renamed_messages;
+    int ignored_non_mails;
     struct timeval tv_start;
 
     _filename_list_t *removed_files;
@@ -295,6 +296,7 @@ add_file (notmuch_database_t *notmuch, const char *filename,
 	    notmuch_message_maildir_flags_to_tags (message);
 	break;
     case NOTMUCH_STATUS_FILE_NOT_EMAIL:
+	state->ignored_non_mails++;
 	fprintf (stderr, "Note: Ignoring non-mail file: %s\n", filename);
 	break;
     /* Fatal issues. Don't process anymore. */
@@ -938,6 +940,11 @@ print_results (const add_files_state_t *state)
 	printf (" Detected %d file %s.", state->renamed_messages,
 		state->renamed_messages == 1 ? "rename" : "renames");
 
+    if (state->ignored_non_mails)
+	printf (" Ignored %d non-mail %s. (Use 'notmuch config append new.ignore <pattern> [...]' to ignore explicitly.)",
+		state->ignored_non_mails,
+		state->ignored_non_mails == 1 ? "file" : "files");
+
     printf ("\n");
 }
 
-- 
2.1.4

Re: [RFC 1/5] cli: fix function name in notmuch new debug logging

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> add_files_recursive has been renamed add_files long ago.

pushed,

d

Re: [RFC 2/5] cli: don't call _entry_in_ignore_list twice in count files debug

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Split file ignores in count_files to fixed and user configured in
> order to not have to call _entry_in_ignore_list twice when debugging
> is enabled. Minor detail.

this change (and patch) seems harmless.

d

Re: [RFC 3/5] cli: support shell globbing patterns in new.ignore

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Keep the existing strcmp on the basename for backwards compatibility,
> and additionally use the new.ignore entries as fnmatch(3) patterns on
> the absolute filename. Note that it's not enough to add e.g. "foo*bar"
> to the list; you will need to do "*/foo*bar" to match the path also.

I guess when you say absolute file name, you mean the obvious thing, not
relative to the root of the notmuch maildir. It seems like it would be
quite useful to e.g. ignore directory foo at the top level of the
maildir, but not anywhere else, by specifying /foo as an ignore
pattern. I guess this would just require stripping the maildir root off
the front of the path before doing the fnmatch? That's not currently
part of the add_files_state_t, but it could be.

d

Re: [RFC 3/5] cli: support shell globbing patterns in new.ignore

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Keep the existing strcmp on the basename for backwards compatibility,
> and additionally use the new.ignore entries as fnmatch(3) patterns on
> the absolute filename. Note that it's not enough to add e.g. "foo*bar"
> to the list; you will need to do "*/foo*bar" to match the path also.

Are there (many?) fnmatch patterns that don't match themselves? I'm just
wondering if the extra strcmp is worth-it / needed?

>  
>  typedef struct _filename_node {
>      char *filename;
> @@ -236,15 +237,31 @@ _entries_resemble_maildir (const char *path, struct dirent **entries, int count)
>  /* Test if the file/directory is to be ignored.
>   */
>  static notmuch_bool_t
> -_entry_in_ignore_list (const char *entry, add_files_state_t *state)
> +_entry_in_ignore_list (const char *path, const char *entry,
> +		       add_files_state_t *state)
>  {

I was a bit confused when reading this (until I saw the calling
context). I guess path is dirname and entry is basename?

Other than needing doc and a test, I don't remember any objections to
this code, although I vaguely remember some alternative proposals for
ignoring?

I guess one point is that we are proposing to use regexps against paths
in one place [1] and proposing to use fnmatch here.  I'm not sure it's
worth trying to be more consistent, but now seems to be the time to
discuss. I think both places could use either, if someone did the
work. Of course the other argument is that the queries should be
consistent among themselves.

[1]: id:20170308024035.6149-1-david@tethera.net

Re: [RFC 5/5] cli: print number of ignored non-mails encountered in notmuch new scan

[David Bremner]

Jani Nikula <jani@nikula.org> writes:
>  
> +    if (state->ignored_non_mails)
> +	printf (" Ignored %d non-mail %s. (Use 'notmuch config append new.ignore <pattern> [...]' to ignore explicitly.)",
> +		state->ignored_non_mails,
> +		state->ignored_non_mails == 1 ? "file" : "files");
> +
>      printf ("\n");
>  }

I guess this should be hushed in the case when the user passes --quiet