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

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, May 10, 2024 at 02:29:19AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> [snip]
> 
> > @@ -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

It's unnecessary indeed. I first wanted to defend this, but then I
noticed that I'm also being inconsistent here where the last branch
won't have `return 0;` at the end of this series.

Will remove.

> > +	}
> > +
> > +	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?

I think it's easier to read that way.

> >  	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?

Headers in the "reftable/" directory which do not have a "reftable-"
prefix are considered to be private. So those shouldn't be used.

Patrick

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