Re: [PATCH] add open_msg to struct mx_ops

[Kevin J. McCarthy]

On Thu, Jun 09, 2016 at 07:54:22PM -0400, Damien Riegel wrote:
> Add the callback to open an existing message to struct mx_ops. For mbox,
> mmdf, maildir, and mh, the code was implemented directly into
> mx_open_message, so it is moved in their respective source files. For
> imap and pop, there were already <mailbox>_fetch_message functions, but
> their argument order has been changed to pass the context as a first
> argument.

Hi Damien,

Thanks for the patches.  I'm a little busy right now, but I'll get to
them all as I can.

> diff -r 93c4ae03689e -r 837e6ea2638b mh.c
>  
> +static int maildir_mh_open_message (CONTEXT *ctx, MESSAGE *msg, int msgno,
> +                                    int is_maildir)
> +{
> +  HEADER *cur = ctx->hdrs[msgno];
> +  char path[_POSIX_PATH_MAX];
> +
> +  snprintf (path, sizeof (path), "%s/%s", ctx->path, cur->path);
> +
> +  msg->fp = fopen (path, "r");
> +  if (msg->fp == NULL && errno == ENOENT && is_maildir)
> +    msg->fp = maildir_open_find_message (ctx->path, cur->path);
> +
> +  return msg->fp == NULL;
> +}


> diff -r 93c4ae03689e -r 837e6ea2638b mx.c
> --- a/mx.c    Thu Jun 09 12:06:10 2016 -0700
> +++ b/mx.c    Wed Jun 01 19:08:23 2016 -0400
...
> -    case MUTT_MH:
> -    case MUTT_MAILDIR:
> -    {
> -      HEADER *cur = ctx->hdrs[msgno];
> -      char path[_POSIX_PATH_MAX];
> -      
> -      snprintf (path, sizeof (path), "%s/%s", ctx->path, cur->path);
> -      
> -      if ((msg->fp = fopen (path, "r")) == NULL && errno == ENOENT &&
> -       ctx->magic == MUTT_MAILDIR)
> -     msg->fp = maildir_open_find_message (ctx->path, cur->path);
> -      
> -      if (msg->fp == NULL)
> -      {
> -     mutt_perror (path);
> -     dprint (1, (debugfile, "mx_open_message: fopen: %s: %s (errno %d).\n",
> -                 path, strerror (errno), errno));

It looks like we lost this perror and dprint line when it was moved over
to mh.c (above).

Also, overall the cleanup looks good, but the resulting
mx_open_message() flow is a little overcomplicated.  What about
something like:

MESSAGE *mx_open_message (CONTEXT *ctx, int msgno)
{
  struct mx_ops *ops;
  MESSAGE *msg;

  ops = mx_get_ops (ctx->magic);
  if (!(ops && ops->open_msg))
  {
    dprint (1, (debugfile, "mx_open_message(): function not implemented for 
mailbox type %d.\n", ctx->magic));
    return NULL;
  }

  msg = safe_calloc (1, sizeof (MESSAGE));
  msg->magic = ctx->magic;
  if (ops->open_msg (ctx, msg, msgno) != 0)
    FREE (&msg);

  return msg;
}


-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

signature.asc
Description: PGP signature