[PATCH 2/3] new: Add all initial tags at once

[Thomas Schwinge]

Hallo!

On Fri, 21 Jan 2011 10:59:36 +0100, Michal Sojka <sojkam1 at fel.cvut.cz> wrote:
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -418,6 +418,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>  	/* success */
>  	case NOTMUCH_STATUS_SUCCESS:
>  	    state->added_messages++;
> +	    notmuch_message_freeze (message);
>  	    for (tag=state->new_tags; *tag != NULL; tag++)
>  	        notmuch_message_add_tag (message, *tag);
>  	    if (state->synchronize_flags == TRUE) {
> @@ -433,6 +434,7 @@ add_files_recursive (notmuch_database_t *notmuch,
>  		    notmuch_message_maildir_flags_to_tags (message);
>  		}
>  	    }
> +	    notmuch_message_thaw (message);
>  	    break;
>  	/* Non-fatal issues (go on to next file) */
>  	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:

I do support the patch's idea (which was recently committed; and what
follows in this message is not at all directed towards Michal, who wrote
this patch) -- but what about return values checking?  This is one aspect
of the notmuch C code (which I generally consider to be nice to read and
of high quality, as I said before already), that I consider totally
lacking -- there are literally hundreds of C functions calls where the
return values are just discarded.  This is bad.  For example (simulating
a full disk):

    $ notmuch dump > /dev/full
    $ echo $?
    0

The command returned ``success'' -- but no data has been saved.  This
could, in some extreme (but still likely) case mean: total tag data loss.
(This is likely due to printf / close or fprintf / fclose (yes, really,
(f)close! -- think of buffering) not getting their return values
checked.)  Here is what is expected:

    $ echo foo > /dev/full
    bash: echo: write error: No space left on device
    $ echo $?
    1

Then, in the few (!) lines quoted above, there are (exactly / only) calls
to notmuch_message_freeze, notmuch_message_add_tag,
notmuch_message_maildir_flags_to_tags, notmuch_message_thaw -- each of
which can fail, will return this via the notmuch_status_t return value
(whose possible values are documented, too), but none has their return
value checked.

Other languages have the concept of exceptions; C doesn't, so we're
supposed to put some ``ABORT_IF_NOT_NOTMUCH_STATUS_SUCCESS(ret)''
statements after each and every non-void (etc.) C function call.  Or make
the functions abort themselves (which is not a too good idea, as we
surely agree).  Or use a different programming language -- now, at the
present state, it wouldn't be too painful to switch, in my opinion.  (I
won't suggest any specific language, though.)  If staying with C (which I
don't object, either), then this needs a whole code audit, and a lot of
discipline in the future.

Comments?  (And I hope this doesn't sound too harsh :-) -- but it is a
serious programming issue.)


Grüße,
 Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110126/5e44cba0/attachment.pgp>