On Sat, Mar 30, 2024 at 11:46:20PM +0100, Karthik Nayak wrote:
> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
