Re: [PATCH 0/9] Add missing includes and forward declares

From: Elijah Newren <newren@gmail.com>
Date: Aug 14, 2018, 06:42 UTC
Message-ID: <CABPp-BHZoWn-mZjop+n9PJ0+A4tZFrU6vJE+A7iSeHDXcDc=Yg@mail.gmail.com>
In-Reply-To: <xmqqr2j25dlm.fsf@gitster-ct.c.googlers.com>

On Mon, Aug 13, 2018 at 11:24 AM Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> As things are slowly moving out of the so-far kitchen-sink "cache.h"
> into more specific subsystem headers (like object-store.h), we may
> actually want to tighten the "header that includes it first" part a
> bit in the future, so that 'git grep cache.h' would give us a more
> explicit and a better picture of what really depends on knowing what
> the lowest level plumbing API are built around.
>
> > So I think the better test is a two-line .c file with:
> >
> >   #include "git-compat-util.h"
> >   #include $header_to_check
>
> But until that tightening happens, I do not actually mind the
> two-line .c file started with inclusion of cache.h instead of
> git-compat-util.h.  That would limit the scope of this series
> further.

Yes, this removes about 2/3 of patch #1. But it makes things kind of odd to me; do we change the relevant paragraph of Documentation/CodingGuidelines to add an additional sentence so it reads:

 - The first #include in C files, except in platform specific compat/
   implementations, must be either "git-compat-util.h", "cache.h" or
   "builtin.h".  You do not have to include more than one of these.
   However, note that including any of a few dozen header files
   may result in compilation failures if cache.h is not included first.

That seems annoying to state, but also annoying to leave undocumented. I'd rather have the various structs (object_id, strbuf, index_state, cache_entry, etc.) forward declared and/or relevant headers (e.g. "cache.h", "strbuf.h", or "string-list.h") #include'd. Are you sure you really want me to remove these fixes? Let me know, and if so I'll do it, even if I'd prefer to leave them in. Let me know how/if you'd like that CodingGuidline to be updated too.

Also sha1dc_git.h fails to compile if cache.h is #included before it (various redefinition errors, such as for "platform_SHA_CTX"); it's possible that's expected, but it does make things a little weirder to add implicit assumptions that cache.h is included first.