Re: [PATCH v2 20/22] object-store.h: reduce unnecessary includes

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 1, 2023, 17:00 UTC
Message-ID: <230501.86ildcht9c.gmgdl@evledraar.gmail.com>
In-Reply-To: <7705cbc2733a52cbaa53adc3dffab58f41eb4105.1682194652.git.gitgitgadget@gmail.com>

On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:

Show 45 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  object-file.c      | 1 +
>  object-name.c      | 1 +
>  object-store.h     | 8 ++++----
>  submodule-config.c | 1 +
>  4 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 8e0df7360ae..921a717d8a5 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -38,6 +38,7 @@
>  #include "packfile.h"
>  #include "object-file.h"
>  #include "object-store.h"
> +#include "oidtree.h"
>  #include "promisor-remote.h"
>  #include "setup.h"
>  #include "submodule.h"
> diff --git a/object-name.c b/object-name.c
> index 5ccbe854b60..88d839f70bc 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -14,6 +14,7 @@
>  #include "remote.h"
>  #include "dir.h"
>  #include "oid-array.h"
> +#include "oidtree.h"
>  #include "packfile.h"
>  #include "pretty.h"
>  #include "object-store.h"
> diff --git a/object-store.h b/object-store.h
> index f9d225783ae..23ea86d3702 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -2,16 +2,16 @@
>  #define OBJECT_STORE_H
>  
>  #include "object.h"
> -#include "oidmap.h"
>  #include "list.h"
> -#include "oid-array.h"

It seems to me that this should be split up, there does seem to be an unnecessary include here, as the subject claims, at least the "oid-array.h include in object-store.h seems to qualify.

Maybe the same applies to thread-utils.h below?

Show 10 quoted lines

> -#include "strbuf.h"
>  #include "thread-utils.h"
>  #include "khash.h"
>  #include "dir.h"
> -#include "oidtree.h"
>  #include "oidset.h"
>  
> +struct oidmap;
> +struct oidtree;
> +struct strbuf;

But as this shows at least three of these weren't unnecessary, you're just replacing them with forward-decls.

I think that's probably sensible, but I think it should at least be discussed.

It's also not clear why we want to just stop at forward-declaring structs, maybe replacing the dir.h include with:

	int fspatheq(const char *a, const char *b);
	unsigned int fspathhash(const char *str);

Would be too verbose, but if we did that we'd spot that e.g. match-trees.c is relying on this header to get its "struct strbuf" definition.

I'm perfectly fine to leave that can of worms for some later date though...