| Message ID | 20240501202229.2695774-7-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: add support for transactional symref updates | expand |
On Wed, May 01, 2024 at 10:22:28PM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > The `refs_create_symref()` function is used to update/create a symref. > But it doesn't check the old target of the symref, if existing. It force > updates the symref. In this regard, the name `refs_create_symref()` is a > bit misleading. So let's rename it to `refs_update_symref()`. This is > akin to how 'git-update-ref(1)' also allows us to create apart from > update. Arguably, as we are already updating all callsites anyway, I don't see a reason why we shouldn't also update the function to accept the old OID or old target so that callers can make raceless updates. The only problem is that we don't have any users yet, and consequently we have no way to verify that it works as intended. So maybe this is better left for a future patch series, unless we have places where we can reasonably update the callers to pass in the old value, as well. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, May 01, 2024 at 10:22:28PM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> The `refs_create_symref()` function is used to update/create a symref. >> But it doesn't check the old target of the symref, if existing. It force >> updates the symref. In this regard, the name `refs_create_symref()` is a >> bit misleading. So let's rename it to `refs_update_symref()`. This is >> akin to how 'git-update-ref(1)' also allows us to create apart from >> update. > > Arguably, as we are already updating all callsites anyway, I don't see a > reason why we shouldn't also update the function to accept the old OID > or old target so that callers can make raceless updates. > > The only problem is that we don't have any users yet, and consequently > we have no way to verify that it works as intended. So maybe this is > better left for a future patch series, unless we have places where we > can reasonably update the callers to pass in the old value, as well. > > Patrick Yeah that did run through my mind too, but without a usecase, its hard to justify a change and also like you mentioned harder to write tests for.
diff --git a/builtin/branch.c b/builtin/branch.c index dd3e3a7dc0..4491f7a20c 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -555,7 +555,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, continue; refs = get_worktree_ref_store(worktrees[i]); - if (refs_create_symref(refs, "HEAD", newref, logmsg)) + if (refs_update_symref(refs, "HEAD", newref, logmsg)) ret = error(_("HEAD of working tree %s is not updated"), worktrees[i]->path); } diff --git a/builtin/worktree.c b/builtin/worktree.c index 7c6c72536b..480202c517 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -517,7 +517,7 @@ static int add_worktree(const char *path, const char *refname, ret = refs_update_ref(wt_refs, NULL, "HEAD", &commit->object.oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR); else - ret = refs_create_symref(wt_refs, "HEAD", symref.buf, NULL); + ret = refs_update_symref(wt_refs, "HEAD", symref.buf, NULL); if (ret) goto done; diff --git a/refs.c b/refs.c index 1b67c87f47..1215b0fae2 100644 --- a/refs.c +++ b/refs.c @@ -2286,10 +2286,8 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled) return peel_object(base, peeled) ? -1 : 0; } -int refs_create_symref(struct ref_store *refs, - const char *ref_target, - const char *refs_heads_master, - const char *logmsg) +int refs_update_symref(struct ref_store *refs, const char *ref, + const char *target, const char *logmsg) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2297,8 +2295,8 @@ int refs_create_symref(struct ref_store *refs, transaction = ref_store_transaction_begin(refs, &err); if (!transaction || - ref_transaction_update(transaction, ref_target, NULL, NULL, - refs_heads_master, NULL, REF_NO_DEREF, + ref_transaction_update(transaction, ref, NULL, NULL, + target, NULL, REF_NO_DEREF, logmsg, &err) || ref_transaction_commit(transaction, &err)) { ret = error("%s", err.buf); @@ -2314,7 +2312,7 @@ int refs_create_symref(struct ref_store *refs, int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { - return refs_create_symref(get_main_ref_store(the_repository), ref_target, + return refs_update_symref(get_main_ref_store(the_repository), ref_target, refs_heads_master, logmsg); } diff --git a/refs.h b/refs.h index c7851bf587..71cc1c58e0 100644 --- a/refs.h +++ b/refs.h @@ -606,7 +606,7 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref, int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg); -int refs_create_symref(struct ref_store *refs, const char *refname, +int refs_update_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg); int create_symref(const char *refname, const char *target, const char *logmsg); diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 82bbf6e2e6..4651e4ced7 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -118,7 +118,7 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv) const char *target = notnull(*argv++, "target"); const char *logmsg = *argv++; - return refs_create_symref(refs, refname, target, logmsg); + return refs_update_symref(refs, refname, target, logmsg); } static struct flag_definition transaction_flags[] = {