From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 10 Oct 2025 11:22:47 GMT
Subject: Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`
Message-ID: <aOjsh4102DYctgQE@pks.im>
In-Reply-To: <20251010-562-add-option-to-check-if-reference-backend-needs-repacking-v1-8-c7962be584fa@gmail.com>

On Fri, Oct 10, 2025 at 12:27:12PM +0200, Karthik Nayak wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 1c37899006..c262ae1a7b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1513,6 +1513,16 @@ static int files_optimize(struct ref_store *ref_store,
>  	return 0;
>  }
>  
> +static int files_optimize_required(struct ref_store *ref_store,
> +				   struct optimize_refs_opts *opts,
> +				   bool *required)
> +{
> +	struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
> +						      "optimize_required");
> +	*required = should_pack_refs(refs, opts);
> +	return 0;
> +}

Okay, this is nice and straight-forward. One might argue that we could
also have the following (uncompiled, so pseudo-code-y) implementation:

	static int files_optimize_required(struct ref_store *ref_store,
					   struct optimize_refs_opts *opts,
					   bool *required)
	{
		struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ,
							      "optimize_required");
		*required = should_pack_refs(refs, opts) ||
			refs_optimize_required(refs->packed_refs, opts);
		return 0;
	}

But on the other hand we know that they aren't ever repacked, so it's
probably dumb.

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index acaa5a6e57..c94948f618 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1784,6 +1784,17 @@ static int packed_optimize(struct ref_store *ref_store UNUSED,
>  	return 0;
>  }
>  
> +static int packed_optimize_required(struct ref_store *ref_store UNUSED,
> +				    struct optimize_refs_opts *opts UNUSED,
> +				    bool *required)
> +{
> +	/*
> +	 * Packed refs are already optimized.
> +	 */
> +	*required = false;
> +	return 0;
> +}

Yup.

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index d77714366a..df39fe9b38 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1732,6 +1732,29 @@ static int reftable_be_optimize(struct ref_store *ref_store,
>  	return ret;
>  }
>  
> +static int reftable_be_optimize_required(struct ref_store *ref_store,
> +					 struct optimize_refs_opts *opts,
> +					 bool *required)
> +{
> +	struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_READ,
> +							       "optimize_refs_required");
> +	struct reftable_stack *stack;
> +
> +	if (refs->err)
> +		return refs->err;
> +
> +	stack = refs->worktree_backend.stack;
> +	if (!stack)
> +		stack = refs->main_backend.stack;
> +
> +	if (opts->flags & OPTIMIZE_REFS_AUTO)
> +		return reftable_stack_compaction_required(stack, required);
> +	else
> +		*required = true;

This doesn't make much sense. We should only indicate that we require
repacking when there are at least two tables in the stack.

Patrick