Re: [PATCH v2 13/22] hash-ll.h: split out of hash.h to remove dependency on repository.h

From: Elijah Newren <newren@gmail.com>
Date: May 2, 2023, 02:53 UTC
Message-ID: <CABPp-BHv93VsAZ8=jzDPcNXOyZ1W4Fhf14gusvEp_i9XYE-Xhg@mail.gmail.com>
In-Reply-To: <230501.86a5yohsme.gmgdl@evledraar.gmail.com>

On Mon, May 1, 2023 at 10:24 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 66 quoted lines

>
> On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > [...]
> > diff --git a/checkout.h b/checkout.h
> > index 1917f3b3230..3c514a5ab4f 100644
> > --- a/checkout.h
> > +++ b/checkout.h
> > @@ -1,7 +1,7 @@
> >  #ifndef CHECKOUT_H
> >  #define CHECKOUT_H
> >
> > -#include "hash.h"
> > +#include "hash-ll.h"
>
> The end-state of this topic is oddly inconsistent in when it uses
> includes, and when it uses forward declarations.
>
> As I note in a reply to 20/22 you're adding forward declares there, I
> think that's fine, but if you opdet for that why not do that here. The
> body of this header only defines one function, which takes a pointer to
> a "struct object_id".
>
> Whereas above you did this change:
>
> > diff --git a/apply.h b/apply.h
> > index b9f18ce87d1..7cd38b1443c 100644
> > --- a/apply.h
> > +++ b/apply.h
> > @@ -1,7 +1,7 @@
> >  #ifndef APPLY_H
> >  #define APPLY_H
> >
> > -#include "hash.h"
> > +#include "hash-ll.h"
> >  #include "lockfile.h"
> >  #include "string-list.h"
> >  #include "strmap.h"
>
> There we really should include it, as we're not dealing with a pointer
> to the "struct object_id", but the struct itself, so we need its
> definition, and don't want to find it implicitly.
>
> > diff --git a/chunk-format.h b/chunk-format.h
> > index 025c38f938e..c7794e84add 100644
> > --- a/chunk-format.h
> > +++ b/chunk-format.h
> > @@ -1,7 +1,7 @@
> >  #ifndef CHUNK_FORMAT_H
> >  #define CHUNK_FORMAT_H
> >
> > -#include "hash.h"
> > +#include "hash-ll.h"
> >
> >  struct hashfile;
> >  struct chunkfile;
>
> Then we have this, where we seemingly could avoid the include as well,
> and just add a:
>
>         struct git_hash_algo;
>
> Anyway, I'm not saying one is better than the other, I'm just wondering
> why you're picking one, but not the other.

Basically, just because I updated all the includes through:
   $ git grep include..hash.h | xargs sed -i s/hash.h/hash-ll.h/
and then tried to compile and fixed up the files that had errors
(usually by making it include hash.h rather than hash-ll.h).  Every
once in a while I might have noticed that a simpler forward-declare
was sufficient but I didn't always look for it.

Anyway, thanks for looking closely and pointing this one out! Switching this to a forward declaration is another nice cleanup to add (though perhaps to a future series, as this one is long enough).

> Is it because you know that hash-ll.h doesn't bring in other headers, so
> its inclusion is OK, whereas later in e.g. 20/22 you avoid including
> strbuf.h, because you know that'll bring in string-list.h?

No, it's more "we gotta start somewhere, and stop well short of complete for the series to be reviewable".

It's really easy to look at pieces of this series and notice all kinds
of additional cleanups that are possible:
  * Emily pointed out that if we're moving a global to a new header,
why not update the code to just delete the global?
  * Calvin pointed out that git-compat-util.h had become a dumping
ground too and needed cleaning.
  * Glen pointed out that some of my reasons for splitting between
hash-ll.h and hash.h would suggest, based on function definitions,
that 2 of the declarations should move back to hash.h.
  * You've pointed out multiple good additional cleanups in your review so far
  * I had an ongoing list of dozens of types of changes to make while
working on this and prior series.

Anyone who looks at this series is going to spot additional "what about this?" things. They're all great. But I picked some cleanups to make, and carried those through. Because of how I did those cleanups (note my grep & sed command above, followed by recompile/edit/repeat cycle), I noticed some additional cleanups that are likely different than what someone else reviewing them will notice. Some of the additional cleanups I noticed are in this series, but most are delayed for some future series.

Show 5 quoted lines

> If that's the case maybe we should just move
> strbuf_add_separated_string_list() into some "used by merge-ort.c and
> merge-recursive.c" file, remove the string-list.h includion from
> strbuf.h, and then include "strbuf.h" without fearing the side-effects
> elsewhere?

Ooh, nice catch. If that function is only used by those two files, it should be moved to merge-recursive.h (merge-ort.c includes that currently). However, Calvin is doing a bunch of work refactoring strbuf.[ch], and I told him I'd avoid touching it to reduce conflicts with his in-progress work. So, I'll let him tackle this one in his series.