Re: [PATCH v6 4/7] refs: add support for transactional symref updates

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

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

Hello Phillip,


Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Karthik
>
> I've left a few comments below - the most important one is about the
> error messages in the reftable backend, non of the others are worth
> re-rolling for on their own.
>
> On 03/05/2024 13:41, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The reference backends currently support transactional reference
>> updates. While this is exposed to users via 'git-update-ref' and its
>> '--stdin' mode, it is also used internally within various commands.
>>
>> However, we never supported transactional updates of symrefs. Let's adds
>
> s/we never supported/we do not support/
>
> s/Let's/This commit/
>
>> support for symrefs in both the 'files' and the 'reftable' backend.
>
> s/backend/backends/
>
>> Here, we add and use `ref_update_has_null_new_value()`, a helper
>> function which is used to check if there is a new_value in a reference
>> update. The new value could either be a symref target `new_target` or a
>> OID `new_oid`.
>>
>> With this, now transactional updates (verify, create, delete, update)
>
> s/With this, //
>

Thanks, will make the above changes.

>> can be used for:
>> - regular refs
>> - symbolic refs
>> - conversion of regular to symbolic refs and vice versa
>
> Excellent
>
>> This also allows us to expose this to users via new commands in
>> 'git-update-ref' in the future.
>
> I'm slightly concerned that splitting out the update-ref changes means
> we don't have any test coverage of the new code beyond the part that is
> used by refs_create_symref()
>

This is definitely true. But I also caught a bunch of edge cases this
way because the tests which indirectly use 'refs_create_symref()' are
quite intensive.

>> Note that a dangling symref update does not record a new reflog entry,
>> which is unchanged before and after this commit.
>>
>> +/*
>> + * Check whether the old_target values stored in update are consistent
>> + * with current_target, which is the symbolic reference's current value.
>> + * If everything is OK, return 0; otherwise, write an error message to
>> + * err and return -1.
>> + */
>> +static int check_old_target(struct ref_update *update,
>> +			    const char *current_target,
>> +			    struct strbuf *err)
>> +{
>> +	if (!update->old_target)
>> +		BUG("called without old_target set");
>> +
>> +	if (!strcmp(update->old_target, current_target))
>> +		return 0;
>> +
>> +	if (!strcmp(current_target, ""))
>> +		strbuf_addf(err, "cannot lock ref '%s': "
>> +			    "reference is missing but expected %s",
>> +			    original_update_refname(update),
>> +			    update->old_target);
>> +	else
>> +		strbuf_addf(err, "cannot lock ref '%s': "
>> +			    "is at %s but expected %s",
>> +			    original_update_refname(update),
>> +			    current_target, update->old_target);
>> +
>> +	return -1;
>> +}
>> @@ -2576,9 +2623,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>>   		}
>>   	}
>>
>> -	if ((update->flags & REF_HAVE_NEW) &&
>> -	    !(update->flags & REF_DELETING) &&
>> -	    !(update->flags & REF_LOG_ONLY)) {
>> +	if (update->new_target && !(update->flags & REF_LOG_ONLY)) {
>> +		if (create_symref_lock(refs, lock, update->refname, update->new_target, err)) {
>
> This line looks quite long
>

Somehow I find it much easier to read these longer lines, unless there
is a logical operator, but I'll split the line mid way with the
arguments.

>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>>   		 * individual refs. But the error messages match what the files
>>   		 * backend returns, which keeps our tests happy.
>>   		 */
>> -		if (u->flags & REF_HAVE_OLD && !oideq(&current_oid, &u->old_oid)) {
>> +		if (u->old_target) {
>> +			if (strcmp(referent.buf, u->old_target)) {
>> +				if (!strcmp(referent.buf, ""))
>> +					strbuf_addf(err, "verifying symref target: '%s': "
>> +						    "reference is missing but expected %s",
>> +						    original_update_refname(u),
>> +						    u->old_target);
>> +				else
>> +					strbuf_addf(err, "verifying symref target: '%s': "
>> +						    "is at %s but expected %s",
>> +						    original_update_refname(u),
>> +						    referent.buf, u->old_target);
>
> The messages in this function differ from the equivalent messages in
> check_old_target() from the files backend above. This is potentially
> confusing to users, creates more work for translators and makes it hard
> to write tests that are independent of the backend. Can we export
> check_old_target() so it can be reused here. If not we should reword
> these messages to match the other messages all of which talk about not
> being able to lock the ref.
>

This is very intentional, the way the backends work at this point are
quite different and while in the files backend, we talk about locking a
particular ref. In the reftable backend we do not lock single refs. We
lock tables. So keeping it consistent doesn't make sense here.

However, we could make the files backend similar to this one, I would be
okay doing that.

>> +				ret = -1;
>> +				goto done;
>> +			}
>> +		} else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
>>   			if (is_null_oid(&u->old_oid))
>>   				strbuf_addf(err, _("cannot lock ref '%s': "
>>   					    "reference already exists"),
>> @@ -1043,7 +1060,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>>   		 * - `core.logAllRefUpdates` tells us to create the reflog for
>>   		 *   the given ref.
>>   		 */
>> -		if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) {
>> +		if ((u->flags & REF_HAVE_NEW) && !(u->type & REF_ISSYMREF) && ref_update_has_null_new_value(u)) {
>
> The old line was already quite long and the new one is even longer -
> perhaps we could break it after the second "&&"
>

I will break it both the && I think that is easier on the eyes.

>> +			if (create_reflog) {
>> +				ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
>> +				log = &logs[logs_nr++];
>> +				memset(log, 0, sizeof(*log));
>> +
>> +				fill_reftable_log_record(log);
>> +				log->update_index = ts;
>> +				log->refname = xstrdup(u->refname);
>> +				memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);
>> +				memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ);
>
> Both these lines would benefit from being folded
>
> Best Wishes
>
> Phillip


Thanks for the review.

Karthik

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