From: Jani Nikula Date: Thu, 2 Feb 2012 17:16:03 +0000 (+0200) Subject: Re: [PATCH] add support for user-specified files & directories to ignore X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=5d6ef7e5ef0bfde27b90b43d4e65ba4f4c732bfb;p=notmuch-archives.git Re: [PATCH] add support for user-specified files & directories to ignore --- diff --git a/79/d69c59c322045536e27aea7c0e75774caa912b b/79/d69c59c322045536e27aea7c0e75774caa912b new file mode 100644 index 000000000..67386e6a2 --- /dev/null +++ b/79/d69c59c322045536e27aea7c0e75774caa912b @@ -0,0 +1,371 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 3D06E431FC4 + for ; Thu, 2 Feb 2012 09:16:11 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id JQTmLysoQeK8 for ; + Thu, 2 Feb 2012 09:16:09 -0800 (PST) +Received: from mail-lpp01m020-f181.google.com (mail-lpp01m020-f181.google.com + [209.85.217.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 66590431FAE + for ; Thu, 2 Feb 2012 09:16:09 -0800 (PST) +Received: by lbbgn5 with SMTP id gn5so587603lbb.26 + for ; Thu, 02 Feb 2012 09:16:07 -0800 (PST) +Received: by 10.112.25.8 with SMTP id y8mr963172lbf.15.1328202967818; + Thu, 02 Feb 2012 09:16:07 -0800 (PST) +Received: from localhost (dsl-hkibrasgw4-fe50f800-253.dhcp.inet.fi. + [84.248.80.253]) + by mx.google.com with ESMTPS id a8sm2466688lba.15.2012.02.02.09.16.04 + (version=SSLv3 cipher=OTHER); Thu, 02 Feb 2012 09:16:06 -0800 (PST) +From: Jani Nikula +To: Tomi Ollila , notmuch@notmuchmail.org +Subject: Re: [PATCH] add support for user-specified files & directories to + ignore +In-Reply-To: <20120202-new-ignore-1-git-send-email-too@iki.fi> +References: + <20120202-new-ignore-1-git-send-email-too@iki.fi> +User-Agent: Notmuch/0.11+139~g4340989 (http://notmuchmail.org) Emacs/23.3.1 + (i686-pc-linux-gnu) +Date: Thu, 02 Feb 2012 19:16:03 +0200 +Message-ID: <87sjitxijw.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +Cc: Tomi Ollila +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Thu, 02 Feb 2012 17:16:11 -0000 + + +Hi Tomi, please find a few more comments and nitpicks inline. No need to +roll another version for them, though. + +BR, +Jani. + +On Thu, 2 Feb 2012 17:17:33 +0200, Tomi Ollila wrote: +> A new configuration key 'new.ignore' is used to determine which +> files and directories user wants not to be scanned as new mails. +> +> Mark the corresponding test as no longer broken +> (test from id:"1328105573-4626-1-git-send-email-pieter@praet.org" ). +> +> This work merges my previous attempts and Andreas Amann's work +> in id:"ylp7hi23mw8.fsf@tyndall.ie" +> +> See notes in id:"20120202-new-ignore-1-git-send-email-too@iki.fi" +> --- +> +> Whenever some already existing directory is added to the exclude list +> and the parent directory timestamp has not changed, notmuch new will not +> notice the directory has gone (as it still is there), user needs to 'touch' +> the parent directory before next 'notmuch new' no make notmuch notice. +> +> 2012-01-26: could notmuch track mtime of the configuration file and if +> that changes, ignore mail directory timestamps ? +> +> See previous notes in id:"20120131-new-ignore-1-git-send-email-too@iki.fi" +> +> notmuch-client.h | 10 ++++++++++ +> notmuch-config.c | 33 +++++++++++++++++++++++++++++++-- +> notmuch-new.c | 45 +++++++++++++++++++++++++++++++++------------ +> test/new | 1 - +> 4 files changed, 74 insertions(+), 15 deletions(-) +> +> diff --git a/notmuch-client.h b/notmuch-client.h +> index e0eb594..26153df 100644 +> --- a/notmuch-client.h +> +++ b/notmuch-client.h +> @@ -245,11 +245,21 @@ notmuch_config_set_user_other_email (notmuch_config_t *config, +> const char ** +> notmuch_config_get_new_tags (notmuch_config_t *config, +> size_t *length); +> + + +Nitpick: unrelated change. + +> void +> notmuch_config_set_new_tags (notmuch_config_t *config, +> const char *new_tags[], +> size_t length); +> +> +const char ** +> +notmuch_config_get_new_ignore (notmuch_config_t *config, +> + size_t *length); +> + +> +void +> +notmuch_config_set_new_ignore (notmuch_config_t *config, +> + const char *new_ignore[], +> + size_t length); +> + +> notmuch_bool_t +> notmuch_config_get_maildir_synchronize_flags (notmuch_config_t *config); +> +> diff --git a/notmuch-config.c b/notmuch-config.c +> index a124e34..1f01004 100644 +> --- a/notmuch-config.c +> +++ b/notmuch-config.c +> @@ -44,7 +44,10 @@ static const char new_config_comment[] = +> " The following options are supported here:\n" +> "\n" +> "\ttags A list (separated by ';') of the tags that will be\n" +> - "\t added to all messages incorporated by \"notmuch new\".\n"; +> + "\t added to all messages incorporated by \"notmuch new\".\n" +> + "\n" +> + "\tignore A list (separated by ';') of file and directory names\n" +> + "\t that will not be searched for messages by \"notmuch new\".\n"; + +Do I understand the code correctly, the ignore list must contain file +and directory names without (absolute or relative) paths? And that there +is no way to exclude only the subdirectory "foo" within directory "foo", +because they would both get ignored? + +I don't see this as a bad thing, not at all. This keeps things nice and +simple, it just needs proper documentation. + +> +> static const char user_config_comment[] = +> " User configuration\n" +> @@ -105,6 +108,8 @@ struct _notmuch_config { +> size_t user_other_email_length; +> const char **new_tags; +> size_t new_tags_length; +> + const char **new_ignore; +> + size_t new_ignore_length; +> notmuch_bool_t maildir_synchronize_flags; +> const char **search_exclude_tags; +> size_t search_exclude_tags_length; +> @@ -264,6 +269,8 @@ notmuch_config_open (void *ctx, +> config->user_other_email_length = 0; +> config->new_tags = NULL; +> config->new_tags_length = 0; +> + config->new_ignore = NULL; +> + config->new_ignore_length = 0; +> config->maildir_synchronize_flags = TRUE; +> config->search_exclude_tags = NULL; +> config->search_exclude_tags_length = 0; +> @@ -361,6 +368,10 @@ notmuch_config_open (void *ctx, +> notmuch_config_set_new_tags (config, tags, 2); +> } +> +> + if (notmuch_config_get_new_ignore (config, &tmp) == NULL) { +> + notmuch_config_set_new_ignore (config, NULL, 0); +> + } +> + +> if (notmuch_config_get_search_exclude_tags (config, &tmp) == NULL) { +> if (is_new) { +> const char *tags[] = { "deleted", "spam" }; +> @@ -504,7 +515,8 @@ _config_set_list (notmuch_config_t *config, +> const char *list[], +> size_t length, const char ***config_var ) +> { +> - g_key_file_set_string_list (config->key_file, group, name, list, length); +> + g_key_file_set_string_list (config->key_file, +> + group, name, list, length); + +Nitpick: unrelated change. + +> talloc_free (*config_var); +> *config_var = NULL; +> } +> @@ -609,6 +621,14 @@ notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length) +> &(config->new_tags_length), length); +> } +> +> +const char ** +> +notmuch_config_get_new_ignore (notmuch_config_t *config, size_t *length) +> +{ +> + return _config_get_list (config, "new", "ignore", +> + &(config->new_ignore), +> + &(config->new_ignore_length), length); +> +} +> + +> void +> notmuch_config_set_user_other_email (notmuch_config_t *config, +> const char *list[], +> @@ -627,6 +647,15 @@ notmuch_config_set_new_tags (notmuch_config_t *config, +> &(config->new_tags)); +> } +> +> +void +> +notmuch_config_set_new_ignore (notmuch_config_t *config, +> + const char *list[], +> + size_t length) +> +{ +> + _config_set_list (config, "new", "ignore", list, length, +> + &(config->new_ignore)); +> +} +> + +> const char ** +> notmuch_config_get_search_exclude_tags (notmuch_config_t *config, size_t *length) +> { +> diff --git a/notmuch-new.c b/notmuch-new.c +> index a569a54..f1c271a 100644 +> --- a/notmuch-new.c +> +++ b/notmuch-new.c +> @@ -39,6 +39,8 @@ typedef struct { +> int verbose; +> const char **new_tags; +> size_t new_tags_length; +> + const char **new_ignore; +> + size_t new_ignore_length; +> +> int total_files; +> int processed_files; +> @@ -181,6 +183,20 @@ _entries_resemble_maildir (struct dirent **entries, int count) +> return 0; +> } +> +> +/* Test if the file/directory is to be ignored. */ +> + + +Nitpick: extra newline. + +> +static notmuch_bool_t +> +_entry_in_ignore_list (const char *entry, add_files_state_t *state) +> +{ +> + size_t i; +> + +> + for (i = 0; i < state->new_ignore_length; i++) +> + if (strcmp (entry, state->new_ignore[i]) == 0) +> + return TRUE; +> + +> + return FALSE; +> +} + +Do I understand correctly that if we later allow glob(7) style patterns +in the ignore list, and add the support in place of strcmp(), it will +work with the change in this single place? That would be neat. + +> + +> /* Examine 'path' recursively as follows: +> * +> * o Ask the filesystem for the mtime of 'path' (fs_mtime) +> @@ -320,15 +336,15 @@ add_files_recursive (notmuch_database_t *notmuch, +> } +> +> /* Ignore special directories to avoid infinite recursion. +> - * Also ignore the .notmuch directory and any "tmp" directory +> - * that appears within a maildir. +> + * Also ignore the .notmuch directory, any "tmp" directory +> + * that appears within a maildir and files/directories +> + * the user has configured to be ignored. +> */ +> - /* XXX: Eventually we'll want more sophistication to let the +> - * user specify files to be ignored. */ +> if (strcmp (entry->d_name, ".") == 0 || +> strcmp (entry->d_name, "..") == 0 || +> (is_maildir && strcmp (entry->d_name, "tmp") == 0) || +> - strcmp (entry->d_name, ".notmuch") ==0) +> + strcmp (entry->d_name, ".notmuch") == 0 || +> + _entry_in_ignore_list (entry->d_name, state)) + +I realize only now that the ignore function *could* also cover the other +ignores, to avoid some code duplication. But that can (and should) be +another refactoring patch, another time, if ever. + +> { +> continue; +> } +> @@ -369,6 +385,10 @@ add_files_recursive (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)) +> + continue; +> + +> /* Check if we've walked past any names in db_files or +> * db_subdirs. If so, these have been deleted. */ +> while (notmuch_filenames_valid (db_files) && +> @@ -648,7 +668,7 @@ add_files (notmuch_database_t *notmuch, +> * initialized to zero by the top-level caller before calling +> * count_files). */ +> static void +> -count_files (const char *path, int *count) +> +count_files (const char *path, int *count, add_files_state_t *state) +> { +> struct dirent *entry = NULL; +> char *next; +> @@ -670,13 +690,13 @@ count_files (const char *path, int *count) +> entry = fs_entries[i++]; +> +> /* Ignore special directories to avoid infinite recursion. +> - * Also ignore the .notmuch directory. +> + * Also ignore the .notmuch directory and files/directories +> + * the user has configured to be ignored. +> */ +> - /* XXX: Eventually we'll want more sophistication to let the +> - * user specify files to be ignored. */ +> if (strcmp (entry->d_name, ".") == 0 || +> strcmp (entry->d_name, "..") == 0 || +> - strcmp (entry->d_name, ".notmuch") == 0) +> + strcmp (entry->d_name, ".notmuch") == 0 || +> + _entry_in_ignore_list (entry->d_name, state)) +> { +> continue; +> } +> @@ -697,7 +717,7 @@ count_files (const char *path, int *count) +> fflush (stdout); +> } +> } else if (S_ISDIR (st.st_mode)) { +> - count_files (next, count); +> + count_files (next, count, state); +> } +> +> free (next); +> @@ -837,6 +857,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) +> return 1; +> +> add_files_state.new_tags = notmuch_config_get_new_tags (config, &add_files_state.new_tags_length); +> + add_files_state.new_ignore = notmuch_config_get_new_ignore (config, &add_files_state.new_ignore_length); +> add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config); +> db_path = notmuch_config_get_database_path (config); +> +> @@ -852,7 +873,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[]) +> int count; +> +> count = 0; +> - count_files (db_path, &count); +> + count_files (db_path, &count, &add_files_state); +> if (interrupted) +> return 1; +> +> diff --git a/test/new b/test/new +> index 740ba05..b616dec 100755 +> --- a/test/new +> +++ b/test/new +> @@ -166,7 +166,6 @@ Note: Ignoring non-mail file: ${MAIL_DIR}/ignored_file +> Added 1 new message to the database." +> +> test_begin_subtest "Ignore files and directories specified in new.ignore" +> -test_subtest_known_broken +> generate_message +> notmuch config set new.ignore .git ignored_file .ignored_hidden_file +> mkdir -p "${MAIL_DIR}"/.git && touch "${MAIL_DIR}"/.git/config +> -- +> 1.7.8.2 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch