Re: Unused #include statements

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> One of our rules is that git-compat-util.h (or one of the well-known
> headers which includes, cache.h or builtin.h) is included first in any
> translation unit.

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.