Re: [PATCH 2/6] refs: move duplicate refname update check to generic layer

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10753 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Feb 07, 2025 at 08:34:37AM +0100, Karthik Nayak wrote:
>> Move the tracking of refnames in `affected_refnames` from individual
>> backends into the generic layer in 'refs.c'. This centralizes the
>> duplicate refname detection that was previously handled separately by
>> each backend.
>
> Exciting, this has been on my TODO list for quite a while already.
>

Yeah, I saw that you left a TODO in the reftable backend too. This
change was not really needed for partial transactions. But it does make
things a bit nicer and easier.

>> Make some changes to accommodate this move:
>>
>>   - Add a `string_list` field `refnames` to `ref_transaction` to contain
>>     all the references in a transaction. This field is updated whenever
>>     a new update is added.
>>
>>   - Modify the backends to use this field internally as needed. The
>>     backends need to check if an update for refname already exists when
>>     splitting symrefs or adding an update for 'HEAD'.
>
> Okay. Is this actually necessary to be handled by the backends? I
> would've expected that it is possible to split up symref updates so that
> we insert both symref and target into the list. I wouldn't be surprised
> if this wasn't easily possible though -- the logic here is surprisingly
> intricate.

It is a bit intricate and requires a bit of unwinding to move it to the
generic layer. But it is possible, I tried to timebox it for this patch
series, but unfortunately it needs a lot more time. So perhaps,
something for later.

>
>>   - In the reftable backend, in `reftable_be_transaction_prepare()`,
>>     move the instance of `string_list_has_string()` above
>>     `ref_transaction_add_update()` to check before the reference is
>>     added.
>>
>> This helps reduce duplication of functionality between the backends and
>> makes it easier to make changes in a more centralized manner.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs.c                  | 17 ++++++++++++
>>  refs/files-backend.c    | 69 ++++++++++---------------------------------------
>>  refs/packed-backend.c   | 25 +-----------------
>>  refs/refs-internal.h    |  2 ++
>>  refs/reftable-backend.c | 53 ++++++++++++-------------------------
>>  5 files changed, 50 insertions(+), 116 deletions(-)
>
> Nice.
>
>> diff --git a/refs.c b/refs.c
>> index f4094a326a9f88f979654b668cc9c3d27d83cb5d..4c9b706461977995be1d55e7667f7fb708fbbb76 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
>>  	CALLOC_ARRAY(tr, 1);
>>  	tr->ref_store = refs;
>>  	tr->flags = flags;
>> +	string_list_init_dup(&tr->refnames);
>
> Do we actually have to duplicate strings? I would've expected that we
> keep strings alive via the `ref_update`s anyway during the transaction's
> lifetime.
>

True. I was more thinking along the lines of the keeping the memory
concerns separate. Also, I  sure if there are any scenario's that
a `ref_transaction` could outlive a `ref_update`.

> It might also be interesting to check whether using a strset for this
> is more efficient. But that is certainly outside the scope of your patch
> series and can be done at a later point. #leftoverbit
>

Agreed.

>> @@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update(
>>  		update->msg = normalize_reflog_message(msg);
>>  	}
>>
>> +	/*
>> +	 * This list is generally used by the backends to avoid duplicates.
>> +	 * But we do support multiple log updates for a given refname within
>> +	 * a single transaction.
>> +	 */
>> +	if (!(update->flags & REF_LOG_ONLY)) {
>> +		item = string_list_append(&transaction->refnames, refname);
>> +		item->util = update;
>> +	}
>> +
>>  	return update;
>>  }
>> @@ -2397,6 +2410,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
>>  		return -1;
>>  	}
>>
>> +	string_list_sort(&transaction->refnames);
>> +	if (ref_update_reject_duplicates(&transaction->refnames, err))
>> +		return TRANSACTION_GENERIC_ERROR;
>> +
>>  	ret = refs->be->transaction_prepare(refs, transaction, err);
>>  	if (ret)
>>  		return ret;
>
> Okay, we keep the list unserted initially, but sort it later before
> passing it to the backends so that `string_list_has_string()` works
> correctly. Good.
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index c6a3f6d6261a894e1c294bb1329fdf8079a39eb4..18da30c3f37dc5c09f7d81a9083d6b41d0463bd5 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2425,7 +2423,6 @@ static int split_head_update(struct ref_update *update,
>>  	 */
>>  	if (strcmp(new_update->refname, "HEAD"))
>>  		BUG("%s unexpectedly not 'HEAD'", new_update->refname);
>> -	string_list_insert(affected_refnames, new_update->refname);
>>
>>  	return 0;
>>  }
>
> Previously we would've inserted "HEAD" into the list of affected
> refnames even if it wasn't directly updated. Why don't we have to do
> that now anymore?
>

We still do, right above this code, there is a call to
`ref_transaction_add_update()`. So any ref_update added to the
transaction via `ref_transaction_add_update()` will also add the refname
to `transaction->refnames`.

