On Mon, Apr 17, 2023 at 1:59 PM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > On 4/15/2023 11:03 PM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@xxxxxxxxx> > > > > 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@xxxxxxxxx/
