[PATCH 1/3] cli: add insert --must-index option

[PATCH 1/3] cli: add insert --must-index option

[Peter Wang]

This option causes notmuch insert to fail as a whole if the message
failed to be added to the notmuch database.  The new message file
will be deleted from disk, and a distinct status code (2) returned.
---
 notmuch-insert.c | 76 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 2207b1e..505b647 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -28,6 +28,10 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+#define INSERT_EXIT_SUCCESS	0
+#define INSERT_EXIT_FAIL_WRITE	1
+#define INSERT_EXIT_FAIL_INDEX	2
+
 static volatile sig_atomic_t interrupted;
 
 static void
@@ -293,12 +297,13 @@ copy_stdin (int fdin, int fdout)
 
 /* Add the specified message file to the notmuch database, applying tags.
  * The file is renamed to encode notmuch tags as maildir flags. */
-static void
+static notmuch_status_t
 add_file_to_database (notmuch_database_t *notmuch, const char *path,
 		      tag_op_list_t *tag_ops)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
+    notmuch_status_t sync;
 
     status = notmuch_database_add_message (notmuch, path, &message);
     switch (status) {
@@ -318,47 +323,52 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
     case NOTMUCH_STATUS_LAST_STATUS:
 	fprintf (stderr, "Error: failed to add `%s' to notmuch database: %s\n",
 		 path, notmuch_status_to_string (status));
-	return;
+	return status;
     }
 
     if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	/* Don't change tags of an existing message. */
-	status = notmuch_message_tags_to_maildir_flags (message);
-	if (status != NOTMUCH_STATUS_SUCCESS)
+	sync = notmuch_message_tags_to_maildir_flags (message);
+	if (sync != NOTMUCH_STATUS_SUCCESS)
 	    fprintf (stderr, "Error: failed to sync tags to maildir flags\n");
     } else {
 	tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);
     }
 
     notmuch_message_destroy (message);
+
+    return status;
 }
 
-static notmuch_bool_t
+static int
 insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
-		const char *dir, tag_op_list_t *tag_ops)
+		const char *dir, tag_op_list_t *tag_ops,
+		notmuch_bool_t must_index)
 {
     char *tmppath;
     char *newpath;
     char *newdir;
     int fdout;
-    char *cleanup_path;
+    notmuch_status_t status;
 
     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
     if (fdout < 0)
-	return FALSE;
+	return INSERT_EXIT_FAIL_WRITE;
 
-    cleanup_path = tmppath;
-
-    if (! copy_stdin (fdin, fdout))
-	goto FAIL;
+    if (! copy_stdin (fdin, fdout)) {
+	close (fdout);
+	unlink (tmppath);
+	return INSERT_EXIT_FAIL_WRITE;
+    }
 
     if (fsync (fdout) != 0) {
 	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
-	goto FAIL;
+	close (fdout);
+	unlink (tmppath);
+	return INSERT_EXIT_FAIL_WRITE;
     }
 
     close (fdout);
-    fdout = -1;
 
     /* Atomically move the new message file from the Maildir 'tmp' directory
      * to the 'new' directory.  We follow the Dovecot recommendation to
@@ -367,25 +377,28 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
      */
     if (rename (tmppath, newpath) != 0) {
 	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
-	goto FAIL;
+	unlink (tmppath);
+	return INSERT_EXIT_FAIL_WRITE;
     }
 
-    cleanup_path = newpath;
-
-    if (! sync_dir (newdir))
-	goto FAIL;
+    if (! sync_dir (newdir)) {
+	unlink (newpath);
+	return INSERT_EXIT_FAIL_WRITE;
+    }
 
-    /* Even if adding the message to the notmuch database fails,
-     * the message is on disk and we consider the delivery completed. */
-    add_file_to_database (notmuch, newpath, tag_ops);
+    status = add_file_to_database (notmuch, newpath, tag_ops);
 
-    return TRUE;
+    /* If must_index is TRUE, then indexing must succeed.  Otherwise, we
+     * consider the delivery completed as long as the message is on disk. */
+    if (must_index &&
+	status != NOTMUCH_STATUS_SUCCESS &&
+	status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+    {
+	unlink (newpath);
+	return INSERT_EXIT_FAIL_INDEX;
+    }
 
-  FAIL:
-    if (fdout >= 0)
-	close (fdout);
-    unlink (cleanup_path);
-    return FALSE;
+    return INSERT_EXIT_SUCCESS;
 }
 
 int
@@ -400,6 +413,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     char *query_string = NULL;
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
+    notmuch_bool_t must_index = FALSE;
     const char *maildir;
     int opt_index;
     unsigned int i;
@@ -408,6 +422,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &must_index, "must-index", 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -471,9 +486,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return 1;
 
-    ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
+    ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops,
+			  must_index);
 
     notmuch_database_destroy (notmuch);
 
