Re: [RFC/PATCHv2 2/6] add metadata-cache infrastructure

[Jeff King <peff@peff.net>]

On Wed, Jul 13, 2011 at 10:18:28AM +0200, Bert Wesarg wrote:

> > +static int record_size(const struct metadata_cache *c)
> > +{
> > +       /* a record is a 20-byte sha1 plus the width of the value */
> > +       return c->mem.width + 20;
> 
> You are circumventing your own API. Why do you don't use the
> decoration_width() accessor here? I don't see any check that
> METADATA_CACHE_INIT("frotz", 0, NULL) is invalid neither in the
> documentation nor in the code.

"struct decoration" has the "0 width means store a void pointer" rule
for compatibility with existing callers. But I never intended for
metadata-cache to have such an exception. Nor would it make sense to
store a void pointer. The pointer would be written to disk, and would
then be meaningless during the next run of the program.

I didn't figure anyone would assume the same special rule held for
metadata-cache; the fact that it is implemented using "struct
decoration" is not part of its public API. But I guess I was wrong.

It might make sense to put:

  if (!c->mem.width)
          die("BUG: zero-width metadata-cache");

into the initialization function to make it more clear, and make a note
in the API documentation.

I considered briefly that a zero-width cache might actually be useful
for storing a membership list (i.e., "is this sha1 in the list or not").
But then you have no way of distinguishing "not in the list" from "have
no checked whether it should be in the list". You are probably better
off storing a single byte flag in such cases.

-Peff