From: Mark Walters <markwalters1009@gmail.com>
To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 1/8] All access to mail files goes through the
	mailstore module
In-Reply-To: <1341160790-14525-2-git-send-email-ethan@betacantrips.com>
References:
 <CAOJ+Ob0MSOez2MvD2fCgF7t32kFPk4g2+xCud88QmBLt_b5pOA@mail.gmail.com>
	<1341160790-14525-1-git-send-email-ethan@betacantrips.com>
	<1341160790-14525-2-git-send-email-ethan@betacantrips.com>
User-Agent: Notmuch/0.13.2+70~gb6a56e7 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 01 Jul 2012 20:48:41 +0100
Message-ID: <87hatr2rfq.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: Ethan Glasser-Camp <ethan@betacantrips.com>
Precedence: list

On Sun, 01 Jul 2012, Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> wrot=
e:
> This commit introduces the mailstore module which provides two
> functions, notmuch_mailstore_open and notmuch_mailstore_close. These
> functions are currently just stub calls to fopen and fclose, but later
> can be made more complex in order to support mail storage systems
> where one message might not be one file.
>
> No functional changes are made, but calls to fopen and fclose are
> replaced with notmuch_mailstore_open and notmuch_mailstore_close
> wherever they are being used to access messages, but not when talking
> about real files like in notmuch-dump or notmuch-restore.
>
> Since the sha1 function notmuch_sha1_of_file is only used on messages,
> we convert it to using notmuch_mailstore_open and
> notmuch_mailstore_close, and rename it notmuch_sha1_of_message. While
> we are there, we also replace a numeric constant with its symbolic
> name BLOCK_SIZE.
>
> Signed-off-by: Ethan Glasser-Camp <ethan@betacantrips.com>
> ---
> This patch does not split notmuch_sha1_of_file the way the previous
> patch series did, because there are no other callers of
> notmuch_sha1_of_file. Instead it renames the function to
> notmuch_sha1_of_message.
>
>  lib/Makefile.local    |    1 +
>  lib/database.cc       |    2 +-
>  lib/index.cc          |    2 +-
>  lib/mailstore.c       |   34 ++++++++++++++++++++++++++++++++++
>  lib/message-file.c    |    6 +++---
>  lib/notmuch-private.h |    2 +-
>  lib/notmuch.h         |   16 ++++++++++++++++
>  lib/sha1.c            |   11 +++++------
>  mime-node.c           |    4 ++--
>  notmuch-show.c        |   12 ++++++------
>  10 files changed, 70 insertions(+), 20 deletions(-)
>  create mode 100644 lib/mailstore.c

This patch, though large, is simple and looks obviously correct. Indeed,
given that it is really just abstracting out fopen/fclose into notmuch
specific versions it would be a good first step for any more general
mail store so it might be reasonable to accept it independently of the
rest of the series.

Best wishes

Mark

