Neomutt: plans for refactoring mutt's code

[toogley]

Hey.

I think it makes sense to collect some general ideas/plans about refactoring mutt. By that, i mean specifically introducing no new behavior/features, just make the current state more maintainable, more safe, stable or whatever.

That means, I'd like to have a collection of focused descriptions of what exactly is problematic and how we would like to refactor that(ideally it would be just a link to a separate github issue). Additionally, i think it makes sense to list things which are blocking this specific issue to be fixed. Furthermore explaining how realistic this fix is in your opinion to be done would be in my view an useful addition.

See also:

• separate header, source and build files into different directories | reformat mutt's code

[guyzmo]

Ok, in a dream world, here's things I'd like to see featured in a refactor:

1. Get rid of globals and use a context that's passed as parameter to functions.
   
   
   
   • it will make understanding and reading of the code way easier
   
   
   • it will make it easier to write proper testing of the functions and mock up the context
   
   
2. Breakdown huge functions like mutt_index_menu() (2415 loc)
   
   
   
   • instead of a massive switch/case use some sort of observer pattern?
   
   
   • avoid ifdefs that makes reading and static analyzing harder, complexifying the cyclometric complexity with more compile-time configured code paths
   
   
3. Better moduralise mutt's sources (cf #295)
   
   
   
   • just by either renaming compilation units correctly (e.g. a single prefix followed by _), or moving related stuff together in a directory (like the imap folder but for curses, gpg, notmuch…)
   
   
   • will help a lot in building configuration
   
   
   • will also provide some homogeneity when looking around in the source code:
     
     
     
     
     
     • why imap stuff is in imap but notmuch is in mutt_notmuch?
     
     
     
     • what's the difference between lib.c and muttlib.c?
     
     
     
     • why have snprintf.c and other libc replacement at the same place we've got browser.c or main.c? (they'd better be in a compat/ directory)
     
     
     
   
   
4. replace all low level interaction with the system using a library such as libuv, following the same refactoring plan as neovim.
5. For the sake of homogeneity, why some commands have _ separators, and others - separators?

[d-k-c]

Big +1 for guyzmo's suggestions. The globals are a big pain, they are modified all over the place. If there is a "struct context", then it should be considered as an opaque pointer by all compilation units but context.c (of course the name might differ, just giving an example), and passed to helpers implemented in context.c to be modified.

Many compilation units do many things, or are split in a weird way. For instance, there is a base64.c file but it uses a global array B64Chars which is in sendlib.c, and sendlib.c has another implementation to do base64-encoding. Some reorganization (renaming to make them consistent, moving them to folders) would also be appreciated.

Remove some configuration flags and their corresponding ifdefs. I think that options that don't depend on external dependencies should be built-in: smtp, imap, pop, debug, sidebar, compressed, etc.

Get rid of __.*_CHECKED__ comments (152 occurences as of today), I'm pretty sure static analyzers can do a better jobs than check_sec.sh.

[guyzmo]

The globals are a big pain, they are modified all over the place. If
there is a "struct context", then it should be considered as an opaque
pointer by all compilation units but context.c (of course the name
might differ, just giving an example), and passed to helpers
implemented in context.c to be modified.

I've dealt with sh!t like that in the past. The plan I've applied and
which worked was:

1. gather all pointers in a single compilation unit (within a .h makes
   it easier, but really it doesn't matter)
2. embed them in a big struct that can be called context_t (one struct
   to include them all)
3. make a singleton out of that struct, and update each and every global
   through the codebase so it's a part of that one global
4. within the call tree, by starting on the leaf functions, replace the
   global access to the singleton by a pointer passed by the caller.
   Ideally, just pass the part of the singleton that really matter to that
   function (it's rarely the whole sh!t that's needed).
5. once you've added a pointer to context_t (or a part of context_t)
   everywhere on the system, and you build context_t at the begining of the
   main() function, you can delete the global.

I believe ① to ③ can be designed in a couple of commits making a change
throughout the code base. And at this stage, it will already be a
radical change, because we'll be able to easily know the closure on
which the function operates.

Then we can go at a slower pace for ④ and ⑤, gradually refactoring
functions so their closure is only on their arguments. For each
function reaching that stage, we can write a test mocking up the
environment passed to the function and compare with the result.

The opaque pointer (with associated methods) isn't a bad idea. It's a
very OO approach to the issue (encapsulation of the data, and its
exposition through a public API). The methods of that context class
(let's call a spade a spade) should be rather simple and easy to test
providing a solid backbone to refactor neomutt around.

Many compilation units do many things, or are split in a weird way.
For instance, there is a base64.c file but it uses a global array
B64Chars which is in sendlib.c, and sendlib.c has another
implementation to do base64-encoding. Some reorganization (renaming to
make them consistent, moving them to folders) would also be
appreciated.

👍

that's a huge job. Necessary, but huge.

Remove some configuration flags and their corresponding ifdefs. I
think that options that don't depend on external dependencies should
be built-in: smtp, imap, pop, debug, sidebar, compressed, etc.

I was in the idea that those features should not be enabled with
intertwined #ifdefs but instead at compilation time, by excluding
compilation of the .o. But that would mean a lot of code change to
make it more flexible, with little gain.

So I agree that such features should be compiled per default, and
enabled at runtime by configuration.

Get rid of __.*_CHECKED__ comments (152 occurences as of today), I'm
pretty sure static analyzers can do a better jobs than check_sec.sh.

the question is more about the accuracy of using the safe_* functions. I
haven't had a proper look at how they're implemented, and I'm a bit
afraid of what I'd find there.

[flatcap]

As a discussion, this has ended. It's raised some interesting ideas.
As a issue, it's no longer useful. The goals it proposes are huge and known.