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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 13, 2011, 19:33 UTC
Message-ID: <7vipr66kmz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110713070405.GB18566@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> +`metadata_cache_lookup_uint32`::
> +`metadata_cache_add_uint32`::

I think these are "uint31" functions, as you cannot signal missing entry by returning a value with the MSB set if higher-end of uint32 range can be a valid value.

Show 5 quoted lines

> +	if (c->validity_fun) {
> +		c->validity_fun(validity);
> +		if (hashcmp(validity, p))
> +			return NULL;
> +	}

Two comments.

 - I would have expected that c->validity_check() would be a way for a
   caller to implement a boolean function to check the validity of the
   cache, with another hook c->validity_token() to generate/update the
   token. I could then use the 20-byte space to store a timestamp and
   check can say "It was still 3-days ago? fresh enough", or something
   like that. But this is not a complaint--such a scheme I wrote in the
   above four lines may be _too_ flexible to be useful.

 - I wonder if validity_fn() callback wants a callback parameter (the
   pointer "c" itself, after adding an extra field to metadata_cache that
   stores the callback parameter pointer of type "void *" and adding a
   parameter to METADATA_CACHE_INIT() macro to initialize it).

Other than that, this is looking fun ;-)