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.
> + 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.
> + 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.
> + /* 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
Attachment:
signature.asc
Description: PGP signature
