| Message ID | 20240330224623.579457-6-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | update-ref: add support for update-symref option | expand |
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 >
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; + } + 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; + } + } + 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; + + if (update->flags & REF_NEEDS_COMMIT) { clear_loose_ref_cache(refs); if (commit_ref(lock)) {