Re: Unused #include statements

Jeff King <peff@peff.net> writes:

Perhaps by now we can spell it out a bit more explicitly and update CodingGuidelines. We have:

 - The first #include in C files, except in platform specific
   compat/ implementations, should be git-compat-util.h or another
   header file that includes it, such as cache.h or builtin.h.

"such as" might be making things unnecessarily vague; I do not think a valid reason why we should say a .c file that includes "advice.h" as the first thing satisfies this requirement, for example.

Because:

 - A command that interacts with the object store, config subsystem,
   the index, or the working tree cannot do anything without using
   what is declared in "cache.h".

 - A built-in command must be declared in "builtin.h", so anything
   in builtin/*.c must include it.

it may be reasonable to say the first *.h file included must be one of git-compat-util.h, cache.h or builtin.h (and then we make sure that compat-util is the first thing included in either of the latter two).

While I very much agree with the principle Robert alluded to [*1*], we may want to loosen that for .c files that include "builtin.h" or "cache.h" for the sake of brevity. For example, if you are builtin (hence you start by #include "builtin.h"), it may be reasonable to allow you to take whatever is in "cache.h" for granted [*2*].

So the rule might be:

 - The first #include in C files, except in platform specific
   compat/ implementations, must be either git-compat-util.h,
   cache.h or builtin.h.

 - A C file must directly include the header files that declare the
   functions and the types it uses, except for the functions and
   types that are made available to it by including one of the
   header files it must include by the previous rule.

Optionally, 

 - A C file must include only one of "git-compat-util.h", "cache.h"
   or "builtin.h"; e.g. if you include "builtin.h", do not include
   the other two, but it can consider what is availble in "cache.h"
   available to it.

Thoughts? I am not looking forward to a torrent of patches whose sole purpose is to make the existing C files conform to any such rule, though. Clean-up patches that trickle in at a low rate is tolerable, but a torrent is too distracting.

[Footnote]

*1* For example, even though "diff.h" may include "tree-walk.h" for its own use, a .c file that includes "diff.h" without including "tree-walk.h" that uses update_tree_entry() or anything that is declared in the latter is very iffy from semantic point of view.

*2* Because that facility is so widely used inside the codebase, "builtin.h" includes "strbuf.h", so in addition to what are in "cache.h", we may want to allow builtin implementations to take strbufs for granted as well.