On Fri, May 06, 2016 at 07:29:39PM -0700, Kevin J. McCarthy wrote: > 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: > ...
Noted. I'll make the changes you suggested. Sorry for letting old habits slip through. > > * 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 point is just to make it easy to cherry pick changes (maybe that's just a git concept. I am not very familiar with mercurial environment). Moreover, why mix changes related to different kind of mailboxes when we can keep them separate? Well, that's just my two cents, if you want me to combine them, I will do it. > * 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. Because that means mx.c would have some kind of knowledge of all the mailboxes it supports. It is yet another switch case or if statement that would need to be modified when adding a new kind of mailbox. I think multiple calls to stat have a very minimal perfomance impact, so it doesn't really matter for now. At some later point, mx_get_magic should be split, and its content dispatched in the different probe functions. But I didn't want to make the patchset larger with such cleanup commits. > > 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. Indeed, that's just wrong. I'll restore that 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. Will do. > > * patch 11: > Don't use malloc, use safe_malloc. Will do. > > * patch 16: > It looks like you hooked up the wrong mx_ops in maildir_open_mailbox(). You're right, I'll change that. > > * 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? ctx->close used to be assigned in open functions. Now, ctx->mx_ops is assigned before open is called. If open fails, we should not need to call close, hence the addition of a function that doesn't call ctx->mx_ops->close, and the reordering to make simpler to create such a function. If it's safe to call close on a mailbox that we failed to open, then we don't need these commits. But it feels wrong to call close if open failed. Anyway, I'll rework these commits tomorrow to find a way to make them better. I'm glad that you like the general idea of this patchset, and thank you for your feedback. -- Damien
