Re: [PATCH v2 20/22] object-store.h: reduce unnecessary includes
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2023-05-01 17:15:59
On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Elijah Newren <redacted> Signed-off-by: Elijah Newren <redacted> --- 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?
-#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...