On Thu, May 05, 2016 at 11:44:42PM -0400, Damien Riegel wrote: > This patchset allows to open a mailbox without relying on ctx->magic.
Hi Damien,
Welcome to mutt-dev! First, let me start by thanking you for sending
the patchset. Despite any criticism below, I think the general idea is
good and is something we should work towards, and I appreciate you
starting on this.
First, let me start with a few style comments:
* Indentation/style:
I would like to see the indentation style:
struct
{
}
while
{
}
And for function calls, we use
foo (arg, arg, arg);
Mutt needs some cleanup and isn't 100% consistent, but please look at
the code around and try to stick to the same style.
You should also review lib.c. In particular, we never use malloc() and
such directly. We use safe_malloc() and FREE(). The check_sec.sh
script will flag some of these things for you.
* Patch size:
In general I like smaller patches, because they are easier to review.
I think in this case, though, some of your patches are too small. :-)
For example, patches 4-10 and 12-20 could probably be combined
into two patches without the patches being too big and unreadable.
* The "probe" design:
In theory this sounds good, but I don't know that the trade-off is worth
it. You still have a single "mx_register_all()" function with #ifdefs
in it to put together the linked list. But now, you have to loop
through one-by-one, performing multiple stats and such until you find
the right one.
Why not just create a "mirror" to mx_get_magic() that returns the
correct mx_ops directly? The goal, of course, would be for this to be
the *only* place in the code with those #ifdefs and co-mingled logic.
Anyway, that's just my initial impression. I'd be interested to hear
what others have to say about it.
Comments about specific patches:
* patch 2:
When you renamed and moved the mbox_close_mailbox() function, you also
deleted the "mx_fastclose_mailbox (ctx);" call.
* patch 3:
The stub function could be confusing. Perhaps a comment indicating that
mx_fastclose_mailbox() does the actual closing of the descriptor, or
something like that, would make it clearer what's going on.
* patch 11:
Don't use malloc, use safe_malloc.
* patch 16:
It looks like you hooked up the wrong mx_ops in maildir_open_mailbox().
* patch 21:
Please don't make changes like this without justifying them. Why did
you change the order?
* patch 22:
In general you shouldn't use a "__" prefix. All such identifiers are
reserved.
In addition, I don't think this refactor is a good idea. It's just
touching the code for no real purpose. The function was already
designed to be run for the case where a mailbox open failed for some
reason, so why not just leave it as is?
--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
