Re: [PATCH v2 03/10] packfile: refactor kept-pack cache to work with packfile stores

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> The kept pack cache is a cache of packfiles that are marked as kept
> either via an accompanying ".kept" file or via an in-memory flag. The
> cache can be retrieved via `kept_pack_cache()`, where one needs to pass
> in a repository.
>
> Ultimately though the kept-pack cache is a property of the packfile
> store, and this causes problems in a subsequent commit where we want to
> move down the packfile store to be a per-object-source entity.
>
> Prepare for this and refactor the kept-pack cache to work on top of a
> packfile store instead. While at it, rename both the function and flags
> specific to the kept-pack cache so that they can be properly attributed
> to the respective subsystems.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/pack-objects.c | 12 ++++++------
>  packfile.c             | 37 ++++++++++++++++++++-----------------
>  packfile.h             | 25 +++++++++++++++++--------
>  reachable.c            |  2 +-
>  revision.c             |  8 ++++----
>  5 files changed, 48 insertions(+), 36 deletions(-)
>
> [snip]
>
> diff --git a/packfile.h b/packfile.h
> index 33cc1c1654..410f85f03d 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -90,9 +90,10 @@ struct packfile_store {
>  	 * is an on-disk ".keep" file or because they are marked as "kept" in
>  	 * memory.
>  	 *
> -	 * Should not be accessed directly, but via `kept_pack_cache()`. The
> -	 * list of packs gets invalidated when the stored flags and the flags
> -	 * passed to `kept_pack_cache()` mismatch.
> +	 * Should not be accessed directly, but via
> +	 * `packfile_store_get_kept_pack_cache()`. The list of packs gets
> +	 * invalidated when the stored flags and the flags passed to
> +	 * `packfile_store_get_kept_pack_cache()` mismatch.
>  	 */
>  	struct {
>  		struct packed_git **packs;
> @@ -210,6 +211,19 @@ struct packed_git *packfile_store_load_pack(struct packfile_store *store,
>  int packfile_store_freshen_object(struct packfile_store *store,
>  				  const struct object_id *oid);
>  
> +enum kept_pack_type {
> +	KEPT_PACK_ON_DISK = (1 << 0),
> +	KEPT_PACK_IN_CORE = (1 << 1),
> +};

Thanks for making it more clear this is used as a bitfield. I also
appreciate the rename.

> +
> +/*
> + * Retrieve the cache of kept packs from the given packfile store. Accepts a
> + * combination of `kept_pack_type` flags. The cache is computed on demand and
> + * will be recomputed whenever the flags change.
> + */

Also this comments helps, thanks.

> +struct packed_git **packfile_store_get_kept_pack_cache(struct packfile_store *store,
> +						       unsigned flags);
> +
>  struct pack_window {
>  	struct pack_window *next;
>  	unsigned char *base;

-- 
Cheers,
Toon