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

[Phillip Wood <phillip.wood123@gmail.com>]

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, //

> 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()

> 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

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

> +				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 "&&"

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