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

On Wed, Jul 13, 2011 at 10:31, Jeff King [off-list ref] wrote:

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

quoted

quoted

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

You're right here, that it is not part of the public API, but you're
not wrong about your guess. But when reading this patch series, the
reader obviously knows that the metadata-cache uses a struct
decoration for the in-memory values. Thus the reader knows that 0 is
special for struct decoration, and that there is an API to get the
width from the struct decoration.

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.

That should be good. Thanks.

Bert

-Peff