From: Tomi Ollila <tomi.ollila@iki.fi>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [Patch v3 05/11] lib: provide config API
In-Reply-To: <1462065879-29860-6-git-send-email-david@tethera.net>
References: <1462065879-29860-1-git-send-email-david@tethera.net>
 <1462065879-29860-6-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.22+9~gb26b5ad (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Sat, 07 May 2016 20:03:51 +0300
Message-ID: <m2zis1er60.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Sun, May 01 2016, David Bremner <david@tethera.net> wrote:

nits here

> This is a thin wrapper around the Xapian metadata API. The job of this
> layer is to keep the config key value pairs from colliding with other
> metadata by transparently prefixing the keys, along with the usual glue
> to provide a C interface.
>
> The split of _get_config into two functions is to allow returning of the
> return value with different memory ownership semantics.
> ---
>  lib/Makefile.local     |  1 +
>  lib/config.cc          | 90 ++++++++++++++++++++++++++++++++++++++++++++=
++++++
>  lib/notmuch.h          | 20 +++++++++++
>  test/T590-libconfig.sh | 58 ++++++++++++++++++++++++++++++++
>  4 files changed, 169 insertions(+)
>  create mode 100644 lib/config.cc
>  create mode 100755 test/T590-libconfig.sh
>
> diff --git a/lib/Makefile.local b/lib/Makefile.local
> index 36c3924..76b57cb 100644
> --- a/lib/Makefile.local
> +++ b/lib/Makefile.local
> @@ -49,6 +49,7 @@ libnotmuch_cxx_srcs =3D		\
>  	$(dir)/index.cc		\
>  	$(dir)/message.cc	\
>  	$(dir)/query.cc		\
> +	$(dir)/config.cc	\
>  	$(dir)/thread.cc
>=20=20
>  libnotmuch_modules :=3D $(libnotmuch_c_srcs:.c=3D.o) $(libnotmuch_cxx_sr=
cs:.cc=3D.o)
> diff --git a/lib/config.cc b/lib/config.cc
> new file mode 100644
> index 0000000..af00d6f
> --- /dev/null
> +++ b/lib/config.cc
> @@ -0,0 +1,90 @@
> +/* metadata.cc - API for database metadata

config.cc or metadata.cc -- decide ! ;D

> + *
> + * Copyright =C2=A9 2015 David Bremner

this could be 2016 now (?) (like it matters)

> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner <david@tethera.net>
> + */
> +
> +#include "notmuch.h"
> +#include "notmuch-private.h"
> +#include "database-private.h"
> +
> +static const std::string CONFIG_PREFIX=3D"C";

spacing. e.g. CONFIG_PREFIX =3D "C";

> +
> +notmuch_status_t
> +notmuch_database_set_config (notmuch_database_t *notmuch,
> +			     const char *key,
> +			     const char *value)
> +{
> +    notmuch_status_t status;
> +    Xapian::WritableDatabase *db;
> +
> +    status =3D _notmuch_database_ensure_writable (notmuch);
> +    if (status)
> +	return status;
> +
> +    try {
> +	db =3D static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
> +	db->set_metadata (CONFIG_PREFIX+key, value);
> +    } catch (const Xapian::Error &error) {
> +	status =3D NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +	notmuch->exception_reported =3D TRUE;
> +	if (! notmuch->exception_reported) {
> +	    _notmuch_database_log (notmuch, "Error: A Xapian exception occurred=
 setting metadata: %s\n",

The above goes up to column 104, but probably nothing is to be done there.

> +				   error.get_msg().c_str());
> +	}
> +    }
> +    return NOTMUCH_STATUS_SUCCESS;
> +}
> +
> +static notmuch_status_t
> +_metadata_value (notmuch_database_t *notmuch,
> +		 const char *key,
> +		 std::string &value)
> +{
> +    notmuch_status_t status =3D NOTMUCH_STATUS_SUCCESS;
> +
> +    try {
> +	value =3D notmuch->xapian_db->get_metadata (CONFIG_PREFIX+key);

spacing: e.g. (CONFIG_PREFIX + key);

> +    } catch (const Xapian::Error &error) {
> +	status =3D NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +	notmuch->exception_reported =3D TRUE;
> +	if (! notmuch->exception_reported) {
> +	    _notmuch_database_log (notmuch, "Error: A Xapian exception occurred=
 getting metadata: %s\n",
> +				   error.get_msg().c_str());
> +	}
> +    }
> +    return status;
> +}
> +
> +notmuch_status_t
> +notmuch_database_get_config (notmuch_database_t *notmuch,
> +			     const char *key,
> +			     char **value) {

In functions IIRC we always have opening brace in its own line.

> +    std::string strval;
> +    notmuch_status_t status;
> +
> +    if (!value)
> +	return NOTMUCH_STATUS_NULL_POINTER;
> +
> +    status =3D _metadata_value (notmuch, key, strval);
> +    if (status)
> +	return status;
> +
> +    *value =3D strdup (strval.c_str ());
> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +}

Tomi