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

From: Bert Wesarg <bert.wesarg@googlemail.com>
Date: Jul 13, 2011, 08:45 UTC
Message-ID: <CAKPyHN1tixwJPJHG+wY34HVLYGT4fD9Sc-qJ8=on8EWfW-H6aw@mail.gmail.com>
In-Reply-To: <20110713083139.GA26838@sigill.intra.peff.net>

On Wed, Jul 13, 2011 at 10:31, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

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

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.

Show 8 quoted lines

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