-    return (ret) ? 0 : 1;
+    return ret;
 }
-- 
1.7.12.1

[PATCH 2/3] man: document insert --must-index

[Peter Wang]

Add documentation for the new option.
---
 man/man1/notmuch-insert.1 | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/man/man1/notmuch-insert.1 b/man/man1/notmuch-insert.1
index d5202ac..44c18d8 100644
--- a/man/man1/notmuch-insert.1
+++ b/man/man1/notmuch-insert.1
@@ -55,15 +55,42 @@ Otherwise the folder must already exist for mail
 delivery to succeed.
 
 .RE
+
+.RS 4
+.TP 4
+.B "--must-index"
+
+The default behaviour of
+.B notmuch insert
+is to succeed if the message is added to the mail directory, even if
+the message could not be indexed and added to the notmuch database.
+In the latter case, a warning will be printed to standard error but
+the message file will be left on disk.
+
+The
+.B "--must-index"
+option causes
+.B notmuch insert
+to fail if the message could not be added to the notmuch database,
+and the newly-created message file deleted.
+
+Failure to set tags does not consitute a failure of the overall
+command.
+
+.RE
 .SH EXIT STATUS
 
-This command returns exit status 0 if the message was successfully
-added to the mail directory, even if the message could not be indexed
-and added to the notmuch database.  In the latter case, a warning will
-be printed to standard error but the message file will be left on disk.
+This command returns exit status 0 on success.
+On failure, it returns a non-zero exit status.
 
-If the message could not be written to disk then a non-zero exit
-status is returned.
+.TP
+.B 1
+Failed to write the message to disk.
+.TP
+.B 2
+Failed to index the message, and
+.B "--must-index"
+was specified.
 
 .RE
 .SH SEE ALSO
-- 
1.7.12.1

[PATCH 3/3] test: test insert --must-index

[Peter Wang]

Test the new insert --must-index option.
---
 test/insert | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/test/insert b/test/insert
index 021edb6..938753e 100755
--- a/test/insert
+++ b/test/insert
@@ -26,6 +26,9 @@ test_expect_code 1 "Insert zero-length file" \
 test_expect_code 0 "Insert non-message" \
     "echo bad_message | notmuch insert"
 
+test_expect_code 2 "Insert non-message, --must-index" \
+    "echo bad_message | notmuch insert --must-index"
+
 test_begin_subtest "Database empty so far"
 test_expect_equal "0" "`notmuch count --output=messages '*'`"
 
-- 
1.7.12.1

Re: [PATCH 1/3] cli: add insert --must-index option

[Mark Walters]

Do you have a particular use case where indexing is required but tagging
is not? For my uses I would prefer failing if either indexing or tagging
failed. (My use being postponing messages; If they don't get the
postponed tag they could be hard to find)

Best wishes

Mark



Peter Wang <novalazy@gmail.com> writes:

