Re: [PATCH 5/8] refs/files-backend: add support for symref updates

[Patrick Steinhardt <ps@pks.im>]

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

On Sat, Mar 30, 2024 at 11:46:20PM +0100, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Add support for transactional symbolic reference updates in the files
> backend. This also adheres to the config of using symlinks for symbolic
> references.
> 
> While this commit is setting up the files-backend to support symrefs in
> transaction's. It will only be used in a consequent commit, when we wire
> up the `update-symref` option for `git-update-ref`.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c | 45 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4dbe73c106..6b4cc80843 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2323,7 +2323,7 @@ static int split_head_update(struct ref_update *update,
>  			transaction, "HEAD",
>  			update->flags | REF_LOG_ONLY | REF_NO_DEREF,
>  			&update->new_oid, &update->old_oid,
> -			update->msg, NULL);
> +			update->msg, update->symref_target);
>  
>  	/*
>  	 * Add "HEAD". This insertion is O(N) in the transaction
> @@ -2386,7 +2386,7 @@ static int split_symref_update(struct ref_update *update,
>  	new_update = ref_transaction_add_update(
>  			transaction, referent, new_flags,
>  			&update->new_oid, &update->old_oid,
> -			update->msg, NULL);
> +			update->msg, update->symref_target);
>  
>  	new_update->parent_update = update;
>  
> @@ -2396,7 +2396,7 @@ static int split_symref_update(struct ref_update *update,
>  	 * done when new_update is processed.
>  	 */
>  	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
> -	update->flags &= ~REF_HAVE_OLD;
> +	update->flags &= ~(REF_HAVE_OLD|REF_UPDATE_SYMREF);
>  
>  	/*
>  	 * Add the referent. This insertion is O(N) in the transaction
> @@ -2567,6 +2567,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>  		}
>  	}
>  
> +	if (update->flags & REF_UPDATE_SYMREF) {
> +		if (create_symref_lock(refs, lock, update->refname, update->symref_target)) {
> +			ret = TRANSACTION_GENERIC_ERROR;
> +			goto out;
> +		}
> +
> +		if (close_ref_gently(lock)) {
> +			strbuf_addf(err, "couldn't close '%s.lock'",
> +				    update->refname);
> +			ret = TRANSACTION_GENERIC_ERROR;
> +			goto out;
> +		}
> +
> +		/*
> +		 * Once we have created the symref lock, the commit
> +		 * phase of the transaction only needs to commit the lock.
> +		 */
> +		if (update->flags & REF_UPDATE_SYMREF)
> +			update->flags |= REF_NEEDS_COMMIT;

As far as I can see the `update->flags` aren't ever modified in this
block, which already is guarded via `update->flags = REF_UPDATE_SYMREF`.
This condition should thus be superfluous, and we can instead set
`REF_NEEDS_COMMIT` unconditionally here.

> +	}
> +
>  	if ((update->flags & REF_HAVE_NEW) &&
>  	    !(update->flags & REF_DELETING) &&
>  	    !(update->flags & REF_LOG_ONLY)) {
> @@ -2862,6 +2883,14 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  
>  		if (update->flags & REF_NEEDS_COMMIT ||
>  		    update->flags & REF_LOG_ONLY) {
> +			if (update->flags & REF_UPDATE_SYMREF) {
> +				if (!refs_resolve_ref_unsafe(&refs->base, update->symref_target,
> +							     RESOLVE_REF_READING, &update->new_oid, NULL)) {
> +					strbuf_addf(err, "refname %s not found", update->symref_target);
> +					goto cleanup;
> +				}
> +			}

So we try to resolve the symref target here so that we can provide a
proper new object ID for the reflog entry. What happens though when the
caller tries to create a dangling symref where the target ref does not
exist? Wouldn't we raise an error and abort in that case?

I think we should handle this case gracefully and set the new object ID
to the zero OID. Which is also what happens when deleting the target of
e.g. the `HEAD` symref.

>  			if (files_log_ref_write(refs,
>  						lock->ref_name,
>  						&lock->old_oid,
> @@ -2879,6 +2908,16 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  				goto cleanup;
>  			}
>  		}
> +
> +		/*
> +		 * We try creating a symlink, if that succeeds we continue to the
> +		 * next updated. If not, we try and create a regular symref.
> +		 */
> +		if (update->flags & REF_UPDATE_SYMREF && prefer_symlink_refs)
> +			if (!create_ref_symlink(lock, update->symref_target))
> +				continue;
> +
> +

There's a superfluous newline here.

>  		if (update->flags & REF_NEEDS_COMMIT) {
>  			clear_loose_ref_cache(refs);
>  			if (commit_ref(lock)) {

What is the purpose of `clear_loose_ref_cache()` here, and do we need to
call it when updating symrefs, too?

Patrick

> -- 
> 2.43.GIT
> 

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