Re: [PATCH v5 09/11] reftable: split up write options

[Justin Tobler <jltobler@gmail.com>]

On 26/06/22 10:28AM, Patrick Steinhardt wrote:
> When initializing the reftable stack the caller may optionally pass some
> write options. These write options mix up two different concerns though:
> 
>   - Of course, they allow the caller to configure how new reftables are
>     being written.
> 
>   - But they also allow the caller to configure the stack itself, like
>     its hash ID and the `on_reload` callback.
> 
> This is somewhat awkward, as it doesn't easily give the caller the
> flexibility to for example write multiple reftables with different
> options. Furthermore, this requires us to eagerly parse relevant
> configuration when initializing the reftable backend.

Naive question: are there any current use cases where callers may want
to write multiple reftables with a different set of options? Can
reftables written with different options pose any correctness issues?

> Refactor the code by splitting out those options that configure the
> stack itself. Creating a new stack will thus only require this limited
> set of options, whereas the caller is expected to pass write options to
> all functions that end up writing tables.

Splitting this up sounds reasonable.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c             |  29 +++---
>  reftable/reftable-stack.h           |  30 +++++-
>  reftable/reftable-writer.h          |  17 +---
>  reftable/stack.c                    | 100 ++++++++++++-------
>  reftable/stack.h                    |   2 +-
>  reftable/writer.c                   |  21 ++--
>  reftable/writer.h                   |   1 +
>  t/helper/test-reftable.c            |   2 +-
>  t/unit-tests/lib-reftable.c         |   8 +-
>  t/unit-tests/lib-reftable.h         |   2 +
>  t/unit-tests/u-reftable-merged.c    |   9 +-
>  t/unit-tests/u-reftable-readwrite.c |  38 ++++++--
>  t/unit-tests/u-reftable-stack.c     | 189 ++++++++++++++++--------------------
>  t/unit-tests/u-reftable-table.c     |   8 +-
>  14 files changed, 258 insertions(+), 198 deletions(-)
> 
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 5115a3f4ce..608d71cf10 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -48,9 +48,9 @@ static void reftable_backend_on_reload(void *payload)
>  
>  static int reftable_backend_init(struct reftable_backend *be,
>  				 const char *path,
> -				 const struct reftable_write_options *_opts)

Ok so now during init we only care about `struct
reftable_stack_options`. The `struct reftable_write_options` are only
needed during reftable writes.

[snip]
> +/* Options related to opening a stack. */
> +struct reftable_stack_options {
> +	/*
> +	 * 4-byte identifier ("sha1", "s256") of the hash. Defaults to SHA1 if
> +	 * unset.
> +	 */
> +	enum reftable_hash hash_id;
> +
> +	/*
> +	 * Callback function to execute whenever the stack is being reloaded.
> +	 * This can be used e.g. to discard cached information that relies on
> +	 * the old stack's data. The payload data will be passed as argument to
> +	 * the callback.
> +	 */
> +	void (*on_reload)(void *payload);
> +	void *on_reload_payload;
> +};

These are the options split out from `struct reftable_write_options` and
are the options used at initialization and expected to remain consistent
across reftable writes. I assume these also won't depend on reading the
config prior to the ref store being initialzed.

[snip]
> diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
> index a66db415c8..6ff4ddfc60 100644
> --- a/reftable/reftable-writer.h
> +++ b/reftable/reftable-writer.h
> @@ -28,11 +28,6 @@ struct reftable_write_options {
>  	/* how often to write complete keys in each block. */
>  	uint16_t restart_interval;
>  
> -	/* 4-byte identifier ("sha1", "s256") of the hash.
> -	 * Defaults to SHA1 if unset
> -	 */
> -	enum reftable_hash hash_id;
> -
>  	/* Default mode for creating files. If unset, use 0666 (+umask) */
>  	unsigned int default_permissions;
>  
> @@ -60,15 +55,6 @@ struct reftable_write_options {
>  	 * negative value will cause us to block indefinitely.
>  	 */
>  	long lock_timeout_ms;
> -
> -	/*
> -	 * Callback function to execute whenever the stack is being reloaded.
> -	 * This can be used e.g. to discard cached information that relies on
> -	 * the old stack's data. The payload data will be passed as argument to
> -	 * the callback.
> -	 */
> -	void (*on_reload)(void *payload);
> -	void *on_reload_payload;
>  };

These write options are explicitly passed around during write
operations. I assume some of these options must be parsed from the
config and thus will need to be lazy-loaded to avoid "onbranch"
conditions prior to the ref store being initialzed.

The rest of this patch looks to be adjusting call sites to wire these
options through as needed and looks correct. I don't see any changes to
lazy-load write option configuration yet, but I suppose that will happen
in a subsequent patch.

-Justin