> This option causes notmuch insert to fail as a whole if the message
> failed to be added to the notmuch database.  The new message file
> will be deleted from disk, and a distinct status code (2) returned.
> ---
>  notmuch-insert.c | 76 ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 2207b1e..505b647 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -28,6 +28,10 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  
> +#define INSERT_EXIT_SUCCESS	0
> +#define INSERT_EXIT_FAIL_WRITE	1
> +#define INSERT_EXIT_FAIL_INDEX	2
> +
>  static volatile sig_atomic_t interrupted;
>  
>  static void
> @@ -293,12 +297,13 @@ copy_stdin (int fdin, int fdout)
>  
>  /* Add the specified message file to the notmuch database, applying tags.
>   * The file is renamed to encode notmuch tags as maildir flags. */
> -static void
> +static notmuch_status_t
>  add_file_to_database (notmuch_database_t *notmuch, const char *path,
>  		      tag_op_list_t *tag_ops)
>  {
>      notmuch_message_t *message;
>      notmuch_status_t status;
> +    notmuch_status_t sync;
>  
>      status = notmuch_database_add_message (notmuch, path, &message);
>      switch (status) {
> @@ -318,47 +323,52 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
>      case NOTMUCH_STATUS_LAST_STATUS:
>  	fprintf (stderr, "Error: failed to add `%s' to notmuch database: %s\n",
>  		 path, notmuch_status_to_string (status));
> -	return;
> +	return status;
>      }
>  
>      if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
>  	/* Don't change tags of an existing message. */
> -	status = notmuch_message_tags_to_maildir_flags (message);
> -	if (status != NOTMUCH_STATUS_SUCCESS)
> +	sync = notmuch_message_tags_to_maildir_flags (message);
> +	if (sync != NOTMUCH_STATUS_SUCCESS)
>  	    fprintf (stderr, "Error: failed to sync tags to maildir flags\n");
>      } else {
>  	tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);
>      }
>  
>      notmuch_message_destroy (message);
> +
> +    return status;
>  }
>  
> -static notmuch_bool_t
> +static int
>  insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
> -		const char *dir, tag_op_list_t *tag_ops)
> +		const char *dir, tag_op_list_t *tag_ops,
> +		notmuch_bool_t must_index)
>  {
>      char *tmppath;
>      char *newpath;
>      char *newdir;
>      int fdout;
> -    char *cleanup_path;
> +    notmuch_status_t status;
>  
>      fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
>      if (fdout < 0)
> -	return FALSE;
> +	return INSERT_EXIT_FAIL_WRITE;
>  
> -    cleanup_path = tmppath;
> -
> -    if (! copy_stdin (fdin, fdout))
> -	goto FAIL;
> +    if (! copy_stdin (fdin, fdout)) {
> +	close (fdout);
> +	unlink (tmppath);
> +	return INSERT_EXIT_FAIL_WRITE;
> +    }
>  
>      if (fsync (fdout) != 0) {
>  	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
> -	goto FAIL;
> +	close (fdout);
> +	unlink (tmppath);
> +	return INSERT_EXIT_FAIL_WRITE;
>      }
>  
>      close (fdout);
> -    fdout = -1;
>  
>      /* Atomically move the new message file from the Maildir 'tmp' directory
>       * to the 'new' directory.  We follow the Dovecot recommendation to
> @@ -367,25 +377,28 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
>       */
>      if (rename (tmppath, newpath) != 0) {
>  	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
> -	goto FAIL;
> +	unlink (tmppath);
> +	return INSERT_EXIT_FAIL_WRITE;
>      }
>  
> -    cleanup_path = newpath;
> -
> -    if (! sync_dir (newdir))
> -	goto FAIL;
> +    if (! sync_dir (newdir)) {
> +	unlink (newpath);
> +	return INSERT_EXIT_FAIL_WRITE;
> +    }
>  
> -    /* Even if adding the message to the notmuch database fails,
> -     * the message is on disk and we consider the delivery completed. */
> -    add_file_to_database (notmuch, newpath, tag_ops);
> +    status = add_file_to_database (notmuch, newpath, tag_ops);
>  
> -    return TRUE;
> +    /* If must_index is TRUE, then indexing must succeed.  Otherwise, we
> +     * consider the delivery completed as long as the message is on disk. */
> +    if (must_index &&
> +	status != NOTMUCH_STATUS_SUCCESS &&
> +	status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
> +    {
> +	unlink (newpath);
> +	return INSERT_EXIT_FAIL_INDEX;
> +    }
>  
> -  FAIL:
> -    if (fdout >= 0)
> -	close (fdout);
> -    unlink (cleanup_path);
> -    return FALSE;
> +    return INSERT_EXIT_SUCCESS;
>  }
>  
>  int
> @@ -400,6 +413,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      char *query_string = NULL;
>      const char *folder = NULL;
>      notmuch_bool_t create_folder = FALSE;
> +    notmuch_bool_t must_index = FALSE;
>      const char *maildir;
>      int opt_index;
>      unsigned int i;
> @@ -408,6 +422,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      notmuch_opt_desc_t options[] = {
>  	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
> +	{ NOTMUCH_OPT_BOOLEAN, &must_index, "must-index", 0, 0 },
>  	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
>      };
>  
> @@ -471,9 +486,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>  			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>  	return 1;
>  
> -    ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
> +    ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops,
> +			  must_index);
>  
>      notmuch_database_destroy (notmuch);
>  
> -    return (ret) ? 0 : 1;
> +    return ret;
>  }
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/3] cli: add insert --must-index option

