On Sun, Apr 21, 2024 at 08:50:07AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > On Fri, Apr 12, 2024 at 11:59:06AM +0200, Karthik Nayak wrote:
> >> From: Karthik Nayak <karthik.188@xxxxxxxxx>
> > [snip]
> >> @@ -268,6 +268,39 @@ static void parse_cmd_create(struct ref_transaction *transaction,
> >> strbuf_release(&err);
> >> }
> >>
> >> +static void parse_cmd_symref_create(struct ref_transaction *transaction,
> >> + const char *next, const char *end)
> >> +{
> >> + struct strbuf err = STRBUF_INIT;
> >> + char *refname, *new_ref;
> >> +
> >> + if (!(update_flags & REF_NO_DEREF))
> >> + die("symref-create: cannot operate with deref mode");
> >> +
> >> + refname = parse_refname(&next);
> >> + if (!refname)
> >> + die("symref-create: missing <ref>");
> >> +
> >> + new_ref = parse_next_refname(&next);
> >> + if (!new_ref)
> >> + die("symref-create %s: missing <new-ref>", refname);
> >> + if (read_ref(new_ref, NULL))
> >> + die("symref-create %s: invalid <new-ref>", refname);
> >
> > This restricts the creation of dangling symrefs, right? I think we might
> > want to lift this restriction, in which case we can eventually get rid
> > of the `create_symref` callback in ref backends completely.
> >
>
> Yes it does. I thought it'd be more consistent with what update-ref does
> with regular ref updates. We verify that the object exists. Also this
> could be an added option 'allow-dangling'.
>
> I'm not sure I understand what you mean 'the `create_symref` callback in
> ref backends completely.'.
Well, once normal reference transactions learn to update symrefs we can
do the following:
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 56641aa57a..2302311282 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -676,7 +676,6 @@ struct ref_storage_be {
ref_transaction_commit_fn *initial_transaction_commit;
pack_refs_fn *pack_refs;
- create_symref_fn *create_symref;
rename_ref_fn *rename_ref;
copy_ref_fn *copy_ref;
There would be no need to specifically have this as a separate callback
anymore as we can now provide a generic wrapper `refs_create_symref()`
(in pseudo-code):
```
int refs_create_symref(struct ref_store *refs, const char *refname,
const char *target)
{
tx = ref_store_transaction_begin(refs);
ref_transaction_create_symref(tx, refname, target);
ref_transaction_commit(tx);
}
```
Patrick
> >> + if (*next != line_termination)
> >> + die("symref-create %s: extra input: %s", refname, next);
> >> +
> >> + if (ref_transaction_create(transaction, refname, NULL, new_ref,
> >> + update_flags | create_reflog_flag |
> >> + REF_SYMREF_UPDATE, msg, &err))
> >> + die("%s", err.buf);
> >> +
> >> + update_flags = default_flags;
> >> + free(refname);
> >> + free(new_ref);
> >> + strbuf_release(&err);
> >> +}
> >> +
> >> static void parse_cmd_delete(struct ref_transaction *transaction,
> >> const char *next, const char *end)
> >> {
> >> @@ -476,6 +509,7 @@ static const struct parse_cmd {
> >> { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN },
> >> { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN },
> >> { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN },
> >> + { "symref-create", parse_cmd_symref_create, 2, UPDATE_REFS_OPEN },
> >> { "symref-delete", parse_cmd_symref_delete, 2, UPDATE_REFS_OPEN },
> >> { "symref-verify", parse_cmd_symref_verify, 2, UPDATE_REFS_OPEN },
> >> { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN },
> >> diff --git a/refs.c b/refs.c
> >> index 6d98d9652d..e62c0f4aca 100644
> >> --- a/refs.c
> >> +++ b/refs.c
> >> @@ -1305,15 +1305,20 @@ int ref_transaction_update(struct ref_transaction *transaction,
> >> int ref_transaction_create(struct ref_transaction *transaction,
> >> const char *refname,
> >> const struct object_id *new_oid,
> >> + const char *new_ref,
> >> unsigned int flags, const char *msg,
> >> struct strbuf *err)
> >> {
> >> - if (!new_oid || is_null_oid(new_oid)) {
> >> + if ((flags & REF_SYMREF_UPDATE) && !new_ref) {
> >> + strbuf_addf(err, "'%s' has a no new ref", refname);
> >> + return 1;
> >> + }
> >> + if (!(flags & REF_SYMREF_UPDATE) && (!new_oid || is_null_oid(new_oid))) {
> >> strbuf_addf(err, "'%s' has a null OID", refname);
> >> return 1;
> >> }
> >> return ref_transaction_update(transaction, refname, new_oid,
> >> - null_oid(), NULL, NULL, flags,
> >> + null_oid(), new_ref, NULL, flags,
> >> msg, err);
> >> }
> >>
> >> diff --git a/refs.h b/refs.h
> >> index 60e6a21a31..c01a517e40 100644
> >> --- a/refs.h
> >> +++ b/refs.h
> >> @@ -744,6 +744,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
> >> int ref_transaction_create(struct ref_transaction *transaction,
> >> const char *refname,
> >> const struct object_id *new_oid,
> >> + const char *new_ref,
> >> unsigned int flags, const char *msg,
> >> struct strbuf *err);
> >>
> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> index 7c894ebe65..59d438878a 100644
> >> --- a/refs/files-backend.c
> >> +++ b/refs/files-backend.c
> >> @@ -2609,6 +2609,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> >> }
> >> }
> >>
> >> + if (update->flags & REF_SYMREF_UPDATE && update->new_ref) {
> >
> > Let's add braces around `(updaet->flags & REF_SYMREF_UPDATE)` to make
> > this easier to read.
> >
>
> This should now be cleaned up as we removed the flag entirely.
>
> >> + if (create_symref_lock(refs, lock, update->refname, update->new_ref)) {
> >> + 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.
> >> + */
> >> + update->flags |= REF_NEEDS_COMMIT;
> >> + }
> >> +
> >> +
> >> if ((update->flags & REF_HAVE_NEW) &&
> >> !(update->flags & REF_DELETING) &&
> >> !(update->flags & REF_LOG_ONLY)) {
> >> @@ -2904,6 +2925,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_SYMREF_UPDATE && update->new_ref) {
> >
> > Here, as well.
> >
>
> Same here.
>
> >> + /* for dangling symrefs we gracefully set the oid to zero */
> >> + if (!refs_resolve_ref_unsafe(&refs->base, update->new_ref,
> >> + RESOLVE_REF_READING, &update->new_oid, NULL)) {
> >> + update->new_oid = *null_oid();
> >> + }
> >
> > Can this actually happenn right now? I thought that the `read_ref()`
> > further up forbids this case.
> >
> > Patrick
>
> With update-ref, it won't happen anymore, because as you mentioned, we
> use `read_ref()`. I thought it was still worthwhile to have. But I guess
> its cleaner to remove this.
Attachment:
signature.asc
Description: PGP signature