>
> diff --git a/lib/Makefile.local b/lib/Makefile.local
> index 8a9aa28..cfc77bb 100644
> --- a/lib/Makefile.local
> +++ b/lib/Makefile.local
> @@ -51,6 +51,7 @@ libnotmuch_c_srcs =3D		\
>  	$(dir)/filenames.c	\
>  	$(dir)/string-list.c	\
>  	$(dir)/libsha1.c	\
> +	$(dir)/mailstore.c	\
>  	$(dir)/message-file.c	\
>  	$(dir)/messages.c	\
>  	$(dir)/sha1.c		\
> diff --git a/lib/database.cc b/lib/database.cc
> index 761dc1a..c035edc 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1773,7 +1773,7 @@ notmuch_database_add_message (notmuch_database_t *n=
otmuch,
>  	if (message_id =3D=3D NULL ) {
>  	    /* No message-id at all, let's generate one by taking a
>  	     * hash over the file's contents. */
> -	    char *sha1 =3D notmuch_sha1_of_file (filename);
> +	    char *sha1 =3D notmuch_sha1_of_message (filename);
>=20=20
>  	    /* If that failed too, something is really wrong. Give up. */
>  	    if (sha1 =3D=3D NULL) {
> diff --git a/lib/index.cc b/lib/index.cc
> index e377732..b607e82 100644
> --- a/lib/index.cc
> +++ b/lib/index.cc
> @@ -441,7 +441,7 @@ _notmuch_message_index_file (notmuch_message_t *messa=
ge,
>  	initialized =3D 1;
>      }
>=20=20
> -    file =3D fopen (filename, "r");
> +    file =3D notmuch_mailstore_open (filename);
>      if (! file) {
>  	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
>  	ret =3D NOTMUCH_STATUS_FILE_ERROR;
> diff --git a/lib/mailstore.c b/lib/mailstore.c
> new file mode 100644
> index 0000000..48acd47
> --- /dev/null
> +++ b/lib/mailstore.c
> @@ -0,0 +1,34 @@
> +/* mailstore.c - code to access individual messages
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + *
> + * 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: Carl Worth <cworth@cworth.org>
> + */
> +#include <stdio.h>
> +
> +#include "notmuch-private.h"
> +
> +FILE *
> +notmuch_mailstore_open (const char *filename)
> +{
> +    return fopen (filename, "r");
> +}
> +
> +int
> +notmuch_mailstore_close (FILE *file)
> +{
> +    return fclose (file);
> +}
> diff --git a/lib/message-file.c b/lib/message-file.c
> index 915aba8..271389c 100644
> --- a/lib/message-file.c
> +++ b/lib/message-file.c
> @@ -86,7 +86,7 @@ _notmuch_message_file_destructor (notmuch_message_file_=
t *message)
>  	g_hash_table_destroy (message->headers);
>=20=20
>      if (message->file)
> -	fclose (message->file);
> +	notmuch_mailstore_close (message->file);
>=20=20
>      return 0;
>  }
> @@ -104,7 +104,7 @@ _notmuch_message_file_open_ctx (void *ctx, const char=
 *filename)
>=20=20
>      talloc_set_destructor (message, _notmuch_message_file_destructor);
>=20=20
> -    message->file =3D fopen (filename, "r");
> +    message->file =3D notmuch_mailstore_open (filename);
>      if (message->file =3D=3D NULL)
>  	goto FAIL;
>=20=20
> @@ -361,7 +361,7 @@ notmuch_message_file_get_header (notmuch_message_file=
_t *message,
>      }
>=20=20
>      if (message->parsing_finished) {
> -        fclose (message->file);
> +        notmuch_mailstore_close (message->file);
>          message->file =3D NULL;
>      }
>=20=20
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index bfb4111..ef1f994 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -466,7 +466,7 @@ char *
>  notmuch_sha1_of_string (const char *str);
>=20=20
>  char *
> -notmuch_sha1_of_file (const char *filename);
> +notmuch_sha1_of_message (const char *filename);
>=20=20
>  /* string-list.c */
>=20=20
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 3633bed..0ca367b 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -1233,6 +1233,22 @@ notmuch_message_thaw (notmuch_message_t *message);
>  void
>  notmuch_message_destroy (notmuch_message_t *message);
>=20=20
> +/* Get access to the underlying data of a message.
> + *
> + * Right now, all messages are simple files in maildirs, but this is
> + * hopefully subject to change.
> + *
> + * If the returned FILE* is not NULL, be sure to call
> + * notmuch_mailstore_close when you're done with it.
> + */
> +FILE *
> +notmuch_mailstore_open (const char *filename);
> +
> +/* Release any resources associated with this file.
> + */
> +int
> +notmuch_mailstore_close (FILE *file);
> +
>  /* Is the given 'tags' iterator pointing at a valid tag.
>   *
>   * When this function returns TRUE, notmuch_tags_get will return a
> diff --git a/lib/sha1.c b/lib/sha1.c
> index cc48108..f4d213a 100644
> --- a/lib/sha1.c
> +++ b/lib/sha1.c
> @@ -65,7 +65,7 @@ notmuch_sha1_of_string (const char *str)
>  }
>=20=20
>  /* Create a hexadecimal string version of the SHA-1 digest of the
> - * contents of the named file.
> + * contents of the named message.
>   *
>   * This function returns a newly allocated string which the caller
>   * should free() when finished.
> @@ -74,7 +74,7 @@ notmuch_sha1_of_string (const char *str)
>   * file not found, etc.), this function returns NULL.
>   */
>  char *
> -notmuch_sha1_of_file (const char *filename)
> +notmuch_sha1_of_message (const char *filename)
>  {
>      FILE *file;
>  #define BLOCK_SIZE 4096
> @@ -84,14 +84,14 @@ notmuch_sha1_of_file (const char *filename)
>      unsigned char digest[SHA1_DIGEST_SIZE];
>      char *result;
>=20=20
> -    file =3D fopen (filename, "r");
> +    file =3D notmuch_mailstore_open (filename);
>      if (file =3D=3D NULL)
>  	return NULL;
>=20=20
>      sha1_begin (&sha1);
>=20=20
>      while (1) {
> -	bytes_read =3D fread (block, 1, 4096, file);
> +	bytes_read =3D fread (block, 1, BLOCK_SIZE, file);
>  	if (bytes_read =3D=3D 0) {
>  	    if (feof (file)) {
>  		break;
> @@ -108,8 +108,7 @@ notmuch_sha1_of_file (const char *filename)
>=20=20
>      result =3D _hex_of_sha1_digest (digest);
>=20=20
> -    fclose (file);
> +    notmuch_mailstore_close (file);
>=20=20
>      return result;
>  }
> -
> diff --git a/mime-node.c b/mime-node.c
> index 97e8b48..a5c60d0 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -49,7 +49,7 @@ _mime_node_context_free (mime_node_context_t *res)
>  	g_object_unref (res->stream);
>=20=20
>      if (res->file)
> -	fclose (res->file);
> +	notmuch_mailstore_close (res->file);
>=20=20
>      return 0;
>  }
> @@ -79,7 +79,7 @@ mime_node_open (const void *ctx, notmuch_message_t *mes=
sage,
>      }
>      talloc_set_destructor (mctx, _mime_node_context_free);
>=20=20
> -    mctx->file =3D fopen (filename, "r");
> +    mctx->file =3D notmuch_mailstore_open (filename);
>      if (! mctx->file) {
>  	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
>  	status =3D NOTMUCH_STATUS_FILE_ERROR;
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 8f3c60e..daffb59 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -693,7 +693,7 @@ format_part_mbox (const void *ctx, mime_node_t *node,=
 unused (int indent),
>  	INTERNAL_ERROR ("format_part_mbox requires a root part");
>=20=20
>      filename =3D notmuch_message_get_filename (message);
> -    file =3D fopen (filename, "r");
> +    file =3D notmuch_mailstore_open (filename);
>      if (file =3D=3D NULL) {
>  	fprintf (stderr, "Failed to open %s: %s\n",
>  		 filename, strerror (errno));
> @@ -717,7 +717,7 @@ format_part_mbox (const void *ctx, mime_node_t *node,=
 unused (int indent),
>=20=20
>      printf ("\n");
>=20=20
> -    fclose (file);
> +    notmuch_mailstore_close (file);
>=20=20
>      return NOTMUCH_STATUS_SUCCESS;
>  }
> @@ -740,7 +740,7 @@ format_part_raw (unused (const void *ctx), mime_node_=
t *node,
>  	    return NOTMUCH_STATUS_FILE_ERROR;
>  	}
>=20=20
> -	file =3D fopen (filename, "r");
> +	file =3D notmuch_mailstore_open (filename);
>  	if (file =3D=3D NULL) {
>  	    fprintf (stderr, "Error: Cannot open file %s: %s\n", filename, stre=
rror (errno));
>  	    return NOTMUCH_STATUS_FILE_ERROR;
> @@ -750,18 +750,18 @@ format_part_raw (unused (const void *ctx), mime_nod=
e_t *node,
>  	    size =3D fread (buf, 1, sizeof (buf), file);
>  	    if (ferror (file)) {
>  		fprintf (stderr, "Error: Read failed from %s\n", filename);
> -		fclose (file);
> +		notmuch_mailstore_close (file);
>  		return NOTMUCH_STATUS_FILE_ERROR;
>  	    }
>=20=20
>  	    if (fwrite (buf, size, 1, stdout) !=3D 1) {
>  		fprintf (stderr, "Error: Write failed\n");
> -		fclose (file);
> +		notmuch_mailstore_close (file);
>  		return NOTMUCH_STATUS_FILE_ERROR;
>  	    }
>  	}
>=20=20
> -	fclose (file);
> +	notmuch_mailstore_close (file);
>  	return NOTMUCH_STATUS_SUCCESS;
>      }
>=20=20
> --=20
> 1.7.9.5
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch