[RFC PATCH] lib: document new database_open API

[RFC PATCH] lib: document new database_open API

[David Bremner]

Several aspects of this are potentially controversial:

1) The use of environment variables as fallback. I understand the
discomfort with having a library function check the environment, but
this seems to be functionality people want, and it is better to
implement it once.

2) The use of both NULL and "" to do different things for config_path.

3) The new API is pretty complex, compared to the previous one.
---

There is not much code to back this so far. This is just me thinking
out loud at this point.  The location calculation is done (and also
easy).  The challenging part is probably updating
notmuch_database_get_config to do what this comment promises.

I suspect database_create will probably need to be updated to match.

Another question is if we should have an opaque set of options to pass
to open (in the style notmuch_indexopts_t).  Currently this is just
future proofing as far as I know.

 lib/notmuch.h | 138 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 111 insertions(+), 27 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index ceb5a018..6a46b80a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -301,52 +301,136 @@ typedef enum {
 } notmuch_database_mode_t;
 
 /**
- * Open an existing notmuch database located at 'path'.
+ * Deprecated alias for notmuch_database_open_with_config with
+ * config_path=error_message=NULL
+ * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
+ */
+NOTMUCH_DEPRECATED(5, 2)
+notmuch_status_t
+notmuch_database_open (const char *path,
+		       notmuch_database_mode_t mode,
+		       notmuch_database_t **database);
+/**
+ * Deprecated alias for notmuch_database_open_with_config with
+ * config_path=NULL
+ *
+ * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
+ *
+ */
+NOTMUCH_DEPRECATED(5, 2)
+notmuch_status_t
+notmuch_database_open_verbose (const char *path,
+			       notmuch_database_mode_t mode,
+			       notmuch_database_t **database,
+			       char **error_message);
+
+/**
+ * Open an existing notmuch database located at 'database_path', using
+ * configuration in 'config_path'.
+ *
+ * @param[in]	database_path
+ * @parblock
+ * Path to existing database.
+ *
+ * A notmuch database is a Xapian database containing appropriate
+ * metadata.
  *
  * The database should have been created at some time in the past,
  * (not necessarily by this process), by calling
- * notmuch_database_create with 'path'. By default the database should be
- * opened for reading only. In order to write to the database you need to
- * pass the NOTMUCH_DATABASE_MODE_READ_WRITE mode.
+ * notmuch_database_create.
+ *
+ * If 'database_path' is NULL, use the location specified
+ *
+ * - in the environment variable NOTMUCH_DATABASE, if non-empty
+ *
+ * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
+ *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
+ *   "default"
+ *
+ * If 'database_path' is non-NULL, but does not appear to be a Xapian
+ * database, check for a directory '.notmuch/xapian' below
+ * 'database_path'.
+ *
+ * @endparblock
+ * @param[in]	mode
+ * @parblock
+ * Mode to open database
+ *
+ * By default the database will be opened for reading only. In order
+ * to write to the database you need to pass the
+ * #NOTMUCH_DATABASE_MODE_READ_WRITE mode.
+ *
+ * @endparblock
+ * @param[in]  config_path
+ * @parblock
+ * Path to config file.
+ *
+ * Config file is key-value, with mandatory sections. See
+ * <em>notmuch-config(5)</em> for more information. The key-value pair
+ * overrides the corresponding configuration data stored in the
+ * database (see <em>notmuch_database_get_config</em>)
  *
- * An existing notmuch database can be identified by the presence of a
- * directory named ".notmuch" below 'path'.
+ * If <em>config_path</em> is NULL use the path specified
+ *
+ * - in environment variable <em>NOTMUCH_CONFIG</em>, if non-empty
+ *
+ * - by  <em>XDG_CONFIG_HOME</em>/notmuch/ where
+ *   XDG_CONFIG_HOME defaults to "$HOME/.config".
+ *
+ * - by $HOME/.notmuch-config
+ *
+ * If <em>config_path</em> is "" (empty string) then do not
+ * open any configuration file.
+ * @endparblock
+ * @param[in] profile:
+ * @parblock
+ * Name of profile (configuration/database variant).
+ *
+ * If non-NULL, append to the directory / file path determined by
+ * <em>config_path</em>.
+ *
+ * If NULL then use
+ * - environment variable NOTMUCH_PROFILE if defined,
+ * - otherwise "default" for directories and "" (empty string) for paths.
+ *
+ * @endparblock
+ * @param[out] database
+ * @parblock
+ * Pointer to database object. May not be NULL.
  *
  * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns an error status and
- * sets *database to NULL (after printing an error message on stderr).
+ * sets *database to NULL.
  *
- * Return value:
+ * @endparblock
+ * @param[out] error_message
+ * If non-NULL, store error message from opening the database.
+ * Any such message is allocated by \a malloc(3) and should be freed
+ * by the caller.
  *
- * NOTMUCH_STATUS_SUCCESS: Successfully opened the database.
+ * @retval NOTMUCH_STATUS_SUCCESS: Successfully opened the database.
  *
- * NOTMUCH_STATUS_NULL_POINTER: The given 'path' argument is NULL.
+ * @retval NOTMUCH_STATUS_NULL_POINTER: The given \a database
+ * argument is NULL.
  *
- * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ * @retval NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
  *
- * NOTMUCH_STATUS_FILE_ERROR: An error occurred trying to open the
- *	database file (such as permission denied, or file not found,
+ * @retval NOTMUCH_STATUS_FILE_ERROR: An error occurred trying to open the
+ *	database or config file (such as permission denied, or file not found,
  *	etc.), or the database version is unknown.
  *
- * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
- */
-notmuch_status_t
-notmuch_database_open (const char *path,
-		       notmuch_database_mode_t mode,
-		       notmuch_database_t **database);
-/**
- * Like notmuch_database_open, except optionally return an error
- * message. This message is allocated by malloc and should be freed by
- * the caller.
+ * @retval NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 
 notmuch_status_t
-notmuch_database_open_verbose (const char *path,
-			       notmuch_database_mode_t mode,
-			       notmuch_database_t **database,
-			       char **error_message);
+notmuch_database_open_with_config (const char *database_path,
+				   notmuch_database_mode_t mode,
+				   const char *config_path,
+				   const char *profile,
+				   notmuch_database_t **database,
+				   char **error_message);
 
 /**
  * Retrieve last status string for given database.
-- 
2.27.0
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [RFC PATCH] lib: document new database_open API

[David Bremner]

David Bremner <david@tethera.net> writes:

> There is not much code to back this so far. This is just me thinking
> out loud at this point.  The location calculation is done (and also
> easy).  The challenging part is probably updating
> notmuch_database_get_config to do what this comment promises.

There's always something I forget.

If you want to look at this in a more friendly way

1) apply the patch
2) make build-man
3) man -l doc/_build/man/man3/notmuch.3
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [RFC PATCH] lib: document new database_open API

[Floris Bruynooghe]

On Fri 03 Jul 2020 at 10:43 -0300, David Bremner wrote:

> Several aspects of this are potentially controversial:
>
> 1) The use of environment variables as fallback. I understand the
> discomfort with having a library function check the environment, but
> this seems to be functionality people want, and it is better to
> implement it once.
>
> 2) The use of both NULL and "" to do different things for config_path.
>
> 3) The new API is pretty complex, compared to the previous one.
> ---
>
> There is not much code to back this so far. This is just me thinking
> out loud at this point.  The location calculation is done (and also
> easy).  The challenging part is probably updating
> notmuch_database_get_config to do what this comment promises.
>
> I suspect database_create will probably need to be updated to match.
>
> Another question is if we should have an opaque set of options to pass
> to open (in the style notmuch_indexopts_t).  Currently this is just
> future proofing as far as I know.
>
>  lib/notmuch.h | 138 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 111 insertions(+), 27 deletions(-)
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index ceb5a018..6a46b80a 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -301,52 +301,136 @@ typedef enum {
>  } notmuch_database_mode_t;
>  
>  /**
> - * Open an existing notmuch database located at 'path'.
> + * Deprecated alias for notmuch_database_open_with_config with
> + * config_path=error_message=NULL
> + * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
> + */
> +NOTMUCH_DEPRECATED(5, 2)
> +notmuch_status_t
> +notmuch_database_open (const char *path,
> +		       notmuch_database_mode_t mode,
> +		       notmuch_database_t **database);
> +/**
> + * Deprecated alias for notmuch_database_open_with_config with
> + * config_path=NULL
> + *
> + * @deprecated Deprecated as of libnotmuch 5.2 (notmuch 0.31)
> + *
> + */
> +NOTMUCH_DEPRECATED(5, 2)
> +notmuch_status_t
> +notmuch_database_open_verbose (const char *path,
> +			       notmuch_database_mode_t mode,
> +			       notmuch_database_t **database,
> +			       char **error_message);
> +
> +/**
> + * Open an existing notmuch database located at 'database_path', using
> + * configuration in 'config_path'.
> + *
> + * @param[in]	database_path
> + * @parblock
> + * Path to existing database.
> + *
> + * A notmuch database is a Xapian database containing appropriate
> + * metadata.
>   *
>   * The database should have been created at some time in the past,
>   * (not necessarily by this process), by calling
> - * notmuch_database_create with 'path'. By default the database should be
> - * opened for reading only. In order to write to the database you need to
> - * pass the NOTMUCH_DATABASE_MODE_READ_WRITE mode.
> + * notmuch_database_create.
> + *
> + * If 'database_path' is NULL, use the location specified
> + *
> + * - in the environment variable NOTMUCH_DATABASE, if non-empty
> + *
> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
> + *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
> + *   "default"

I like the profile support, is the plan for
$XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the
database?

> + *
> + * If 'database_path' is non-NULL, but does not appear to be a Xapian
> + * database, check for a directory '.notmuch/xapian' below
> + * 'database_path'.
> + *
> + * @endparblock
> + * @param[in]	mode
> + * @parblock
> + * Mode to open database
> + *
> + * By default the database will be opened for reading only. In order
> + * to write to the database you need to pass the
> + * #NOTMUCH_DATABASE_MODE_READ_WRITE mode.
> + *
> + * @endparblock
> + * @param[in]  config_path
> + * @parblock
> + * Path to config file.
> + *
> + * Config file is key-value, with mandatory sections. See
> + * <em>notmuch-config(5)</em> for more information. The key-value pair
> + * overrides the corresponding configuration data stored in the
> + * database (see <em>notmuch_database_get_config</em>)
>   *
> - * An existing notmuch database can be identified by the presence of a
> - * directory named ".notmuch" below 'path'.
> + * If <em>config_path</em> is NULL use the path specified
> + *
> + * - in environment variable <em>NOTMUCH_CONFIG</em>, if non-empty
> + *
> + * - by  <em>XDG_CONFIG_HOME</em>/notmuch/ where
> + *   XDG_CONFIG_HOME defaults to "$HOME/.config".
> + *
> + * - by $HOME/.notmuch-config
> + *
> + * If <em>config_path</em> is "" (empty string) then do not
> + * open any configuration file.
> + * @endparblock
> + * @param[in] profile:
> + * @parblock
> + * Name of profile (configuration/database variant).
> + *
> + * If non-NULL, append to the directory / file path determined by
> + * <em>config_path</em>.
> + *
> + * If NULL then use
> + * - environment variable NOTMUCH_PROFILE if defined,
> + * - otherwise "default" for directories and "" (empty string) for paths.
> + *
> + * @endparblock
> + * @param[out] database
> + * @parblock
> + * Pointer to database object. May not be NULL.
>   *
>   * The caller should call notmuch_database_destroy when finished with
>   * this database.
>   *
>   * In case of any failure, this function returns an error status and
> - * sets *database to NULL (after printing an error message on stderr).
> + * sets *database to NULL.
>   *
> - * Return value:
> + * @endparblock
> + * @param[out] error_message
> + * If non-NULL, store error message from opening the database.
> + * Any such message is allocated by \a malloc(3) and should be freed
> + * by the caller.
>   *
> - * NOTMUCH_STATUS_SUCCESS: Successfully opened the database.
> + * @retval NOTMUCH_STATUS_SUCCESS: Successfully opened the database.
>   *
> - * NOTMUCH_STATUS_NULL_POINTER: The given 'path' argument is NULL.
> + * @retval NOTMUCH_STATUS_NULL_POINTER: The given \a database
> + * argument is NULL.
>   *
> - * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
> + * @retval NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
>   *
> - * NOTMUCH_STATUS_FILE_ERROR: An error occurred trying to open the
> - *	database file (such as permission denied, or file not found,
> + * @retval NOTMUCH_STATUS_FILE_ERROR: An error occurred trying to open the
> + *	database or config file (such as permission denied, or file not found,
>   *	etc.), or the database version is unknown.
>   *
> - * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
> - */
> -notmuch_status_t
> -notmuch_database_open (const char *path,
> -		       notmuch_database_mode_t mode,
> -		       notmuch_database_t **database);
> -/**
> - * Like notmuch_database_open, except optionally return an error
> - * message. This message is allocated by malloc and should be freed by
> - * the caller.
> + * @retval NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
>   */
>  
>  notmuch_status_t
> -notmuch_database_open_verbose (const char *path,
> -			       notmuch_database_mode_t mode,
> -			       notmuch_database_t **database,
> -			       char **error_message);
> +notmuch_database_open_with_config (const char *database_path,
> +				   notmuch_database_mode_t mode,
> +				   const char *config_path,
> +				   const char *profile,
> +				   notmuch_database_t **database,
> +				   char **error_message);
>  
>  /**
>   * Retrieve last status string for given database.

It's nice that the environment variable handling is done in the library,
should make it more consistent for bindings.  As long as it can be
overwritten I guess.

The API is rather complex though, perhaps easier when split across
several functions?  Maybe a notmuch_database_open_profile(const char
*profile, notmuch_database_t**) is useful as the simple one which always
does the right thing when called with NULL for profile.  Not sure what
other combinations would be needed.

Re: [RFC PATCH] lib: document new database_open API

[David Bremner]

Floris Bruynooghe <flub@devork.be> writes:

>> + *
>> + * - in the environment variable NOTMUCH_DATABASE, if non-empty
>> + *
>> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
>> + *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
>> + *   "default"
>
> I like the profile support, is the plan for
> $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the
> database?

Yes, with "NOTMUCH_PROFILE=default" by default.

> It's nice that the environment variable handling is done in the library,
> should make it more consistent for bindings.  As long as it can be
> overwritten I guess.

Overwritten how? By passing parameters?

> The API is rather complex though, perhaps easier when split across
> several functions?  Maybe a notmuch_database_open_profile(const char
> *profile, notmuch_database_t**) is useful as the simple one which always
> does the right thing when called with NULL for profile.  Not sure what
> other combinations would be needed.

I have no objections to a "do the write thing" wrapper or two. I don't
think that increases maintence cost too much.

d

Re: [RFC PATCH] lib: document new database_open API

[Floris Bruynooghe]

On Sat 04 Jul 2020 at 14:01 -0300, David Bremner wrote:

> Floris Bruynooghe <flub@devork.be> writes:
>
>>> + *
>>> + * - in the environment variable NOTMUCH_DATABASE, if non-empty
>>> + *
>>> + * - by $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE where XDG_DATA_HOME
>>> + *   defaults to "$HOME/.local/share" and NOTMUCH_PROFILE defaults to
>>> + *   "default"
>>
>> I like the profile support, is the plan for
>> $XDG_DATA_HOME/notmuch/$NOTMUCH_PROFILE to be written when creating the
>> database?
>
> Yes, with "NOTMUCH_PROFILE=default" by default.
>
>> It's nice that the environment variable handling is done in the library,
>> should make it more consistent for bindings.  As long as it can be
>> overwritten I guess.
>
> Overwritten how? By passing parameters?

yes, that's what I meant.  Which I think you design here allows.  Just
takes a while to figure out what the right parameter combination
is... ;)

>> The API is rather complex though, perhaps easier when split across
>> several functions?  Maybe a notmuch_database_open_profile(const char
>> *profile, notmuch_database_t**) is useful as the simple one which always
>> does the right thing when called with NULL for profile.  Not sure what
>> other combinations would be needed.
>
> I have no objections to a "do the write thing" wrapper or two. I don't
> think that increases maintence cost too much.
>
> d