[PATCH 0/3] Fix some error handling in notmuch new

[PATCH 0/3] Fix some error handling in notmuch new

[Austin Clements]

This series cleans up some bad error handling in notmuch new.  In many
cases, fatal errors were being ignored, sometimes blatantly and
sometimes because the caller did not treat them as fatal even if the
callee did.

[PATCH 1/3] new: Consistently treat fatal errors as fatal

[Austin Clements]

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.
---
 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;
     }
 
@@ -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

[PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory

[Austin Clements]

Previously such errors were simply ignored.  Now they cause an
immediate cleanup and abort.
---
 notmuch-new.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index bd9786a..0cbd479 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch,
 	add_files_state->renamed_messages++;
 	if (add_files_state->synchronize_flags == TRUE)
 	    notmuch_message_maildir_flags_to_tags (message);
-    } else
+	status = NOTMUCH_STATUS_SUCCESS;
+    } else if (status == NOTMUCH_STATUS_SUCCESS) {
 	add_files_state->removed_messages++;
+    }
     notmuch_message_destroy (message);
     notmuch_database_end_atomic (notmuch);
     return status;
@@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch,
 
 /* Recursively remove all filenames from the database referring to
  * 'path' (or to any of its children). */
-static void
+static notmuch_status_t
 _remove_directory (void *ctx,
 		   notmuch_database_t *notmuch,
 		   const char *path,
 		   add_files_state_t *add_files_state)
 {
+    notmuch_status_t status;
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
     char *absolute;
@@ -807,8 +810,10 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	remove_filename (notmuch, absolute, add_files_state);
+	status = remove_filename (notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    return status;
     }
 
     for (subdirs = notmuch_directory_get_child_directories (directory);
@@ -817,11 +822,14 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (subdirs));
-	_remove_directory (ctx, notmuch, absolute, add_files_state);
+	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    return status;
     }
 
     notmuch_directory_destroy (directory);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 int
@@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	remove_filename (notmuch, f->filename, &add_files_state);
+	ret = remove_filename (notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
@@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
-	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "directories",
-- 
1.7.7.3

[PATCH 3/3] new: Print final fatal error message to stderr

[Austin Clements]

This was going to stdout.  I removed the newline at the beginning of
printing the fatal error message because it wouldn't make sense if you
were only looking at the stderr stream (e.g., you had redirected
stdout to /dev/null).
---
 notmuch-new.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 0cbd479..9ad790d 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1029,8 +1029,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     printf ("\n");
 
     if (ret)
-	printf ("\nNote: A fatal error was encountered: %s\n",
-		notmuch_status_to_string (ret));
+	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
+		 notmuch_status_to_string (ret));
 
     notmuch_database_close (notmuch);
 
-- 
1.7.7.3

Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal

[Mark Walters]

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

