From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 13 Oct 2025 09:46:33 GMT
Subject: Re: [PATCH 8/9] refs: add a `optimize_required` field to `struct ref_storage_be`
Message-ID: <CAOLa=ZTQWj7BUDNx5iuoYJ6YR5JqhFVK9O2vWiKqL5CNExpsUw@mail.gmail.com>
In-Reply-To: <aOjsh4102DYctgQE@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

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

I also think that individually the packed-backend shouldn't care about
the files backend. So it shouldn't be calling `should_pack_refs()`. In
short, the packed backend knows that it is always optimized and it
should simply state that.

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

Fair enough, I was thinking that when users say 'git refs optimize'
without any options. We always call `reftable_stack_compact_all()`. So
it would always make sense to say required = true.

But `reftable_stack_compact_all()` does make that check. So I'll add it
to `reftable_stack_compaction_required()` and call that with a
`use_heuristics` parameter.

Thanks