[Peter Wang]

On Sun, 21 Jul 2013 09:31:28 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> 
> Do you have a particular use case where indexing is required but tagging
> is not? For my uses I would prefer failing if either indexing or tagging
> failed. (My use being postponing messages; If they don't get the
> postponed tag they could be hard to find)

You're right.

What about a failure to sync tags to maildir flags?

I now noticed that database modifications aren't flushed until the
notmuch_database_destroy call (right?), which has no return value and
failure of which is silently ignored.  That's acceptable in the default
mode, but with --must-index the failure should be reported (and the
file deleted).

Peter

Re: [PATCH 1/3] cli: add insert --must-index option

[Mark Walters]

Hi

>> Do you have a particular use case where indexing is required but tagging
>> is not? For my uses I would prefer failing if either indexing or tagging
>> failed. (My use being postponing messages; If they don't get the
>> postponed tag they could be hard to find)
>
> You're right.
>
> What about a failure to sync tags to maildir flags?

Personally, I wouldn't mind ignoring this failure: it should be
relatively easy to fix after the fact (but others may disagree).

> I now noticed that database modifications aren't flushed until the
> notmuch_database_destroy call (right?), which has no return value and
> failure of which is silently ignored.  That's acceptable in the default
> mode, but with --must-index the failure should be reported (and the
> file deleted).

Yes I think you are right: flushed by notmuch_database_close which is
called by notmuch_database_destroy.

Perhaps the easiest would be to add a notmuch_database_flush with a
return value and then you can call that (and then call
notmuch_database_destroy)? 

Alternatively maybe add notmuch_database_destroy_with_flush or something
which does give a return value. notmuch_database_close is only called 3
times and notmuch_database_destroy lots of times so changing close is
much less intrusive than changing destroy. But I don't know whether we
would break any  bindings or external programs etc.

What do you think?

Best wishes

Mark

Re: [PATCH 1/3] cli: add insert --must-index option

[Peter Wang]

On Tue, 10 Sep 2013 09:06:00 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
> 
> Alternatively maybe add notmuch_database_destroy_with_flush or something
> which does give a return value. notmuch_database_close is only called 3
> times and notmuch_database_destroy lots of times so changing close is
> much less intrusive than changing destroy. But I don't know whether we
> would break any  bindings or external programs etc.
> 
> What do you think?

I think notmuch_database_close and notmuch_database_destroy should
return the status, and we update the three language bindings
and bump the soname.

Peter

Re: [PATCH 1/3] cli: add insert --must-index option

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> On Tue, 10 Sep 2013 09:06:00 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>> 
>> Alternatively maybe add notmuch_database_destroy_with_flush or something
>> which does give a return value. notmuch_database_close is only called 3
>> times and notmuch_database_destroy lots of times so changing close is
>> much less intrusive than changing destroy. But I don't know whether we
>> would break any  bindings or external programs etc.
>> 
>> What do you think?
>
> I think notmuch_database_close and notmuch_database_destroy should
> return the status, and we update the three language bindings
> and bump the soname.
>

I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
breaking changes that we have been holding back on? Can these maybe go
through at the same time?

d

Re: [PATCH 1/3] cli: add insert --must-index option

[Tomi Ollila]

On Thu, Oct 10 2013, David Bremner <david@tethera.net> wrote:

> Peter Wang <novalazy@gmail.com> writes:
>
>> On Tue, 10 Sep 2013 09:06:00 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>>> 
>>> Alternatively maybe add notmuch_database_destroy_with_flush or something
>>> which does give a return value. notmuch_database_close is only called 3
>>> times and notmuch_database_destroy lots of times so changing close is
>>> much less intrusive than changing destroy. But I don't know whether we
>>> would break any  bindings or external programs etc.
>>> 
>>> What do you think?
>>
>> I think notmuch_database_close and notmuch_database_destroy should
>> return the status, and we update the three language bindings
>> and bump the soname.
>>
>
> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
> breaking changes that we have been holding back on? Can these maybe go
> through at the same time?

Maybe something along these lines...

(Quick draft for the API part; to start discussion before working too much
for something that is going to be dropped...)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 9dab555..ae52dab 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -106,6 +106,17 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
 
