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

[Jeff King <peff@peff.net>]

On Fri, Aug 10, 2018 at 09:32:10PM -0700, Elijah Newren wrote:

> diff --git a/argv-array.h b/argv-array.h
> index a39ba43f57..c46238784c 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -1,6 +1,8 @@
>  #ifndef ARGV_ARRAY_H
>  #define ARGV_ARRAY_H
>  
> +#include "git-compat-util.h"   /* for LAST_ARG_MUST_BE_NULL */

Following up on what I said in the other message, I think we should omit
this. All header files should assume the environment provided by
git-compat-util.h is already loaded (and all non-compat C files should
make sure they load it first).

> diff --git a/color.h b/color.h
> index 5b744e1bc6..74edbffc9d 100644
> --- a/color.h
> +++ b/color.h
> @@ -1,6 +1,8 @@
>  #ifndef COLOR_H
>  #define COLOR_H
>  
> +#include <stdio.h>

This one is more actively wrong than the git-compat-util.h one. Anybody
who already included git-compat-util.h would not be helped by this. And
anybody who _didn't_ may be hurt, because now git-compat-util.h might
run into ordering problems caused by headers already loaded as part of
stdio.h (and keep in mind that stdio.h often brings in other
system-dependent definitions, too).

> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index a85527318b..e036231636 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -4,6 +4,8 @@
>  #ifndef BULK_CHECKIN_H
>  #define BULK_CHECKIN_H
>  
> +#include "cache.h" /* for object_type */

As I said earlier, I don't mind includes like these that make
everybody's lives a little easier, and shouldn't introduce any funny
ordering constraints.

IMHO, though, comments like "for X" are probably a bad idea. That may be
accurate _now_, but it is very likely to grow stale without anybody
noticing. You only care about that "X" if you are thinking about
removing the include, and then the right answer is not to trust the
comment, but to remove the include and make sure there is no
other breakage.

The one exception is if there is some subtlety, like including a header
file that uses macros to replace a function, and removing it wouldn't
break compilation but produce a different result. But then:

  - I'd hope we don't have anything like that, because it sounds
    horrible ;)

  - you'd probably want a much more explanatory comment

> [...]

The rest of them are all variants on these three (or forward
declarations, which I put in the same boat as this third recursive
include; i.e., it's a good change).

-Peff