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

[Elijah Newren <newren@gmail.com>]

On Mon, May 1, 2023 at 10:10 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:
>
> > 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?

Removing the thread-utils.h inclusion from object-store.h will break
compilation on non-linux platforms; that header is needed for the
definition of pthread_mutex_t used later in the file.

> > -#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.

Something was necessary, yes, but an #include statement certainly
wasn't.  Here, nothing would need to be recompiled if those other
headers changed, so the include is unnecessary.

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

We have now discussed it.  :-)

Did you want a simple call-out in the commit message, e.g. "Replace
the includes with simple forward declarations of the relevant
structs." ?

> 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);

I don't like the idea of having to update function signatures in 3 or
more places when we need to make changes.  In contrast, forward
declarations of structs aren't susceptible to the same need to
update while making other changes.

We have hundreds of forward declarations of structs in the code;
everyone seems to be fine with them.

Whenever we've had duplicate declarations of functions, it's been
treated as a code smell that we've gotten rid of when possible.  In
fact, in v2.40.0, by my search we only had two of these --
xdl_emit_diff() and read_in_full().  And we've since gotten rid of one
of those (namely, read_in_full()).

> 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...

That's a good find; I also found it separately later in some follow-on
patches so I'll also vote for leaving that can of worms for a later
date.  :-)


And thanks for carefully reading through this series!