On Sat, Jun 11, 2016 at 12:00:37PM -0700, Kevin J. McCarthy wrote:
> > 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).
Right, should have moved these two lines as well. Logging seems a bit
inconsistent from a mailbox to another, so it could be worth it to check
what each of them logs.
>
> Also, overall the cleanup looks good, but the resulting
> mx_open_message() flow is a little overcomplicated. What about
> something like:
Agreed, you're version is a bit easier to read. A few notes inlined:
> MESSAGE *mx_open_message (CONTEXT *ctx, int msgno)
> {
> struct mx_ops *ops;
I would prefer
struct mx_ops *ops = mx_get_ops (ctx->magic);
> MESSAGE *msg;
>
> ops = mx_get_ops (ctx->magic);
> if (!(ops && ops->open_msg))
if (!ops || !ops->open_msg) seems clearer to me.
> {
> dprint (1, (debugfile, "mx_open_message(): function not implemented for
> mailbox type %d.\n", ctx->magic));
We should get rid of ctx->magic and add a const char *name in struct
mx_ops. That's for another patchset tough.
> return NULL;
> }
>
> msg = safe_calloc (1, sizeof (MESSAGE));
> msg->magic = ctx->magic;
> if (ops->open_msg (ctx, msg, msgno) != 0)
I know that it's done a lot in mutt, but I just don't like when
functions are called and tested in an if statement, I would prefer that
the call and the test are two separate lines.
So either do:
ret = ops->open_msg (ctx, msg, msgno)
if (ret)
or if you want to keep them on one line, drop the "!= 0":
if (ops->open_msg (ctx, msg, msgno))
Thanks for your feedback. Let me know if you want me to send another
version of this patch which restores the perror and dprint, and change
mx_open_message.
--
Damien
