Re: [PATCH 06/11] refs/reftable: allow configuring block size

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 10, 2024, 09:29 UTC
Message-ID: <CAOLa=ZR7=hVXyGr2iSZW2C8mtwR2SHLikuCVKS-qTB7AE+0+hQ@mail.gmail.com>
In-Reply-To: <b4e4db5735beb106511980adca48dc416c4b0d95.1714630191.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 16 quoted lines

> @@ -230,6 +231,23 @@ static int read_ref_without_reload(struct reftable_stack *stack,
>  	return ret;
>  }
>
> +static int reftable_be_config(const char *var, const char *value,
> +			      const struct config_context *ctx,
> +			      void *_opts)
> +{
> +	struct reftable_write_options *opts = _opts;
> +
> +	if (!strcmp(var, "reftable.blocksize")) {
> +		unsigned long block_size = git_config_ulong(var, value, ctx->kvi);
> +		if (block_size > 16777215)
> +			die("reftable block size cannot exceed 16MB");
> +		opts->block_size = block_size;
> +		return 0;

nit: unecessary return

Show 14 quoted lines

> +	}
> +
> +	return 0;
> +}
> +
>  static struct ref_store *reftable_be_init(struct repository *repo,
>  					  const char *gitdir,
>  					  unsigned int store_flags)
> @@ -245,12 +263,24 @@ static struct ref_store *reftable_be_init(struct repository *repo,
>  	base_ref_store_init(&refs->base, repo, gitdir, &refs_be_reftable);
>  	strmap_init(&refs->worktree_stacks);
>  	refs->store_flags = store_flags;
> -	refs->write_options.block_size = 4096;
> +

Nit: do we need this newline?

Show 17 quoted lines

>  	refs->write_options.hash_id = repo->hash_algo->format_id;
>  	refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);
>  	refs->write_options.disable_auto_compact =
>  		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
>
> +	git_config(reftable_be_config, &refs->write_options);
> +
> +	/*
> +	 * It is somewhat unfortunate that we have to mirror the default block
> +	 * size of the reftable library here. But given that the write options
> +	 * wouldn't be updated by the library here, and given that we require
> +	 * the proper block size to trim reflog message so that they fit, we
> +	 * must set up a proper value here.
> +	 */
> +	if (!refs->write_options.block_size)
> +		refs->write_options.block_size = 4096;
> +

Wouldn't it be to import and use `reftable/constants.h` here?

[snip]