Date: Wed, 1 Feb 2012 11:30:52 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH] added support for user-specified files & directories to
	ignore
Message-ID: <20120201163052.GG10898@mit.edu>
References: <1315949524-4948-1-git-send-email-tomi.ollila@iki.fi>
	<20120131-new-ignore-1-git-send-email-too@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20120131-new-ignore-1-git-send-email-too@iki.fi>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Tomi Ollila on Jan 31 at  6:28 pm:
> A new configuration key 'new.ignore' is used to determine which
> files and directories user wants not to be scanned as new mails.
> 
> This work merges my previous attempts and Andreas Amann's work
> in id:"ylp7hi23mw8.fsf@tyndall.ie"
> 
> See notes in id:"20120131-new-ignore-1-git-send-email-too@iki.fi"
> ---
> Notes
> 
> 1) Currently there is comment for new.ignore in newly created configuration
> file but as the list is initially empty there will be not tag in place.
> 
> 2) 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 ?
> 
> 
> 3) in id:"1327572718-13411-2-git-send-email-tomi.ollila@iki.fi" dropped...
> 
>  notmuch-client.h |    8 ++++++++
>  notmuch-config.c |   35 +++++++++++++++++++++++++++++++++--
>  notmuch-new.c    |   45 +++++++++++++++++++++++++++++++++------------
>  3 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index e0eb594..c62ce78 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -250,6 +250,14 @@ 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);

Missing blank line.

> +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..f1cc5c2 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 files and directories that"
> +    "\t	will not be searched for messages by \"notmuch new\".\n";

This can be interpreted in a lot of ways (e.g., absolute paths, paths
relative to database.path, etc) but is expecting something very
specific.  Maybe "file and directory names"?

>  
>  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;
> @@ -360,7 +367,11 @@ notmuch_config_open (void *ctx,
>          const char *tags[] = { "unread", "inbox" };
>  	notmuch_config_set_new_tags (config, tags, 2);
>      }
> -

Looks like you lost a blank line.

> +#if 0 /* No point setting empty list -- it's not written */
> +    if (notmuch_config_get_new_ignore (config, &tmp) == NULL) {
> +	notmuch_config_set_new_ignore (config, NULL, 0);
> +    }
> +#endif

This *does* matter.  It affects whether or not this key appears in
saved configuration files (both updated ones and new ones).  For
discoverability's sake (especially since we don't have a way to update
the section's comment), it should appear, even though it's blank.

(For the record, I think 'notmuch config set's behavior of removing a
key if given no arguments is broken; because of how we handle
defaults, this is completely asymmetric behavior, plus it seems to
screw up group comments.)

Also, there should be another blank line after the if.

>      if (notmuch_config_get_search_exclude_tags (config, &tmp) == NULL) {
>  	if (is_new) {
>  	    const char *tags[] = { "deleted", "spam" };
> @@ -609,6 +620,15 @@ 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);
> +}
> +
> +

One extra blank line.

>  void
>  notmuch_config_set_user_other_email (notmuch_config_t *config,
>  				     const char *list[],
> @@ -627,6 +647,17 @@ notmuch_config_set_new_tags (notmuch_config_t *config,
>  		     &(config->new_tags));
>  }
>  
> +#if 0 /* UNNEEDED SO FAR */

Include this, even if we don't need it.

> +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));
> +}
> +#endif
> +
>  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..36d5c5d 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;
>  }
>  
> +/* Check if user asked to ignore these files/directories */
> +

No blank line.

> +static int

Could be notmuch_bool_t (and TRUE and FALSE below).  Up to you.

> +_entry_in_ignore_list (const char *entry, add_files_state_t *state)
> +{
> +    size_t i, ignore_length = state->new_ignore_length;
> +
> +    for (i = 0; i < ignore_length; i++)
> +	if (strcmp (entry, state->new_ignore[i]) == 0)
> +	    return 1;
> +
> +    return 0;
> +}
> +
>  /* 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
> +	 * user have configured to be ignored.

s/user have/the user has/

>  	 */
> -	/* 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))

Ah, nice simplification.

>  	{
>  	    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
> +	 * user have 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;
>