+/* Structure to provide logging interface to the notmuch library
+ *
+ * ...
+ */
+
+typedef struct _notmuch_loghook {
+    void (*func)(struct _notmuch_loghook *, int level, int status,
+		 const char * format, ...);
+} notmuch_loghook_t;
+
+
 /* Get a string representation of a notmuch_status_t value.
  *
  * The result is read-only.
@@ -159,7 +170,9 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 notmuch_status_t
-notmuch_database_create (const char *path, notmuch_database_t **database);
+notmuch_database_create (const char *path,
+			 notmuch_loghook_t *loghook,
+			 notmuch_database_t **database);
 
 typedef enum {
     NOTMUCH_DATABASE_MODE_READ_ONLY = 0,
@@ -200,6 +213,7 @@ typedef enum {
 notmuch_status_t
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
+		       notmuch_loghook_t *loghook,
 		       notmuch_database_t **database);
 
 /* Close the given notmuch database.

>
> d

Tomi

Re: [PATCH 1/3] cli: add insert --must-index option

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
>> breaking changes that we have been holding back on? Can these maybe go
>> through at the same time?
>
> Maybe something along these lines...
>
> (Quick draft for the API part; to start discussion before working too much
> for something that is going to be dropped...)
>
>  notmuch_status_t
> -notmuch_database_create (const char *path, notmuch_database_t **database);
> +notmuch_database_create (const char *path,
> +			 notmuch_loghook_t *loghook,
> +			 notmuch_database_t **database);

Another idea floated (by Austin?) was to pass in an options struct, to
allow future options to be added without changing the function
signature. I guess with some care this could be done in an upwardly
compatible way.

d

Re: [PATCH 1/3] cli: add insert --must-index option

[Tomi Ollila]

On Thu, Oct 10 2013, David Bremner <david@tethera.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
>>> breaking changes that we have been holding back on? Can these maybe go
>>> through at the same time?
>>
>> Maybe something along these lines...
>>
>> (Quick draft for the API part; to start discussion before working too much
>> for something that is going to be dropped...)
>>
>>  notmuch_status_t
>> -notmuch_database_create (const char *path, notmuch_database_t **database);
>> +notmuch_database_create (const char *path,
>> +			 notmuch_loghook_t *loghook,
>> +			 notmuch_database_t **database);
>
> Another idea floated (by Austin?) was to pass in an options struct, to
> allow future options to be added without changing the function
> signature. I guess with some care this could be done in an upwardly
> compatible way.

Maybe something like

#define NOTMUCH_API_OPTIONS_VERSION 1
typedef struct {
        int options_version;
        void (*log)(void *, int level, int status, const char * format, ...);
        void * logdata;
} notmuch_options_t;        

...

notmuch_status_t
notmuch_database_create (const char *path,
			 notmuch_options_t *options,
			 notmuch_database_t **database);
...

notmuch_status_t
notmuch_database_open (const char *path,
                       notmuch_database_mode_t mode,
                       notmuch_options_t *options,
                       notmuch_database_t **database);

then in use:

notmuch_options_t options = {
       .options_version = NOTMUCH_API_OPTIONS_VERSION,

.. äsh, this has problem that the macro changes in header file
but the structure initialization is not following automatically
(in other fields than that). Therefore perhaps "fixing"
the version macros:

#define NOTMUCH_API_OPTIONS_VERSION_1 1
/* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */

notmuch_options_t options = {
       .options_version = NOTMUCH_API_OPTIONS_VERSION_1,
       .log = log_to_stderr,
       .logdata = NULL
};

Well, this is one idea (does not sound as good as I initially
thought...) how does other software tackle this kind of issues...

If something like this is finally chosen we could provide easy
transition path to allow NULL as options -- making notmuch CLI
behave as it used to be (log to stderr...).


> d

Tomi

Re: [PATCH 1/3] cli: add insert --must-index option

[Austin Clements]

