Re: [PATCH 14/23] hash.h, repository.h: reverse the order of these dependencies

[Elijah Newren <newren@gmail.com>]

On Mon, Apr 17, 2023 at 1:59 PM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 4/15/2023 11:03 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Previously, hash.h depended upon and included repository.h, due to
> > the definition and use of the_hash_algo (defined as
> > the_repository->hash_algo).  Move this #define, and the associated
> > inline functions that depend upon it, from hash.h to repository.h.
> > Due to that movement, reverse the dependencies so repository.h includes
> > hash.h.
> >
> > This allows hash.h and object.h to be fairly small, minimal headers.  It
> > also exposes a lot of hidden dependencies on both path.h (which was
> > brought in by repository.h) and repository.h (which was previously
> > implicitly brought in by object.h).
>
> This is the first patch in the series where I don't immediately agree
> with the patch. This is a big list of methods that don't seem like
> they fit in repository.h:
>
> > diff --git a/repository.h b/repository.h
> > +static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
> > +static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
> > +static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
> > +static inline int oideq(const struct object_id *oid1, const struct object_id *oid2)
> > +static inline int is_null_oid(const struct object_id *oid)
> > +static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src)
> > +static inline void oidcpy_with_padding(struct object_id *dst,
> > +                                    const struct object_id *src)
> > +static inline void hashclr(unsigned char *hash)
> > +static inline void oidclr(struct object_id *oid)
> > +static inline void oidread(struct object_id *oid, const unsigned char *hash)
> > +static inline int is_empty_blob_sha1(const unsigned char *sha1)
> > +static inline int is_empty_blob_oid(const struct object_id *oid)
> > +static inline int is_empty_tree_sha1(const unsigned char *sha1)
> > +static inline int is_empty_tree_oid(const struct object_id *oid)

Yeah, their names suggest they in no way depend upon internals of a
repository, but that's largely because they are misnamed, similar to
our old friends like get_oid() and repo_get_oid().

> The goal to remove repository.h from hash.h and object.h makes sense
> as a goal, but is there another way to do it? Can we redefine
> 'extern struct repository *the_repository;' in hash.h so we have
> access to that pointer?

Having access to the pointer is insufficient.  These functions all
make use of the_repository->hash_algo, and are inline, so the
definition and layout of struct repository is necessary.

An alternate solution might be to just wrap the #define of
the_hash_algo, as well as these functions, with
NO_THE_REPOSITORY_COMPATIBILITY_MACROS, try to convince Ævar that we
put those #ifdefs in before he wrote his series[1] and that he thus
missed some uses of that macro, and then watch him transform all of
them with a series of cocci changes so these functions just go away.
;-)

But that sounds like a separate big series, and I'm dubious about my
ability to hoodwink Ævar.

Any other ideas?


[1] https://lore.kernel.org/git/cover-v2-00.17-00000000000-20230328T110946Z-avarab@gmail.com/