Re: [PATCH v5 1/3] reftable/stack: allow disabling of auto-compaction

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5099 bytes --]

On Thu, Apr 04, 2024 at 06:29:27PM +0000, Justin Tobler via GitGitGadget wrote:
> From: Justin Tobler <jltobler@gmail.com>
> 
> Move the `disable_auto_compact` option into `reftable_write_options` to
> allow a stack to be configured with auto-compaction disabled. In a
> subsequent commit, this is used to disable auto-compaction when a
> specific environment variable is set.

This patch looks good to me. I think the commit subject and message
could use a bit of polishing though: we do not add a new way to disable
auto-compaction as that already exists. The important bit though is that
this toggle is purely internal right now and thus cannot be accessed by
library users.

I'd thus propose something along the following lines:

    ```
    reftable/stack: expose option to disable auto-compaction

    While the reftable stack already has a bit controls whether or not
    to run auto-compation, this bit is not accessible to users of the
    library. There are usecases though where a caller may want to have
    more control over auto-compaction.

    Move the `disable_auto_compact` option into `reftable_write_options`
    to allow external callers to disable auto-compaction. This will be
    used in a subsequent commit.
    ```

Patrick

> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  reftable/reftable-writer.h |  3 +++
>  reftable/stack.c           |  2 +-
>  reftable/stack.h           |  1 -
>  reftable/stack_test.c      | 11 ++++++-----
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
> index 7c7cae5f99b..155bf0bbe2a 100644
> --- a/reftable/reftable-writer.h
> +++ b/reftable/reftable-writer.h
> @@ -46,6 +46,9 @@ struct reftable_write_options {
>  	 *   is a single line, and add '\n' if missing.
>  	 */
>  	unsigned exact_log_message : 1;
> +
> +	/* boolean: Prevent auto-compaction of tables. */
> +	unsigned disable_auto_compact : 1;
>  };
>  
>  /* reftable_block_stats holds statistics for a single block type */
> diff --git a/reftable/stack.c b/reftable/stack.c
> index dde50b61d69..1a7cdad12c9 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -680,7 +680,7 @@ int reftable_addition_commit(struct reftable_addition *add)
>  	if (err)
>  		goto done;
>  
> -	if (!add->stack->disable_auto_compact) {
> +	if (!add->stack->config.disable_auto_compact) {
>  		/*
>  		 * Auto-compact the stack to keep the number of tables in
>  		 * control. It is possible that a concurrent writer is already
> diff --git a/reftable/stack.h b/reftable/stack.h
> index d919455669e..c862053025f 100644
> --- a/reftable/stack.h
> +++ b/reftable/stack.h
> @@ -19,7 +19,6 @@ struct reftable_stack {
>  	int list_fd;
>  
>  	char *reftable_dir;
> -	int disable_auto_compact;
>  
>  	struct reftable_write_options config;
>  
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 351e35bd86d..4fec823f14f 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -325,7 +325,7 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
>  		 * we can ensure that we indeed honor this setting and have
>  		 * better control over when exactly auto compaction runs.
>  		 */
> -		st->disable_auto_compact = i != n;
> +		st->config.disable_auto_compact = i != n;
>  
>  		err = reftable_stack_new_addition(&add, st);
>  		EXPECT_ERR(err);
> @@ -497,6 +497,7 @@ static void test_reftable_stack_add(void)
>  	struct reftable_write_options cfg = {
>  		.exact_log_message = 1,
>  		.default_permissions = 0660,
> +		.disable_auto_compact = 1,
>  	};
>  	struct reftable_stack *st = NULL;
>  	char *dir = get_tmp_dir(__LINE__);
> @@ -508,7 +509,6 @@ static void test_reftable_stack_add(void)
>  
>  	err = reftable_new_stack(&st, dir, cfg);
>  	EXPECT_ERR(err);
> -	st->disable_auto_compact = 1;
>  
>  	for (i = 0; i < N; i++) {
>  		char buf[256];
> @@ -935,7 +935,9 @@ static void test_empty_add(void)
>  
>  static void test_reftable_stack_auto_compaction(void)
>  {
> -	struct reftable_write_options cfg = { 0 };
> +	struct reftable_write_options cfg = {
> +		.disable_auto_compact = 1,
> +	};
>  	struct reftable_stack *st = NULL;
>  	char *dir = get_tmp_dir(__LINE__);
>  
> @@ -945,7 +947,6 @@ static void test_reftable_stack_auto_compaction(void)
>  	err = reftable_new_stack(&st, dir, cfg);
>  	EXPECT_ERR(err);
>  
> -	st->disable_auto_compact = 1; /* call manually below for coverage. */
>  	for (i = 0; i < N; i++) {
>  		char name[100];
>  		struct reftable_ref_record ref = {
> @@ -994,7 +995,7 @@ static void test_reftable_stack_add_performs_auto_compaction(void)
>  		 * we can ensure that we indeed honor this setting and have
>  		 * better control over when exactly auto compaction runs.
>  		 */
> -		st->disable_auto_compact = i != n;
> +		st->config.disable_auto_compact = i != n;
>  
>  		strbuf_reset(&refname);
>  		strbuf_addf(&refname, "branch-%04d", i);
> -- 
> gitgitgadget
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]