Re: [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory

[Mark Walters]

On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously such errors were simply ignored.  Now they cause an
> immediate cleanup and abort.

This one looks fine except for a minor query.

> ---
>  notmuch-new.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index bd9786a..0cbd479 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch,
>  	add_files_state->renamed_messages++;
>  	if (add_files_state->synchronize_flags == TRUE)
>  	    notmuch_message_maildir_flags_to_tags (message);
> -    } else
> +	status = NOTMUCH_STATUS_SUCCESS;
> +    } else if (status == NOTMUCH_STATUS_SUCCESS) {
>  	add_files_state->removed_messages++;
> +    }
>      notmuch_message_destroy (message);
>      notmuch_database_end_atomic (notmuch);
>      return status;
> @@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch,
>  
>  /* Recursively remove all filenames from the database referring to
>   * 'path' (or to any of its children). */
> -static void
> +static notmuch_status_t
>  _remove_directory (void *ctx,
>  		   notmuch_database_t *notmuch,
>  		   const char *path,
>  		   add_files_state_t *add_files_state)
>  {
> +    notmuch_status_t status;
>      notmuch_directory_t *directory;
>      notmuch_filenames_t *files, *subdirs;
>      char *absolute;
> @@ -807,8 +810,10 @@ _remove_directory (void *ctx,
>      {
>  	absolute = talloc_asprintf (ctx, "%s/%s", path,
>  				    notmuch_filenames_get (files));
> -	remove_filename (notmuch, absolute, add_files_state);
> +	status = remove_filename (notmuch, absolute, add_files_state);
>  	talloc_free (absolute);
> +	if (status)
> +	    return status;
>      }
>  
>      for (subdirs = notmuch_directory_get_child_directories (directory);
> @@ -817,11 +822,14 @@ _remove_directory (void *ctx,
>      {
>  	absolute = talloc_asprintf (ctx, "%s/%s", path,
>  				    notmuch_filenames_get (subdirs));
> -	_remove_directory (ctx, notmuch, absolute, add_files_state);
> +	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
>  	talloc_free (absolute);
> +	if (status)
> +	    return status;
>      }
>  
>      notmuch_directory_destroy (directory);
> +    return NOTMUCH_STATUS_SUCCESS;
>  }

In the two "return status" lines above seem to mean we don't call
notmuch_directory_destroy. Does that matter?

The other query is not actually about this patch: just something that
came up when reading it. Should notmuch_database_begin_atomic and
notmuch_database_end_atomic always be paired? One of the (existing)
error cases in remove_filename seems to return without calling end.

Best wishes

Mark

>  int
> @@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      gettimeofday (&tv_start, NULL);
>      for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
> -	remove_filename (notmuch, f->filename, &add_files_state);
> +	ret = remove_filename (notmuch, f->filename, &add_files_state);
> +	if (ret)
> +	    goto DONE;
>  	if (do_print_progress) {
>  	    do_print_progress = 0;
>  	    generic_print_progress ("Cleaned up", "messages",
> @@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      gettimeofday (&tv_start, NULL);
>      for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
> -	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
> +	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
> +	if (ret)
> +	    goto DONE;
>  	if (do_print_progress) {
>  	    do_print_progress = 0;
>  	    generic_print_progress ("Cleaned up", "directories",
> -- 
> 1.7.7.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/3] new: Consistently treat fatal errors as fatal

[Austin Clements]

Quoth Mark Walters on Apr 16 at  4:53 pm:
> 
> 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?

Hmm.  This won't *actively* remove files, since that only happens if
we successfully scan a directory and find a message that's in the
database but not in that directory (*not* scanning the directory won't
add anything to the remove list).  However, you are right that if a
message is moved from some other directory into a directory that we
fail to open, that message will be deleted "by omission".

I've added the error back, along with a comment explaining it.

> 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);

Re: [PATCH 2/3] new: Handle fatal errors in remove_filename and _remove_directory

[Austin Clements]

Quoth Mark Walters on Apr 16 at  5:02 pm:
> On Mon, 27 Feb 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously such errors were simply ignored.  Now they cause an
> > immediate cleanup and abort.
> 
> This one looks fine except for a minor query.
> 
> > ---
> >  notmuch-new.c |   24 ++++++++++++++++++------
> >  1 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/notmuch-new.c b/notmuch-new.c
> > index bd9786a..0cbd479 100644
> > --- a/notmuch-new.c
> > +++ b/notmuch-new.c
> > @@ -780,8 +780,10 @@ remove_filename (notmuch_database_t *notmuch,
> >  	add_files_state->renamed_messages++;
> >  	if (add_files_state->synchronize_flags == TRUE)
> >  	    notmuch_message_maildir_flags_to_tags (message);
> > -    } else
> > +	status = NOTMUCH_STATUS_SUCCESS;
> > +    } else if (status == NOTMUCH_STATUS_SUCCESS) {
> >  	add_files_state->removed_messages++;
> > +    }
> >      notmuch_message_destroy (message);
> >      notmuch_database_end_atomic (notmuch);
> >      return status;
> > @@ -789,12 +791,13 @@ remove_filename (notmuch_database_t *notmuch,
> >  
> >  /* Recursively remove all filenames from the database referring to
> >   * 'path' (or to any of its children). */
> > -static void
> > +static notmuch_status_t
> >  _remove_directory (void *ctx,
> >  		   notmuch_database_t *notmuch,
> >  		   const char *path,
> >  		   add_files_state_t *add_files_state)
> >  {
> > +    notmuch_status_t status;
> >      notmuch_directory_t *directory;
> >      notmuch_filenames_t *files, *subdirs;
> >      char *absolute;
> > @@ -807,8 +810,10 @@ _remove_directory (void *ctx,
> >      {
> >  	absolute = talloc_asprintf (ctx, "%s/%s", path,
> >  				    notmuch_filenames_get (files));
> > -	remove_filename (notmuch, absolute, add_files_state);
> > +	status = remove_filename (notmuch, absolute, add_files_state);
> >  	talloc_free (absolute);
> > +	if (status)
> > +	    return status;
> >      }
> >  
> >      for (subdirs = notmuch_directory_get_child_directories (directory);
> > @@ -817,11 +822,14 @@ _remove_directory (void *ctx,
> >      {
> >  	absolute = talloc_asprintf (ctx, "%s/%s", path,
> >  				    notmuch_filenames_get (subdirs));
> > -	_remove_directory (ctx, notmuch, absolute, add_files_state);
> > +	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
> >  	talloc_free (absolute);
> > +	if (status)
> > +	    return status;
> >      }
> >  
> >      notmuch_directory_destroy (directory);
> > +    return NOTMUCH_STATUS_SUCCESS;
> >  }
> 
> In the two "return status" lines above seem to mean we don't call
> notmuch_directory_destroy. Does that matter?

Good point.  I've fixed this to use the usual goto DONE cleanup idiom.

> The other query is not actually about this patch: just something that
> came up when reading it. Should notmuch_database_begin_atomic and
> notmuch_database_end_atomic always be paired? One of the (existing)
> error cases in remove_filename seems to return without calling end.

Yes, they should be.  I've added a patch to fix that.

> Best wishes
> 
> Mark
> 
> >  int
> > @@ -939,7 +947,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
> >  
> >      gettimeofday (&tv_start, NULL);
> >      for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
> > -	remove_filename (notmuch, f->filename, &add_files_state);
> > +	ret = remove_filename (notmuch, f->filename, &add_files_state);
> > +	if (ret)
> > +	    goto DONE;
> >  	if (do_print_progress) {
> >  	    do_print_progress = 0;
> >  	    generic_print_progress ("Cleaned up", "messages",
> > @@ -950,7 +960,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
> >  
> >      gettimeofday (&tv_start, NULL);
> >      for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
> > -	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
> > +	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
> > +	if (ret)
> > +	    goto DONE;
> >  	if (do_print_progress) {
> >  	    do_print_progress = 0;
> >  	    generic_print_progress ("Cleaned up", "directories",

[PATCH v2 0/4] Fix some error handling in notmuch new

[Austin Clements]

Version 2 should address Mark's comments.  It also adds a patch to fix
an additional error handling error he pointed out in remove_filename.

[PATCH v2 1/4] new: Consistently treat fatal errors as fatal

[Austin Clements]

Previously, fatal errors in add_files_recursive were not treated as
fatal by its callers (including itself!).  This makes
add_files_recursive errors consistently fatal and updates all callers
to treat them as fatal.
---
 notmuch-new.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..15c0b36 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -308,6 +308,10 @@ add_files_recursive (notmuch_database_t *notmuch,
     if (num_fs_entries == -1) {
 	fprintf (stderr, "Error opening directory %s: %s\n",
 		 path, strerror (errno));
+	/* We consider this a fatal error because, if a user moved a
+	 * message from another directory that we were able to scan
+	 * into this directory, skipping this directory will cause
+	 * that message to be lost. */
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
@@ -351,8 +355,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 +939,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 +973,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 +1021,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.9.1

[PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory

[Austin Clements]

Previously such errors were simply ignored.  Now they cause an
immediate cleanup and abort.
---
 notmuch-new.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 15c0b36..92e0489 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch,
 	add_files_state->renamed_messages++;
 	if (add_files_state->synchronize_flags == TRUE)
 	    notmuch_message_maildir_flags_to_tags (message);
-    } else
+	status = NOTMUCH_STATUS_SUCCESS;
+    } else if (status == NOTMUCH_STATUS_SUCCESS) {
 	add_files_state->removed_messages++;
+    }
     notmuch_message_destroy (message);
     notmuch_database_end_atomic (notmuch);
     return status;
@@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch,
 
 /* Recursively remove all filenames from the database referring to
  * 'path' (or to any of its children). */
-static void
+static notmuch_status_t
 _remove_directory (void *ctx,
 		   notmuch_database_t *notmuch,
 		   const char *path,
 		   add_files_state_t *add_files_state)
 {
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
     char *absolute;
@@ -812,8 +815,10 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	remove_filename (notmuch, absolute, add_files_state);
+	status = remove_filename (notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    goto DONE;
     }
 
     for (subdirs = notmuch_directory_get_child_directories (directory);
@@ -822,11 +827,15 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (subdirs));
-	_remove_directory (ctx, notmuch, absolute, add_files_state);
+	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    goto DONE;
     }
 
+  DONE:
     notmuch_directory_destroy (directory);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 int
@@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	remove_filename (notmuch, f->filename, &add_files_state);
+	ret = remove_filename (notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
@@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
-	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "directories",
-- 
1.7.9.1

[PATCH v2 3/4] new: Print final fatal error message to stderr

[Austin Clements]

This was going to stdout.  I removed the newline at the beginning of
printing the fatal error message because it wouldn't make sense if you
were only looking at the stderr stream (e.g., you had redirected
stdout to /dev/null).
---
 notmuch-new.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 92e0489..2103b18 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1035,8 +1035,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     printf ("\n");
 
     if (ret)
-	printf ("\nNote: A fatal error was encountered: %s\n",
-		notmuch_status_to_string (ret));
+	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
+		 notmuch_status_to_string (ret));
 
     notmuch_database_close (notmuch);
 
-- 
1.7.9.1

[PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error

[Austin Clements]

Previously, if we failed to find the message by filename in
remove_filename, we would return immediately from the function without
ending its atomic block.  Now this code follows the usual goto DONE
idiom to perform cleanup.
---
 notmuch-new.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 2103b18..9eebea4 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch,
 	return status;
     status = notmuch_database_find_message_by_filename (notmuch, path, &message);
     if (status || message == NULL)
-	return status;
+	goto DONE;
+
     status = notmuch_database_remove_message (notmuch, path);
     if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	add_files_state->renamed_messages++;
@@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch,
 	add_files_state->removed_messages++;
     }
     notmuch_message_destroy (message);
+
+  DONE:
     notmuch_database_end_atomic (notmuch);
     return status;
 }
-- 
1.7.9.1

Re: [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory

[Mark Walters]

On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously such errors were simply ignored.  Now they cause an
> immediate cleanup and abort.
> ---
>  notmuch-new.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 15c0b36..92e0489 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch,
>  	add_files_state->renamed_messages++;
>  	if (add_files_state->synchronize_flags == TRUE)
>  	    notmuch_message_maildir_flags_to_tags (message);
> -    } else
> +	status = NOTMUCH_STATUS_SUCCESS;
> +    } else if (status == NOTMUCH_STATUS_SUCCESS) {
>  	add_files_state->removed_messages++;
> +    }
>      notmuch_message_destroy (message);
>      notmuch_database_end_atomic (notmuch);
>      return status;
> @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch,
>  
>  /* Recursively remove all filenames from the database referring to
>   * 'path' (or to any of its children). */
> -static void
> +static notmuch_status_t
>  _remove_directory (void *ctx,
>  		   notmuch_database_t *notmuch,
>  		   const char *path,
>  		   add_files_state_t *add_files_state)
>  {
> +    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      notmuch_directory_t *directory;
>      notmuch_filenames_t *files, *subdirs;
>      char *absolute;
> @@ -812,8 +815,10 @@ _remove_directory (void *ctx,
>      {
>  	absolute = talloc_asprintf (ctx, "%s/%s", path,
>  				    notmuch_filenames_get (files));
> -	remove_filename (notmuch, absolute, add_files_state);
> +	status = remove_filename (notmuch, absolute, add_files_state);
>  	talloc_free (absolute);
> +	if (status)
> +	    goto DONE;
>      }
>  
>      for (subdirs = notmuch_directory_get_child_directories (directory);
> @@ -822,11 +827,15 @@ _remove_directory (void *ctx,
>      {
>  	absolute = talloc_asprintf (ctx, "%s/%s", path,
>  				    notmuch_filenames_get (subdirs));
> -	_remove_directory (ctx, notmuch, absolute, add_files_state);
> +	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
>  	talloc_free (absolute);
> +	if (status)
> +	    goto DONE;
>      }
>  
> +  DONE:
>      notmuch_directory_destroy (directory);
> +    return NOTMUCH_STATUS_SUCCESS;

Doesn't this need to be return status or something?

Best wishes

Mark


>  }
>  
>  int
> @@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      gettimeofday (&tv_start, NULL);
>      for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
> -	remove_filename (notmuch, f->filename, &add_files_state);
> +	ret = remove_filename (notmuch, f->filename, &add_files_state);
> +	if (ret)
> +	    goto DONE;
>  	if (do_print_progress) {
>  	    do_print_progress = 0;
>  	    generic_print_progress ("Cleaned up", "messages",
> @@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
>  
>      gettimeofday (&tv_start, NULL);
>      for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
> -	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
> +	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
> +	if (ret)
> +	    goto DONE;
>  	if (do_print_progress) {
>  	    do_print_progress = 0;
>  	    generic_print_progress ("Cleaned up", "directories",
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 4/4] new: Fix missing end_atomic in remove_filename on error

[Mark Walters]

On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, if we failed to find the message by filename in
> remove_filename, we would return immediately from the function without
> ending its atomic block.  Now this code follows the usual goto DONE
> idiom to perform cleanup.

The whole series looks good to me modulo the return value in Patch 2/4.

Best wishes

Mark 

> ---
>  notmuch-new.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 2103b18..9eebea4 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch,
>  	return status;
>      status = notmuch_database_find_message_by_filename (notmuch, path, &message);
>      if (status || message == NULL)
> -	return status;
> +	goto DONE;
> +
>      status = notmuch_database_remove_message (notmuch, path);
>      if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
>  	add_files_state->renamed_messages++;
> @@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch,
>  	add_files_state->removed_messages++;
>      }
>      notmuch_message_destroy (message);
> +
> +  DONE:
>      notmuch_database_end_atomic (notmuch);
>      return status;
>  }
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 2/4] new: Handle fatal errors in remove_filename and _remove_directory

[Austin Clements]

Quoth Mark Walters on Apr 22 at  8:34 am:
> On Sun, 22 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously such errors were simply ignored.  Now they cause an
> > immediate cleanup and abort.
> > ---
> >  notmuch-new.c |   25 +++++++++++++++++++------
> >  1 files changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/notmuch-new.c b/notmuch-new.c
> > index 15c0b36..92e0489 100644
> > --- a/notmuch-new.c
> > +++ b/notmuch-new.c
> > @@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch,
> >  	add_files_state->renamed_messages++;
> >  	if (add_files_state->synchronize_flags == TRUE)
> >  	    notmuch_message_maildir_flags_to_tags (message);
> > -    } else
> > +	status = NOTMUCH_STATUS_SUCCESS;
> > +    } else if (status == NOTMUCH_STATUS_SUCCESS) {
> >  	add_files_state->removed_messages++;
> > +    }
> >      notmuch_message_destroy (message);
> >      notmuch_database_end_atomic (notmuch);
> >      return status;
> > @@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch,
> >  
> >  /* Recursively remove all filenames from the database referring to
> >   * 'path' (or to any of its children). */
> > -static void
> > +static notmuch_status_t
> >  _remove_directory (void *ctx,
> >  		   notmuch_database_t *notmuch,
> >  		   const char *path,
> >  		   add_files_state_t *add_files_state)
> >  {
> > +    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> >      notmuch_directory_t *directory;
> >      notmuch_filenames_t *files, *subdirs;
> >      char *absolute;
> > @@ -812,8 +815,10 @@ _remove_directory (void *ctx,
> >      {
> >  	absolute = talloc_asprintf (ctx, "%s/%s", path,
> >  				    notmuch_filenames_get (files));
> > -	remove_filename (notmuch, absolute, add_files_state);
> > +	status = remove_filename (notmuch, absolute, add_files_state);
> >  	talloc_free (absolute);
> > +	if (status)
> > +	    goto DONE;
> >      }
> >  
> >      for (subdirs = notmuch_directory_get_child_directories (directory);
> > @@ -822,11 +827,15 @@ _remove_directory (void *ctx,
> >      {
> >  	absolute = talloc_asprintf (ctx, "%s/%s", path,
> >  				    notmuch_filenames_get (subdirs));
> > -	_remove_directory (ctx, notmuch, absolute, add_files_state);
> > +	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
> >  	talloc_free (absolute);
> > +	if (status)
> > +	    goto DONE;
> >      }
> >  
> > +  DONE:
> >      notmuch_directory_destroy (directory);
> > +    return NOTMUCH_STATUS_SUCCESS;
> 
> Doesn't this need to be return status or something?

Arrg.  Yes, of course.  I wish I knew a good way to test this stuff.

[PATCH v3 0/4] Fix some error handling in notmuch new

[Austin Clements]

This version fixes a silly mistake Mark pointed out.  No further
review is necessary, per id:"877gx8kyad.fsf@qmul.ac.uk".

[PATCH v3 1/4] new: Consistently treat fatal errors as fatal

[Austin Clements]

Previously, fatal errors in add_files_recursive were not treated as
fatal by its callers (including itself!).  This makes
add_files_recursive errors consistently fatal and updates all callers
to treat them as fatal.
---
 notmuch-new.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..15c0b36 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -308,6 +308,10 @@ add_files_recursive (notmuch_database_t *notmuch,
     if (num_fs_entries == -1) {
 	fprintf (stderr, "Error opening directory %s: %s\n",
 		 path, strerror (errno));
+	/* We consider this a fatal error because, if a user moved a
+	 * message from another directory that we were able to scan
+	 * into this directory, skipping this directory will cause
+	 * that message to be lost. */
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
@@ -351,8 +355,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 +939,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 +973,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 +1021,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.9.1

[PATCH v3 2/4] new: Handle fatal errors in remove_filename and _remove_directory

[Austin Clements]

Previously such errors were simply ignored.  Now they cause an
immediate cleanup and abort.
---
 notmuch-new.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 15c0b36..2faf2f8 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -785,8 +785,10 @@ remove_filename (notmuch_database_t *notmuch,
 	add_files_state->renamed_messages++;
 	if (add_files_state->synchronize_flags == TRUE)
 	    notmuch_message_maildir_flags_to_tags (message);
-    } else
+	status = NOTMUCH_STATUS_SUCCESS;
+    } else if (status == NOTMUCH_STATUS_SUCCESS) {
 	add_files_state->removed_messages++;
+    }
     notmuch_message_destroy (message);
     notmuch_database_end_atomic (notmuch);
     return status;
@@ -794,12 +796,13 @@ remove_filename (notmuch_database_t *notmuch,
 
 /* Recursively remove all filenames from the database referring to
  * 'path' (or to any of its children). */
-static void
+static notmuch_status_t
 _remove_directory (void *ctx,
 		   notmuch_database_t *notmuch,
 		   const char *path,
 		   add_files_state_t *add_files_state)
 {
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
     char *absolute;
@@ -812,8 +815,10 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	remove_filename (notmuch, absolute, add_files_state);
+	status = remove_filename (notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    goto DONE;
     }
 
     for (subdirs = notmuch_directory_get_child_directories (directory);
@@ -822,11 +827,15 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (subdirs));
-	_remove_directory (ctx, notmuch, absolute, add_files_state);
+	status = _remove_directory (ctx, notmuch, absolute, add_files_state);
 	talloc_free (absolute);
+	if (status)
+	    goto DONE;
     }
 
+  DONE:
     notmuch_directory_destroy (directory);
+    return status;
 }
 
 int
@@ -944,7 +953,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	remove_filename (notmuch, f->filename, &add_files_state);
+	ret = remove_filename (notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
@@ -955,7 +966,9 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
-	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	ret = _remove_directory (ctx, notmuch, f->filename, &add_files_state);
+	if (ret)
+	    goto DONE;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "directories",
-- 
1.7.9.1

[PATCH v3 3/4] new: Print final fatal error message to stderr

[Austin Clements]

This was going to stdout.  I removed the newline at the beginning of
printing the fatal error message because it wouldn't make sense if you
were only looking at the stderr stream (e.g., you had redirected
stdout to /dev/null).
---
 notmuch-new.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 2faf2f8..bf9b120 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1035,8 +1035,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     printf ("\n");
 
     if (ret)
-	printf ("\nNote: A fatal error was encountered: %s\n",
-		notmuch_status_to_string (ret));
+	fprintf (stderr, "Note: A fatal error was encountered: %s\n",
+		 notmuch_status_to_string (ret));
 
     notmuch_database_close (notmuch);
 
-- 
1.7.9.1

[PATCH v3 4/4] new: Fix missing end_atomic in remove_filename on error

[Austin Clements]

Previously, if we failed to find the message by filename in
remove_filename, we would return immediately from the function without
ending its atomic block.  Now this code follows the usual goto DONE
idiom to perform cleanup.
---
 notmuch-new.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index bf9b120..473201e 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -779,7 +779,8 @@ remove_filename (notmuch_database_t *notmuch,
 	return status;
     status = notmuch_database_find_message_by_filename (notmuch, path, &message);
     if (status || message == NULL)
-	return status;
+	goto DONE;
+
     status = notmuch_database_remove_message (notmuch, path);
     if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	add_files_state->renamed_messages++;
@@ -790,6 +791,8 @@ remove_filename (notmuch_database_t *notmuch,
 	add_files_state->removed_messages++;
     }
     notmuch_message_destroy (message);
+
+  DONE:
     notmuch_database_end_atomic (notmuch);
     return status;
 }
-- 
1.7.9.1

Re: [PATCH v3 1/4] new: Consistently treat fatal errors as fatal

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> Previously, fatal errors in add_files_recursive were not treated as
> fatal by its callers (including itself!).  This makes
> add_files_recursive errors consistently fatal and updates all callers
> to treat them as fatal.

series pushed.

d