Re: [PATCH v4 6/8] refs: implement batch reference update support
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Mar 24, 2025, 14:54 UTC
- Message-ID
- <CAOLa=ZQ7nYNMZ2rHBao=vf-HxpWOy6g_jywi0bU5nTf6Tx=F+g@mail.gmail.com>
- In-Reply-To
- <Z9x590KcwP8WGGG-@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 52 quoted lines
> On Thu, Mar 20, 2025 at 12:44:01PM +0100, Karthik Nayak wrote:
>> diff --git a/refs.c b/refs.c
>> index 3d0b53d56e..b34ec198f5 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1206,11 +1210,45 @@ void ref_transaction_free(struct ref_transaction *transaction)
>> free((char *)transaction->updates[i]->old_target);
>> free(transaction->updates[i]);
>> }
>> +
>> + if (transaction->rejections)
>> + free(transaction->rejections->update_indices);
>> + free(transaction->rejections);
>> +
>> string_list_clear(&transaction->refnames, 0);
>> free(transaction->updates);
>> free(transaction);
>> }
>>
>> +int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
>> + size_t update_idx,
>> + enum ref_transaction_error err)
>> +{
>> + if (update_idx >= transaction->nr)
>> + BUG("trying to set rejection on invalid update index");
>> +
>> + if (!(transaction->flags & REF_TRANSACTION_ALLOW_FAILURE))
>> + return 0;
>> +
>> + if (!transaction->rejections)
>> + BUG("transaction not inititalized with failure support");
>> +
>> + /*
>> + * Don't accept generic errors, since these errors are not user
>> + * input related.
>> + */
>> + if (err == REF_TRANSACTION_ERROR_GENERIC)
>> + return 0;
>> +
>> + transaction->updates[update_idx]->rejection_err = err;
>> + ALLOC_GROW(transaction->rejections->update_indices,
>> + transaction->rejections->nr + 1,
>> + transaction->rejections->alloc);
>> + transaction->rejections->update_indices[transaction->rejections->nr++] = update_idx;
>> +
>> + return 1;
>> +}
>
> If we had a `struct ref_update_rejection` we could store the update
> index and rejection errors in the same location, which might be a bit
> easier to reason about.
>I struggled with this a bit, I was thinking the same at the start. But then I was also thinking that a rejection is specific to an update so it should lie in `ref_update`, however tracking failed updates in a transaction is specific to a transaction and should lie in `ref_transaction`. It probably would be easier code-wise to have them both in the same place, but it didn't feel like the correct place.
Show 27 quoted lines
>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
>> index d90bd815a3..7bf57ca948 100644
>> --- a/refs/packed-backend.c
>> +++ b/refs/packed-backend.c
>> @@ -1327,10 +1327,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,
>> * remain locked when it is done.
>> */
>> static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs,
>> - struct string_list *updates,
>> + struct ref_transaction *transaction,
>> struct strbuf *err)
>> {
>> enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
>> + struct string_list *updates = &transaction->refnames;
>> struct ref_iterator *iter = NULL;
>> size_t i;
>> int ok;
>> @@ -1411,6 +1412,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>> "reference already exists",
>> update->refname);
>> ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
>> +
>> + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
>> + strbuf_setlen(err, 0);
>
> Nit: you can use `strbuf_reset()` for this and other instances.
>Yes, indeed, will change!
> Patrick