On Sat, May 07, 2016 at 04:14:36AM -0400, Damien Riegel wrote: > On Fri, May 06, 2016 at 07:29:39PM -0700, Kevin J. McCarthy wrote: > > * 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.
I understand your intention, but yes I'd like them grouped into bigger aggregates. Please take a look through our commit history to get a feel for the size of changes we make. Since we use list-based patch review, a smaller number of patches in a set makes the reviewing easier too (as long as the patches aren't huge). > 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. Alright, I don't feel too strongly about this, but again I'd appreciate if the other contributors would take a look and comment. One thing I'd like to see changed, though, is to move the mx_register_all() to when mutt is initialzed. Perhaps somewhere in mutt_init(). > > * patch 22: > > 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. When you refactor like this though, you need to take a close look (no pun intended). The ctx->close was assigned in the open, but if you look at pop_open_mailbox() or imap_open_mailbox() you can see failures that occur *after* that assignment, where the connection was opened but something strange occured. The close needs to be called to clean things up. Yes, there is a call to mx_fastclose_mailbox() before the open even occurs, but the close functions should deal properly with this case (because the ctx was memset to 0), and if they don't, we should fix them. I know you are new to the codebase, and this is your first set of patches, but when you are changing core code like this I would like for you to be conservative with arbitrary changes, at least until you have a bit more experience in the code. Sorry if I come across as harsh. Again, overall I like the effort and appreciate your effort! -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
