Re: [PATCH v4 6/8] refs: implement batch reference update support

[Patrick Steinhardt <ps@pks.im>]

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.

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

Patrick