Re: [PATCH 5/8] refs/files-backend: add support for symref updates
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 2, 2024, 12:20 UTC
- Message-ID
- <Zgv4DShrInEE30GA@tanuki>
- In-Reply-To
- <20240330224623.579457-6-knayak@gitlab.com>
On Sat, Mar 30, 2024 at 11:46:20PM +0100, Karthik Nayak wrote:
Show 69 quoted lines
> 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.
Show 16 quoted lines
> + }
> +
> 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.
Show 17 quoted lines
> 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 >