Quoth Tomi Ollila on Oct 23 at 10:05 pm:
> On Thu, Oct 10 2013, David Bremner <david@tethera.net> wrote:
> 
> > Tomi Ollila <tomi.ollila@iki.fi> writes:
> >>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
> >>> breaking changes that we have been holding back on? Can these maybe go
> >>> through at the same time?
> >>
> >> Maybe something along these lines...
> >>
> >> (Quick draft for the API part; to start discussion before working too much
> >> for something that is going to be dropped...)
> >>
> >>  notmuch_status_t
> >> -notmuch_database_create (const char *path, notmuch_database_t **database);
> >> +notmuch_database_create (const char *path,
> >> +			 notmuch_loghook_t *loghook,
> >> +			 notmuch_database_t **database);
> >
> > Another idea floated (by Austin?) was to pass in an options struct, to
> > allow future options to be added without changing the function
> > signature. I guess with some care this could be done in an upwardly
> > compatible way.
> 
> Maybe something like
> 
> #define NOTMUCH_API_OPTIONS_VERSION 1
> typedef struct {
>         int options_version;
>         void (*log)(void *, int level, int status, const char * format, ...);
>         void * logdata;
> } notmuch_options_t;        
> 
> ...
> 
> notmuch_status_t
> notmuch_database_create (const char *path,
> 			 notmuch_options_t *options,
> 			 notmuch_database_t **database);
> ...
> 
> notmuch_status_t
> notmuch_database_open (const char *path,
>                        notmuch_database_mode_t mode,
>                        notmuch_options_t *options,
>                        notmuch_database_t **database);
> 
> then in use:
> 
> notmuch_options_t options = {
>        .options_version = NOTMUCH_API_OPTIONS_VERSION,
> 
> .. äsh, this has problem that the macro changes in header file
> but the structure initialization is not following automatically
> (in other fields than that). Therefore perhaps "fixing"
> the version macros:
> 
> #define NOTMUCH_API_OPTIONS_VERSION_1 1
> /* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */
> 
> notmuch_options_t options = {
>        .options_version = NOTMUCH_API_OPTIONS_VERSION_1,
>        .log = log_to_stderr,
>        .logdata = NULL
> };
> 
> Well, this is one idea (does not sound as good as I initially
> thought...) how does other software tackle this kind of issues...
> 
> If something like this is finally chosen we could provide easy
> transition path to allow NULL as options -- making notmuch CLI
> behave as it used to be (log to stderr...).

Using a version number on the options makes it tricky to maintain
backwards compatibility, since you need code to read all past versions
of the structure.  A similar but, I think, better way would be to take
the size of the structure in the structure.  Something like:

typedef struct {
    size_t options_length;
    ...
} notmuch_options_t;

#define NOTMUCH_OPTIONS_INIT { sizeof(notmuch_options_t) }
// or without pre-processor, but requiring GCC attributes
static __attribute__((__unused__))
notmuch_options_t notmuch_options_init = { sizeof(notmuch_options_t) };

NOTMUCH_OPTIONS_INIT/notmuch_options_init could also contain other
defaults, though it's best if the defaults are simply the zero
initializations of the various fields.


Then, in code calling libnotmuch, you'd do something like

notmuch_options_t options = NOTMUCH_OPTIONS_INIT;
options.log = log_to_stderr;
// ...


And in libnotmuch, we would do something like

notmuch_status_t
notmuch_database_open (const char *path,
                       notmuch_database_mode_t mode,
                       const notmuch_options_t *options,
                       notmuch_database_t **database)
{
    notmuch_option_t real_options = NOTMUCH_OPTIONS_INIT;
    if (real_options.options_length < options.options_length)
        return error;
    memmove(&real_options, options, options.options_length);
    // ...
}

This approach makes it free to add fields and we can always deprecate
fields as long as we leave the space in the structure.  This is based
roughly on how the Linux kernel deals with various structures that
have grown over time in the syscall ABI.

Another much more verbose but also more robust approach would be to
make notmuch_options_t opaque and provide setters (and getters, if
we're feeling benevolent).  I kind of like the simplicity of a struct.

One thing to consider is what works best with bindings.  Presumably
the opaque structure wouldn't be a problem.  I've never had to wrap
anything like the struct approach, though, so I don't know if that's
easy or hard.

Re: [PATCH 1/3] cli: add insert --must-index option

[Tomi Ollila]

On Wed, Oct 23 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Tomi Ollila on Oct 23 at 10:05 pm:
>> On Thu, Oct 10 2013, David Bremner <david@tethera.net> wrote:
>> 
>> > Tomi Ollila <tomi.ollila@iki.fi> writes:
>> >>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
>> >>> breaking changes that we have been holding back on? Can these maybe go
>> >>> through at the same time?
>> >>
>> >> Maybe something along these lines...
>> >>
>> >> (Quick draft for the API part; to start discussion before working too much
>> >> for something that is going to be dropped...)
>> >>
>> >>  notmuch_status_t
>> >> -notmuch_database_create (const char *path, notmuch_database_t **database);
>> >> +notmuch_database_create (const char *path,
>> >> +			 notmuch_loghook_t *loghook,
>> >> +			 notmuch_database_t **database);
>> >
>> > Another idea floated (by Austin?) was to pass in an options struct, to
>> > allow future options to be added without changing the function
>> > signature. I guess with some care this could be done in an upwardly
>> > compatible way.
>> 
>> Maybe something like
>> 
>> #define NOTMUCH_API_OPTIONS_VERSION 1
>> typedef struct {
>>         int options_version;
>>         void (*log)(void *, int level, int status, const char * format, ...);
>>         void * logdata;
>> } notmuch_options_t;        
>> 
>> ...
>> 
>> notmuch_status_t
>> notmuch_database_create (const char *path,
>> 			 notmuch_options_t *options,
>> 			 notmuch_database_t **database);
>> ...
>> 
>> notmuch_status_t
>> notmuch_database_open (const char *path,
>>                        notmuch_database_mode_t mode,
>>                        notmuch_options_t *options,
>>                        notmuch_database_t **database);
>> 
>> then in use:
>> 
>> notmuch_options_t options = {
>>        .options_version = NOTMUCH_API_OPTIONS_VERSION,
>> 
>> .. äsh, this has problem that the macro changes in header file
>> but the structure initialization is not following automatically
>> (in other fields than that). Therefore perhaps "fixing"
>> the version macros:
>> 
>> #define NOTMUCH_API_OPTIONS_VERSION_1 1
>> /* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */
>> 
>> notmuch_options_t options = {
>>        .options_version = NOTMUCH_API_OPTIONS_VERSION_1,
>>        .log = log_to_stderr,
>>        .logdata = NULL
>> };
>> 
>> Well, this is one idea (does not sound as good as I initially
>> thought...) how does other software tackle this kind of issues...
>> 
>> If something like this is finally chosen we could provide easy
>> transition path to allow NULL as options -- making notmuch CLI
>> behave as it used to be (log to stderr...).
>
> Using a version number on the options makes it tricky to maintain
> backwards compatibility, since you need code to read all past versions
> of the structure.  A similar but, I think, better way would be to take
> the size of the structure in the structure.  Something like:
>
> typedef struct {
>     size_t options_length;
>     ...
> } notmuch_options_t;

I thought this option too, but went to this version number scheme
for more possibilities. We could discipline the backwards compatibility
by always expecting old fields being the same and requiring structure
to extend when version changes -- but could divert from that if absolutely
necessary... But this would minimally require storing all the version
structures and have version mapping there... But as this is more complex
than your suggestion let's think it as the current solution of choice...

> #define NOTMUCH_OPTIONS_INIT { sizeof(notmuch_options_t) }
> // or without pre-processor, but requiring GCC attributes
> static __attribute__((__unused__))
> notmuch_options_t notmuch_options_init = { sizeof(notmuch_options_t) };
>
> NOTMUCH_OPTIONS_INIT/notmuch_options_init could also contain other
> defaults, though it's best if the defaults are simply the zero
> initializations of the various fields.

Perhaps we should do 
#define NOTMUCH_OPTIONS_INIT { .options_length = sizeof (notmuch_options_t) }

IIRC this C99 initialization format makes rest of the structure initialize
to zeros (I don't know/remember whether this is true in the one you
suggested).

Whenever new fields are added to the options structure they will always be
zero in old code that doesn't know those fields -- therefore zero needs
always be sensible default for every field. If we want to support setting
new fields based on their existence then we need to add something like
#define NOTMUCH_OPTIONS_VER 1 (or 20131024) so that code can #ifdef setting
those and still support older notmuch versions...


> Then, in code calling libnotmuch, you'd do something like
>
> notmuch_options_t options = NOTMUCH_OPTIONS_INIT;
> options.log = log_to_stderr;
> // ...
>
>
> And in libnotmuch, we would do something like
>
> notmuch_status_t
> notmuch_database_open (const char *path,
>                        notmuch_database_mode_t mode,
>                        const notmuch_options_t *options,
>                        notmuch_database_t **database)

Ah, the 'const'. I had the same in mind but forgot when doing it...

> {
>     notmuch_option_t real_options = NOTMUCH_OPTIONS_INIT;
>     if (real_options.options_length < options.options_length)
>         return error;
>     memmove(&real_options, options, options.options_length);
>     // ...
> }
>
> This approach makes it free to add fields and we can always deprecate
> fields as long as we leave the space in the structure.  This is based
> roughly on how the Linux kernel deals with various structures that
> have grown over time in the syscall ABI.

Ack, good reference.

> Another much more verbose but also more robust approach would be to
> make notmuch_options_t opaque and provide setters (and getters, if
> we're feeling benevolent).  I kind of like the simplicity of a struct.
>
> One thing to consider is what works best with bindings.  Presumably
> the opaque structure wouldn't be a problem.  I've never had to wrap
> anything like the struct approach, though, so I don't know if that's
> easy or hard.

Ruby & Go bindings use C wrapper(s), Python ctypes(*). Attaching working 
function pointer (& possibly data) can provide to be nontrivial challenge ;/
We can provide some support code and/or structures in the library (in
addtion/place of options as NULL...

We'd like to hear comments from bindings' experts/developers.

Tomi


(*) A small attempt to show how c structures are available in python ctypes: 

from ctypes import Structure, c_size_t

class notmuch_options_t(Structure):
    _fields__ = [ ('options_length', c_size_t), ... ]

... adding function pointer to the Structure goes beyond my
current skills (we cannot assume function pointer has the same size as
(void) data pointer...)

Re: [PATCH 1/3] cli: add insert --must-index option

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> And in libnotmuch, we would do something like
>
> notmuch_status_t
> notmuch_database_open (const char *path,
>                        notmuch_database_mode_t mode,
>                        const notmuch_options_t *options,
>                        notmuch_database_t **database)
> {
>     notmuch_option_t real_options = NOTMUCH_OPTIONS_INIT;
>     if (real_options.options_length < options.options_length)
>         return error;
>     memmove(&real_options, options, options.options_length);
>     // ...
> }
>

Does the C standard guarantee that if two structs have the same initial
set of members, that they are aligned in a compatible way? I suppose it
must work, but I'm still curious.

Yet another approach would be to pass in array of descriptors, something
like the command line argument parsing code does now.

libnotmuch_opt_desc_t options[] = {
  { LIBNOTMUCH_OPT_LOGHOOK, loghook },
  { 0, 0} 
}

I guess passing a (void *) as the second element of the pair?

Of course it's a bit more work to unpack this way.

Re: [PATCH 1/3] cli: add insert --must-index option

[Tomi Ollila]

On Thu, Oct 24 2013, David Bremner <david@tethera.net> wrote:

> Austin Clements <amdragon@MIT.EDU> writes:
>
>> And in libnotmuch, we would do something like
>>
>> notmuch_status_t
>> notmuch_database_open (const char *path,
>>                        notmuch_database_mode_t mode,
>>                        const notmuch_options_t *options,
>>                        notmuch_database_t **database)
>> {
>>     notmuch_option_t real_options = NOTMUCH_OPTIONS_INIT;
>>     if (real_options.options_length < options.options_length)
>>         return error;
>>     memmove(&real_options, options, options.options_length);
>>     // ...
>> }
>>
>
> Does the C standard guarantee that if two structs have the same initial
> set of members, that they are aligned in a compatible way? I suppose it
> must work, but I'm still curious.

If we have structures

struct s1 { 
       long l1;
       void * vp1;
       short s1;
       int i1;
       char c1;
}

&

struct s2 { 
       long l1;
       void * vp1;
       short s1;
       int i1;
       char c1;
       char c2;
       // void * vp2;
};

The variables l1, vp1, s1, i1 & c1 have same offset from the beginning of
structure and uses same amount of space in both structures...

... now the interesting point is what is the size of the structures (does C
compiler add padding to the end (to make the size multiple of something ?)
In any case all the other data is zeroed in the structure and sizeof data
is copied to "real" structure if Austin's suggestion is used...

> Yet another approach would be to pass in array of descriptors, something
> like the command line argument parsing code does now.
>
> libnotmuch_opt_desc_t options[] = {
>   { LIBNOTMUCH_OPT_LOGHOOK, loghook },
>   { 0, 0} 
> }
>
> I guess passing a (void *) as the second element of the pair?

Voidp and only that. Then we can hack other variables using
((char *)0 + (intvar)) or GINT_TO_POINTER... -- beautiful, eh ? ;)

> Of course it's a bit more work to unpack this way.

That is negligible to the elegance loss -- and what about bindings ! >;)


Tomi