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

From: Jeff King <peff@peff.net>
Date: May 12, 2017, 02:59 UTC
Message-ID: <20170512025912.smxc47pz64jzeclw@sigill.intra.peff.net>
In-Reply-To: <20170511224639.GC21723@aiede.svl.corp.google.com>

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

Show 14 quoted lines

> > +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...

Show 9 quoted lines

> +/**
> + * 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