Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s

[Jeff King <peff@peff.net>]

On Thu, May 11, 2017 at 03:46:39PM -0700, Jonathan Nieder wrote:

> > +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)
> > +{
> > +	for (; refs; refs = refs->next)
> > +		oidset_insert(oids, &refs->old_oid);
> > +}
> > +
> > +static int tip_oids_contain(struct oidset *tip_oids,
> > +			    struct ref *unmatched, struct ref *newlist,
> > +			    const struct object_id *id)
> > +{
> > +	if (!tip_oids->map.cmpfn) {
> 
> This feels like a layering violation.  Could it be e.g. a static inline
> function oidset_is_initialized in oidset.h?

It definitely is a layering violation, though we normally are pretty
"open" with letting callers peek in at our data structures.

I'd be fine with it as-is or with the helper function you suggested.
But...

> +/**
> + * Returns true iff "set" has been initialized (for example by inserting
> + * an entry). An oidset is considered uninitialized if it hasn't had any
> + * oids inserted since it was last cleared.
> + */
> +static inline int oidset_initialized(const struct oidset *set)
> +{
> +	return set->map.cmpfn ? 1 : 0;
> +}

Now we're committing our own layering violation. I was surprised to find
that hashmap_free() clears "cmpfn", and I'm not sure if we would want to
count on that (not that it probably matters all that much, but it is
required for the description you gave to be accurate).

The hashmap documentation suggests using "tablesize" for checking
whether something is initialized. Maybe we ought to use that.

-Peff