On Mon, May 15, 2023 at 5:16 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > Move this function to hash-ll, so that khash.h can stop depending upon
> > hashmap.h.
>
> [snip]
>
> > Diff best viewed with `--color-moved`. Note that there's a small but
> > important change to khash.h in this patch as well to allow this move,
> > which is easy to overlook.
>
> [snip]
>
> > diff --git a/khash.h b/khash.h
> > index a0a08dad8b7..ff881631778 100644
> > --- a/khash.h
> > +++ b/khash.h
> > @@ -26,7 +26,6 @@
> > #ifndef __AC_KHASH_H
> > #define __AC_KHASH_H
> >
> > -#include "hashmap.h"
> > #include "hash.h"
>
> Is this the small but important change to khash.h? I thought that this
> is the reason for the change, not what "allow[ed]" this move.
No, the small but important change to khash.h was only in v1, and was
split out into a separate patch based on feedback. I just forgot to
update this commit's message when I did so. Sorry about that; I'll
fix it up.
>
> > diff --git a/object-name.c b/object-name.c
> > index 45f4d51305b..7e96c97051e 100644
> > --- a/object-name.c
> > +++ b/object-name.c
>
> [snip]
>
> > +struct raw_object_store {
> > + /*
> > + * Set of all object directories; the main directory is first (and
> > + * cannot be NULL after initialization). Subsequent directories are
> > + * alternates.
> > + */
> > + struct object_directory *odb;
> > + struct object_directory **odb_tail;
> > + struct kh_odb_path_map *odb_by_path;
>
> This is not present as a minus line (that line uses kh_odb_path_map_t).
> I thought this was so that we could forward declare "struct
> kh_odb_path_map", but I don't see such a forward declaration. (Was it
> declared elsewhere?)
I think this hunk probably should have been part of the khash.h patch
that was separated out, to make it more clear. I'll fix that up.
> Everything else looks good.
Thanks, but I realized some point on Saturday that this patch was an
accidental squash of two patches -- one splitting object-store-ll.h
out of object-store.h (the majority of the patch), and the other about
the hash-ll/hashmap/khash changes. I'll split it back out in my
re-roll. There won't be any code changes, just splitting the patch
into its individual parts.