>> @@ -2441,7 +2438,6 @@ static int split_head_update(struct ref_update *update,
>> @@ -2491,15 +2487,6 @@ static int split_symref_update(struct ref_update *update,
>>  	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
>>  	update->flags &= ~REF_HAVE_OLD;
>>
>> -	/*
>> -	 * Add the referent. This insertion is O(N) in the transaction
>> -	 * size, but it happens at most once per symref in a
>> -	 * transaction. Make sure to add new_update->refname, which will
>> -	 * be valid as long as affected_refnames is in use, and NOT
>> -	 * referent, which might soon be freed by our caller.
>> -	 */
>> -	string_list_insert(affected_refnames, new_update->refname);
>> -
>>  	return 0;
>>  }
>
> Same question here, but for symref updates.
>

Same as above. In summary, the need for adding new refnames to the list
is now centralized and a part of adding a ref update to the transaction.

It's a good question, so I'll also add a hint in the commit message.

>
>> @@ -3030,13 +2995,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>>  	if (transaction->state != REF_TRANSACTION_PREPARED)
>>  		BUG("commit called for transaction that is not prepared");
>>
>> -	/* Fail if a refname appears more than once in the transaction: */
>> -	for (i = 0; i < transaction->nr; i++)
>> -		if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
>> -			string_list_append(&affected_refnames,
>> -					   transaction->updates[i]->refname);
>> -	string_list_sort(&affected_refnames);
>> -	if (ref_update_reject_duplicates(&affected_refnames, err)) {
>> +	string_list_sort(&transaction->refnames);
>> +	if (ref_update_reject_duplicates(&transaction->refnames, err)) {
>>  		ret = TRANSACTION_GENERIC_ERROR;
>>  		goto cleanup;
>>  	}
>
> Can't we also make this check generic for initial transactions?
>

This one is handled in the next commit, I mostly separated them out
because I was not sure why this needs to be here and to draw attention
if I'm missing something when removing this.

>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
>> index a7b6f74b6e35f897f619c540cbc600bbd888bc67..6e7acb077e81435715a1ca3cc928550147c8c56a 100644
>> --- a/refs/packed-backend.c
>> +++ b/refs/packed-backend.c
>> @@ -1653,34 +1648,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
>>  	 */
>>
>>  	CALLOC_ARRAY(data, 1);
>> -	string_list_init_nodup(&data->updates);
>>
>>  	transaction->backend_data = data;
>>
>> -	/*
>> -	 * Stick the updates in a string list by refname so that we
>> -	 * can sort them:
>> -	 */
>> -	for (i = 0; i < transaction->nr; i++) {
>> -		struct ref_update *update = transaction->updates[i];
>> -		struct string_list_item *item =
>> -			string_list_append(&data->updates, update->refname);
>> -
>> -		/* Store a pointer to update in item->util: */
>> -		item->util = update;
>> -	}
>> -	string_list_sort(&data->updates);
>> -
>> -	if (ref_update_reject_duplicates(&data->updates, err))
>> -		goto failure;
>> -
>>  	if (!is_lock_file_locked(&refs->lock)) {
>>  		if (packed_refs_lock(ref_store, 0, err))
>>  			goto failure;
>>  		data->own_lock = 1;
>>  	}
>>
>> -	if (write_with_updates(refs, &data->updates, err))
>> +	if (write_with_updates(refs, &transaction->refnames, err))
>>  		goto failure;
>>
>>  	transaction->state = REF_TRANSACTION_PREPARED;
>
> This change is a lot more straight-forward because the packed backend
> does not support symrefs at all. Nice.
>

Yes indeed.

>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index d39a14c5a469d7d219362e9eae4f578784d65a5b..dd2099d94948a4f23fd9f7ddc06bf3d741229eba 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1202,12 +1184,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>  				goto done;
>>  			}
>>
>> -			new_update = ref_transaction_add_update(
>> -					transaction, "HEAD",
>> -					u->flags | REF_LOG_ONLY | REF_NO_DEREF,
>> -					&u->new_oid, &u->old_oid, NULL, NULL, NULL,
>> -					u->msg);
>> -			string_list_insert(&affected_refnames, new_update->refname);
>> +			ref_transaction_add_update(
>> +				transaction, "HEAD",
>> +				u->flags | REF_LOG_ONLY | REF_NO_DEREF,
>> +				&u->new_oid, &u->old_oid, NULL, NULL, NULL,
>> +				u->msg);
>>  		}
>>
>>  		ret = reftable_backend_read_ref(be, rewritten_ref,
>
> Equivalent question as for the files backend.
>

I hope my answer earlier helps, especially since the diff here shows the
call to `ref_transaction_add_update()`.

>> @@ -1277,6 +1258,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>  				if (!strcmp(rewritten_ref, "HEAD"))
>>  					new_flags |= REF_UPDATE_VIA_HEAD;
>>
>> +				if (string_list_has_string(&transaction->refnames, referent.buf)) {
>> +					strbuf_addf(err,
>> +						    _("multiple updates for '%s' (including one "
>> +						    "via symref '%s') are not allowed"),
>> +						    referent.buf, u->refname);
>> +					ret = TRANSACTION_NAME_CONFLICT;
>> +					goto done;
>> +				}
>> +
>>  				/*
>>  				 * If we are updating a symref (eg. HEAD), we should also
>>  				 * update the branch that the symref points to.
>
> This change surprised me a bit. You mention it in the commit message,
> but don't state a reason why you do it.
>

When a `ref_update` is added to the `transaction` via
`ref_transaction_add_update()`, the refname is automatically added to
`transaction->refnames`. As a result, checking for the refname in this
list will always return true. I'll clarify this in the commit message.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]