Date: Fri, 27 Jan 2012 17:50:31 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH 2/2] added support for user-specified directories to
	exclude
Message-ID: <20120127225031.GD10053@mit.edu>
References: <8762g0sj6f.fsf@praet.org>
	<1327572718-13411-1-git-send-email-tomi.ollila@iki.fi>
	<1327572718-13411-2-git-send-email-tomi.ollila@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1327572718-13411-2-git-send-email-tomi.ollila@iki.fi>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Tomi Ollila on Jan 26 at 12:11 pm:
> A new configuration key 'database.exclude' is used to determine
> which directories user wants not to be scanned for new mails.

Not just directories.

> ---
> 
> Notes (from 2011-09-13):
> 
> 1) Currently the comments for newly created configuration file are not
> updated, so for not this is 'undocumented feature'. Should there be an
> empty configuration line as a placeholder ... ?

This is unfortunate and hard to do anything about with the current
config file design.  For precisely this reason, I think option
documentation should be in per-option comments instead of per-section
comments, but this would be a large change to the config structure.
For now, it should definitely be added to the section comment so that
newly generated config files get it.  If you want to be fancy, you can
check for the old section comment (or perhaps just its hash) and
automatically upgrade it.

> 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 ?

notmuch actively descends into every subdirectory of your Maildir (it
has to, since mtime changes don't propagate up), which seems like the
perfect opportunity to check if an ignored directory is present in the
database.  This is harder for ignored files and probably not worth
solving since people are probably ignoring files that aren't mail
anyway (whereas I know people want to ignore directories that contain
mail).

> 3) count_files() function is not touched. The functionality there has fallen
> behind of add_files_recursive (maildir+tmp check and following symlinks).
> The question there should it be updated, or attempted to merge with
> add_files (as the comment says). count_files() is only called at the beginning
> when database is not yet initialised.

Given that count_files is already broken, it's probably best to leave
it as is for now and maybe fix it thoroughly later.  I wouldn't be too
worried about this though, since a new database is likely to also have
no configured ignores.

> ---
>  notmuch-client.h |    3 +++
>  notmuch-config.c |   13 +++++++++++++
>  notmuch-new.c    |   22 ++++++++++++++++++++--
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index e0eb594..78460fc 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -219,6 +219,9 @@ void
>  notmuch_config_set_database_path (notmuch_config_t *config,
>  				  const char *database_path);
>  
> +const char **
> +notmuch_config_get_database_exclude (notmuch_config_t *config,
> +				     size_t *length);

As I mentioned in another email, I would prefer to see this in the
'new' section (though I could probably be persuaded otherwise).  Also,
I think "ignore" would be more typical terminology (think .gitignore,
etc).

Also, every other getter has a corresponding setter.

>  const char *
>  notmuch_config_get_user_name (notmuch_config_t *config);
>  
> diff --git a/notmuch-config.c b/notmuch-config.c
> index a124e34..e236114 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -99,6 +99,8 @@ struct _notmuch_config {
>      GKeyFile *key_file;
>  
>      char *database_path;
> +    const char **database_exclude;
> +    size_t database_exclude_length;
>      char *user_name;
>      char *user_primary_email;
>      const char **user_other_email;
> @@ -258,6 +260,8 @@ notmuch_config_open (void *ctx,
>      config->key_file = g_key_file_new ();
>  
>      config->database_path = NULL;
> +    config->database_exclude = NULL;
> +    config->database_exclude_length = 0;
>      config->user_name = NULL;
>      config->user_primary_email = NULL;
>      config->user_other_email = NULL;
> @@ -537,6 +541,15 @@ notmuch_config_set_database_path (notmuch_config_t *config,
>      config->database_path = NULL;
>  }
>  
> +const char **
> +notmuch_config_get_database_exclude (notmuch_config_t *config,
> +				     size_t *length)
> +{
> +    return _config_get_list (config, "database", "exclude",
> +			     &(config->database_exclude),
> +			     &(config->database_exclude_length), length);
> +}
> +

All of the other options are fetched by notmuch_config_open and set if
they don't exist.  This has the effect that they will get added to the
configuration file if it is written out, which won't happen to this
option.

>  const char *
>  notmuch_config_get_user_name (notmuch_config_t *config)
>  {
> diff --git a/notmuch-new.c b/notmuch-new.c
> index a569a54..d607f5b 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 **database_exclude;
> +    size_t database_exclude_length;
>  
>      int total_files;
>      int processed_files;
> @@ -300,6 +302,8 @@ add_files_recursive (notmuch_database_t *notmuch,
>      is_maildir = _entries_resemble_maildir (fs_entries, num_fs_entries);
>  
>      for (i = 0; i < num_fs_entries; i++) {
> +	size_t j;
> +
>  	if (interrupted)
>  	    break;
>  
> @@ -323,8 +327,6 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	 * Also ignore the .notmuch directory and any "tmp" directory
>  	 * that appears within a maildir.
>  	 */
> -	/* 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) ||
> @@ -332,6 +334,12 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	{
>  	    continue;
>  	}
> +	/* Ignore user-specified directories */
> +	for (j = 0; j < state->database_exclude_length; j++)
> +	    if (strcmp(entry->d_name, state->database_exclude[j]) == 0)
> +		break;
> +	if (j < state->database_exclude_length)
> +	    continue;

As Jani said, move this into its own function (which would simplify
the control flow, too).

>  
>  	next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
>  	status = add_files_recursive (notmuch, next, state);
> @@ -364,11 +372,20 @@ add_files_recursive (notmuch_database_t *notmuch,
>      /* Pass 2: Scan for new files, removed files, and removed directories. */
>      for (i = 0; i < num_fs_entries; i++)
>      {
> +	size_t j;
> +
>  	if (interrupted)
>  	    break;
>  
>          entry = fs_entries[i];
>  
> +	/* Ignore user-specified files & directories */
> +	for (j = 0; j < state->database_exclude_length; j++)
> +	    if (strcmp(entry->d_name, state->database_exclude[j]) == 0)
> +		break;
> +	if (j < state->database_exclude_length)
> +	    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) &&
> @@ -837,6 +854,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.database_exclude = notmuch_config_get_database_exclude (config, &add_files_state.database_exclude_length);
>      add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>      db_path = notmuch_config_get_database_path (config);
>