Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 14:20:36 -0700
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZRPwdsWNV_YUDNmUY2J839=SbkBbtqrbfgBxjDVZ6PrxA@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> On Wed, Aug 19, 2026 at 03:19:37PM +0200, Karthik Nayak wrote:
>> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,
>> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so
>> that callers of `reftable_stack_init_addition()` can also reload the
>> stack if there was a concurrent update made before the lock was
>> obtained.
>>
>> Then 16684b6fae (refs/reftable: always reload stacks when creating
>> lock, 2025-08-12) updated all of the remaining call-sites to propagate
>> this flag to ensure that we always reload the stack whenever there was a
>> concurrent update.
>>
>> As all calls to `reftable_stack_init_addition()` inevitably propagate
>> the flag, it is safe to remove the flag and its associated code and make
>> the reloading of the stack the default flow. This makes it easier to
>> follow the flow and simplifies the logic.
>>
>> The only exceptions are:
>>
>>   1. Unit tests, where we explicitly do not propagate the flag. These
>>      tests are now modified with the new status quo.
>>
>>   2. `reftable_stack_clean_locked()`, which was propagating 0 to
>>      `reftable_stack_new_addition()` but was then manually reloading the
>>      stack after. Here the new flow will achieve the same, while also
>>      allowing us to remove the manual reload.
>
> libgit2 uses this flag though, so we'd have to adapt it, too. As far as
> I can see though all of the calls to `reftable_stack_add()` it has pass
> this flag.
>

Okay, that should be simple then! I can send in a pull request when/if
this lands.

>> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
>> index 5d22d84e80..5d224f8079 100644
>> --- a/reftable/reftable-stack.h
>> +++ b/reftable/reftable-stack.h
>> @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);
>>  /* holds a transaction to add tables at the top of a stack. */
>>  struct reftable_addition;
>>
>> -enum {
>> -	/*
>> -	 * Reload the stack when the stack is out-of-date after locking it.
>> -	 */
>> -	REFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),
>> -};
>> -
>>  /*
>>   * returns a new transaction to add reftables to the given stack. As a side
>> - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*
>> - * flags.
>> + * effect, the ref database is locked.
>>   */
>>  int reftable_stack_new_addition(struct reftable_addition **dest,
>>  				struct reftable_stack *st,
>> -				const struct reftable_write_options *opts,
>> -				unsigned int flags);
>> +				const struct reftable_write_options *opts);
>>
>>  /* Adds a reftable to transaction. */
>>  int reftable_addition_add(struct reftable_addition *add,
>
> We're already busy adapting this function anyway, so do we maybe want to
> fix its name to `reftable_stack_addition_new` while at it?
>
> Patrick

I'm assuming you're talking about `reftable_stack_new_addition`? We
could, I could add another commit here.
signature.asc (application/pgp-signature, 690 B)