From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal
In-Reply-To: <1330357759-1337-2-git-send-email-amdragon@mit.edu>
References: <1330357759-1337-1-git-send-email-amdragon@mit.edu>
	<1330357759-1337-2-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.12+110~gbc97b4a (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 16 Apr 2012 16:53:10 +0100
Message-ID: <87zkab4qs9.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, fatal errors in add_files_recursive were not treated as
> fatal by its callers (including itself!) and add_files_recursive
> sometimes returned errors on non-fatal conditions.  This makes
> add_files_recursive errors consistently fatal and updates all callers
> to treat them as fatal.

Hi I have attempted to review this but am feeling a little out of my
depth. This patch seems fine except for one thing I am unsure about:

> ---
>  notmuch-new.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 4f13535..bd9786a 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -308,7 +308,6 @@ add_files_recursive (notmuch_database_t *notmuch,
>      if (num_fs_entries == -1) {
>  	fprintf (stderr, "Error opening directory %s: %s\n",
>  		 path, strerror (errno));
> -	ret = NOTMUCH_STATUS_FILE_ERROR;
>  	goto DONE;
>      }
>  

If I understand this, then this change makes a failure to open a
directory non-fatal. In the comment before the function it says

 *     Also, we don't immediately act on file/directory removal since we
 *     must ensure that in the case of a rename that the new filename is
 *     added before the old filename is removed, (so that no information
 *     is lost from the database).
 
I am  worried that we could fail to find some files because of the
file error above, and then we delete them from the database. Maybe this
could only happen if those emails have just been moved to this
file-error directory?

Best wishes

Mark

> @@ -351,8 +350,10 @@ add_files_recursive (notmuch_database_t *notmuch,
>  
>  	next = talloc_asprintf (notmuch, "%s/%s", path, entry->d_name);
>  	status = add_files_recursive (notmuch, next, state);
> -	if (status && ret == NOTMUCH_STATUS_SUCCESS)
> +	if (status) {
>  	    ret = status;
> +	    goto DONE;
> +	}
>  	talloc_free (next);
>  	next = NULL;
>      }
> @@ -933,6 +934,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>      }
>  
>      ret = add_files (notmuch, db_path, &add_files_state);
> +    if (ret)
> +	goto DONE;
>  
>      gettimeofday (&tv_start, NULL);
>      for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
> @@ -965,6 +968,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  	}
>      }
>  
> +  DONE:
>      talloc_free (add_files_state.removed_files);
>      talloc_free (add_files_state.removed_directories);
>      talloc_free (add_files_state.directory_mtimes);
> @@ -1012,10 +1016,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      printf ("\n");
>  
> -    if (ret) {
> -	printf ("\nNote: At least one error was encountered: %s\n",
> +    if (ret)
> +	printf ("\nNote: A fatal error was encountered: %s\n",
>  		notmuch_status_to_string (ret));
> -    }
>  
>      notmuch_database_close (notmuch);
>  
> -- 
> 1.7.7.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch